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
