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).
Apparently in C, if you have `void test();`, it's completely okay to do
`test(2);`. The function will take in the argument, but just discard it
and throw it away. It's like a trash can, and a rude one at that. If you
declare it like `void test(void);`, this is prevented.
This is not a problem in C++ - doing `void test();` and `test(2);` is
guaranteed to result in a compile error (this also means that right now,
at least in all `.cpp` files, nobody is ever calling a void parameter
function with arguments and having their arguments be thrown away).
However, we may not be using C++ in the future, so I just want to lay
down the precedent that if a function takes in no arguments, you must
explicitly declare it as such.
I would've added `-Wstrict-prototypes`, but it produces an annoying
warning message saying it doesn't work in C++ mode if you're compiling
in C++ mode. So it can be added later.
Just like I refactored text splitting to no longer use std::vectors,
std::strings, or temporary heap allocations, decreasing memory usage and
improving performance; there's no reason to use a temporary
heap-allocated std::vector to grab all extra binary blob indices, when
instead the iteration can just be more immediate.
Instead, what I've done is replaced binaryBlob::getExtra() with
binaryBlob::nextExtra(), which takes in a pointer to an index variable,
and will increment the index variable until it reaches an extra track.
After the caller processes the extra track, it is the caller's
responsibility to increment the variable again before passing it back to
getExtra().
This avoids all heap allocations and brings down the memory usage of
processing extra tracks.
Since musicWriteBlob is a temporary object that gets destroyed at the
end of musicclass::init(), in order to make sure we don't leak memory
and lose all the pointers to the blocks we just allocated in
musicWriteBlob, we need to call its clear() method after writing
BinaryMusic.vvv.
musicReadBlob was used for both MMMMMM and PPPPPP soundtracks. This
causes a memory leak if you have mmmmmm.vvv installed, because the
pointers holding each allocated block of MMMMMM would be lost when
PPPPPP got loaded. Valgrind complains about this memory leak.
This is in contrast to 2.2 and previous behavior, where musicReadBlob
was only a temporary object instead of being held in musicclass.
However, this wasn't really a memory leak (moreso something that just
didn't get cleaned up when closing the game), but it did get turned into
a leak when per-level assets mounting and unmounting got introduced in
2.3 (loading a level with custom assets after starting the game with an
mmmmmm.vvv, or exiting out of a level that had an mmmmmm.vvv, would
cause the game to leak memory). Leo recognized this, and moved
musicReadBlob onto musicclass in a separate 2.3 PR, but either he didn't
think about what was happening here too closely, or he didn't use
Valgrind, because he forgot about the memory leak caused by re-using the
same binaryBlob for PPPPPP and MMMMMM.
So instead, just use two different binaryBlob objects for MMMMMM and
PPPPPP. That way, no memory leaks happen.
I'm going to introduce another binaryBlob object in to the mix, and I
want to be able to re-use an existing FOREACH_TRACK #define without
having to copy-paste it again. So, TRACK_NAMES now takes in a blob
parameter, which will be passed to the temporary FOREACH_TRACK #define.
This removes the music cleanup code from musicclass::init(), and
requires that we also call destroy() in Graphics::reloadresources().
This is because we'll need to re-use the musicclass cleanup code
elsewhere, and we don't want to copy-paste the cleanup code. Or at
least, I don't (but I'm not a game dev, game devs copy-paste all the
friggin' time).
This patch cleans up unnecessary exports from header files (there were
only a few), as well as adds the static keyword to all symbols that
aren't exported and are specific to a file. This helps the linker out in
not doing any unnecessary work, speeding it up and avoiding silent
symbol conflicts (otherwise two symbols with the same name (and
type/signature in C++) would quietly resolve as okay by the linker).
With commit 48313169b6 (PR #453),
AllyTally added a single-case patch for a regression, instead of fixing
it at its root cause.
In fact, that commit only fixes the music if Presenting VVVVVV is
playing while exiting to the menu, not if you enter a level that plays
Presenting VVVVVV - so it only fixes it going one way, and not going the
other way around; neither fixing also all the other cases this could
happen.
It doesn't, say, fix the case where you are exited to the menu
automatically after collecting the last crewmate in the level (or if the
level calls gamestate 1013 itself), which is what happens in my MIRA-VIU
TAS video at the end, and which I noted in the description of that video
( https://www.youtube.com/watch?v=OYQO4ePbYW4&t=111 ).
So, the problem here is that when musicclass::play() is called, it sees
that currentsong is the same as its input, and decides that since the
music is already playing, it shouldn't play the music again. Thus, the
music fades out, and we get silence instead of the music playing again.
But I said this was a regression. Why didn't this happen in 2.2? Well,
it's because of the fact that 2.2 sets currentsong to -1 (no music
playing at all) immediately when starting a fadeout, and not when the
fadeout completes (commit facb079b35,
PR #316). As you can imagine, this discrepancy could lead to bugs, given
that the game would think that music wasn't playing when in actuality it
was, but fixing this bug could also break code that expected this wrong
behavior. And in this case, it has.
So to properly fix the root cause of this, instead of naïvely
single-case patching out every case that comes up randomly, in
musicclass::play(), the function will now ignore if the input given is
the same as currentsong if the music is currently fading out.
