Two problems: the fRandom() range was from 0..36, but that's 37
characters, not 36. And the check to sort the lower 26 values into the
Latin alphabet used a 'lesser-than-or-equal-to 26' check, even though
that checks for the range of values of 0..26, which is 27 letters, even
though the alphabet only has 26 letters. So just drop the equals sign
from that check.
It was checking for .vvv-mnt-temp-XXXXXX/LEVELNAME.vvvvvv instead of
LEVELNAME.vvvvvv. When PhysFS enumerates the folder, it only gives us
LEVELNAME.vvvvvv, and not .vvv-mnt-temp-XXXXXX/LEVELNAME.vvvvvv.
This fixes a regression that desyncs my Nova TAS after re-removing the
1-frame input delay.
Quick stopping is simply holding left/right but for less than 5 frames.
Viridian doesn't decelerate when you let go and they immediately stop in
place. (The code calls this tapping, but "quick stopping" is a better
name because you can immediately counter-strafe to stop yourself from
decelrating in the first place, and that works because of this same
code.)
So, the sequence of events in 2.2 and previous looks like this:
- gameinput()
- If quick stopping, set vx to 0
- gamerender()
- Change drawframe depending on vx
- gamelogic()
- Use drawframe for collision (whyyyyyyyyyyyyyyyyyyyyyyyyyyy)
And now (ignoring the intermediate period where the whole loop order was
wrong), the sequence of events in 2.3 looks like this:
- gamerenderfixed()
- Change drawframe depending on vx
- gamerender()
- gameinput()
- If quick stopping, set vx to 0
- gamelogic()
- Use drawframe for collision (my mind has become numb to pain)
So, this means that all the player movement stuff is completely the
same. Except their drawframe is going to be different.
Unfortunately, I had overlooked that gameinput() sets vx and that
animateentities() (in gamerenderfixed()) checks vx. Although, to be
fair, it's a pretty dumb decision to make collision detection be based
on the actual sprites' pixels themselves, instead of a hitbox, in the
first place, so you'd expect THAT to be the end of the dumb parade. Or
maybe you shouldn't, I don't know.
So, what's the solution?
What I've done here is added duplicates of framedelay, drawframe, and
walkingframe, for collision use only. They get updated in gamelogic(),
after gameinput(), which is after when vx could be set to 0.
I've kept the original framedelay, drawframe, and walkingframe around,
to keep the same visuals as closely as possible.
However, due to the removal of the input delay, whenever you quick stop,
your sprite will be wrong for just 1 frame - because when you let go of
the direction key, the game will set your vx to 0 and the logical
drawframe will update to reflect that, but the previous frame cannot
know in advance that you'll release the key on the next frame, and so
the visual drawframe will assume that you keep holding the key.
Whereas in 2.2 and below, when you release a direction key, the player's
position will only update to reflect that on the next frame, but the
current frame can immediately recognize that and update the drawframe
now, instead of retconning it later.
Basically the visual drawframe assumes that you keep holding the key,
and if you don't, then it takes on the value of the collision drawframe
anyway, so it's okay. And it's only visual, anyway - the collision
drawframe of the next frame (when you release the key) will be the same
as the drawframe of the frame you release the key in 2.2 and below.
But I really don't care to try and fix this for if you re-enable the
input delay because it's minor and it'd be more complicated.
In the past, people have reported having glitched levels where they
can't get the trinket star or can't complete the level because the
number of trinkets or crewmates is one higher than what can be obtained
in the level.
How did this happen? Well, it turns out that if you place an entity, and
then resize the level to be smaller, that entity still exists. This is
inconsequential for most entities, but if the entity is a trinket or
crewmate, that entity is still counted towards the number of trinkets or
crewmates in the level.
One fix would be to just remove entities whenever the level is
downsized, but then if someone accidentally downsizes the level and
wants to go back, that entity will be gone. Plus, it would be
inconsistent with tiles, because tiles don't get removed when you
downsize the level. Also, it wouldn't fix existing levels where people
have managed to place trinkets or crewmates out of bounds.
So instead, ed.numtrinkets() and ed.numcrewmates() should simply ignore
trinkets and crewmates that are outside the playable area. That way,
levels with glitched trinkets and crewmates can still be completed, and
can still be completed with the trinket star.
This fixes a regression where you're unable to activate activity zones
in in-editor playtesting if your interact button is not separate from
the map button.
When I originally did #743, I didn't have an option to set the bind to
be non-separate, so I removed this logic without adding a
game.separate_interact check. But when I added the option, I overlooked
this code, and so this regression happened. Whoops.
Not every music path will trip the quick_fade bool that resets the timer to
500ms, so we need to do this as soon as it's asked of us. This fixes the fade
when quitting to the main menu.
Fixes#764
Without this you end up with two problems:
- Fades will start past their fade time, causing it to just not fade at all
- Fades will start in the middle of their fade time, causing dramatic changes
in volume that are unintentional
The fade system already preserves the volume that music is playing during a
previous fade, so we can always reset the timer and get a good result.
Part of #764
This fixes one of two desyncs in my Nova TAS.
The problem is that by adding two frames of edge-flipping to vertically
moving platforms, Viridian's framedelay is updated for one extra frame
after they step off of a vertically-moving platform. This then messes up
Viridian's drawframe for the rest of the TAS until they die in a
drawframe-sensitive trick.
The solution here is to only set the visual onroof/onground to 1
instead. The logical onroof/onground is still 2, so players still have
two frames of edge-flipping off of vertically-moving platforms - it just
won't really look like it (not that you could easily tell anyway).
- use fseeko and ftello like FreeBSD in tinyxml2
- use current directory as basePath if NULL (OpenBSD doesn't actually support this feature it is disabled via a patch in their ports)
In order to help players spot the difference between outlined text and
non-outlined text, we now outline the text outline text itself (if text
outline is enabled, of course). But drawing the outline alone doesn't
stand out enough, so we have to draw a solid backing against the text as
well, in order to properly show the contrast.
This fixes a regression where you're able to start flipped by restarting
and then holding ACTION.
This happens because when the game resets all variables, it turns
hascontrol back on (because of hardreset()). However, this is handled in
the input function, and it's handled before player input is handled, so
the player is able to get 1 frame of being able to flip after a time
trial resets.
Why didn't this happen in 2.2? Because resetplayer() in 2.2 would set
lifeseq to 10, as if the player had died. However, this is inconsistent,
because loading in to the game for the first time would not result in a
lifeseq of 10. So, in 2.2, restarting the time trial would remove that 1
frame of being able to flip because of lifeseq, while 2.3 doesn't set
lifeseq because the player hasn't died.
I could have fixed this by setting lifeseq in the time trial restart
code, but I decided to just set hascontrol to false instead.
Fixes#770.
In earlier 2.3, if the roomname was empty, Dimension VVVVVV was used
instead. However, instead of doing that, it's better to just use the
hiddenname instead. Both because it's less hardcoded, and some rooms
have hidden names that aren't Dimension VVVVVV.
This makes the text much more readable against certain backgrounds (if
you have text outline enabled), especially against the Warp Zone
background (when you start in "This is how it is").
If you enter the Secret Lab from the title screen, all rooms will be
explored. However, if you enter the Secret Lab via the Secret Lab
entrance cutscene (epilogue), not all rooms will be explored, which is
inconsistent.
To do this, just do an SDL_memset() for the entersecretlab script
command.
SDL_memset() conveys intent better and is snappier than using a
for-loop. Also, using SDL_memset() to explore all rooms is more
future-proof, in case the size of map.explored were to change in the
future, and it's more conducive to optimization.
However, the `i` variable has to be explicitly set because it was
previously used here, but it's much better that it's explicitly set here
rather than being subtlely hidden in the inner for-loop initialization.
This is more future-proofing than anything else. The position of the
indicators is just the x-position of the gravitron square divided by 10,
but the gravitron squares will always only ever move at 7 pixels per
frame - so the distance an indicator travels on each frame will only
ever be at most 1 pixel. But just in case in the future gravitron
squares become faster than 10 pixels per frame, their indicators will be
interpolated as well.
When rollcredits is ran during in-editor playtesting, all unsaved data
is lost. To prevent this, just return to the editor if rollcredits is
ran, with a note saying "Rolled credits".
The text box drawn at the bottom of the map screen isn't wide enough, so
it's possible to see the corners on the right side of the text box if
you have custom graphics like I do.
The solution is to increase the width of the text box by one tile.
The game automatically writes settings to disk after any other setting
is changed, so it should do the same whenever the user changes
controller keybinds.
For consistency, the Viridian squeak will now play whenever you start
editing a level description field, or finish editing it (either by
pressing Esc or Enter).
If a level zip is named LEVELNAME.zip, the level file inside it must
also be named LEVELNAME.vvvvvv, else custom assets won't work.
This is because when we mount the zip file, we simply add
LEVELNAME.vvvvvv to the levels directory. Then whenever we load
LEVELNAME.vvvvvv, we look at the filename, remove the extension, and
look for the assets inside the zip of the same name, LEVELNAME.zip.
As a result, if someone were to make a level zip with assets but
mismatch the filename, the assets wouldn't load. Furthermore, if someone
were to add extra levels in the same zip, they wouldn't have any assets
load for them as well, which could be confusing.
To make things crystal-clear to the user, we now filter out any zips
that have incorrect structures like that, and print a message to the
terminal. Unfortunately nothing gets shown for non-terminal users, but
at least doing this and filtering out the zips is less confusing than
letting them through but with the issues mentioned above.
FILESYSTEM_mountAssets() has a big comment describing the magic numbers
needed to grab FILENAME from a string that looks like
"levels/FILENAME.vvvvvv".
Instead of doing that (and having to write a comment every time the
similar happens), I've written a macro (and helper function) instead
that does the same thing, but clearly conveys the intent.
I mean, just look at the diff. Using VVV_between() is much better than
having to read that comment, and the corresponding SDL_strlcpy().
This is so it can be reused without having to copy-paste.
generateBase36() is guaranateed to completely initialize and
null-terminate the buffer that is passed in.
This fixes a bug where the player's y-position would be incorrect if
they loaded a save that was on a conveyor and it was their first time
loading in since the game was opened.
This is because on the first load, the game creates a new player entity,
but on subsequent loads, the game re-uses the player entity. Subsequent
loads use mapclass::resetplayer(), which already has the newxp/newyp
fix, but as for the first time, the game does not set newxp/newyp.
So just set newxp/newyp, like in mapclass::resetplayer().
Upon further discussion it was decided to keep the soundtrack as originally
shipped, instead of changing it after the fact.
This reverts commit cf51379097.
There is a pattern in the Super Gravitron that is meant to "staircase",
similar to the Gravitron in Intermission 2. Something like:
[]
[]
[]
[] []
[] []
Unfortunately, due to an oversight, this pattern can only ever produce 1
square or 4 squares, which look out of place.
Both gravitrons are state machines (of course). States 20 and 21 in the
Super Gravitron are this staircase pattern (state 20 spawns the squares
on the left, state 21 spawns the squares on the right).
The only way states 20 and 21 can be reached is through state 1, and the
only way state 1 can be reached is through state 3. The only way state 3
can be reached is through states 28, 29, 30, and 31.
In states 20 and 21, the variable used to keep track of the amount of
squares spawned is swnstate4. However, states 28, 29, 30, and 31 all end
up using swnstate4, and at the end of states 28 and 29, swnstate4 will
be 7, and at the end of states 30 and 31, swnstate4 will be 3. This
means if we go to states 20 and 21 after coming from states 28 and 29,
we will only get 1 square, and if we go to states 20 and 21 after coming
from states 30 and 31, we will only get 4 squares.
This can be clearly filed under a failure to reset appropriate state.
What's the solution here? Just reset swnstate4 in state 3, so there will
be 7 squares, as intended. This also fixes the bug for state 22 as well,
which is affected in the same manner.
This fixes an oversight that could lead to confusion by the player.
showtargets is the variable that shows all unexplored teleporters on the
map as a question mark, so players know where to head to to make
progress. However, it previously was not directly saved to the main game
file. Instead, it would be set to true if flag 12 was turned on in the
save file.
How well does flag 12 correlate with showtargets?
Well, the script that turns on showtargets (bigopenworld and
bigopenworldskip) doesn't turn it on. Neither does completing Space
Station 1.
This flag is only turned on when the player activates Violet's activity
zone for the first time.
Therefore, it's entirely possible that a new player could complete Space
Station 1, then save their game, and come back to resume playing later.
When they do come back, the question marks that Violet told them about
won't show up on the minimap, and they'll be confused. They may not know
where to go.
And it is completely unintuitive for them to know that in order to get
the question marks to show up again, they have to not only talk to
Violet, but then save the game again, and reload the save. Especially
since the question marks only show up after you reload the save, and not
when you talk to Violet (because flag 12 is only a proxy for
showtargets, not the actual variable itself).
So what's the solution? Just save showtargets to the save file directly.
If you have invincibility enabled, the tower camera behavior is
inconsistent.
In ascending towers, you can "push" the camera upwards; however you
cannot push it downwards; at least it stays still when it comes up to
you if you stay still. In descending towers, the camera moves quicker
when you're at the bottom of the screen, but it's slower than your
falling speed and quickly loses sight of you; the camera can be pushed
upwards; unfortunately it also does a "bumping" motion if you're
standing still when the camera reaches you, which gets real annoying and
isn't particularly pleasant to look at.
There are two problems, so this does two fixes:
1. Pushing the camera now applies the appropriate counter-offset
depending on the direction of the tower. You can now push the camera
downwards in ascending towers.
2. To fix the "bumping" when the camera reaches you if you stand still,
there are now a 8-pixel-high "gray areas" at the top and bottom of
the screen where the camera simply won't move if you're in them.
Doing these camera offsets instead of simply canceling the movement if
the player is offscreen is a bit ugly... but it works for now.
This is a lot of copy-pasted code, but a little bit of copy-pasting
never hurt anyone...
The keybind to interact with activity zones and teleporters is now
separate from the keybind to open the map, or return to the editor from
in-editor playtesting, or restart a time trial. The keybind is now E,
and the default controller bind is X. No controller button prompts, but
the game didn't have controller button prompts anyways, so whatever.
Doing this now because if people's muscle memory are going to be broken
by not being able to spam the map keybind anymore, at least we can help
a bit by changing the keybind so they can keep spamming it - their
muscle memory is going to be broken anyways.
This option has to be enabled by going to the speedrunner menu options
and selecting "interact button". It is disabled by default.
All prompt text needs to be string-interpolated every time they are
drawn, because it is possible for people to change which interact button
they use in the middle of gameplay, via the in-game options.
Closes#736.
Colors in over-30-FPS mode shouldn't be updating every deltaframe;
mostly to ensure determinism between switching 30-mode and over-30 mode.
I'm going to overhaul RNG in 2.4 anyway, but right now I'm going to fix
this because I missed it.
The RNG of each special text box is stored in a temporary variable on
the text box itself, and only updated if the color uses it (hence the
big if-statement). Lots of code duplication, but this is acceptable for
now.
After the dimension destabilizes, the song that plays is Positive Force.
Which has already been played twice in the game at that point (first in
Tower, then in the Gravitron). Since Piercing the Sky is unused, why not
play a song that the player hasn't heard before? It would also be
musically fitting for the scenario.
The song gets played in two places - one for if you have cutscenes
enabled, and one for if you don't - so we just need to change both of
them.
I asked Terry in Discord DMs if he wanted this change and he approved of
it.
If you have completed No Death Mode, and entered the Master of the
Universe trophy room in the Secret Lab in over-30-FPS mode, it would
appear to start at one position before quickly zipping to another during
the deltaframes.
This is because it updates its position after the initial assignments of
lerpoldxp/lerpoldyp in entityclass::createentity().
Other entities do this too, and what's been done for them is to
copy-paste the lerpoldxp/lerpoldyp updates alongside the xp/yp updates.
However, instead of single-case patching this deltaframe glitch, I've
opted to fix ALL cases by simply moving the lerpoldxp/lerpoldyp
assignments to the end of the function, guaranteeing that all entities
that update their position after the initial assignment in the function
will not have any deltaframe glitches.
Of course, there's still the duplicate lerpoldxp/lerpoldyp updates in
entityclass::updateentities()... I'm not sure what to do about those.
If you had Flip Mode enabled when exiting from in-game options, the game
would flash the in-game options menu as flipped for 1 frame before
returning to the pause menu.
To fix this, just defer the Flip Mode variable assignment to be done at
the end of the frame.
This is a small quality-of-life fix in the same vein as allowing the
player to press Esc in the teleporter menu (which they weren't able to
do in 2.2, either).
This fixes the finalstretch tile shifting persisting if you return to
the main dimension and final_colormode isn't reset properly.
It's possible to do so in the main game by using a teleporter in
finalmode while having the Intermission 1 or 2 companion active.
For custom levels, level makers can make a setup that automatically
turns on finalstretch, goes to finalmode, and then returns to the main
dimension. The only thing being... as a level maker myself, this tile
shifting REALLY doesn't seem useful (and no one has ever used it because
the setup to do so hadn't really been found or documented until this
year). For one, the exact shift is randomized every time (there's an
fRandom() call to cycle the colors). For two, it goes away after the
player saves and reloads the level. And for three, it doesn't animate
like it does in finalmode (this is the biggest reason IMO).
Nevertheless, I've decided to keep support for this in custom levels, in
case someone in the future does want to use it and is okay with the
limitations.
There's a bit of inconsistency with how long each color lasts for during
final stretch. Initially, each color lasts for 40 frames, but when you
enter either of the minitowers, the color switches to lasting for 15
frames only. This is because a final_colorframe of 1 makes it go for 40
frames, but a final_colorframe of 2 makes it go for 15 frames - and
final_colorframe gets set to 2 whenever you enter a minitower.
This seems like an oversight because (1) final_colorframe doesn't affect
anything inside the minitower, (2) final_colorframe doesn't get saved to
the save file and always gets set to 1 if your save file has
finalstretch set to true, so saving and reloading will set the colors
back to 40 frames each, and (3) final_colorframe doesn't get set back to
1 when leaving the minitowers.
When you enter the Super Gravitron, you have to wait until the Super
Gravitron actually starts before being able to press Enter to return to
the Secret Lab. This is annoying if you just want to get back to the
Secret Lab. So, I've made it so the press-Enter-to-return functionality
is enabled from the moment that the Super Gravitron starts.
It turns out, despite the game attempting to prevent you from using
invincibility or slowdown in the Super Gravitron by simply preventing
you from entering the Secret Lab from the menu, it's still possible to
enter the Super Gravitron with it anyways. Just have invincibility or
slowdown (or both!) enabled, enter the game normally, and talk to
Victoria when you have 20 trinkets, to start the epilogue cutscene.
Yeah, that's a pretty big gaping hole right there...
It's also possible to do a trick that speedrunners use called
telejumping to the Secret Lab to bypass the invincibility/slowdown
check, too.
So rather than single-case patch both of these, I'm going to fix it as
generally as possible, by moving the invincibility/slowdown check to the
gamestate that starts the Super Gravitron, gamestate 9. If you have
invincibility/slowdown enabled, you immediately get sent back to the
Secret Lab. However, this check is ignored in custom levels, because
custom levels may want to use the Super Gravitron and let players have
invincibility/slowdown while doing so (and there are in fact custom
levels out in the wild that use the Super Gravitron; it was like one of
the first things done when people discovered internal scripting).
No message pops up when the game sends you back to the Secret Lab, but
no message popped up when the Secret Lab menu option was disabled
previously in the first place, so I haven't made anything WORSE, per se.
A nice effect of this is that you can have invincibility/slowdown
enabled and still be able to go to the Secret Lab from the menu. This is
useful if you just want to check your trophies and leave, without having
to go out of your way to disable invincibility/slowdown just to go
inside.
This factors out the slowdown and invincibility conditionals to a
function. This means less copy-pasted code, and it also conveys intent
(that we don't want to allow competitive options if we have either of
these cheats enabled).
This function isn't implemented in the header because then we would have
to include Map.h for map.invincibility, and transitive includes are
evil. Although, map.invincibility ought to be on Game instead (it was
only mapclass due to 2.2-and-previous argument passing), but that's a
bunch of variable reshuffling that can be done later.
They are now factored out to an inline function named incompetitive().
This is so their usage can be changed without having to change each
individual one in every place. This also clarifies the intent of using
these conditionals (they are for when we're in a "competitive" mode).
Tower backgrounds have a bypos and bscroll. bypos is just the y-position
of the background, and bscroll is the amount of pixels to scroll the
background by on each frame, which is used to scroll it (if it's not
being redrawn) and for linear interpolation.
For the tower background (and not the title background), bypos is
map.ypos / 2, and bscroll is (map.ypos - map.oldypos) / 2. However,
usually bscroll gets assigned at the same time bypos is incremented or
decremented, so you never see that calculation explicitly - except in
the previous commit, where I worked out the calculation because the
change in y-position isn't a known constant.
Having to do all these calculations every time introduces the
possibility of errors where you forget to do it, or you do it wrongly.
But that's not even the worst; you could cause a linear interpolation
glitch if you decide to overwrite bscroll without taking into account
map.oldypos and map.ypos.
So that's why I'm adding a function that automatically updates the tower
background, using the values of map.oldypos and map.ypos, that is used
every time map.ypos is assigned. That way, we have to write less code,
you can be sure that there's no place where we forget to do the
calculations (or at least it will be glaringly obvious) or we do it
wrongly, and it plays nicely with linear interpolation. This also
replaces every instance where the manual calculations are done with the
new function.
If you have invincibility enabled and push the camera, the background
would smear. This is because the game doesn't calculate the proper
bscroll and bypos of the tower background, and also doesn't end up
redrawing it.
We do both these things now, so this is fixed.
These places didn't assign map.oldypos when they assigned map.ypos. This
could have only resulted in visual glitches, but it's good to be
consistent and proactively fix these.
This fixes issues where they would be silent for 1 frame due to frame
ordering, resulting in a weird-sounding beginning of these tracks due to
a lack of attack (in the musical sense).
This is similar to the issue where tracks fading in would suddenly be
loud for 1 frame, again due to frame ordering.
This fixes issues with music playing, only for it to fade out
afterwards. This happened if tracks 0 or 7 were played after fading out,
because playing other tracks reset the fade booleans (by calling a
fade-in), but not tracks 0 or 7.
The previous fade system used only one variable, the amount of volume to
fade per frame. However, this variable was an integer, meaning any
decimal portion would be truncated, and would lead to a longer fade
duration than intended.
The fade per volume is calculated by doing MIX_MAX_VOLUME / (fade_ms /
game.get_timestep()). MIX_MAX_VOLUME is 128, and game.get_timestep() is
usually 34, so a 3000 millisecond fade would be calculated as 128 /
(3000 / 34). 3000 / 34 is 88.235..., but that gets truncated to 88, and
then 128 / 88 becomes 1.454545..., which then gets truncated to 1. This
essentially means 1 is added to or subtracted from the volume every
frame, and given that the max volume is 128, this means that the fade
lasts for 128 frames. Now, instead of the fade duration lasting 3
seconds, the fade now lasts for 128 frames, which is 128 * 34 / 1000 =
4.352 seconds long.
This could be fixed using floats, but when you introduce floats, you now
have 1.9999998 problems. For instance, I'm concerned about
floating-point determinism issues.
What I've done instead is switch the system to use four different
variables instead: the start volume, the end volume, the total duration,
and the duration completed so far (called the "step"). For every frame,
the game interpolates which value should be used based on the step, the
total duration, and the start and end volumes, and then adds the
timestep to the step. This way, fades will be correctly timed, and we
don't have potential determinism issues.
Doing this also fixes inaccuracies with the game timestep changing
during the fade, since the timestep is only used in the calculation
once at the beginning in the previous system.
To exclude gravitron squares, the game excluded all entities whose
`size` was 12 or higher. The `size` of the player when they transform
into VVVVVV-Man is 13.
We have already inadvertently fixed VVVVVV-Man not warping vertically in
2.2. This was done with the previous room transition/warping code
refactors; the gravitron square conditionals were simply excluded from
the vertical warp code, because there's no situation where there would
ever be a gravitron square outside the screen vertically.
As with making rescuable crewmates warpable, I have yet to ever see
people use VVVVVV-Man in a custom level. It's not like they would want
to use it anyway; VVVVVV-Man is really, really buggy. And it's probably
better to make it less buggy, starting with this commit.
That being said, VVVVVV-Man's collision when warping horizontally is
really janky, so I still wouldn't use it.
The game excluded every entity whose `type` was 50 or higher. The `type`
of rescuable crewmates is 55.
Could some levels be broken by this behavior? Unlikely; without warping,
the crewmates would end up falling out of the room and would become
unrescuable. So this is more likely to fix than to break.
But more importantly, *no one knows that rescuable crewmates don't
warp*. If anyone would know, it would be me, because I've been in the
custom levels community for over 7 years - and yet, during that time, I
have not seen anyone run into this corner case. If they did, I would
remember! This implies that people simply have never thought about
putting rescuable crewmates in places where they would warp - or they
have, ran into this issue, and worked around it.
With those two reasons, I'm comfortable fixing this inconsistency.
This saves one indentation level. I also fixed the comments a bit
(multiline instead of single-line, "gravitron squares" instead of "SWN
enemies", also commented the player exclusion from horizontal wrapping
in vertically-wrapping rooms).
This fixes a bug where using the fullscreen toggle keybind (Alt+Enter,
Alt+F, or F11) wouldn't update the color of the "resize to nearest" menu
option. The color doesn't functionally change anything - the option
still won't work, and will still have the message telling you that you
need to be in windowed mode when you move your menu selection to it -
but it's an easy inconsistency to fix; just move the menu recreation in
to Screen::toggleFullScreen() itself.
The game dereferences graphics.screenbuffer without checking it first...
it's unlikely to happen, but the least we can to do be safe is to add a
check and assert here.
If there were two scripts with the same name, removing one of them would
only remove the other script from the script name list, and not also
remove the contents of said script - leading to a desync in state, which
is probably bad.
Fixing this isn't as simple as removing the break statement - I either
also have to decrement the loop variable when removing the script, or
iterate backwards. I chose to iterate backwards here because it
relocates less memory than iterating forwards.
No need to use it when good ol' loops work just fine.
Iterating backwards is correct here, in case there happen to be more
than one of the item in the vectors, and also to minimize the amount of
memory that needs to be relocated.
This is a simple change - we draw minimap.png, instead of the generated
custom map, if it is a per-level mounted custom asset.
Custom levels have already been able to utilize minimap.png, but it was
limited - they could do gamemode(teleporter) in a script, and that would
show their customized minimap.png, but it's not like the player could
look at it during gameplay.
I would have done this earlier if I had figured out how to check if a
specific asset was mounted or not.
Previously, if the game couldn't set the write dir to the base
directory, or couldn't make the base directory, or couldn't calculate
the base directory, it would probably dereference NULL or read from
uninitialized memory or murder your family or something. But now, I've
eliminated the potential Undefined Behavior from the code dealing with
the base path.
Previously, this function had a bug due to failing to account for array
decay. My solution was to just repeat the MAX_PATH again. But in
hindsight I realize that's bad because it hardcodes it, and introduces
the opportunity for an error where we update the size of the original
path but not the size in the function.
So instead, just pass the size through to the function.
I don't want to add too many asserts, because sometimes it's okay if a
file is missing (mmmmmm.vvv). But currently, the game basically expects
all images and sound effects to be present. That might change in the
future, but for now, these asserts are okay.
FILESYSTEM_loadFileToMemory() dereferenced pointers without checking if
they were valid... I don't know of any cases where they could have been
NULL, but better safe than sorry.
So, the codebase was kind of undecided about who is responsible for
initializing the parameters passed to FILESYSTEM_loadFileToMemory() - is
it the caller? Is it FILESYSTEM_loadFileToMemory()? Sometimes callers
would initialize one variable but not the other, and it was always a
toss-up whether or not FILESYSTEM_loadFileToMemory() would end up
initializing everything in the end.
All of this is to say that the game dereferences an uninitialized
pointer if it can't load a sound effect. Which is bad. Now, I could
either fix that single case, or fix every case. Judging by the title of
this commit, you can infer that I decided to fix every case - fixing
every case means not just all cases that currently exist (which, as far
as I know, is only the sound effect one), but all cases that could exist
in the future.
So, FILESYSTEM_loadFileToMemory() is now guaranteed to initialize its
parameters even if the file fails to be loaded. This is better than
passing the responsibility to the caller anyway, because if the caller
initialized it, then that would be wasted work if the file succeeds
anyway because FILESYSTEM_loadFileToMemory() will overwrite it, and if
the file fails to load, well that's when the variables get initialized
anyway.
My next commit will involve using goto to jump to the end of a function
to initialize the variables to NULL, but that results in a compiler
error if we have initializations in the middle of the function. We might
as well put all declarations at the top of each block anyway, to help
the move to C, so I'm doing this now.
Since the length variable in the STDIN block now overshadows the length
variable in the outer block, I've renamed the length variable in the
block to stdin_length.
These casts are sprinkled all throughout the graphics code when creating
and initializing an SDL_Rect on the same line. Unfortunately, most of
these are unnecessary, and at worst are wasteful because they result in
narrowing a 4-byte integer into a 2-byte one when they don't need to
(SDL_Rects are made up of 4-byte integers).
Now, removing them reveals why they were placed there in the first place
- a warning is raised (-Wnarrowing) that implicit narrowing conversions
are prohibited in initializer lists in C++11. (Notably, if the
conversion wasn't narrowing, or implicit, or done in an initializer
list, it would be fine. This is a really specific prohibition that
doesn't apply if any of its sub-cases are true.)
We don't use C++11, but this warning can be easily vanquished by a
simple explicit cast to int (similar to the error of implicitly
converting void* to any other pointer in C++, which works just fine in
C), and we only need to do it when the warning is raised (not every
single time we make an SDL_Rect), so there we go.
This fixes a bug where after loading in to the level editor, pressing
Esc and then switching your option to something other than the first
option, then pressing Esc again to close the menu, then pressing Esc
once more would not keep your menu option.
This is because the code that checks if Menu::ed_settings is already in
the stack doesn't account for if ed_settings is the current menu - the
current menu doesn't get put in to the stack.
In hindsight, maybe I could have designed the new menu system better so
the current menu IS on the stack, and/or should have used a
statically-allocated linked list for each menu name for the stack frames
(instead of an std::vector) and asserted if a menu that already existed
in the stack was created instead... that'll have to be done later,
though.
Pressing Esc to cancel the confirm quit menu didn't play the squeak, in
contrast to pressing ACTION to cancel it, so now it does; pressing Esc
to close the pause menu or pressing ACTION will also now play the
Viridian squeak too.
vx/vy mean x-velocity and y-velocity... except here, where it seems like
they're used as extra parameters that do different things depending on
the entity. But it seems like at one point they were actually meant to
be the speed of the entity (this is the case for the unused decorative
particle entities), and then just never got renamed when they weren't.
The custom levels community named these two parameters meta1 and meta2
in the reference list of entities for the createentity() script command,
so that's what I'm naming them here. This will avoid confusion (I know
that some people reading this function have genuinely mistaken the vx/vy
for actually meaning x-velocity and y-velocity, simply because they were
named that way).
I have spelled out each overloaded version instead, and only the
overloads that are actually used - which just happens to be everything
except the 8-argument one. I don't want to deal with callers right now
(there are too many of them), so I'm not going to change the names that
the callers use, nor do I want to change the amount of arguments any
existing callers use right now - but we will have to deal with them in
one way or another when we move to C.
The script command createentity() is always an int. But not only that,
every time createentity() is used, its arguments are always treated like
ints. Always. I knew that vx/vy were floats because of the int casts in
the function, but I didn't even realize that xp/yp were floats, too,
until I checked just now! That's how much they're treated like ints.
All int casts in createentity() have also been removed, due to being
unnecessary (either because of us suppressing MSVC implicit conversion
warnings, or because there are now no longer any conversions happening).
This boolean is assigned, and it is checked... but it's never assigned
to true, thus making it useless. I also checked 2.2 source and the same
thing happens there; to prevent any confusion, I'm removing this.
So... I did see that map.ypos was a float when I added over-30-FPS mode,
because map.oldypos wasn't there before... I'm guessing that I kind of
just ignored it at the time. But, c'mon, map.ypos and map.oldypos are
always treated as ints, so there's literally no reason for them to be
actually floats in reality. I didn't even know they were anything other
than ints until I checked Map.h.
This is quite simple - whenever the user uses their keyboard or
controller, we hide the mouse cursor. Whenever they move the mouse, we
show it again. This makes it so the cursor gets out of the way when they
play the game, but reappears when they need it.
There is also a timeout, to prevent strobing if the user decides to use
the keyboard/controller and mouse at the same time. There is no timeout
from hiding the mouse cursor, but there is a timeout from showing the
mouse cursor - this because it's okay if the mouse lingers for a few
frames when you start using the keyboard, but really annoying if the
mouse doesn't instantly appear when you move it.
The config option has been removed. I'm going to implement something
that automatically shows and hides the mouse cursor whenever
appropriate, which is better than a config option.
These are two C++ features that we don't need, don't use, and will never
use in the future. Apparently the best way of doing this in CMake is to
fiddle with the CXX_FLAGS using regex.
Now this is one less flag I need to supply myself when I invoke CMake...
This variable is not defined anywhere and never has been since the
source code release (which is when this CMake file was first created).
To make things clearer, I'm cleaning this variable up.
A function like add_definitions() adds definitions to ALL targets, not
just VVVVVV. This kind of namespace pollution is messy, and could result
in bugs if you pollute with the right kind of pollutant.
So instead of using add_definitions(), use target_compile_definitions().
And instead of using include_directories(), use
target_include_directories().
All the C third-party dependencies are C90, and all the C files we have
are also C90 (well, almost, but that's easily sorted). So we have
basically no reason to not go with C90 here.
The only wrinkle is, turning C extensions off for physfs-static results
in linker errors because PhysFS implicitly uses alloca() without
including it properly (on Linux). I am not the only one who has ran into
this - see https://icculus.org/pipermail/physfs/2020-April/001293.html -
and it's a bug with PhysFS. The workaround I've gone with is to enable C
extensions. (There might also be some funkiness with PhysFS's use of the
`inline` keyword, so enabling extensions will paper over that as well.)
So there were actually only two instances of C99-style end-of-line
comments in C files - and technically one of them was just a C file
including MakeAndPlay.h.
It seems like CMake 3.1.3 introduced the C/C++ standard properties,
while the minimum version of this CMake file is 2.8.12. So we do what
FAudio does, which is print a warning if the CMake version is too old
and otherwise use it if we have the feature.
They're the same thing, but using option() better conveys intent.
However this can't be done for anything that isn't a bool, which the
CUSTOM_LEVEL_SUPPORT option is not (it's a tri-state string).
These were introduced in 098fb77611 - did
Leo not know that they were already there at the top of the file? This
does the same thing, except it only sets it for VVVVVV instead of
everything (so this wouldn't set it for the third-party dependencies).
If a track was restarted after it faded out, then it wouldn't play. This
is because currentsong wasn't set to -1 after fading out, and that is
because the fade out calls pause() instead of haltdasmusik() when it
finishes.
Unlike f196fcd896, this fixes the time
trial music while keeping it to the same behavior as 2.2, and fixes
every single possible case that this music bug could have happened.
This reverts only a part of f196fcd896 -
as the original commit author did not do their changes atomically, they
also squashed in a de-duplication within the same commit. So I'm only
reverting the part of the commit that wasn't the de-duplication, which
is simply the changes to the music.fadeout() calls.
This is being (partially) reverted for several reasons:
1. It's not the correct behavior. What this does instead is persist the
track through after you restart the time trial, instead of fading it
out, then restarting it again. This is in contrast to behavior in
2.2, and I see no reason to not keep the same behavior.
2. It's a single-case patch. The time trials are not the only time in
the game a music track could fade out and then be restarted with the
same track - custom levels could do the same thing too. Instead of
fixing only one case, we should strive to fix EVERY case.
The original commit author (trelbutate) also didn't write anything in
the commit description of f196fcd896. What
you should write in the commit description is things like rationale,
analysis, and other good information that would be useful to anyone
looking at your commit to understand why you did what you did. Having no
commit description leaves readers in the dark as to why you did what you
did.
Thus, I don't know why trelbutate went with this solution, or if they
knew that it was only a single-case patching, or if they knew that it
wasn't 2.2 behavior.
By not writing the commit description, they miss a chance for
reflection; speaking from personal experience, I myself have gone back
and improved my commits countless times because I wrote commit
descriptions for every single one of them, and sometimes whenever I
write them, I think to myself "hang on a minute, that doesn't sound
quite right" and end up finding improvements.
If trelbutate wrote a commit description, they might have realized that
it wasn't 2.2 behavior, and gone back and fixed up their commit to be
correct. As it stands, though, they didn't have to think about it in the
first place because they never bothered to write a commit description.
edteleportent is a global variable that gets assigned whenever the
player collides with a warp token, and gets read from later down the
line in gamelogic(). While I don't know of any way to cause anything bad
with this (and I did try), storing a temporary indexing variable like
this is only bound to be a liability in the future - so we might as well
prevent badness now by adding a bounds check here.
This fixes a bug where quitting to the menu from command-line
playtesting with -playassets specified would always use those assets
when loading back in to any custom level. This also fixes loading in to
a custom level quicksave always using the command-line playtesting
arguments instead of using the actual quicksave.
In a vertically-warping room, the 'height' of the room becomes 232
pixels, regardless of if you have a room name or not. So the remaining 8
rows of pixels at the bottom of the screen corresponds with the first 8
rows of pixels at the top of the screen, and entities in the bottom 8
rows of pixels get teleported to the top of the screen.
The screen wrapping drawing code doesn't draw entities in the top 8 rows
of pixels at the bottom, leading to a discontinuous effect where it
looks like vertically-warping entities don't neatly change from the
bottom to the top or vice versa - this is especially noticeable with
enemies. To fix this, just increase the threshold for drawing top
entities at the bottom of the screen by 8 pixels.
When an entity vertically warps, it teleports upwards or downwards by
232 pixels. However, the graphics code draws them with an offset of 230
pixels. This is off by 2 pixels, but it's enough to make a
downwards-moving enemy look like it suddenly collides with the bottom of
the screen (in a room without a room name) before it warps, especially
if you go frame-by-frame.
It seems like for whatever reason that the frames portion of save files
is never read from, and always zeroed. Well, technically they get parsed
but the result is immediately discarded afterwards.
I see no reason to do this, so I'm removing these zeroes.
This fixes being able to make music fully fade in (or out) by unfocusing
the game, or making the fade bars fully fade in (or out) by unfocusing
the game, or racking up the timer while the game is unfocused.
In 2.2 and previous, the game would call resetgameclock() every frame
for the last 30 frames of the time trial countdown in order to make sure
it gets reset. This was in a render function, and didn't get brought out
in 2.3, so 2.3 resets the game clock *while rendering*, which is kinda
bad and is an oversight on my part for not noticing.
Instead of doing that, just add a conditional to the timer so that it
won't tick during the time trial countdown. This fixes#699 even further
by making it so the time trial par can't even be lost during the
countdown, because the timer won't tick up - so you can never get a sad
squeak to play by pausing the game or unfocus-pausing it during the
countdown.
For some reason, resetgameclock() is only ever used in gamerender(), and
everywhere else just zeroes the clock manually. This is weird to me, so
I've made it so everywhere that zeroes the clock uses the
resetgameclock() function to do so.
Otherwise, if the timer ticked up past the par (via using the unfocus
pause or pause menu), it would result in the sad squeak being played
every frame because the game would constantly be setting
timetrialparlost, then moving to the code block below, assuming that
since timetrialparlost that we haven't lost the par already, and playing
the squeak.
timetrialparlost gets reset in hardreset() and startgamemode() anyways,
so there's no need to be constantly resetting this variable.
Fixes#699.
It turns out this entire chunk of code is simply unneeded (and is
actively harmful) since when we're done with the time trial,
quittomenu() gets called, and that removes the previous stack frame
anyway.
I'm guessing that I added this code, then added quittomenu(), then
didn't consider how this code and quittomenu() would mix. But anyways,
this bug is fixed.
Fixes#714.
This seems to be a comment left by Ethan that he never got around to. So
I did it for him.
What I've done is made it so FileSystemUtils.cpp knows what a binary
blob is, and moved the binary blob loading code directly to
FileSystemUtils.cpp. To do this, I removed the private access modifier
from binaryBlob - I don't think we'll need it, and anyways when we move
to C we can't use it.
Along the way, I also cleaned up the style of the function a bit - the
null termination offset is no longer hardcoded, and the function no
longer mixes code and declarations together in the same block.
I also noticed that when printing all the filenames at the end, a single
invalid header would stop the whole loop instead of just being skipped
over... this seems to be a bug to me, so I've made it so invalid headers
just get skipped over instead of stopping the whole loop.
In FileSystemUtils.h, I used a forward declaration. In hindsight,
incomplete forward declarations should basically always be done in
header files if possible, otherwise this introduces the possibility of
transitive includes - if a file includes this header and it does a full
include, the file is silently able to use the full header, whereas if
it's a forward declaration, then the moment the file tries to use the
full header it fails, and then it's forced to include the full header
for itself. But uh, that's a code cleanup for later.
While fixing all the other music bugs, I discovered that starting
playtesting in the editor wouldn't play the level music.
The problem is that the editor playtesting start code calls
music.fadeout() before calling music.play(). This queues up the track
from the music.play() call. After that, what should happen is that
processmusic() processes the fade, the fade is then finished, and then
after that it sees that the music is halted so it can play the queued
track.
Instead what happens is that the function first attempts to play the
music before the fade is processed and finished, so play() will re-queue
the music again, but the queue gets cleared right after that (this is a
subtle bit of behavior - it means if the game fails to play a queued
track due to it fading, it's not going to re-queue it again and end up
in some sort of infinite loop).
This is a frame ordering issue - the function is tripping over itself
when it shouldn't be. To fix it, just put the queue processing code
after the fade processing code.
This fixes the 2.2-and-below music blocking workaround not working in
2.3.
The issue was that when the music got halted by the script, the fade
volume would still be processing, silently being decremented in the
background. So the script playing the track afterwards would make the
game queue it (as it was called during the fade), but then the music is
halted so the game would attempt to play it, but the fade is STILL
happening so it wouldn't actually play it and would attempt to queue the
track again.
However, that queue gets discarded immediately afterwards because the
music.play() call happened inside the code responsible for playing the
queued music, and that code unconditionally clears the queue variables
immediately after calling play(). So that's good to know - if the game
queues a song, but fails to play it because of a fade, it's not going to
immediately re-queue it and potentially get stuck in a loop of
infinitely queueing the same song over and over again each frame.
Anyways, the source of the problem is not resetting the fade booleans
when halting music, so I've reset them.
Fixes#701.
The problem here is that even though we start playing the music when the
volume is set to zero, mixer's state doesn't have volume zero, so
whatever it plays next will be the very first quanta of the track but at
the previous volume (in this case, the maximum volume). To fix this,
just update mixer when we update the volume here - it's okay to not
account for user volume because it ends up being zero anyway.
Fixes#710.
This fixes a bug where fading music in but not going through the
music.play() path wouldn't start the fade volume from zero. If this
happened, then the previous volume would persist, and if the previous
volume was the max volume, then that essentially canceled out the
fade-in and prevented it from happening at all. But now all paths to
fadeMusicVolumeIn() set the volume to zero first, instead of only the
caller of music.play().
When you pick up a trinket in the wild, the music gets silenced, so it
silently plays in the background until you advance the trinket text.
However, foundtrinket (used when Victoria or Vitellary give you a
trinket) is inconsistent with this, and halts the music instead of
silencing it.
This was probably due to the musicfadein script command not being
implemented, so Terry or Simon had to simply make do and halt the music
instead. But musicfadein is implemented and is being used in the trinket
cutscenes, so this is another inconsistency that I will fix.
When you pick up a trinket in the wild, the music will fade back in
afterwards. However, the special trinket cutscenes (where Victoria or
Vitellary will directly give you a trinket) are inconsistent with this,
and restart the music instead of fading it back in.
Looking at the scripts themselves, it immediately becomes obvious the
reason for this inconsistency - 2.2 and previous didn't implement the
musicfadein command, so it couldn't be used, and Terry or Simon simply
had to make do with simply restarting the music. However, 2.3 implements
musicfadein, so we can simply swap it out and remove the
trinketscriptmusic command.
This is 2.2 behavior, which I forgot to keep. Otherwise, if music has
halted and you try to play the same track, it simply won't work, because
the current song is the same as the song you're trying to play. This is
what happened with the trinket scripts - the game halted music, then
tried to play the same track.
Fixes#712.
It's not really used because CreateDirectory doesn't support setting
chmod values, but it does clarify intent of the argument.
Co-authored-by: Ethan Lee <flibitijibibo@gmail.com>
In #52 I fixed VVVVVV not being able to handle filepaths with non-ASCII
characters on Windows. 2f0a0bce4c and
aa5c2d9dc2 reintroduce this problem,
however, by reverting the definition of mkdir to how it was before the
fix and using the non-Unicode version of CreateDirectory. And I can
confirm that VVVVVV indeed doesn't make its folder anymore with a
Windows username of "тест". This commit fixes that issue.
This adds music and volume sliders to the audio options. To use the
sliders, you navigate to the given option, then press ACTION, and your
selection will be transferred to the slider. Pressing left or right will
move the slider accordingly. Then you can press ACTION to confirm the
volume is what you want and deselect it, or you can press Esc to cancel
the volume change, and it will revert to the previous volume; both
actions will write your settings to disk.
Most of this commit is just adding infrastructure to support having
sliders in menus (without copy-pasting code), which is a totally
completely new user interface that has never been used before in this
game. If we're going to be adding something new, I want to make sure
that it at least is done the RIGHT way.
Closes#706.
This adds <musicvolume> and <soundvolume> tags to unlock.vvv and
settings.vvv, so users' volume preferences will be persistent across
game sessions. This does not add the user interface to change them from
in-game; the next commit will do that.
This function is simple - it takes a given buffer and its size, fills it
with a certain character, and null-terminates it. It's meant to be used
with freshly-created buffers, so we don't copy-paste code.
Pressing return in gameplay options would send you back to the pause
menu instead of the general options menu, and pressing return in graphic
options would send you back to the pause menu instead of the general
options menu, too. Additionally, pressing Esc in graphic options would
also send you back to the pause menu instead of the general options
menu.
Like I said before, the menu system is still a bit hardcoded in some
places, and these happened because Terry forgot to update them when he
changed the menus around.
Fixes#711.
The in-game menu code is better than it was in 2.2 but still pretty
hardcoded, so to fix this just change each individual case around. This
bug happened because the "options" button was in the place where "quit
to menu" was previously, but Terry forgot to update it when changing all
the options around.
PhysFS requires a write dir to create a directory, so the first PHYSFS_mkdir
never could have worked. Because of that we need to go back to the old mkdir,
and since we're bringing that back we can reuse it for saves/levels, because we
know it works and we don't have to worry about middlewares ruining anything.
When a text box in the script system (not the gamestate system) is
displayed onscreen and "- Press ACTION to advance text -" is up, the
game sets pausescript to true, so the script system won't blare past the
text box and keep executing. Then it also sets advancetext to true.
Crucially, these two variables are different, so if you have pausescript
true but advancetext false, then what happens?
Well, you get softlocked. There's no way to continue the script.
How is this possible? Well, you can teleport to the (0,0) teleporter
(the teleporter in the very top-left of the map) and regain control
during the teleporter animation. To do that, in 2.2 and below, you have
to press R at the same time you press Enter on the teleporter, or in 2.3
you can simply press R during the cutscene. Then once you teleport to
the room, it's really precise and a bit difficult (especially if
Viridian is invisible), but you can quickly walk over to the terminal in
that room and press Enter on it.
Then what will happen is the terminal script will run, but the
teleporter gamestate sequence will finish and turn advancetext off in
the middle of it. And then you're softlocked.
To fix this, just add a check so if we're in gamestate 0 and there's a
script running, but we have pausescript on and advancetext off, just
turn pausescript off so the game automatically advances the script.
This softlock was reported by Tzann on the VVVVVV speedrunning Discord.
If you manage to get softlocked by being stuck in completestop, the next
thing you'll notice is that quitting to the menu or loading back in will
not reset this.
So you can actually softlock yourself in 2.3 by doing the trinket
cutscene, then quitting to the menu (because 2.3 lets you open the pause
menu during completestop). This is a bug, and should be fixed.
You can skip the "You have found a shiny trinket!" cutscene. The
conditions are that this can only be done in the main game, in the main
dimension (no Polar Dimension), the checkpoint that you last touched
must not be in the same room as the trinket, and you have to have
skipped the Comms Relay cutscene. To do the skip, you press R on the
exact frame (or previous frame, if input delay is enabled) that Viridian
touches the trinket. Then, the gamestate will be immediately set to 0
(because of the gotoroom) and the cutscene will be skipped.
Speedrunners of the main game, well, run the main game already, the
only trinket in the Polar Dimension is not one you want to do a death
warp at, and they have a habit of automatically skipping over the Comms
Relay cutscene because they press R at the beginning of the run when
Viridian teleports to Welcome Aboard, to warp back to the Ship and so
they can leave rescuing Violet for later.
So someone reported softlocking themselves by doing the trinket text
skip in 2.3. The softlock is because they're stuck in a state where
completestop is true but can't advance to a state that turns it off. How
does this happen? It's because they pressed R too late and interrupted
the gamestate sequence. In 2.2 and previous, if you're in the gamestate
sequence then you can't press R at all, but 2.3 removes this restriction
(on account of aiming to prevent softlocks). So only on the very first
frame can you death warp and interrupt the gamestate sequence before it
happens at all.
Anyways to fix this, just turn completestop off automatically if we're
in gamestate 0 and there's no script running.
This softlock was reported by Euni on the VVVVVV speedrunning Discord.
So some people reported the levels list crashing when they loaded it.
But this wasn't reproducible every time. They didn't provide any
debugging information, so I had to use my backup plan: doing a full
audit of the code path taken for loading the levels list.
And then I found this. It turns out this was because I used a
LOAD_ARRAY_RENAME() macro on an std::vector. You can't do that because
you need to use push_back() to resize a vector, so the macro will end up
indexing into nothing, causing a segfault. However, this code path would
only be taken if you have an old levelstats.vvv, from 2.2 and previous -
which explains why it wasn't 100% reproducible. But now that I know you
need an old levelstats.vvv, this bug happens 100% of the time.
Anyways, to fix this, just ditch the macro and expand it manually, while
replacing the indexing with a proper usage of push_back().
While the game does support playing levels with filenames that don't
have the .vvvvvv extension, it doesn't do it well.
Namely, those files can't be loaded or saved into the editor (because a
.vvvvvv always gets tacked on to your input when saving or loading). In
2.3, this gets worse because you can't load a level without a .vvvvvv
extension from command-line playtesting (because a .vvvvvv automatically
gets added) and you can't load per-level custom assets.
The only place where extensionless level files are supported is when
loading level metadata. But this makes it so they no longer work. This
is technically an API break, but it's easily fixed (just add the
.vvvvvv), plus there's nothing to be gained from not having an
extension, plus basically no one ever actually did this in the first
place (as far as I know, I am the only person to have ever done this,
and no one else ever has).
This fixes an issue where you would be able to mount things other than
custom assets in per-level custom asset directories and zips.
To be fair, the effects of this issue were fairly limited - about the
only thing I could do with it was to override a user-made quicksave of a
custom level with one of my own. However, since the quicksave check
happens before assets are mounted, if the user didn't have an existing
quicksave then they wouldn't be able load my quicksave. Furthermore,
mounting things like settings.vvv simply doesn't work because assets
only get mounted when the level gets loaded, but the game only reads
from settings.vvv on startup.
Still, this is an issue, and just because it only has one effect doesn't
mean we should single-case patch that one effect only. So what can we
do?
I was thinking that we should (1) mount custom assets in a dedicated
directory, and then from there (2) mount each specific asset directly -
namely, mount the graphics/ and sounds/ folders, and mount the
vvvvvvmusic.vvv and mmmmmm.vvv files. For (1), assets are now mounted at
a (non-existent) location named .vvv-mnt/assets/. However, (2) doesn't
fully work due to how PhysFS works.
What DOES work is being able to mount the graphics/ and sounds/ folders,
but only if the custom assets directory is a directory. And, you
actually have to use the real directory where those graphics/ and
sounds/ folders are located, and not the mounted directory, because
PHYSFS_mount() only accepts real directories. (In which case why bother
mounting the directory in the first place if we have to use real
directories anyway?) So already this seems like having different
directory and zip mounting paths, which I don't want...
I tried to unify the directory and zip paths and get around the real
directory limitation. So for mounting each individual asset (i.e.
graphics/, sounds/, but especially vvvvvvmusic.vvv and mmmmmm.vvv), I
tried doing PHYSFS_openRead() followed by PHYSFS_mountHandle() with that
PHYSFS_File, but this simply doesn't work, because PHYSFS_mountHandle()
will always create a PHYSFS_Io object, and pass it to a PhysFS internal
helper function named openDirectory() which will only attempt to treat
it as a directory if the PHYSFS_Io* passed is NULL. Since
PHYSFS_mountHandle() always passes a non-NULL PHYSFS_Io*,
openDirectory() will always treat it like a zip file and never as a
directory - in contrast, PHYSFS_mount() will always pass a NULL
PHYSFS_Io* to openDirectory(), so PHYSFS_mount() is the only function
that works for mounting directories.
(And even if this did work, having to keep the file open (because of the
PHYSFS_openRead()) results in the user being unable to touch the file on
Windows until it gets closed, which I also don't want.)
As for zip files, PHYSFS_mount() works just fine on them, but then we
run into the issue of accessing the individual assets inside it. As
covered above, PHYSFS_mount() only accepts real directories, so we can't
use it to access the assets inside, but then if we do the
PHYSFS_openRead() and PHYSFS_mountHandle() approach,
PHYSFS_mountHandle() will treat the assets inside as zip files instead
of just mounting them normally!
So in short, PhysFS only seems to be able to mount directories and zip
files, and not any loose individual files (like vvvvvvmusic.vvv and
mmmmmm.vvv). Furthermore, directories inside directories works, but
directories inside zip files doesn't (only zip files inside zip files
work).
It seems like our asset paths don't really work well with PhysFS's
design. Currently, graphics/, sounds/, vvvvvvmusic.vvv, and mmmmmm.vvv
all live at the root directory of the VVVVVV folder. But what would work
better is if all of those items were organized into a subfolder, for
example, a folder named assets/. So the previous assets mounting system
before this patch would just have mounted assets/ and be done with it,
and there would be no risk of mounting extraneous files that could do
bad things. However, due to our unorganized asset paths, the previous
system has to mount assets at the root of the VVVVVV folder, which
invites the possibility of those extraneous bad files being mounted.
Well, we can't change the asset paths now, that would be a pretty big
API break (maybe it should be a 2.4 thing). So what can we do?
What I've done is, after mounting the assets at .vvv-mnt/assets/, when
the game loads an asset, it checks if there's an override available
inside .vvv-mnt/assets/, and if so, the game will load that asset
instead of the regular one. This is basically reimplementing what PhysFS
SHOULD be able to do for us, but can't. This fixes the issue of being
able to mount a quicksave for a custom level inside its asset directory.
I should also note, the unorganized asset paths issue also means that
for .zip files (which contain the level file), the level file itself is
also technically mounted at .vvv-mnt/assets/. This is harmless (because
when we load a level file, we never load it as an asset) but it's still
a bit ugly. Changing the asset paths now seems more and more like a good
thing to do...
This will clarify which directory, exactly, failed to mount. I know it
gets printed earlier in the mounting process, but it can't hurt to print
it twice, just to be sure. Also this is for consistency.
Default function arguments are the devil, and it's better to be more
explicit about what you're passing into the function. Also because we
might become C-only in the future and to help faciliate that, we should
get rid of C++-isms like default function arguments now.
PHYSFS_getDirSeparator() already gets called and stored in pathSep at
the top of FILESYSTEM_init(). So clearly, two people worked on this
function and forgot that both pieces of code existed at the same time
(or it was one person independently forgetting both).
PhysFS uses platform-independent notation, so we really don't need to
care about getting the correct dir separator here. Especially since we
don't ever do so anywhere else (e.g. load/saveTiXml2Document()), either.
This is to make it clear that this is not a general-purpose mounting
function; it is a helper function for FILESYSTEM_mountAssets()
specifically for treating a directory or file as an assets directory,
and mounting assets from there.
There's no reason to handle mounting .zip files differently than
mounting a directory... we already mount .data.zip files using
FILESYSTEM_mount(), so why go through the trouble of opening a .zip
manually (which means on Windows the .zip can't be touched for the
duration of playing the custom level), making up a place to mount it at,
and then mount that made-up name, instead of just using
FILESYSTEM_mount()?
Whoever cobbled this asset mounting thing together really didn't fully
understand what they were doing.
This way, we avoid the unnecessary graphics.reloadresources() call - if
we can't mount assets, why bother reloading resources?
The return type of FILESYSTEM_mount() has been changed from void to bool
to indicate success, accomodating its callers accordingly.
I haven't been able to reproduce this old thing on any setup I have. The patch
from 2013 was originally for X11, and Wayland's fullscreen doesn't allow for
this sort of thing, so let's start scoping this down for eventual removal when
X11 is finally out of our minds forever.
So it looks like facb079b35 (PR #316) had
a few issues.
The SDL performance counter doesn't really work that well. Testing
reveals that unfocusing and focusing the game again results in
the resumemusic() script command resuming the track at the wrong time.
Even when not unfocusing the game at all, stopping a track and resuming
it resumes it at the wrong time. (Only disabling the unfocus pause fixes
this.)
Furthermore, there's also the fact that the SDL performance counter
keeps incrementing when the game is paused under GDB. So... yeah.
Instead of dealing with the SDL performance counter, I'm just going to
pause and resume the music directly (so the stopmusic() script command
just pauses the music instead). As a result, we no longer can keep
constantly calling Mix_PauseMusic() or Mix_ResumeMusic() when focused or
unfocused, so I've moved those calls to happen directly when the
relevant SDL events are received (the constant calls were originally in
VCE, and whoever added them (I'm pretty sure it was Leo) was not the
sharpest tool in the shed...).
And we are going to switch over to using our own fade system instead of
the SDL mixer fade system. In fact, we were already using our own fade
system for fadeins after collecting a trinket or a custom level
crewmate, but we were still using the mixer system for the rest. This is
an inconsistency that I am glad to correct, so we're also doing our own
fadeouts now.
There is, however, an issue with the fade system where the length it
goes for is inaccurate, because it's based on a volume-per-frame second
calculation that gets truncated. But that's an issue to fix later - at
least what I'm doing right now makes resumemusic() and musicfadein()
work better than before.
musicclass already had a resume() function for music.
These are just wrappers around the appropriate SDL_mixer functions, to
avoid direct function calls to the mixer API. So if we ever need to do
something with all callers of pausing and resuming in the future, or we
switch to a different audio backend, the work is already done for us.
Also it just looks cleaner to be calling our musicclass function instead
of doing a direct API call to the mixer.
This makes it so to reuse this code, we don't have to copy-paste it.
Additionally, I added a check for the milliseconds being 0, to avoid a
division by zero. Logically and mathematically, if the fade amount is 0
milliseconds, then that means the fade should happen instantly -
however, dividing by zero is undefined (both in math and in C/C++), so
this check needs to be added.
This is an option for speedrunners whose muscle memory is precisely
trained and used to the 1-frame input delay that existed in 2.2 and
below. It is located in Game Options -> Advanced Options, and is off by
default.
To re-add the 1-frame input delay, we simply move the key.Poll() to the
start of the frame, instead of before an input function gets ran -
undoing what #535 did.
There is a frame ordering-sensitive issue here, where toggling
game.inputdelay at the wrong time could cause double-polling. However,
we only toggle it in an input function, which regardless is always
guaranteed to be ran after key.Poll() (it either happened at the start
of the frame or just before the input function got ran), so this is not
an issue. But, in case we ever need to toggle this variable in the
future, we can just use the defer callbacks system to defer the toggle
to the end of the frame - also added by #535.
Added at the request of Habeechee on the VVVVVV speedrunning Discord
server.
This fixes being unable to use teleporters while the "- Press ACTION to
advance text -" prompt is up, which is used to perform credits warp.
In 2.2 and 2.0, this advancetext check was only in gamerender() for
rendering the "- Press ENTER to Teleport -" prompt and didn't affect any
logic. In 2.3, I moved the check (and the rest of the conditional it was
in) to gamelogic() - same as the activity zone prompt conditionals - so
if you gained control while being in a prompt zone, the prompt wouldn't
suddenly appear[1].
As a side effect, this ended up aligning rendering and logic together,
so if you couldn't see the teleporter prompt, you weren't able to
teleport - whereas in 2.2 and 2.0, you could still use the teleporter
even though the prompt wasn't up.
So by removing the advancetext check, you are now able to use the
teleporter again, AND the "- Press ENTER to Teleport -" prompt will also
show up as well.
Habeechee reported this regression on the VVVVVV speedrunning Discord
server.
[1]: f07a8d2143, PR #421
One of the solutions to the quit signal unfocus pause regression is to
add a no-op delta func to the unfocused func table. However, this
results in the game being stuck in unfocus pause forever, because when
it reaches the end of a list on a delta func, it won't reassign the
active functions - only when the end of a list is a fixed func will it
do so. A workaround is to then add a no-op fixed func afterwards, but
that's inelegant.
The solution in the end to the quit signal regression is to not bother
with adding a delta func, so the game as of right now actually never has
a delta func at the end of a list, and probably never will - but this is
one piece of technical debt I don't want to leave laying around. In case
we're ever going to put a delta function at the end of a list, I've made
it so that delta functions will now reassign the list of active funcs if
they happen to be at the end of the func list.
This fixes a regression introduced by #535 where a quit signal (e.g.
Ctrl-C) sent to the window while the game was in unfocus pause wouldn't
close the game.
One problem was that key.quitProgram would only be checked when control
flow switched back to the outer loop in main(), which would only happen
when the loop order state machine switched to a delta function. As the
unfocused func table didn't have any delta functions, this means
key.quitProgram would never be checked.
So a naïve solution to this would just be to add a no-op delta func
entry to the unfocused func table. However, we then run into a separate
issue where a delta function at the end of a func list never reassigns
the active funcs, causing the game to be stuck in the unfocus pause
forever. Active func reassignment only happens after fixed funcs. So
then a naïve solution after that would be to simply add a no-op fixed
func entry after that. And indeed, that would fix the whole issue.
However, I want to do things the right way. And this does not seem like
the right way. Even putting aside the separate last-func-being-delta
issue, it mandates that every func list needs a delta function. Which
seems quite unnecessary to me.
Another solution I considered was copy-pasting the key.quitProgram check
to the inner loops, or adding some sort of signal propagation to
the inner loops - implemented by copy-pasting checks after each loop -
so we didn't need to copy-paste key.quitProgram... but that seems really
messy, too.
So, I realized that we could throw away key.quitProgram, and simply call
VVV_exit() directly when we receive an SDL_QUIT event. This fixes the
issue, this removes an unnecessary middleman variable, and it's pretty
cleanly and simply the right thing to do.
This includes all text from the Gravitron and Super Gravitron.
This is to make the text more readable if they are placed in weird
situations - for example, in custom levels, where the background these
texts get placed on could be anything (custom level makers are crazy!).
It's just like bigprint() except it duplicates some of the calculations
because I didn't want to make a bigprintoff() function which would
duplicate even more code. I'm beginning to think these text printing
functions are completely horrible to work with...
In case they get drawn against a non-contrasting background, it's still
useful to keep them readable by outlining them. This could happen if
someone were to use the Game Complete gamestate sequence in a custom
level (or presses R during Game Complete).
Flip Mode flips all the unfocus pause screen text upside-down, to make
it read in reverse order. This looks kind of strange to me, and I don't
think it was intended. So I'm flipping the text again so it's the right
way up in Flip Mode.
During the final stretch, after Viridian turns off the Dimensional
Stability Generator, the map goes all psychedelic and changes colors
every 40 frames. Entities change their colors too, including conveyors,
moving platforms, and disappearing platforms.
But play around with the disappearing platforms for a bit and you'll
notice they seem a bit glitchy. If you run on them at the right time,
the tile they use while disappearing seems to abruptly change whenever
the color of the room changes. If there's a color change while they're
reappearing (when you die and respawn in the same room as them), they'll
have the wrong tile and look like a conveyor. And even if you've never
interacted with them at all, dying and respawning in the same room as
them will change their tile to something wrong and also look like a
conveyor.
So, what's the problem? Well, first off, the tile of every untouched
disappearing platform changing into a conveyor after you die and respawn
in the same room is caused by a block of code in gamelogic() that gets
run on each entity whenever you die. This block of code is the exact
same block of code that gets ran on a disappearing platform if it's in
the middle of disappearing.
As a quick primer, every entity in the game has a state, which is just a
number. You can view each entity's state in
entityclass::updateentities().
State 0 of disappearing platforms is doing nothing, and they start with
an onentity of 1, which means they turn to state 1 when they get
touched. State 1 moves to state 2. State 2 does some decrementing, then
moves to state 3 and sets the onentity to 4. State 3 also does nothing.
After being touched, state 4 makes the platform reappear and move to
state 5, but state 5 does the actual reappearing; state 5 then sets the
state back to 0 and onentity back to 1.
So, back to the copy-pasted block of code. The block of code was
originally intended to fast-forward disappearing platforms if they were
in the middle of disappearing, so the player respawn code would properly
respawn the disappearing platform, instead of leaving it disappeared.
What it does is keep updating the entity, while the state of the entity
is 2, until it is no longer in state 2, then sets it to state 4.
Crucially, the original block of code only ran if the disappearing
platform was in state 2. But the other block of code, which was
copy-pasted with slight modifications, runs on ALL disappearing
platforms in final stretch, regardless of if they are in state 2 or not.
Thus, all untouched platforms will be set to state 4, and state 4 will
do the animation of the platform reappearing, which is invalid given
that the platform never disappeared in the first place. So that's why
dying and respawning in the same room as some disappearing platforms
during final stretch will change their tiles to be conveyors.
It seems to me that doing anything with death is wrong, here. The root
cause is that map.changefinalcol() "resets" the tile of every
disappearing platform, which is a function that gets called on every
color change. The color change has nothing to do with dying, so why
fiddle with the death code?
Thus, I've deleted that entire block of code.
What I've done to fix the issue is to make it so the tile of
disappearing platforms aren't manually controlled. You see, unlike other
entities in the game, the tile of disappearing platforms gets manually
modified whenever it disappears or reappears. Other entities use the
tile as a base and store their tile offset in the separate walkingframe
attribute, which will be added to the tile attribute to produce the
drawframe, which is the final thing that gets rendered - but for
disappearing platforms, their tile gets directly incremented or
decremented whenever they disappear or reappear, so when
map.changefinalcol() gets ran to update the tile of every platform and
conveyor, it basically discards the tile offset that was manually added
in.
Instead, what I've done is make it so disappearing platforms now use
walkingframe, and thus their final drawframe will be their tile plus
their walkingframe. Whenever map.changefinalcol() gets called, it is now
free to modify the tile of disappearing platforms accordingly - after
all, the tile offset is now stored in walkingframe, so no weird
glitchiness can happen there.
Ethan, you forgot this other one.
I do have to rejiggle the control flow of the function a bit, so it
doesn't leak memory upon failure. (Although the SDL message box leaks
memory anyway because of X11 so... whatever.) Also, there's a NULL check
for if SDL_GetBasePath() fails now.
According to SDL documentation[1], the returned pointer needs to be
freed. A glance at the source code confirms that the function allocates,
and also Valgrind complains about it.
Also if it couldn't allocate, the game no longer segfaults (std::strings
do not check if the pointer is non-NULL for operator+=).
[1]: https://wiki.libsdl.org/SDL_GetClipboardText
