This replaces all calls to SDL_free with a new macro, VVV_free, that
nulls the pointer afterwards. This mitigates any use-after-frees and
also completely eliminates double-frees. The same is done for any
function to free specific objects such as SDL_FreeSurface, with the
VVV_freefunc macro.
No exceptions for any of these calls, even if the pointer is discarded
or zeroed afterwards anyway. Better safe than sorry.
This is a macro rather than a function that takes in a
pointer-to-pointer because such a function would have type issues that
require casting and that's just not safe.
Even though SDL_free and other SDL functions already check for NULL, the
macro has a NULL check for other functions that don't. For example,
FAudioVoice_DestroyVoice does not check for NULL.
FILESYSTEM_freeMemory has been axed in favor of VVV_free because it
functionally does the same thing except for `unsigned char*` only.
If a music track has a loop comment with a negative value, ignore all
comments of the track. This is just to prevent any weirdness from
happening as it's safer to just let the track loop improperly. Also log
to the console to let users know.
This is the same thing that SDL_mixer does now:
libsdl-org/SDL_mixer@e819489459
This commit happened as a result of discussion on the VVVVVV Discord
server about SDL_mixer 2.0.4 behavior with weird loop comment values
(e.g. octal input with leading zeroes). This is simply updating the code
to be in line with what newer versions of SDL_mixer do.
Just in case it happens. Comments aren't really important to the game
(at worst a track will just loop in the wrong place) so it's fine to
carry on here and ignore all comments if this happens.
This does the following:
- Const-qualify variables if they are not modified
- Place each statement onto their own separate lines
- Place the asterisk with the type instead of the variable name
- Combine declarations and initializations where possible
This fixes the following warnings:
desktop_version/src/Music.cpp: At global scope:
desktop_version/src/Music.cpp:240:23: warning: non-static data member initializers only available with ‘-std=c++11’ or ‘-std=gnu++11’ [-Wc++11-extensions]
240 | Uint8 *wav_buffer = NULL;
| ^
desktop_version/src/Music.cpp:414:32: warning: non-static data member initializers only available with ‘-std=c++11’ or ‘-std=gnu++11’ [-Wc++11-extensions]
414 | Uint8* decoded_buf_playing = NULL;
| ^
desktop_version/src/Music.cpp:415:32: warning: non-static data member initializers only available with ‘-std=c++11’ or ‘-std=gnu++11’ [-Wc++11-extensions]
415 | Uint8* decoded_buf_reserve = NULL;
| ^
desktop_version/src/Music.cpp:416:21: warning: non-static data member initializers only available with ‘-std=c++11’ or ‘-std=gnu++11’ [-Wc++11-extensions]
416 | Uint8* read_buf = NULL;
| ^
These warnings are because the non-static data members (i.e. data
members that will be different for each instance of a class) are being
initialized at the same time as they're being declared, which means
that's what their value will be when the class instance is initialized.
However, this is only a C++11 feature, and we don't use C++11. Luckily,
we don't need to do this, and this is in fact redundant because we
already zero out the class instance in its constructor using
SDL_zerop(). Therefore, we should remove these initializers to fix
compliance with C++03.
During review of #869, I looked at this part of the codebase again. I
have no idea how or why, but during the course of 2.4 this whole area
just became a mess.
The issues I fixed (in no particular order):
- Copy-pasting the code that loads from the binary blobs
- Making sure SDL_RWFromConstMem is used over SDL_RWFromMem wherever
possible
- Adding checks to make sure the index from the binary blob is valid
(it's possible it could not exist)
- Adding checks to make sure we gracefully handle
SDL_RWFromConstMem/PHYSFSRWOPS_openRead returning NULL
- Moving the pointer asterisk to the type instead of the name :)
In hindsight, the FAudio pointer will likely be in SoundTrack since we will
want to keep the mastering voice closer to the sounds and their source voice
arrays, while the MusicTrack will likely just be one source voice that gets
PCM from different streams.
This looks redundant but will actually help in the transition to FAudio; we
mostly want to keep the game logic the same while reimplementing the current
mixer, weirdness and all. Once that's done and confirmed to be stable and
consistent we can start cutting out the workarounds and quirks.
This meant making the track vectors static, but that's kind of what we do with musicclass anyway?
In any case, this will make the transition to FAudio MUCH less invasive.
This fixes a regression where entering playtesting while a track was
fading out (by exiting out of playtesting with a track playing and then
immediately entering back in with the level start music set) would
result in no music.
The cause is the game doing fades even though nothing is playing, which
puts it in a confusing state.
This wrapper function is for (a) future-proofing (b) proactive
prevention of future copy-pasting (c) to clarify that we never actually
halt music in the SDL_mixer sense, we only pause it, so to check if the
music is halted we actually check if the music is paused instead. This
is important because Mix_PlayingMusic() does not check if the music is
paused and Mix_PausedMusic() does not check if the music is halted.
The main game used a set of copy-pasted code to set the music of each
area. There WAS some redundancy built-in, but only three rooms in each
direction from the entrance of a zone.
Given this, it's completely possible for players to mismatch the music
of the area and level. In fact, it's easy to do it even on accident,
especially since 2.3 now lets you quicksave and quit during cutscenes.
Just play a cutscene that has Pause music, then quicksave, quit, and
reload. Also some other accidental ways that I've forgotten about.
To fix this, I've done what mapclass has and made an areamap. Except for
music. This map is the map of the track number of every single room,
except for three special cases: -1 for do nothing and don't change music
(usually because multiple different tracks can be played in this room),
-2 for Tower music (needs to be track 2 or 9 depending on Flip Mode),
and -3 for the start of Space Station 2 (track 1 in time trials, track 4
otherwise).
I've thoroughly tested this areamap by playing through the game and
entering every single room. Additionally I've also thoroughly tested all
special cases (entering the Ship through the teleporter or main
entrance, using the Ship's jukebox, the Tower in Flip Mode and regular
mode, and the start of Space Station 2 in time trial and in regular
mode).
Closes#449.
2.3 has a regression where if you move back and forth between a zone,
you can get the wrong music playing in a zone. An example is the
Overworld and Lab. Just walk in to the Lab and immediately walk back
out, and you'll get Potential for Anything playing in the Overworld.
This regression was caused by facb079b35.
That commit removed assigning -1 to currentsong when a fadeout was
called.
Basically, the previous behavior was: currentsong is 4, we enter Lab and
nicechange gets queued to 3 but currentsong gets set to -1, then going
back nicechange gets queued to 4 again.
However, if we don't assign -1, then going back will keep nicechange at
3. Why? Because niceplay() checks for currentsong before assigning
nicechange. If currentsong is still the same then it doesn't assign
nicechange.
To fix this, just always unconditionally assign nicechange.
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 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.
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.
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().
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.
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.
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 fixes a bug where the resumemusic() script command would always
play MMMMMM track 15 (or, if you're using PPPPPP, just not work). This
is because musicclass::haltdasmusik() assigns resumesong AFTER calling
Mix_HaltMusic(), but the songend() callback fires before the resumesong
assignment, meaning resumesong gets set to -1 instead of whatever
currentsong was previously.
To fix this, just move the assignment into the callback itself (I don't
know why this wasn't done before). I could have moved it to before the
Mix_HaltMusic() call, but moving it into the callback itself fixes it
for all cases of the music stopping (such as when the music fades out).
