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

Feature #9059: Add buttons to toggle music in the Game Options menu. #9727

Merged
merged 5 commits into from Nov 8, 2022

Conversation

a2aaron
Copy link
Contributor

@a2aaron a2aaron commented Dec 3, 2021

Motivation / Problem

Players could not start or stop playing music while on the title screen. This would mean that, if you stopped music during gameplay and return to the titlescreen, the music would still be stopped and there would be no way to restart it unless you went into a game and turned it back on. This PR addresses this issue by allowing you to turn the music on in the titlescreen.
Closes #9059

Description

This is a small enhancement to allow the player to play or stop music while on the title screen by adding start and stop buttons to the Game Options menu. They are located next to the music volume widget.

image

Limitations

There was discussion on the issue (#9059) that adding buttons might clutter the interface and that it would be better if the music simply auto-played when you selected a new music set, but it didn't seem like there was any followup. I can change this PR to do that, (although personally I think having explicit buttons is clearer and doesn't add much clutter)

Also, I ended up using the existing play and stop sprites that are used for the actual music GUI, but I'm not sure if those look the best. I can make some sprites if people do not think they fit very well.

Checklist for review

None of the below bullet points apply.

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@nielsmh
Copy link
Contributor

nielsmh commented Dec 3, 2021

It starts to get messy, and at this point maybe it's better to just add a button to the main menu to open the Jukebox window.
Another possibility would be to make it clearer how to change to the NoMusic set, by changing the list from a dropdown to an always-visible list.

@2TallTyler
Copy link
Member

2TallTyler commented Dec 3, 2021

I second the suggestion of adding a button to open the Jukebox window. More functionality with less code duplication, etc. A possible complication: the way music is currently set up, only song 00 is available in the main menu, and it is not available in any of the main game programmes (except custom programmes). If the Jukebox was added to the main menu screen, it might need either a hidden main menu programme which contains only 00, or for this behavior to change to allow all songs to play in the main menu.

I am -1 to using NoMusic to choose silence. I would look for a Stop button for a long time before I thought to simply choose a blank music set.

@a2aaron
Copy link
Contributor Author

a2aaron commented Dec 4, 2021

Ok, I will try implementing the Jukebox idea some time this weekend. Thank you for the feedback.

@nielsmh
Copy link
Contributor

nielsmh commented Dec 4, 2021

@2TallTyler I made a mockup of what I mean by showing the list right away.
image
The idea is that you get a place to click right away, that obviously says "No Music".

As it is, the music on the main menu is already implemented via a sixth "secret" playlist that only contains the title theme song.

@2TallTyler
Copy link
Member

I rescind my -1 to your solution. That looks pretty slick.

@a2aaron
Copy link
Contributor Author

a2aaron commented Dec 5, 2021

Okay, I've changed it so that the jukebox is now accessible via the game options menu. Like discussed in the comments above, the jukebox only allows you to access song 00, but you can start and stop the music using it. I will note that doing this is somewhat silly looking, since the jukebox opens in the upper left corner of the game's window by default.

Note: Should I squash these two commits into one commit for this PR? (I did read through CONTRIBUTING.md but wasn't sure if I should wait till I receive approval before fixing-up the commit history or not)

Here is an image of what the UI now looks like below. I used the same sprite image that the toolbar uses for opening the jukebox. I'm not sure if it's clear in this context. Should I use a text button instead?

image

src/settings_gui.cpp Outdated Show resolved Hide resolved
@ldpl
Copy link
Contributor

ldpl commented Dec 5, 2021

The idea is that you get a place to click right away, that obviously says "No Music".

Seeing how players get confused with simplest things when it comes to basesets I feel like this approach is not as obvious as you think. Mute button is a universally recognizable symbol, big scary block with bunch of text and buttons is not. Especially in a panic mute situation.

@LC-Zorg
Copy link

LC-Zorg commented Dec 5, 2021

I think that in a panic situation the inscription "No Music" would be very "obvious" ;) Overall it wouldn't be bad, but it would definitely require adding a scroll bar, because there can be many more than a few positions. This window would also have to be much wider than on the mockup due to possible long names of add-ons.

Generally the idea of adding an icon to open the Jazz Jukebox is quite nice imho, but it looks flawed now. The icon increased the height of the slider and the music set selection box. Correcting the height will cause the distance between the selection box and the text to be different than in the rest of the window.
Maybe it would be better to put this button below the slider? This would require a narrowing of the text field a bit, but the rest is the same.
Jazz Jukebox in Game Options Window

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, I think I agree with @LC-Zorg that the button below the volume slider looks a bit nicer. Might be trickier to implement though, depending on how flexible that text widget is.

Probably needs some experimentation with different icon/text zoom levels as well

Good work!

src/settings_gui.cpp Outdated Show resolved Hide resolved
src/widgets/settings_widget.h Outdated Show resolved Hide resolved
src/settings_gui.cpp Outdated Show resolved Hide resolved
@2TallTyler 2TallTyler added preview This PR is receiving preview builds size: trivial This Pull Request is trivial labels Nov 7, 2022
@DorpsGek DorpsGek temporarily deployed to preview-pr-9727 November 7, 2022 18:08 Inactive
@2TallTyler
Copy link
Member

@a2aaron Are you interested in finishing this PR with the requested changes? I'm making an effort to review and approve "low-hanging fruit" PRs and this is on my list. 🙂

@a2aaron
Copy link
Contributor Author

a2aaron commented Nov 8, 2022

Sure, I can make the changes later tonight!

@DorpsGek DorpsGek temporarily deployed to preview-pr-9727 November 8, 2022 02:17 Inactive
@a2aaron
Copy link
Contributor Author

a2aaron commented Nov 8, 2022

Alright, I've made the changes! I did the two minor style changes (i'm deciding to keep the braces in the switch case, since I think it looks more in line with the other code blocks around it). I also was able to move the Open Jukebox widget below the volume slider as requested.

2TallTyler
2TallTyler previously approved these changes Nov 8, 2022
Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the code fixes. Here's a screenshot of the new button layout using OpenGFX. Looks good to me.

jukeboxButton

@DorpsGek DorpsGek temporarily deployed to preview-pr-9727 November 8, 2022 15:03 Inactive
@LordAro LordAro merged commit ede0560 into OpenTTD:master Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds size: trivial This Pull Request is trivial waiting on author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There is no way to toggle play/stop music in the title screen
8 participants