When exiting a level, music.init() gets called again, and every time it
gets called after the first time it gets called, it will free all music
tracks.
To do so, it calls Mix_FreeMusic(). Unfortunately, if there is music
fading, Mix_FreeMusic() will call SDL_Delay(), which will result in
annoying no-draw frames. Meaning, the screen freezes and doesn't draw
anything, and has to wait a bit before continuing.
Here's the relevant piece of code from SDL2_mixer's music.c:
if (music == music_playing) {
/* Wait for any fade out to finish */
while (music->fading == MIX_FADING_OUT) {
Mix_UnlockAudio();
SDL_Delay(100);
Mix_LockAudio();
}
if (music == music_playing) {
music_internal_halt();
}
}
This is especially annoying if you're a TASer, because no-draw frames in
a libTAS movie aren't guaranteed to last for a consistent number of
frames when you change your inputs around.
After this patch, as long as your computer can unmount and re-mount
assets fast enough (it doesn't seem like mine can, unfortunately), then
you won't get any freezes when exiting a level that has custom assets.
(This freeze didn't happen when loading a level because the title screen
music fadeout upon pressing ACTION had enough time to fully complete
before the level got loaded.)
There's a small inconsistency where the first time you load in to the
game, game.lifeseq is at 0, but when you exit to the menu and load in
again, game.lifeseq becomes 10. This is visible as Viridian blinking
when loading in only after the first time you load in, and this also
means that after the first time you load in, you also have to wait 5
frames before being able to move Viridian.
The reason for this inconsistency is because on the first time you load
in to the game, there are no entities loaded in obj.entities yet, so the
game creates a player entity, and doesn't mess with game.lifeseq. When
you exit and then load in for the second time, obj.entities contains at
least one entity (all the entities from the room you just exited out of
- map.gotoroom() hasn't even been called yet, so it doesn't even check
for only the player entity), so the game calls map.resetplayer()
instead, and map.resetplayer() sets game.lifeseq to 10.
There's even an inconsistency to this inconsistency - when you die in No
Death Mode, all entities will be removed from obj.entities, so the next
time you load in to the game, game.lifeseq will be 0.
This inconsistency is pretty minor in the grand scheme of things, but
it still bothers me, so I'm going to fix it.
CI builds were added to this repository on the first day it was
released, and haven't really been touched since then. And since then,
2.3 has added NO_CUSTOM_LEVELS, NO_EDITOR, and OFFICIAL_BUILD builds to
the CMake file.
On top of the MAKEANDPLAY define already existing, this means CI
coverage is a bit sparse - covering compile failures for changes made to
most of the codebase, except for Steam and GOG, and not covering compile
failures if certain parts of the code get stripped out. And people do
forget to check for those configurations as well.
These mess of configurations is kind of a wake-up call to refactor and
generalize the code a bit, because we would probably be able to get rid
of at least two of these (Make & Play, and no-custom-levels) by making
it so custom levels behave indistinguishably from the main game. But,
that's something to do in 2.4. At the very least, we should cover these
in CI right now.
On a small note, I had to add a MAKEANDPLAY configure option to the
CMake file to be able to easily configure a Make & Play build from the
CI runner. This option shouldn't really be used otherwise, so I added a
note to it telling people to consider modifying MakeAndPlay.h instead.
Since INTERIM_COMMIT is a char array whose size we know for sure at
compile time, and which we also know is an array (instead of being a
pointer), we can take the SDL_arraysize() of it. However,
SDL_arraysize() doesn't account for the null terminator unlike
SDL_strlen(), so we'll have to do it ourselves. But at least we are
guaranteed to get a constant value at compile time, unlike if we use
SDL_strlen(), which would be repeatedly evaluating a constant value at
runtime.
To my knowledge, there's no equivalent SDL_arraysize() for constant
strings, and a quick `rg` (ripgrep) for "sizeof" in the SDL include/
folder doesn't show anything like that. So we'll just have to use the
SDL_arraysize() - 1 and deal with it.
The commit hash is now properly updated every time it gets changed, and
not just when CMake gets re-ran.
For this to work, we need to use a few CMake tricks. We add a custom
target with ADD_CUSTOM_TARGET(), which is apparently always considered
out-of-date (but I had to add a BYPRODUCTS line to get it to actually
work), and we use the target to run a .cmake file every time we build.
Also, VVVVVV needs to depend on this custom target, to ensure that the
game gets built AFTER the version gets generated - otherwise there'll be
an error. So we do an ADD_DEPENDENCIES() after the ADD_EXECUTABLE() for
VVVVVV.
This file, version.cmake, is just the Version.h.out generation that I
added previously, but the important thing about all of this is that if
the contents of Version.h.out doesn't change, and thus if the commit
hash hasn't changed, then CMake will never recompile and relink anything
at all. (At least with the Ninja generator.)
On a small note, since the Version.h.out generation is now a separate
script that is guaranteed to get ran on every single build, while the
Git FIND_PACKAGE() gets ran only at configure time, it is possible for
the cached path of the Git executable to get out of date. Fixing this
requires a simple re-configure (ideally), but in case it wasn't fixed,
the INTERIM_COMMIT and COMMIT_DATE variables would get set to empty
strings instead of containing a value. To prevent this from happening,
I've removed ERROR_QUIET from the EXECUTE_PROCESS() calls in
version.cmake, because it's better to explicitly error if the Git
executable wasn't found than implicitly carry on like nothing happened.
The previous implementation of showing the commit hash on the title
screen used a preprocessor definition added at CMake time to pass the
hash and date. This was passed for every file compiled, so if the date
or hash changed, then every file would be recompiled. This is especially
annoying if you're working on the game and switching branches all the
time - the game has at least 50 source files to recompile!
To fix this, we'll switch to using a generated file, named
Version.h.out, that only gets included by the necessary files (which
there is only one of - Render.cpp). It will be autogenerated by CMake
(by using CONFIGURE_FILE(), which takes a templated file and does a
find-and-replace on it, not unlike C macros), and since there's only one
file that includes it, only one file will need to be recompiled when it
changes.
And also to prevent Version.h.out being a required file, it will only be
included if necessary (i.e. OFFICIAL_BUILD is off). Since the C
preprocessor can't ignore non-existing include files and will always
error on them, I wrapped the #include in an #ifdef VERSION_H_EXISTS, and
CMake will add the VERSION_H_OUT_EXISTS define when generating
Version.h.out. The wrapper is named Version.h, so any file
that #includes the commit hash and date should #include Version.h
instead of Version.h.out.
As an added bonus, I've also made it so CMake will print "This is
interim commit [HASH] (committed [DATE])" at configure time if the game
is going to be compiled with an interim commit hash.
Now, there is also the issue that the commit hash change will only be
noticed in the first place if CMake needs to be re-ran for anything, but
that's a less severe issue than requiring recompilation of 50(!) or so
files.
While working on #535, I noticed this bug.
When going to Graphic Options or Game Options from the pause menu,
kludge_ingametemp was intended to save the current menu stack frame
BEFORE either of those menus got created. However, it was actually
assigned afterwards, meaning kludge_ingametemp would always be either
Menu::graphicoptions or Menu::options.
This meant that the returntomenu() in returntopausemenu() would always
attempt to return to the current in-game menu, and seeing as it's the
same menu, would re-create the menu, instead of returning to the
previous menu before it.
This patch also fixes a potential source of a trivial memory leak, if
someone were to keep entering and exiting Graphic Options or Game
Options from the pause menu. It would keep piling up duplicate Graphic
Options or Game Options stack frames, which would never get removed.
However, they do get removed when you exit to the menu properly, by
returntomenu() again, so this doesn't seem like that serious of an
issue, but it's still good to fix.
While I was working on #535, I noticed that all the call sites of
script.run() have the exact same code, namely:
if (script.running)
{
script.run();
}
I figured, why not move the script.running check into the function
itself? That way, we won't have to duplicate the check every single
time, and we don't risk forgetting to add the check and causing a bug
because of that.
The check was already duplicated once since 2.0 (it's used in both
GAMEMODE and TELEPORTERMODE), and with the fix of the two-frame delay in
2.3, it's now duplicated twice, leading to THREE instances of this check
in the code, when there should be only one.
To fix this bug, all we have to do is just pass the existing
ScreenSettings* that we have in loadstats() to savestats(), and in
loadsettings() to savesettings().
Fixes#556. Depends on #558.
Another step to fix the bug #556 is to allow Game::savestats() to accept
a pointer to an existing ScreenSettings struct. This entails refactoring
Game::savesettings() and Game::serializesettings() to accept the
function as well, along with adding Screen::GetSettings() so the
settings of the current Screen can be easily grabbed.
In order to be able to fix the bug #556, I'm planning on adding
ScreenSettings* to the settings.vvv write function. However, that
entails adding another argument to Game::savesettings(), which is going
to be really messy given the default argument of Game::savestats().
That, combined with the fact that the code comment at the site of the
implementation of Game::savestats() being wrong (!!!), leads me to
believe that using default function arguments here isn't worth it.
Instead, what I've done is made it so callers are explicit about whether
or not they're calling savestats(), savesettings(), or both at the same
time. If they are calling both at the same time, then they will be using
a new function named savestatsandsettings().
In short, these are the interface changes:
* bool Game::savestats(bool) has been removed
* bool Game::savestatsandsettings() has been added
* void Game::savestats_menu() has been renamed to
void Game::savestatsandsettings_menu()
* All previous callers of bool Game::savestats() are now using bool
Game::savestatsandsettings()
* The one caller of bool Game::savestats(bool) is now using bool
Game::savestats()
While there already exists an option to skip the fake loading screen
entirely (without requiring an ACTION press), there are several reasons
for including this option as well:
* So people upgrading from 2.2 won't have to sit through the fake
loading screen the first time they open 2.3.
* So if people are too lazy to use the existing option, they can use
this one instead.
* So tool-assisted speedruns (TASes) of this game can skip the fake
loading screen without requiring an existing settings.vvv beforehand.
This last one is the biggest reason for me, since I'm not sure what
TASVideos.org rules are regarding existing save files, but with this
change nobody has to worry about their rules and can safely just
press ACTION to skip the fake loading screen automatically.
`success = success && savesettings();` is now changed to
`success &= savesettings();`. It's bitwise, and I think C++ should have
had a &&= for completeness, but it shouldn't matter here.
Changing settings would most of the time attempt to save unlock.vvv and
now also settings.vvv, but there would be no feedback whether the files
have been saved successfully or not. Now, if saving fails when changing
settings in the menu, a warning message will be shown. The setting will
still be applied of course, but the user will be informed it couldn't
be saved. This message can be silenced until the game is restarted,
because I can imagine this could get very annoying when someone already
knows their settings aren't writeable.
Also, some options didn't even save settings in the first place. These
are turning off invincibility, and by coincidence precisely all the
options in the advanced options menu. I made sure these options now do
so.
As part of fixing #464, I'll need to move these pieces of code around
easily. In #220 I just kind of shoved them awkwardly in whatever
fixed function would be last called in the gamestate loop, which I
shouldn't have done as I've now had to make formal fixed-render
functions anyway. Because these fixed functions need to be called
directly before a render function, and I'm fixing the order to put
render functions in their proper place, so I need to be able to move
these around easily, and making them function calls instead of inlined
makes them easier to manipulate.
Today, I saw a video posted by Chelito on the VVVVVV speedrunning
Discord where he died inside a gravitron square over and over after the
Gravitron in Intermission 2 ended.
https://cdn.discordapp.com/attachments/234787368522088448/779074864660480020/What.mp4
This is caused by the fact that after the Gravitron ends, the game no
longer considers you to be inside swnmode, so it won't move the enemies
offscreen when you die.
To fix this, after the Gravitron ends, the game will switch to swngame
8, where it will keep you inside swnmode until all gravitron squares are
offscreen. This means that gravitron squares that are onscreen after the
Gravitron ends will be moved offscreen if you die, preventing the above
death loop from happening.
PR #468 made it so you can use the menus while in a cutscene, in order
to counteract softlocks. However, this has resulted in more
unintentional behavior:
- `gamemode(teleporter)` breaks when opening the ENTER menu (Misa
mentioned this)
- The player can now interrupt shakes and walks, and have their timers
run out before resuming the cutscene
- After completing the game, the player can warp to the ship while a
dialogue is active, and prevent themselves from advancing text (plus
it's always rude to just teleport away while someone's talking)
- The player can peek at the map before hidecoordinates is run, and can
also peek at what the game does with missing/rescued crewmates during
cutscenes
This commit fixes the latter two issues. While a script is running,
only the SAVE tab is now available. Therefore the player can still get
themselves out of softlocks as intended, but they do things like
looking at the map or teleporting away during a cutscene.
It wasn't a direct duplicate of key.sensitivity, but it was still
basically the same thing. Although to be fair, at least the case-switch
conversion didn't get duplicated everywhere unlike game.slowdown.
So now key.sensitivity functions the same as game.controllerSensitivity,
and it only gets converted to its actual value whenever a joystick input
happens in key.Poll(), unlike previously where it got converted every
single frame on the title screen (there was even a comment that said
"TODO bit wasteful doing this every poll").
game.gameframerate seems to exist for converting the value of
game.slowdown into an actual timestep value, when really the timestep
value should just use game.slowdown directly with a fast lookup table.
Otherwise, there's a bunch of duplicated game.slowdown case-switches
everywhere, which adds up to a large, annoying pile should the values be
changed in the future. But now the duplicate variable has been removed,
and with it, all the copy-pasted case-switches.
Also, the game speed text rendering in Menu::accessibility and
Menu::setslowdown has been factored out to a function and de-duplicated
as well.
There were some duplicate Screen configuration variables that were on
Game, when there didn't need to be.
- game.fullScreenEffect_badSignal is a duplicate of
graphics.screenbuffer->badSignalEffect
- game.fullscreen is a duplicate of !graphics.screenbuffer->isWindowed
- game.stretchMode is a duplicate of graphics.screenbuffer->stretchMode
- game.useLinearFilter is a duplicate of
graphics.screenbuffer->isFiltered
These duplicate variables have been removed now.
I put indentation when handling the ScreenSettings struct in main() so
the local doesn't live for the entirety of main() (which is the entirety
of the program).
Apparently, the amount of digits in a commit hash that git will output
varies depending on how many objects are in the repository that the hash
gets pulled from. The more objects, the more digits needed to avoid a
hash collision.
Sources:
https://stackoverflow.com/q/18134627/#comment26560283_18134919https://stackoverflow.com/a/21015031/
So that means we'll have to dynamically account for the length of the
commit hash in order to get it properly right-aligned with the rest of
the text.
There are probably going to be situations where we'll want to compile in
release mode, but still want the hash and don't want Steam/GOG enabled.
So Ethan can use OFFICIAL_BUILD when tagging major versions of the game.
For some reason, music.usingmmmmmm automatically gets set to true in
musicclass::init(). I assume this was because it would get re-assigned
by game.usingmmmmmm in the game startup code anyway, but now that
musicclass::init() can be called more than once, this variable will just
get set to true when it shouldn't be, causing a confusing desync just
like the one I described in my previous commit, where you would have
PPPPPP or MMMMMM on the title screen, but closing the game and
re-launching it would play the other soundtrack instead.
Again, these duplicate variables should be removed, but that's going to
be a separate patch. In the meantime, I'm removing this variable
assignment.
musicclass::init() re-initializes every attribute of musicclass
unnecessarily, when initialization should be put in a constructor
instead. This is bad, because music.init() gets called whenever we enter
and exit a custom level that has assets.
Otherwise, this would result in a bug where music.usingmmmmmm would be
reset, causing you to revert to PPPPPP on the title screen whenever you
enter a level with MMMMMM selected and exit it.
This also causes a confusing desync between game.usingmmmmmm and
music.usingmmmmmm since the values of the two variables are now
different (these duplicate variables should probably be removed, too,
and a lot of other duplicate variables like these exist, too, which are
a real headache). Which means despite MMMMMM playing on the title
screen, exiting the game and re-launching it will play PPPPPP instead.
What's even more is that going to game options and switching to PPPPPP
will play PPPPPP, but afterwards launching and re-entering will play
MMMMMM. Again, having duplicate variables is very bad, and should
probably be fixed, but that's a separate patch.
...you die and the platform's x-coordinate is to the left of x=152.
Which means if you die and the platform isn't completely clear of the
space of its adjacent disappearing platform.
The block needs to be updated accordingly with calls to
obj.nocollisionat() and obj.moveblockto(), else the block will simply be
left behind and the platform will no longer have any collision. This is
in contrast to 2.2 behavior, where the platform would simply
unconditionally create a new block, which would actually end up with a
duplicate block since the previous block didn't get cleaned up, but this
didn't cause any problems because the room was carefully designed so you
would never be able to touch that previous block after you died and
respawned at the checkpoint. But it's still there.
I also added comments to document what this kludge code did, because
otherwise it would be mysterious to readers who are unfamiliar with it.
Fixes#543.
What's the difference between a slash sign and a percent sign? Well, a
percent sign is just a slash sign with two extra oranges in between, but
those two oranges make a huuuuge difference...
I can't really make the filter update only every timestep, because it's
per-pixel and operates on deltaframes too, so it TECHNICALLY runs faster
in over-30-FPS mode than not. That said, it's not really noticeable, the
filter doesn't look bad for updating more often or anything. However, I
can at least interpolate the scrolling, so it's smooth in over-30-FPS
mode.
Originally this function was made because it needed to be exported to
gameinput(), but this piece of code is actually also used in
gamecompletelogic(). So it's good to de-duplicate it here, too.
Assigning these variables is now wholly unnecessary ever since #522 got
merged, and in fact setting graphics.backgrounddrawn to false actually
causes the warp background to "skip" when the map screen gets closed. So
this fixes that bug, too.
This kludge variable was used to re-set the warp background after coming
back from the in-game settings menus. But since #522 got merged, this
has no longer been necessary.
This check is clearly meant for destroying the factory clouds in the
room "Level Complete!" in the main game, but it covers all rooms on row
8, instead of only (13,8). Adding an x-room check restricts this
behavior to only (13,8).
Trinket9 reported that this weird behavior of destroying specifically
above y-position 60 was undesirable, since they were creating an enemy
with this `behave` in a room on row 8 and it kept disappearing
instantly.
First, the variable has been inverted, because it's bad practice to have
booleans with negative names. Secondly, instead of magically setting a
signaling variable when calling fadeout(), fadeout() now has a parameter
to set the quick_fade attribute, which is much cleaner than doing the
magical assignment that could potentially look unrelated.
fadeoutqueuesong basically does the same thing as nicechange - they both
queue a song to be played when the current track is done fading out.
Except, for some reason, I decided to add fadeoutqueuesong instead of
using the existing nicechange/nicefade system.
This has consequences where fadeoutqueuesong would step on the toes of
nicechange. In the case of #390, nicechange would say "let's play
Potential for Anything" when entering the Super Gravitron, but
fadeoutqueuesong had previously said "let's play Pipe Dream" because of
the player having just exited the Super Gravitron. fadeoutqueuesong took
priority because it came first in musicclass::processfade(), and when it
called play(), the Mix_PlayingMusic() in the nicechange check afterwards
would say music would already be playing at that point, so the
nicechange wouldn't take effect.
In the end, the solution is to just merge the new system into the
already-existing system.
Fixes#390.
Just looked over this and had to do a double-take. It should be
num_mmmmmm_tracks, not num_pppppp_tracks, because MMMMMM comes first in
the vector of music tracks.
Here's what causes #401: After the fade to menu delay ticks down to 0,
the game calls game.quittomenu(), but the rest of mapinput() still
executes. This means that the block that detects your ACTION press gets
executed, because there's a check that fadetomenudelay is less than or
equal to 0, and, well, it is.
So if you've pressed ACTION on the exact frame that it counts down to 0,
then the game detects your ACTION press, then processes it accordingly,
and then sets the fadetomenudelay, which means it'll get reactivated the
next time you open the map screen. But at this point, you get sent to
TITLEMODE, because game.quittomenu() set game.gamestate accordingly.
(This is why resetting game.fadetomenu or game.fadetomenudelay in
game.quittomenu() or script.hardreset() won't fix this bug.)
The solution here is to add a game.fadetomenu check to the ACTION press
processing.
Same-frame state transition logic is hard... actually, any sort of thing
where two things happen on the same frame is really annoying.
This also applies to fadetolab and fadetolabdelay, too.
Fixes#401.
Misa
Terry Cavanagh
Dav999-v
leo60228
