For whatever reason, not all arguments of createentity() are exposed in
the command.
We have to keep in mind that (1) unspecified arguments default to 0
(instead of the 320 and 240 for argument 8 and 9 that createentity()
usually defaults to), and that (2) arguments persist across commands.
(Why not get rid of argument persistence, you say? Unfortunately, some
levels rely on argument persistence to call gotoposition() without
specifying the third argument, even though you're supposed to specify
all three arguments.)
To add these arguments without breaking levels, I re-added the
createentity() defaults of 320 and 240 for args 8 and 9, and then I
reset the new arguments afterwards when I'm done. Technically this could
be bad if other commands used those higher arguments, but none of them
really do. (Except createcrewman(), but it only sets argument 6 to 0
sometimes anyway, but argument 6 is already supposed to default to 0.)
This patch is very kludge-y, but at least it fixes a semi-noticeable
visual issue in custom levels that use internal scripts to spawn
entities when loading a room.
Basically, the problem here is that when the game checks for script
boxes and sets newscript, newscript has already been processed for that
frame, and when the game does load a script, script.run() has already
been processed for that frame.
That issue can be fixed, but it turns out that due to my over-30-FPS
game loop changes, there's now ANOTHER visible frame of delay between
room load and entity creation, because the render function gets called
in between the script being loaded at the end of gamelogic() and the
script actually getting run.
So... I have to temporary move script.run() to the end of gamelogic()
(in map.twoframedelayfix()), and make sure it doesn't get run next
frame, because double-evaluations are bad. To do that, I have to
introduce the kludge variable script.dontrunnextframe, which does
exactly as it says.
And with all that work, the two-frame (now three-frame) delay is fixed.
Ironically enough, resetting more variables in script.hardreset() makes
the glitchy fadeout system even more glitchy. Resetting map.towermode,
for example, makes it so that if you're in towers when you quit to the
menu, script.hardreset() makes it so that the game thinks you're no
longer inbounds (because it no longer thinks you're in a tower and thus
considers coordinates in the space of 40x30 tiles to be inbounds instead
of 40x700 or 40x100 tiles to be inbounds), calls map.gotoroom(), which
resets the gamestate to 0. So if we're using the old system, it's better
to reset only as much as needed.
And furthermore, we shouldn't be relying on script.hardreset() to
initialize variables for us. That should be done at the class
constructor level. So I've gone ahead and initialized the variables in
class constructors, too.
This is the second part of what is necessary for credits warp to work.
The speedrunners call this "text storage". You need to get the
advancetext prompt up without a text box in order to be able to
increment the gamestate without bound. In 2.0, script.hardreset() reset
the text boxes, but not the prompt.
It seems like they were unfinished. This commit makes them properly
work.
When a track is stopped with stopmusic() or musicfadeout(),
resumemusic() will resume from where the track stopped. musicfadein()
does the same but does it with a gradual fade instead of suddenly
playing it at full volume.
I changed several interfaces around for this. First, setting currentsong
to -1 when music is stopped is handled in the hook callback that gets
called by SDL_mixer whenever the music stops. Otherwise, it'd be
problematic if currentsong was set to -1 when the song starts fading out
instead of when the song actually ends.
Also, music.play() has a few optional arguments now, to reduce the
copying-and-pasting of music code.
Lastly, we have to roll our own tracker of music length by using
SDL_GetPerformanceCounter(), because there's no way to get the music
position if a song fades out. (We could implicitly keep the music
position if we abruptly stopped the song using Mix_PauseMusic(), and
resume it using Mix_ResumeMusic(), but ignoring the fact that those two
functions are also used on the unfocus-pause (which, as it turns out, is
basically a non-issue because the unfocus-pause can use some other
functions), there's no equivalent for fading out, i.e. there's no
"fade out and pause when it fully fades out" function in SDL_mixer.) And
then we have to account for the unfocus-pause in our manual tracker.
Other than that, these commands are now fully functional.
Now it's really, really smooth. Except for like the last frame when it
goes down, which I sometimes didn't notice (but maybe it didn't happen
every time due to being lucky on the delta timesteps or something,
whatevs.)
Looks like duplicate player entities persisting across rooms is a
semi-useful feature used by some levels. Still, though, it's a bit of a
nuisance to have duplicate player entities persisting across game
sessions. And levels can't rely on this persistence anyway, anyone could
just close the game and re-open it to get rid of the duplicate entities
regardless.
Continuing from #280, another potential source of out-of-bounds indexing
(and thus, Undefined Behavior badness) comes from script commands. A
majority of them don't do any input validation at all, which means the
potential for out-of-bounds indexing and segfaulting in custom levels.
So it's always good to add bounds checks to them.
Interesting note, the only existing command that has bounds checks is
the flag() command. That means you can't turn out-of-bounds flags on or
off. But there's no bounds checks for ifflag(), or customifflag(), which
means you CAN index out-of-bounds with those commands! That's a bit bad
to do, so.
Also, I decided to add the bounds checks for playef() at the
musicclass::playef() level, instead of just the level of the playef()
command. I don't know of any other cases outside of the command where
musicclass::playef() will index out of bounds, but musicclass is the one
containing the indexed vector anyway, I wanted to cover more cases, and
it's better to be safe than sorry.
obj.getplayer() can return -1, which can cause out-of-bounds indexing of
obj.entities, which is really bad. This was by far the most changes, as
obj.getplayer() is the most used entity-getting function that returns
-1, as well as the most-used function whose sentinel value goes
unchecked.
To deal with the usage of obj.getplayer() in mapclass::warpto(), I just
added general bounds checks inside that function instead of changing all
the callers.
A few months ago, I added ghosts to the VVVVVV: Community Edition editor. I was told recently I should think
about upstreaming it, and with Terry saying go ahead I finally ported them into VVVVVV. There's one slight
difference however--you can choose whether you have them or not in the editor's settings menu. They're off by
default, and this is saved to the save file.
Anyway, when you're playtesting, the game saves the players position, color, room coordinates and sprite every 3
frames. The max is 100, where if it tries to add more, the oldest one gets removed.
When you exit playtesting, the saved positions appear one at a time, and you can use the Z key to speed it up.
[Here's a video of them in action.](https://o.lol-sa.me/4H21zCv.mp4)
The actual unindent is done in a separate commit to minimize noise,
because diffs are terrible at clearly conveying unindents (it should put
all the minus lines together and all the plus lines together, too).
The entirety of the rest of scriptclass::loadcustom() is encased in a
block that first checks if the script with the name even exists. Instead
of indenting the rest of the function, just invert the check and reduce
indentation level.
This commit refactors custom level scripts to no longer be stored in one
giant vector containing not only every single script name, but every
single script's contents as well. More specifically,
scriptclass::customscript has been converted to an std::vector<Script>
scriptclass::customscripts (note the extra S), and a Script is just a
struct with an std::string name and std::vector<std::string> contents.
This is an improvement in both performance and maintainability. The game
no longer has to look through script contents in case they're actually
script names, and then manually extract the script contents from there.
Instead, all it has to do is look for script names only. And the
contents are provided for free. This results in a performance gain.
Also, the old system resulted in lots of boilerplate everywhere anytime
scripts had to be handled or parsed. Now, the boilerplate is only done
when saving or loading a custom level. This makes code quality much,
much better.
To be sure I didn't actually change anything, I tested by first saving
Dimension Open in current 2.3 (because current 2.3 gets rid of the
awful edentity whitespace), and then resaved it on this patch. There is
absolutely no difference between the current-2.3-resave and
this-patch-resave.
This resulted in two bugs:
1. Custom assets would not be unmounted when quitting to the menu.
2. Custom assets would be unmounted when playtesting a level.
The solution is to unmount assets in Game::quittomenu() instead.
Main game would retain custom level assets, now fixed. Also, custom fonts load properly. Finally, levels can be stored as a zip and placed in the levels folder, with the .vvvvvv file at the root of the zip and custom asset folders (graphics, sounds etc) also at the root.
This commit adds bounds checks to those commands in case say()/reply()
asks for more lines than there are left in the all-script-lines buffer
(not just the current script, so in order for it to segfault your script
has to be last in the all-script-lines vector), and in case text() asks
for more lines than there are in the rest of the rest of the parsed
internal script.
For some reason, scriptclass::loadcustom() sometimes refers to
`customscript` as `script.customscript` instead of `customscript`. The
`script.` is unnecessary.
Otherwise, it would end up indexing a vector out-of-bounds, which is UB
and causes a segfault, too. This is used in some constructs like
say(-1)
<internal command>
where the text contents don't matter, only that a text box shows up.
The `speak` command is the exact same as the `speak_active` command,
except without one line of code. So instead of copy-pasting the entire
thing, it's better to just combine them into the same chunk of code.
Resetting game.activeactivity fixes triggering Undefined Behavior if you
quit to the menu while standing inside an activity zone, and then
re-entered the game. Resetting game.act_fade also fixes the activity
zone prompt fadeout that happens once the above segfault is fixed.
Don't worry, other activity-zone like variables such as teleporter
prompts and trophy text are already covered. obj.trophytext is reset in
scriptclass::hardreset(), too, and game.readytotele is reset every time
mapclass::gotoroom() is called, and mapclass::gotoroom() is called every
time a gamemode is started.
Instead of using somewhat-obtuse for-loops to initialize or reset these
vectors, it takes up less lines of code and is clearer if we use
std::vector::resize() and std::vector::clear() instead.
Otherwise, if you died after entering a room with a horizontal or
vertical warp background (but not the all-sides warp background), the
warp background would be the first thing you see when going to the Game
Over screen, and would then start scrolling downwards with the proper
tower background coming in from the top.
This oversight seems to have always been in the game.
Was No Death Mode actually tested? Like, did anyone ever play through
the entire game without dying in the Warp Zone, or even AFTER completing
the Warp Zone, like, ever?
It's less code being copied and pasted, especially since for my
over-30-FPS patch I would have to make a separate function for each if
both of them were still there, but if they're unified into one then I
will only have to make one more function.
And since map.scrolldir is now used outside of GAMEMODE, we'll need to
reset it in hardreset() and when exiting playtesting.
Stringly-typed things are bad, because if you make a typo when typing
out a string, it's not caught at compile-time. And in the case of this
menu system, you'd have to do an excessive amount of testing to uncover
any bugs caused by a typo. Why do that when you can just use an enum and
catch compile-time errors instead?
Also, you can't use switch-case statements on stringly-typed variables.
So every menu name is now in the enum Menu::MenuName, but you can simply
refer to a menu name by just prefixing it with Menu::.
Unfortunately, I've had to change the "continue" menu name to be
"continuemenu", because "continue" is a keyword in C and C++. Also, it
looks like "timetrialcomplete4" is an unused menu name, even though it
was referenced in Render.cpp.
It's treated like a bool anyway, so might as well make it one.
This also necessitates updating every single instance where it or an
element inside it is used, too.
Same principle as removing the separate variable to track number of
collected trinkets. This means it's less error-prone as we're no longer
tracking number of trinkets separately.
In the function that counts the number of trinkets, I would've liked to
have used std::count_if(). However, the most optimal way would require
using a lambda, and lambdas are too new for the C++ standard we're
using. So I just bit the bullet and counted them manually.
game.trinkets is supposed to be correlated with obj.collect, however why
not just count obj.collect directly?
This turns game.trinkets into a function, game.trinkets(), which will
directly count the number of collected trinkets and return it. This will
fix a few corner cases where the number of trinkets can desync with the
actual collection statuses of trinkets.
In order to keep save compatibility with previous versions of VVVVVV,
the game will still write the <trinkets> variable. However, it will not
read the <trinkets> variable from a save file.
This removes the variables obj.nentity and obj.nlinecrosskludge, as well
as removing the 'active' attribute from the entity class object. Now
every entity you access is guaranteed to be real and you don't have to
check the 'active' variable.
The biggest part of this is changing createentity() to modify a
newly-created entity object and push it back instead of already
modifying an indice in obj.entities.
As well, removing an entity now uses the new obj.removeentity() function
and removeentity_iter() macro.
Looks like this function was created because editorclass::load() takes
in a string by reference, not by value, and thus mutates it afterwards,
so if you passed a string in when you didn't want it to be mutated, bad
things would happen.
However, a better workaround for the above issue would simply to
duplicate the string and pass that string instead, thus the original
string wouldn't be affected.
This removes global arg passing from all functions on editorclass.
Callers have been updated correspondingly. Additionally, all 'dwgfx' has
been replaced with 'graphics' in editor.cpp.
