OpenLoco / OpenLoco

An open source re-implementation of Chris Sawyer's Locomotion
https://openloco.io/
MIT License
1.3k stars 159 forks source link

Music stops working after song finishes if there is only one song #2667

Open LeeSpork opened 1 month ago

LeeSpork commented 1 month ago

Version information:

Describe the bug If there is only one song available in the "Currently playing" drop-down of the music options, it will fail to continue playing music after this song is finished.

This is especially problematic in the years such as 1938 and 1939, where with "Play only music for current era" selected, "Flying High" is apparently the only option. Thus, if you are playing the game normally with the in-game music enabled (and do not reset the music by pausing and resuming the game), the game will become eerily silent until 1940 rolls around.

The music stays silent even if you toggle "Play Music" (under the ear button) off and on.

This doesn't appear to happen in the original Locomotion: the single music track appears to loop instead of stalling, in the Steam version.

To Reproduce Steps to reproduce the behaviour:

  1. Be in 1938 or 1939 and have "Play only music for current era" selected.
  2. Wait for the song "Flying High" to finish playing.

Note that there will no longer be any music playing, despite the UI indicating that music should be playing (screenshot bellow).

Expected behaviour I would have expected "Flying High" to loop. (The original Locomotion behaviour appears to be that).

Screenshots Image (imagine hearing a distinct lack of music while looking at this screenshot)

LeeSpork commented 1 month ago

The issue seems to be caused by this check: https://github.com/OpenLoco/OpenLoco/blob/45ce5429df6cc30037f2c92db67da08898ece83f/src/OpenLoco/src/Audio/Audio.cpp#L1152 I'm not sure why this check exists. Removing those four lines fixes the issue.

That would however make it possible for the same track to play twice in a row, which makes me question why trackToExclude is being set to the track before the last one that was played: https://github.com/OpenLoco/OpenLoco/blob/45ce5429df6cc30037f2c92db67da08898ece83f/src/OpenLoco/src/Audio/Audio.cpp#L1122

But I guess that is a separate issue?

(Moving the declaration of trackToExclude down one line would prevent it from playing the same track twice in a row, although in the case of 1938-1939, this would cause chooseNextMusicTrack() to come up empty, resulting in it picking a track from addAllSongsToPlaylist() instead of era-specific music.)

LeftofZen commented 1 month ago

Having a brief look at it, I think the if (currentTrackPathId == sample) check is there in case a song A is playing, and you manually request song A again, to prevent it from restarting. Then again you could argue that isn't necessary, or maybe the user actually wants to restart it. You could always pass in whether the new track was requested and its the same as the current one, and only then don't restart it.

To make a single track loop if its the only track in the playlist, that check does need to be removed though, so I'm in support of that. Just spitballing, but potentially adding a check at the end of addCurrentEraSongsToPlaylist if the playlist is empty, and then adding excludeTrack if it exists, would fix this issue, right?

However I think my preferred change would be to ensure no year has only one track in it. From a gameplay perspective, this would be boring and repetitive and the player would just turn the music off. I think having at least 3 tracks active for any one year would be a good goal. What do you think @AaronVanGeffen?

Then again in custom games with custom tracks (in the future), its feasible for a playlist to have a single track active, so we should probably fix the handling of that as described

LeftofZen commented 1 month ago

I like diagrams, and you're right, there is an odd 2 year window where only 1 track is valid, [1938, 1939], and then again in [1941, 1942]. This visualisation makes me strongly believe that there is a track missing in the game, perhaps it was intended and then cut at the last minute, or they just plain forgot it, but this is interesting.

Image

LeeSpork commented 4 weeks ago

Another way to reproduce this issue is by (re-)selecting an audio device in the sound options. (My above pull request also resolves the issue when it is triggered by that method). Might be useful as a faster way to test other potential fixes for this issue if my pull request is not accepted.

LeeSpork commented 2 weeks ago

2699 does not fix this