New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scenario music does not play when loading saves #2602

Closed
Vultraz opened this Issue Mar 6, 2018 · 25 comments

Comments

Projects
None yet
8 participants
@Vultraz
Member

Vultraz commented Mar 6, 2018

There's an issue in the sound code that prevents scenario music from playing when a game is loaded. Loading a start-of-scenario save works correctly, though.I think this has something to do with sound::commit_music_changes. playsingle_controller::play_scenario properly parses scenario music, but when commit_music_changes is called, it exists early at sound.cpp:790 and the main menu music continues to play. It's also reported that the scenario playlist does not begin playing after the currently playing track ends (though I did not confirm this first hand).

This issue also affects the MP Lobby. Despite game_config::lobby_music being set to silence.ogg, the main menu music never stops playing.

@Vultraz Vultraz added this to the 1.14.0 milestone Mar 6, 2018

@jyrkive

This comment has been minimized.

Member

jyrkive commented Mar 6, 2018

The early exit in sound.cpp:790 means that the main menu track is present in the current playlist. Obviously, it shouldn't be: there aren't exactly many scenarios which have the main menu music in their playlist.

It's expected behavior that the track doesn't change until the current track ends. That's how Wesnoth 1.13 has worked as long as I can remember (starting from summer 2016, if not earlier).

@Vultraz

This comment has been minimized.

Member

Vultraz commented Mar 6, 2018

No, it just means the track is marked as "play once". The "track is in list" exit is L796.

Perhaps we need an explicit fadeout call?

@jyrkive

This comment has been minimized.

Member

jyrkive commented Mar 6, 2018

Oh, right. I see.

An explicit fadeout call shouldn't be necessary: https://github.com/wesnoth/wesnoth/blob/master/src/sound.cpp#L723-L725

@Vultraz

This comment has been minimized.

Member

Vultraz commented Mar 6, 2018

Hmmmmm....I wonder if the problem is that there is no immediate=yes in any of the scenario tracks... But that begs the question why it works with SoS saves and brand new games.

@jyrkive

This comment has been minimized.

Member

jyrkive commented Mar 6, 2018

Changing the music when loading a save has worked for a long time, though. This must be a recent regression, and I think it points towards some kind of saving bug - an extremely severe problem.

@Vultraz

This comment has been minimized.

Member

Vultraz commented Mar 9, 2018

I've determined that the playlists are at least properly constructed. The issue has something to do with what's being played.

@Vultraz

This comment has been minimized.

Member

Vultraz commented Mar 9, 2018

The bit about the scenario playlist not starting after the main menu track finishes appears to be false. Waiting for the main menu theme to finish in a DiD save then gives me the last track in the scenario playlist (battle.ogg).

@Vultraz

This comment has been minimized.

Member

Vultraz commented Mar 9, 2018

Also, this bug appears to function both ways. Scenario music doesn't stop when returning to titlescreen IF you loaded a scenario save. Starting the save normally, such as through the campaign menu, does result in scenario music correctly playing, AND the main menu music correctly starting again when quitting the game.

@jyrkive

This comment has been minimized.

Member

jyrkive commented Mar 9, 2018

The bit about the scenario playlist not starting after the main menu track finishes appears to be false. Waiting for the main menu theme to finish in a DiD save then gives me the last track in the scenario playlist (battle.ogg).

In that case this isn't a bug. That's how Wesnoth 1.13 has worked since forever.

@jyrkive jyrkive closed this Mar 9, 2018

@jyrkive jyrkive added Invalid and removed Blocker Bug labels Mar 9, 2018

@Vultraz

This comment has been minimized.

Member

Vultraz commented Mar 10, 2018

Somehow that doesn't feel right...Are we sure it isn't a long-standing regression in 1.13?

@CelticMinstrel

This comment has been minimized.

Member

CelticMinstrel commented Mar 10, 2018

The music system is certainly working the way it's intended to - if you set a new playlist without immediate=yes, the currently-playing track will continue to play to the end, and then the game will move onto the new playlist.

You could however make an argument that, when loading a saved game, the currently-playing track should be stopped to allow the scenario music to take over. Which would be what this issue is about. So... I'd argue that it's not invalid, but it certainly isn't a blocker either. I'm not even sure it would be a bug rather than an enhancement.

@gfgtdf

This comment has been minimized.

Contributor

gfgtdf commented Mar 10, 2018

when loading a save the current track should be stopped and somethgin from the new list shodul be played instead. Unless the currently playing track is also on the new playlist in which case it shodul finish the current track as if nothgin happened. The later is specialy important when loading a game from inside a game from the same scenario, you don't want an interrupotion of the music in that case.

@Vultraz Vultraz reopened this Mar 21, 2018

@shikadiqueen

This comment has been minimized.

Member

shikadiqueen commented Mar 21, 2018

This is not how music used to work when loading saves in 1.12.x, therefore it is a regression.

1.12.x would instantly switch to the scenario's playlist (probably selecting a random track from the playlist if it had more than one entry, haven't checked that) upon loading a save. I do believe this behaviour did not always exist and may have been introduced during 1.9.x, but it doesn't change the fact that some of us do rely on it and don't want the title screen music or whatever else was playing before to keep playing over what we chose for our scenarios.

@shikadiqueen shikadiqueen added Bug and removed Invalid labels Mar 21, 2018

@Vultraz Vultraz added the Regression label Mar 21, 2018

@Vultraz

This comment has been minimized.

Member

Vultraz commented Mar 21, 2018

7dea2b9 appears to be the cause.

@CelticMinstrel

This comment has been minimized.

Member

CelticMinstrel commented Mar 21, 2018

What is GNA23065 that the commit claims to fix?

@sevu

This comment has been minimized.

Member

sevu commented Mar 25, 2018

Who has a local backup of gna 🎉

@Vultraz

This comment has been minimized.

Member

Vultraz commented Mar 25, 2018

There’s a copy on archive.org.

@sapientN3T

This comment has been minimized.

Contributor

sapientN3T commented Mar 25, 2018

GNA-23065 [music] unable to replace playlist without interrupting current track

"It should be possible to change the playlist without interrupting the current track by using immediate,append=no,no. Currently, doing so does cause an immediate switch to the new playlist. "

https://web.archive.org/web/20170204102655/https://gna.org/bugs/index.php?23065

@CelticMinstrel

This comment has been minimized.

Member

CelticMinstrel commented Mar 25, 2018

At the moment I'm thinking the best fix is likely just to call sound::stop_music() as soon as you're committed to loading a game (so when you click the load button, and after selecting the difficulty if you're switching difficulties). That would mean there's no music for the loading screen, but I can't think of a way around this.

@Vultraz

This comment has been minimized.

Member

Vultraz commented Mar 25, 2018

I have a local change that adds a force parameter to commit_music_changes that skips the play_once check. It seems to work as expected, though music doesn't stop until you enter the game itself.

@CelticMinstrel

This comment has been minimized.

Member

CelticMinstrel commented Apr 12, 2018

A force parameter was one of the things I considered, actually. As long as the currently-playing track doesn't stop if the playlist is changed mid-scenario, this should be fine.

@Wedge009

This comment has been minimized.

Member

Wedge009 commented May 2, 2018

Personally, I like that there's no forced change on loading/exiting a saved game, I think I noticed this change some time ago. But I agree it is a regression and starting a new campaign does force a reset of the music to whatever the scenario specifies.

@shikadiqueen

This comment has been minimized.

Member

shikadiqueen commented May 21, 2018

Any progress?

@jyrkive

This comment has been minimized.

Member

jyrkive commented Jun 5, 2018

I'm taking this one.

@Wedge009

This comment has been minimized.

Member

Wedge009 commented Jun 5, 2018

Aside from the unwanted music change-on-reload that @shikadiqueen mentioned in #wesnoth-dev, I'm not sure if @Vultraz still wants the MP Lobby to have the MP Lobby music restored to silence.

jyrkive added a commit that referenced this issue Jun 5, 2018

Revert "Fix #2602: music doesn't change immediately on loading a save"
This reverts commit 1fb9378.

The commit caused the music to change even when the player loaded a save
from the same scenario. A different fix is needed.

jyrkive added a commit that referenced this issue Jun 5, 2018

Fix #2602: music doesn't change immediately on loading a save
This time I added an option to disable the feature to allow the currently
playing track to finish when changing the playlist. This allows more
fine-grained control of distinct use cases.

In wesnoth.cpp:do_gameloop(), I reversed the order of the title screen
music and default music because otherwise adding the default music would
enable play_once for the title screen music and prevent instant music
change when the player loads a save. I play title screen music with
immediate=yes, so it's still played first.

jyrkive added a commit to jyrkive/wesnoth that referenced this issue Jun 5, 2018

jyrkive added a commit to jyrkive/wesnoth that referenced this issue Jun 5, 2018

Fix wesnoth#2602: music doesn't change immediately on loading a save
This time I added an option to disable the feature to allow the currently
playing track to finish when changing the playlist. This allows more
fine-grained control of distinct use cases.

In wesnoth.cpp:do_gameloop(), I reversed the order of the title screen
music and default music because otherwise adding the default music would
enable play_once for the title screen music and prevent instant music
change when the player loads a save. I play title screen music with
immediate=yes, so it's still played first.

jostephd added a commit to jostephd/wesnoth that referenced this issue Oct 6, 2018

jostephd added a commit to jostephd/wesnoth that referenced this issue Oct 6, 2018

Fix wesnoth#2602: music doesn't change immediately on loading a save
This time I added an option to disable the feature to allow the currently
playing track to finish when changing the playlist. This allows more
fine-grained control of distinct use cases.

In wesnoth.cpp:do_gameloop(), I reversed the order of the title screen
music and default music because otherwise adding the default music would
enable play_once for the title screen music and prevent instant music
change when the player loads a save. I play title screen music with
immediate=yes, so it's still played first.

jostephd pushed a commit to jostephd/wesnoth that referenced this issue Oct 7, 2018

jostephd pushed a commit to jostephd/wesnoth that referenced this issue Oct 7, 2018

Revert "Fix wesnoth#2602: music doesn't change immediately on loading…
… a save"

This reverts commit 94b69e0.

(cherry-picked from commit cafede4)

jostephd pushed a commit to jostephd/wesnoth that referenced this issue Oct 7, 2018

Fix wesnoth#2602: music doesn't change immediately on loading a save
This time I added an option to disable the feature to allow the currently
playing track to finish when changing the playlist. This allows more
fine-grained control of distinct use cases.

In wesnoth.cpp:do_gameloop(), I reversed the order of the title screen
music and default music because otherwise adding the default music would
enable play_once for the title screen music and prevent instant music
change when the player loads a save. I play title screen music with
immediate=yes, so it's still played first.

(cherry-picked from commit 91afbfd)

@jostephd jostephd referenced this issue Oct 14, 2018

Closed

Forward-port 1.14 to development #3614

959 of 959 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment