savegame: add triggered music tracks to the savegame

[walkawayy]

Resolves #371.

Checklist

• I have read the coding conventions
• I have added a changelog entry about what my pull request accomplishes, or it is an internal change

Description

Added the triggered music tracks to the savegame so one shot tracks don't replay on load. I'm afk this weekend. But I wanted some feedback on the naming, behavior, etc of this. This saves off g_MusicTrackFlags for new save format saves. The original game only did this for Pierre, Cowboy, Baldy, and Larson (which for some reason uses MX_BALDY_SPEECH which could be a T1M bug, but I haven't looked into it yet). The implementation means the game behaves similar to how a level plays out if the player never reloads. Each MUSIC_TRACK_ID can only be played once in a level unless it's marked as one shot. This could be limiting to custom levels potentially, but it's also how the OG game behaves now. And custom levels do get 60 music slots to work with. Below are some examples of how it behaves and how it depends on level files.

This one shot Colosseum trigger doesn't play again on reload:

But this non one shot St Francis Folly music trigger will always play:

[lahm86]

Noticed a bit of an issue, I'm not sure if it's just timed triggers but it seems thar after the first time the switch here is used, the ambient track resets when you step on the tile again. Update: this seems to be an original issue, just double checked.

https://www.youtube.com/watch?v=Al7mT2asvJA

It also somehow restored the other track when I saved and reloaded even though it had already finished.

[walkawayy]

Ok so there definitely are two things happening here:

1. Maybe there is a bug from the previous PR for saving / reloading the timestamp where it saves an inactive track and restores it.
2. For that Caves switch, there is definitely some weird stuff going on. The PS1 version didn't have ambient tracks, but the game plays the correct music trigger track 12 when that switch is pulled. In the PC version, there are two identical ambient tracks. MX_CAVES_AMBIENT = 5 and MX_ST_FRANCIS_FOLLY_AMBIENCE = 57 (looks like I might need to change those names). The gameflow gives Caves music as "music": 57,. This shouldn't be a big deal because the 57 looped ambience is the same as 5 (57 does have an extra 4 seconds of silence though strangely). However, in the level file, Caves actually has a trigger on level start saying to play track 5 (screenshot below). And in Music_Play there is a special condition where track 5 can never play. This condition was probably added because the level file's opening music trigger is screwed up because the ambience shouldn't use a music trigger. It should be a looped ambience.

if (track == MX_CAVES_AMBIENT) {
    return false;
}

So this is a bunch of background info to say that Room_TriggerMusicTrack is calling Music_StopTrack(track); when the switch timer ends from what I can tell. Which might be semi related to #783 @lahm86. Here's the relevant code from room.c. Is this a an OG bug? Or a side effect of the PR I linked?

    if ((g_MusicTrackFlags[track] & IF_CODE_BITS) == IF_CODE_BITS) {
        if (flags & IF_ONESHOT) {
            g_MusicTrackFlags[track] |= IF_ONESHOT;
        }
        Music_Play(track);
    } else {
        Music_StopTrack(track);
    }

[lahm86]

You're right, I don't think it's related to this PR at all (apologies for confusing things). I just checked TombATI and if you step on a switch tile again as in the video above, the ambient track stops altogether, so there is definitely something lingering there. #783 could be related indeed; at least for now, restarting the ambient is better than it stopping entirely like TombATI. We should probably raise another issue to investigate that though.

For this PR, other than the potential timestamp issue, the g_MusicTrackFlags restore looks good imo.

[rr-]

Potential timestamp issue

Can we double-check this?

[walkawayy]

I'm gonna put this on hold and probably do two new PRs before this.

The first will investigate the potential timestamp bug. I'd rather solve that separately before adding this PR to the mix if it's an unrelated bug.

The second will bump the savegame version. Then I can add the appropriate savegame checks to this PR after.

[walkawayy]

You're right, I don't think it's related to this PR at all (apologies for confusing things). I just checked TombATI and if you step on a switch tile again as in the video above, the ambient track stops altogether, so there is definitely something lingering there. #783 could be related indeed; at least for now, restarting the ambient is better than it stopping entirely like TombATI. We should probably raise another issue to investigate that though.

For this PR, other than the potential timestamp issue, the g_MusicTrackFlags restore looks good imo.

Ok I'll open an issue for it later tonight. The weird thing is your PR mentioned the triggers where the tracks would stop were caused by incorrectly flagged triggers. But this switch trigger seemed like normal flags to me? Though you know about them more than me.

[lahm86]

You're right, I don't think it's related to this PR at all (apologies for confusing things). I just checked TombATI and if you step on a switch tile again as in the video above, the ambient track stops altogether, so there is definitely something lingering there. #783 could be related indeed; at least for now, restarting the ambient is better than it stopping entirely like TombATI. We should probably raise another issue to investigate that though.
For this PR, other than the potential timestamp issue, the g_MusicTrackFlags restore looks good imo.

Ok I'll open an issue for it later tonight. The weird thing is your PR mentioned the triggers where the tracks would stop were caused by incorrectly flagged triggers. But this switch trigger seemed like normal flags to me? Though you know about them more than me.

Yeah, it could be the way the flags are changed based on the trigger type in Room_TriggerMusicTrack. I think it's then calling Music_StopTrack and that's falling through and restarting the ambient - the initial condition there is probably passing because even though the track has stopped, Music_CurrentTrack still holds the last played track. Maybe there needs to be a flag to store whether or not the track has finished?

[rr-]

WDYT about this?

diff --git a/src/game/music.c b/src/game/music.c
index b0000132..edd6ee4f 100644
--- a/src/game/music.c
+++ b/src/game/music.c
@@ -18,6 +18,7 @@ static int m_AudioStreamID = -1;
 static int16_t m_Track = 0;
 static int16_t m_TrackLooped = -1;
 
+static bool Music_IsBrokenTrack(int16_t track);
 static void Music_StopActiveStream(void);
 static void Music_StreamFinished(int stream_id, void *user_data);
 static char *Music_GetTrackFileName(int track);
@@ -36,6 +37,11 @@ static void Music_StopActiveStream(void)
     S_Audio_StreamSoundClose(m_AudioStreamID);
 }
 
+static bool Music_IsBrokenTrack(int16_t track)
+{
+    return track == MX_UNUSED_0 || track == MX_UNUSED_1 || track == MX_UNUSED_2;
+}
+
 static char *Music_GetTrackFileName(int track)
 {
     char file_path[64];
@@ -70,11 +76,7 @@ void Music_Shutdown(void)
 
 bool Music_Play(int16_t track)
 {
-    if (track == Music_CurrentTrack()) {
-        return false;
-    }
-
-    if (track <= MX_UNUSED_1) {
+    if (track == Music_CurrentTrack() || Music_IsBrokenTrack(track)) {
         return false;
     }
 
@@ -88,10 +90,6 @@ bool Music_Play(int16_t track)
             SFX_BALDY_SPEECH + track - MX_BALDY_SPEECH, NULL, SPM_ALWAYS);
     }
 
-    if (track == MX_CAVES_AMBIENT) {
-        return false;
-    }
-
     Music_StopActiveStream();
 
     char *file_path = Music_GetTrackFileName(track);
@@ -114,7 +112,7 @@ bool Music_Play(int16_t track)
 
 bool Music_PlayLooped(int16_t track)
 {
-    if (track == Music_CurrentTrack()) {
+    if (track == Music_CurrentTrack() || Music_IsBrokenTrack(track)) {
         return false;
     }
 
@@ -153,7 +151,7 @@ void Music_Stop(void)
 
 void Music_StopTrack(int16_t track)
 {
-    if (track != Music_CurrentTrack()) {
+    if (track != Music_CurrentTrack() || Music_IsBrokenTrack(track)) {
         return;
     }
 
diff --git a/src/global/types.h b/src/global/types.h
index e1831dde..ed363a99 100644
--- a/src/global/types.h
+++ b/src/global/types.h
@@ -482,7 +482,7 @@ typedef enum MUSIC_TRACK_ID {
     MX_TR_THEME = 2,
     MX_WHERE_THE_DEPTHS_UNFOLD_1 = 3,
     MX_TR_THEME_ALT_1 = 4,
-    MX_CAVES_AMBIENT = 5,
+    MX_UNUSED_2 = 5,
     MX_TIME_TO_RUN_1 = 6,
     MX_FRIEND_SINCE_GONE = 7,
     MX_T_REX_1 = 8,
@@ -534,7 +534,7 @@ typedef enum MUSIC_TRACK_ID {
     MX_NATLA_SPEECH = 54,
     MX_PIERRE_SPEECH = 55,
     MX_SKATEKID_SPEECH = 56,
-    MX_ST_FRANCIS_FOLLY_AMBIENCE = 57,
+    MX_CAVES_AMBIENCE = 57,
     MX_CISTERN_AMBIENCE = 58,
     MX_WINDY_AMBIENCE = 59,
     MX_ATLANTIS_AMBIENCE = 60,

[walkawayy]

Ok I did some testing and it works. The exception being triggers not marked as one shot like the Great Pyramid Torso music.

If the track finishes, the players saves, then reloads, the music will restart.
If the player saves while the track is playing, the track resumes on reload then finishes but doesn't replay because the game knows it's the current track.

This might be the best we can do? Unless we go injecting these non one shot triggers into one shot. Lmk what you guys think.

[rr-]

Almost there. It'd be good to give it one final round of thorough testing, ideally in caves, with the botched lever trigger, the wolf music, the secret sfx, save/reload and check for audio clipping too.

Edit: I just noticed the comment above. Yes, that's fine. I'm not sure how to tackle the Great Pyramid Torso trigger issue, other than with injections. Which I don't think are super-necessary.

[lahm86]

Converting to one shot triggers would be straight-forward, and I'd be happy to take it on. Perhaps we can make a list in #755 of exactly where these are needed, and then begin to plan implementation along with the other FD tasks. We have the fix_floor_data_issues option already so imo these would fall under that category.

[rr-]

Sounds good to me :)

[walkawayy]

Ok joined sg VERSION_3 conditions. Should we keep the one shot trigger fixes in this PR or put those in a new one?

[rr-]

Let's tackle that separately