Skip to content
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

Loop main menu background music #2542

Closed
wants to merge 2 commits into from
Closed

Loop main menu background music #2542

wants to merge 2 commits into from

Conversation

arpan98
Copy link
Member

@arpan98 arpan98 commented Oct 14, 2016

Contains

Fix for #2527

How to test

  • Open game
  • Wait for music to stop
  • Wait for more than 3 minutes 29 seconds (Duration of Menu Theme Song)

@Cervator
Copy link
Member

So this one kinda works! But a little too well :D

The endListener is a little too thorough. It replays the main menu music every time it ends, including when it should end. For instance it starts playing again when the player enters a game (where there is either no music or the main game soundtrack may be enabled to play)

Probably it could have a little check added to make sure the player is actually still in the main menu. That might be enough of a fix.

On a related note of interest (no action needed here from you @arpan98 - just notes in general): This sort of side-steps #2364 since the abrupt stop now with this PR results in the music immediately replaying again. Yet the underlying issue there still exists, it just gets hidden by this.

We need to remember that when trying to fix the other issue, which would likely involve music not being affected unless whatever context it cares about changes. Context changes several times in the main menu (loading modules, previewing a world, starting to load a game). Something like module availability cares about that (the preview screen needs to have the enabled modules available) so that context would change, but it shouldn't interfere with the menu music.

@Cervator Cervator added the Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience label Oct 15, 2016
@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@arpan98
Copy link
Member Author

arpan98 commented Oct 15, 2016

I added a boolean to keep track of whether the music needs to be stopped or not. Also lambdified the endListener as suggested by @rzats.

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@Cervator
Copy link
Member

Can confirm it works properly now :-)

This PR has the advantage of working around the context swap, while #2545 (we ended up with two PRs for the same issue) adds the looping into the audio manager (but a bit too much so). Both have good parts, maybe they could be spliced together somehow.

For reference here is good logging for this PR being tested, playing Heroes exactly once:

03:00:08.174 [main] INFO  o.t.musicdirector.MusicDirectorImpl - Enqueued Heroes
03:00:08.178 [main] INFO  o.t.musicdirector.MusicDirectorImpl - Starting to play 'Heroes'
...
03:02:14.151 [main] INFO  o.t.musicdirector.MusicDirectorImpl - Enqueued Snowflakes
03:02:14.152 [main] INFO  o.t.musicdirector.MusicDirectorImpl - Removed Heroes
03:02:14.152 [main] INFO  o.t.musicdirector.MusicDirectorImpl - Enqueued Heaven
...
03:03:23.716 [main] INFO  o.t.musicdirector.MusicDirectorImpl - Song 'Heroes' ended
03:03:23.721 [main] INFO  o.t.musicdirector.MusicDirectorImpl - Starting to play 'Snowflakes'

@msteiger
Copy link
Member

msteiger commented Jan 5, 2017

I suggest integrating the looping functionality into the AudioManager, as suggested in #2545

@GooeyHub
Copy link
Member

GooeyHub commented Jan 5, 2017

Hooray Jenkins reported success with all tests good!

@flo
Copy link
Contributor

flo commented Jan 21, 2017

@msteiger Do you mind if we merge this PR? Even if not perfect I think it is a solution that could work for now till someone does it a better way.

@msteiger
Copy link
Member

Apologies for the late reply. How about merging #2545 instead? The conflict seems to be a small issue.

I can have a look if the original author is no longer available.

@arpan98
Copy link
Member Author

arpan98 commented Mar 5, 2017

Sorry for the long delay. What's the status on this? Does this need any changes?

@Cervator
Copy link
Member

Cervator commented Mar 6, 2017

I think we need to retest this one and #2545 again, that one also just got re-awakened and has new commits. Ideally we'd grab the best from both PRs. Maybe you can look at the code there and advise on whether some of it could be merged together? Other PR needs a rebase at least, it has ended up with a bunch of "noise" in its history.

@arpan98
Copy link
Member Author

arpan98 commented Mar 6, 2017

This PR is a workaround for this specific case. #2545 seems more complete as it integrates the looping functionality into the AudioManager and only seems to need a few style fixes.

@Cervator
Copy link
Member

Closing as finished by #2945 - thanks again @arpan98 :-)

@Cervator Cervator closed this May 11, 2017
@Cervator Cervator added this to the Alpha 8 milestone May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants