These variables basically serve no purpose. map.customx and map.customy
are clearly never used. map.finalx and map.finaly, on the other hand,
are basically always game.roomx and game.roomy respectively if
map.finalmode is on, and if it's off, then they don't matter.
Also, there are some weird and redundant variable assignments going on
with these; most notably in map.gotoroom(), where rx/ry (local
variables) get assigned to finalx/finaly, then finalx/finaly get
assigned to game.roomx/game.roomy, then finalx/finaly get assigned to
rx/ry. If finalx/finaly made a difference, then there'd be no need to
assign finalx/finaly back to rx/ry. So it makes the code clearer to
remove these weird bits of code.
When gamemode(teleporter) gets run in a script, it brings up a read-only
version of the teleporter screen, intended only for displaying rooms on
the minimap.
However, ever since 2.3 allowed bringing up the map screen during
cutscenes (in order to prevent softlocks), bringing up the map screen
during this mode would (1) do an unnecessary animation of suddenly
switching back to the game and bringing up the menu screen again (even
though the menu screen has already been brought up), and (2) would let
you close the menu entirely and go back to GAMEMODE, thus
unintentionally closing the teleporter screen and kind of ruining the
cutscene.
To fix this, when you bring up the map screen, it will instead instantly
transition to the map screen. And when you bring it down, it will also
instantly transition back to the teleporter screen.
But that's not all. The previous behavior was actually kind of a nice
failsafe, in that if you somehow got stuck in a state where a script ran
gamemode(teleporter), but stopped running before it could take you out
of that mode by running gamemode(game), then you could return to
GAMEMODE yourself by bringing up the map screen and then bringing it
back down. So I've made sure to keep that failsafe behavior, only as
long as there isn't a script running.
When bringing up the map screen, the game does a small menu animation
where the menu comes in from the bottom. The code to calculate the menu
offset is copy-pasted everywhere, so I thought I'd de-duplicate it to
make my life easier when working with it. I also included the
game.gamestate assignment in the de-duplicated function, so it would be
easier for a future bugfix.
At the same time, I'm also removing all the BlitSurfaceStandard()s that
copied menubuffer to backBuffer. The red flag is that this blit happened
for every single entry point to MAPMODE and TELEPORTERMODE, except for
the script command gamemode(teleporter). Pressing Enter to bring up the
map screen, pressing Enter to quit the Super Gravitron, pressing Esc to
bring up the pause screen, and pressing Enter to bring up the teleporter
screen all do this blit, so if this blit was there to fix a bug, then
there's a bug with using the script command gamemode(teleporter)... but,
as far as I can tell, there isn't.
That's because the blit basically does nothing. All the blit does is
copy menubuffer onto backBuffer. Then the next thing that happens is
that either maprender() or teleporterrender() will be called, and the
first thing that those functions will always do is fill backBuffer with
solid black, completely overriding the previous blit. So that's why
removing this blit won't have any effect, and it can be safely removed
for code clarity.
When I did #567, I didn't test it. And I should have tested it, because
it made the player invisible. This is because map.resetplayer() also
sets the invis attribute of the player to true as well, and I only undid
it setting game.lifeseq to 10.
So instead, I'll just add a flag to map.resetplayer() that by default
doesn't set game.lifeseq or the player's invis attribute. And I tested
it this time, and it works fine. I tested both respawning after death
and exiting to the menu and loading in the game again.
There's a small inconsistency where the first time you load in to the
game, game.lifeseq is at 0, but when you exit to the menu and load in
again, game.lifeseq becomes 10. This is visible as Viridian blinking
when loading in only after the first time you load in, and this also
means that after the first time you load in, you also have to wait 5
frames before being able to move Viridian.
The reason for this inconsistency is because on the first time you load
in to the game, there are no entities loaded in obj.entities yet, so the
game creates a player entity, and doesn't mess with game.lifeseq. When
you exit and then load in for the second time, obj.entities contains at
least one entity (all the entities from the room you just exited out of
- map.gotoroom() hasn't even been called yet, so it doesn't even check
for only the player entity), so the game calls map.resetplayer()
instead, and map.resetplayer() sets game.lifeseq to 10.
There's even an inconsistency to this inconsistency - when you die in No
Death Mode, all entities will be removed from obj.entities, so the next
time you load in to the game, game.lifeseq will be 0.
This inconsistency is pretty minor in the grand scheme of things, but
it still bothers me, so I'm going to fix it.
While I was working on #535, I noticed that all the call sites of
script.run() have the exact same code, namely:
if (script.running)
{
script.run();
}
I figured, why not move the script.running check into the function
itself? That way, we won't have to duplicate the check every single
time, and we don't risk forgetting to add the check and causing a bug
because of that.
The check was already duplicated once since 2.0 (it's used in both
GAMEMODE and TELEPORTERMODE), and with the fix of the two-frame delay in
2.3, it's now duplicated twice, leading to THREE instances of this check
in the code, when there should be only one.
In order to be able to fix the bug #556, I'm planning on adding
ScreenSettings* to the settings.vvv write function. However, that
entails adding another argument to Game::savesettings(), which is going
to be really messy given the default argument of Game::savestats().
That, combined with the fact that the code comment at the site of the
implementation of Game::savestats() being wrong (!!!), leads me to
believe that using default function arguments here isn't worth it.
Instead, what I've done is made it so callers are explicit about whether
or not they're calling savestats(), savesettings(), or both at the same
time. If they are calling both at the same time, then they will be using
a new function named savestatsandsettings().
In short, these are the interface changes:
* bool Game::savestats(bool) has been removed
* bool Game::savestatsandsettings() has been added
* void Game::savestats_menu() has been renamed to
void Game::savestatsandsettings_menu()
* All previous callers of bool Game::savestats() are now using bool
Game::savestatsandsettings()
* The one caller of bool Game::savestats(bool) is now using bool
Game::savestats()
First, the variable has been inverted, because it's bad practice to have
booleans with negative names. Secondly, instead of magically setting a
signaling variable when calling fadeout(), fadeout() now has a parameter
to set the quick_fade attribute, which is much cleaner than doing the
magical assignment that could potentially look unrelated.
With the previous commit in place, we can now simply move some usages of
the previous towerbg to use a separate object instead. That way, we
don't have to mess with a monolithic state, or some better way to phrase
what I just said, and we instead have two separate objects that can
coexist side-by-side.
Previously, the tower background was controlled by a disparate set of
attributes on Graphics and mapclass, and wasn't really encapsulated. (If
that's what that word means, I don't particularly care about
object-oriented lingo.) But now, all relevant things that a tower
background has has been put into a TowerBG struct, so it will be easy to
make multiple copies without having to duplicate the code that handles
it.
These commands now use the INBOUNDS_ARR() macro to convey intent, and to
make sure that if the size of the array changes in the future, that the
bounds check wouldn't end up being wrong. Also fixed some code style for
the flag() and ifcrewlost() commands.
I was investigating a desync in my Nova TAS, and it turns out that
the gravity line collision functions check for the `oldxp` and `oldyp`
of the player, i.e. their position on the previous frame, along with
their position on the current frame. So, if the player either collided
with the gravity line last frame or this frame, then the player collided
with the gravity line this frame.
Except, that's not actually true. It turns out that `oldxp` and `oldyp`
don't necessarily always correspond to the `xp` and `yp` of the player
on the previous frame. It turns out that your `oldyp` will be updated if
you stand on a vertically moving platform, before the gravity line
collision function gets ran. So, if you were colliding with a gravity
line on the previous frame, but you got moved out of there by a
vertically moving platform, then you just don't collide with the gravity
line at all.
However, this behavior changed in 2.3 after my over-30-FPS patch got
merged (#220). That patch took advantage of the existing `oldxp` and
`oldyp` entity attributes, and uses them to interpolate their positions
during rendering to make everything look real smooth.
Previously, `oldxp` and `oldyp` would both be updated in
`entityclass::updateentitylogic()`. However, I moved it in that patch to
update right before `gameinput()` in `main.cpp`.
As a result, `oldyp` no longer gets updated whenever the player stands
on a vertically moving platform. This ends up desyncing my TAS.
As expected, updating `oldyp` in `entityclass::movingplatformfix()` (the
function responsible for moving the player whenever they stand on a
vertically moving platform) makes it so that my TAS syncs, but the
visuals are glitchy when standing on a vertically moving platform. And
as much as I'd like to get rid of gravity lines checking for whether
you've collided with them on the previous frame, doing that desyncs my
TAS, too.
In the end, it seems like I should just leave `oldxp` and `oldyp` alone,
and switch to using dedicated variables that are never used in the
physics of the game. So I'm introducing `lerpoldxp` and `lerpoldyp`, and
replacing all instances of using `oldxp` and `oldyp` that my over-30-FPS
patch added, with `lerpoldxp` and `lerpoldyp` instead.
After doing this, and applying #503 as well, my Nova TAS syncs after
some minor but acceptable fixes with Viridian's walkingframe.
By "unnecessary qualifiers to self", I mean something like using the
'game.' qualifier for a variable on the Game class when you're inside a
function on the Game class itself. This patch is to enforce consistency
as most of the code doesn't have these unnecessary qualifiers.
To prevent further unnecessary qualifiers to self, I made it so the
extern in each header file can be omitted by using a define. That way,
if someone writes something referring to 'game.' on a Game function,
there will be a compile error.
However, if you really need to have a reference to the global name, and
you're within the same .cpp file as the implementation of that object,
you can just do the extern at the function-level. A good example of this
is editorinput()/editorrender()/editorlogic() in editor.cpp. In my
opinion, they should probably be split off into their own separate file
because editor.cpp is getting way too big, but this will do for now.
The entity getters I'm referring to are entityclass::getscm(),
entityclass::getlineat(), entityclass::getcrewman(), and
entityclass::getcustomcrewman().
Even though the player should always exist, and the player should always
be indice 0, I wouldn't want to make that assumption. I've been wrong
before.
Also, these functions returning 0 lull you into a false sense of
security. If you assume that commands using these functions are fine,
you'll forget about the fact that `i` in those commands could be
potentially anything, given an invalid argument. In fact, it's possible
to index createactivityzone(), flipgravity(), and customposition()
out-of-bounds by setting `i` to anything! Well, WAS possible. I fixed it
so now they can't.
Furthermore, in the game.scmmoveme block in gamelogic(), obj.getplayer()
wasn't even checked, even though it's been checked in all other places.
I only caught it just now because I wanted to bounds-check all usages of
obj.getscm(), too, and that game.scmmove block also used obj.getscm()
without bounds-checking it as well.
When this is done, there is potential for a mistake to occur when
writing out the bounds check, which is eliminated when using the macro
instead. Luckily, this doesn't seem to have happened, but what's even
worse is I hardcoded 400 instead of using SDL_arraysize(ed.level), so if
the size of ed.level the bounds checks would all be wrong, which
wouldn't be good. But that's fixed now, too.
This is because if they are manually written out, they are more likely
to contain mistakes.
In fact, after further review, there are several functions with
incorrect manually-written bounds checks:
* entityclass::entitycollide()
* entityclass::removeentity()
* entityclass::removeblock()
* entityclass::copylinecross()
* entityclass::revertlinecross()
All of those functions forgot to do 'greater than or equal to' instead
of 'greater than' when comparing against the size of the vector. So they
were erroneous. But they are now fixed.
It's better to do INBOUNDS_VEC(i, obj.entities) instead of 'i > -1'.
'i > -1' is used in cases like obj.getplayer(), which COULD return a
sentinel value of -1 and so correct code will have to check that value.
However, I am now of the opinion that INBOUNDS_VEC() should be used and
isn't unnecessary.
Consider the case of the face() script command: it's not enough to check
i > -1, you should read the routine carefully. Because if you look
closely, you'll see that it's not guaranteed that 'i' will be initialized
at all in that command. Indeed, if you call face() with invalid
arguments, it won't be. And so, 'i' could be something like 215, and
that would index out-of-bounds, and that wouldn't be good. Therefore,
it's better to have the full bounds check instead of checking only one
bounds. Many commands are like this, after some searching I can also
name position(), changemood(), changetile(), changegravity(), etc.
It also makes the code more explicit. Now you don't have to wonder what
-1 means or why it's being checked, you can just read the 'INBOUNDS' and
go "oh, that checks if it's actually inbounds or not".
In 2.0, 2.1, and 2.2, calling flipgravity() on an entity that wasn't
rule 6 would change it to rule 7. In 2.3 currently, doing this will only
change it to rule 7 if it's already rule 6, starting with the
introduction of the change where if an entity was rule 7 it would be
changed to rule 6.
The crewmate conversion trick has been restored, but converting an
entity to a crewmate will change its rule to 6, not 7 like in pre-2.3.
If you want it to be changed to rule 7 instead of 6, you'd have to call
flipgravity() twice in 2.3 and only once in pre-2.3, which would make
maintaining compatibility between versions a bit harder.
So to fix this, I'm inverting it so that if you call flipgravity() on an
entity that isn't rule 7, it will be converted to rule 7, and only if
it's rule 7 will it be converted to rule 6.
This is a followup to b7cf6855b0 and
10ed0058fd.
In 2.2, if you had a duplicate player entity, there'd be no way to get
rid of it. Except for the recently-discovered Arbitrary Entity
Manipulation glitch, where you set `i` to the indice of the entity and
call flipgravity() on it, turning its rule to 7 and making it no longer
a player entity.
However, I patched this useful mechanic out when I made it so that you'd
no longer be able to convert entities with rule 0 to rule 6
(10ed0058fd, upheld in
b7cf6855b0), because doing so would mean
being able to softlock the game by not having any player entity.
So, in this patch, I'm making it so that you CAN convert duplicate
player entities to crewmates (and thus basically destroy them), but you
can't do that to the TRUE player entity (i.e. the first entity indice
that has rule 0, which is basically always indice 0).
So there's this trick that I recently discovered, since many script
commands don't initialize `i` it's possible to use them to manipulate
arbitrary entities by specifying their indice.
This means in 2.2 you can convert entities to pseudo-crewmates by
changing their rule to 6. Except in 2.3, this was fixed when I fixed the
command to work on flipped crewmates as well. So I'm restoring this
functionality, but I recognize the protection that my previous change to
the command did in preventing levels from destroying the player entity
by changing the player's rule to something nonzero, so instead of
removing the if-conditional entirely, I'm making it so that it will only
set the rule if the entity's rule isn't 0.
It's trivially easy to send the scripting system into an infinite loop
on the same frame (i.e. without any script delay in between, i.e. within
the same execution of script.run()). Just take a look at these two
scripts:
a:
iftrinkets(0,b)
#
b:
iftrinkets(0,a)
#
The hashes are to prevent the scripting system from parsing iftrinkets()
using the internal version instead of the simplified version, because
after doing a simplified iftrinkets(), the parser will (to oversimplify)
execute the last line of the script as internal.
Anyway, sending the game into an infinite loop like this will cause the
Not Responding dialog on Windows.
So to prevent this from happening, I've added an execution counter to
scriptclass::run(). If it gets too high, we're in an infinite loop and
so we stop running the script.
Fixes#402 (Violet appearing 1 frame after the Ship teleporter room
appears).
The root cause of this bug is due to the game loop order changes made
with the over-30-FPS patch. 2.2's game loop order was gameinput(),
gamerender(), then gamelogic(). In 2.3, gamerender() was moved to the end
as it required special code to render more than 30 frames a second. So
2.3's game loop order is gameinput(), gamelogic(), then gamerender().
In hindsight, I could have preserved the game loop order, but this would
require some more complex code in the game loop than what is there
currently. Fixing it now would fix rendering glitches such as this one
(along with being able to remove script.dontrunnextframe with the
two-frame-delay fix), but it could also introduce new rendering glitches
we don't yet know about. After discussing this in Discord DMs with
flibit, we agreed that the game loop order should be fixed in 2.4
instead.
When the game teleports you, gamelogic() runs script.teleport(). This
function will gotoroom to the teleporter destination, then it loads the
teleport script. Some teleport scripts (such as levelonecomplete, which
creates Violet) expect that their entities will be created, and more
generally that their script will be ran, on the same frame that the
gotoroom happens, i.e. by the time that the next gamerender() happens,
i.e. script.run() should be ran before the next gamerender() happens.
This would be true on the old game loop order, but with the new game
loop order, gamerender() gets ran directly after gamelogic() with no
script.run() in between.
To fix this, I did the same thing I did with the two-frame-delay fix
(#317), where I ran the script for that frame, but in order to prevent
running it twice I set script.dontrunnextframe to true.
This fixes the bug where Viridian would appear to "zip" when they would
be teleported to the position of the teleporter before being flung out
of it.
As discussed in #393, I've also set the oldxp/oldyp when Viridian is
temporarily positioned in the center of the room, even though at this
point they should already be invisible. This is just to be safe.
Fixes#393.
Okay, so basically here's the include layout that this game now
consistently uses:
[The "main" header file, if any (e.g. Graphics.h for Graphics.cpp)]
[blank line]
[All system includes, such as tinyxml2/physfs/utfcpp/SDL]
[blank line]
[All project includes, such as Game.h/Entity.h/etc.]
And if applicable, another blank line, and then some special-case
include screwy stuff (take a look at editor.cpp or FileSystemUtils.cpp,
for example, they have ifdefs and defines with their includes).
Including a header file inside another header file means a bunch of
files are going to be unnecessarily recompiled whenever that inner
header file is changed. So I minimized the amount of header files
included in a header file, and only included the ones that were
necessary (system includes don't count, I'm only talking about includes
from within this project). Then the includes are only in the .cpp files
themselves.
This also minimizes problems such as a NO_CUSTOM_LEVELS build failing
because some file depended on an include that got included in editor.h,
which is another benefit of removing unnecessary includes from header
files.
graphics.textbox.clear() should be used instead of
graphics.textboxremove() or graphics.textboxremovefast(), because even
with graphics.textboxremovefast(), you'll still have to process at least
one frame of GAMEMODE logic before the text boxes are actually properly
removed, and this caused a 1-frame glitch when exiting playtesting with
text boxes on-screen and then re-entering playtesting.
Technically I could've only fixed it in Game::returntoeditor(), but I
wanted to be safe, so I also fixed it in scriptclass::hardreset(), too.
When I moved duplicate player entity removal to
scriptclass::hardreset(), I also inadvertently made it so all non-player
entities got removed as well, even though this wasn't my intent. And
thus, pressing Enter to restart a time trial removes every entity except
the player, since it calls script.hardreset().
The time trial script.hardreset() is bad for other reasons (see #367),
however it's still a good idea to reset only what's needed in
script.hardreset().
So you get a trophy and achievement for completing the game in Flip
Mode. Which begs the question, how does the game know that you've played
through the game in Flip Mode the entire way, and haven't switched it
off at any point? It looks like if you play normally all the way up
until the checkpoint in V, and then turn on Flip Mode, the game won't
give you the trophy. What gives?
Well, actually, what happens is that every time you press Enter on a
teleporter, the game will set flag 73 to true if you're NOT in Flip
Mode. Then when Game Complete runs, the game will check if flag 73 is
off, and then give you the achievement and trophy accordingly.
However, what this means is that you could just save your game before
pressing Enter on a teleporter, then quit and go into options, turn on
Flip Mode, use the teleporter, then save your game (it's automatically
saved since you just used a teleporter), quit and go into options, and
turn it off. Then you'd get the Flip Mode trophy even though you haven't
actually played the entire game in Flip Mode.
Furthermore, in 2.3 you can bring up the pause menu to toggle Flip Mode,
so you don't even have to quit to circumvent this detection.
To fix both of these exploits, I moved the turning on of flag 73 to
starting a new game, loading a quicksave, and loading a telesave (cases
0, 1, and 2 respectively in scriptclass::startgamemode()). I also added
a Flip Mode check to the routine that runs whenever you exit an options
menu back to the pause menu, so you can't circumvent the detection that
way, either.
For some reason, there were just exact duplicates of the talkgreen_2
script and alarmon/alarmoff commands. I have no idea why, but cppcheck
identified them.
There's no need to use a template here. Just manually call SDL_tolower()
or SDL_toupper() as needed.
Oh yeah, and use SDL_tolower() and SDL_toupper() instead of libc
tolower() and toupper().
They're always fixed-size anyways, there's no need for them to be
vectors.
Also used the new INBOUNDS_ARR() macro for the map.explored bounds
checks in Script.cpp, and made map.explored a proper bool array instead
of an int array.
Since they're always fixed-size, they don't need to be dynamically-sized
vectors.
entityclass::customcrewmoods is now a proper bool instead of an int now,
and I replaced the hardcoded constant 6 with a static const int Game
attribute to make it easier to change.
It could index the `words` array out-of-bounds if there were more than
40 arguments in a command. Not like that would ever happen, but it's
still good to be sure.
This was caused by the fact that not all unlocks were done through the
Game::unlocknum() function. Some just set the unlock number directly.
But it's fixed now.
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.
This commit removes all global args from the parameters of each function
on the scriptclass object, and updates all places they are called
accordingly. It also changes all instances of 'dwgfx' to 'graphics' in
Script.cpp.
Interestingly enough, it looks like editor.h depended on Script.h's
class define of the musicclass. I've temporarily placed the class define
in editor.h, but by the end of this patchset it'll be gone.
This commit removes the global args being passed around from the
function args on the mapclass object, as well as updating all callers in
other files to not have those args. Furthermore, 'dwgfx' has been
renamed to 'graphics' in Map.cpp.
This commit removes all global args from functions on the entityclass
object, and updates the callers of those functions in other files
accordingly (most significantly, the game level files Finalclass.cpp,
Labclass.cpp, Otherlevel.cpp, Spacestation2.cpp, WarpClass.cpp, due to
them using createentity()), as well as renaming all instances of 'dwgfx'
in Entity.cpp to 'graphics'.
I've decided to call dwgfx/game/map/obj/key/help/music the "global args".
Because they're essentially global variables that are being passed
around in args.
This commit removes global args from all functions on the Game class,
and deals with updating the callsites of said functions accordingly. It
also renames all usages of 'dwgfx' in Game.cpp to 'graphics', since the
global variable is called 'graphics' now.
Interesting to note, I was removing the class defines from Game.h, but
it turns out that Graphics.h depends on the mapclass and entityclass
defines from Game.h. And also Graphics.h spelled mapclass wrong (it
forgot the "class") so I just decided to use that existing line instead.
This is only temporary and after all is said and done, at the end of
this pull request those class defines will be gone.
This is a refactor that turns the script-related arrays `ed.sb`, and
`ed.hooklist` into C++ vectors (`script.commands` was already a vector, it was
just misused). The code handling these vectors now looks more like idiomatic
C++ than sloppily-pasted pseudo-ActionScript. This removes the variables
`script.scriptlength`, `ed.sblength`, and `ed.numhooks`, too.
This reduces the amount of code needed to e.g. simply remove something from
any of these vectors. Previously the code had to manually shift the rest of
the elements down one-by-one, and doing it manually is definitely error-prone
and tedious.
But now we can just use fancy functions like `std::vector::erase()` and
`std::remove()` to do it all in one line!
Don't worry, I checked and `std::remove()` is in the C++ standard since at least
1998.
This patch makes it so the `commands` vector gets cleared when
`scriptclass::load()` is ran. Previously, the `commands` vector never actually
properly got cleared, so there could potentially be glitches that rely on the
game indexing past the bounds set by `scriptlength` but still in-bounds in the
eyes of C++, and people could potentially rely on such an exploit...
However, I checked, and I'm pretty sure that no such glitch previously existed
at all, because the only times the vector gets indexed are when `scriptlength`
is either being incremented after starting from 0 (`add()`) or when it's
underneath a `position < scriptlength` conditional.
Furthermore, I'm unaware of anyone who has actually found or used such an
exploit, and I've been in the custom level community for 6 years.
So I think it's fine.
The problem with flipme() was that it WAS properly reflecting
("reflecting" as in mirroring over a given line) the text box over the
line y=120, BUT it forgot to account for the height of the text box.
Thus, the text box position would be off by the length of its own
height. And when the text box got taller, this offset would worsen and
worsen.
The game uses magic values x=-500 and y=-500 to indicate when a text box
should be centered horizontally or vertically. It does this for x=-1
too, but it's buggy because it only looks at the first line of the text
box to center it. In this commit I fix it so that it will look at all of
the lines of the text box to center it instead.
This fixes a bug where if warpdir() was used during in-editor
playtesting, the changed warp direction would persist even when leaving
playtesting.
This would be very annoying to correct back every time you playtested
and warpdir() was used, so I've added some kludge to store the actual
warp direction of each room when entering playtesting, and then set the
warp directions back when leaving playtesting.
This has two benefits:
(1) The game uses less resources when it is asked to gotoroom to the
same room because it is no longer redrawing the warp background
every single frame, which is very wasteful.
(2) The warp background no longer freezes or flickers if the player is
standing inside a gotoroom script box (which calls gotoroom every
frame or every other frame, because every time the gotoroom happens
the script box gets reloaded).
I ran the game through Valgrind to catch various issues.
One thing caught my attention -- map.resumedelay. This is
used by The Tower/etc to add a delay before the game is
resumed (probably for camera controls delaying it) for
spawning the player. This variable was uninitialized. Notably,
if I explicitly set it to 592851 or similar, it reproduces the
bug, even if I cannot reproduce it with my typical compilation
flags. So fixing this by initializing it to 0, alongside some
other Valgrind warnings, should fix the problem entirely.
Previously, if it was called in a custom level, it would say "out of
Twenty" no matter what, even if the level had zero trinkets. This commit
fixes that.
This fixes an issue where you could softlock the game if you exited
playtesting mode by pressing Esc while a "- Press ACTION to advance text
-" prompt was onscreen, then re-entered playtesting and then started any
script.
The "- Press ACTION to advance text -" prompt is most directly
controlled by game.advancetext, but when it appears game.pausescript is
usually set as well. You need to have both variables on at the same time
in order to press ACTION. If only advancetext is on, then you won't be
able to flip but can still move around, but if only pausescript is on,
then the game softlocks because the only way you can turn pausescript
off is by pressing ACTION, but the game will only let you press ACTION
if *both* advancetext and pausescript are on.
This fixes a bug in the editor where if you had cutscene bars active
while exiting playtesting, when you re-entered playtesting, the bars
would do their closing animation even though they should be already
gone.
If you're in the room being targeted when a `warpdir()` command is run,
and it sets the warp direction to none (warp dir 0), and if you're in a
Lab or Warp Zone room, the background will be set to the wrong one,
because it will always set the background to the stars-going-left
background, instead of setting it to the Lab background or the
stars-going-up background.
However, this is only a visual glitch, and is a temporary one, because
if you manage to trigger a `gotoroom()` on the room, it will get set to
its proper background.
This commit makes it so if you're in a Lab room, the background gets set
to the Lab background, and if you're in a Warp Zone room, the background
gets set to the stars-going-up background.
There are three map-related vectors that need to be reset in hardreset:
`map.roomdeaths`, `map.roomdeathsfinal`, and `map.explored`. All three
of these vectors use the concatenated rows system, whereby each room is
given a room number, calculated by doing X + Y*20, and this becomes
their index in each vector.
There's a double-nested for-loop to handle resetting all of these, where
the outer for-loop iterates over the Y-axis and the inner for-loop
iterates over the X-axis, but only `map.explored` is properly reset.
That's because it's the only vector that properly indexes using X +
Y*20. The other vectors reset using X, so previously, only the first row
of `map.roomdeaths` and `map.roomdeathsfinal` got reset, corresponding
with only the first row of rooms in both Dimension VVVVVV and Outside
Dimension VVVVVV getting reset.
This commit makes sure that both get reset properly.
There are two main problems with flipgravity():
1. It doesn't work for an already-flipped crewmate.
2. It doesn't work on the player.
This commit addresses both of those issues.
Resizing the vector does the same thing that the loops did, it changes
the size for the vector and initializes it with default-constructed
elements (or 0 or its equivalent for POD types).
Where a specific value is needed, it is set with the second
parameter of resize().
