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

Add volume setting to ini #26

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add volume setting to ini #26

wants to merge 4 commits into from

Conversation

dagit
Copy link

@dagit dagit commented Mar 17, 2023

Description

I found that I kept resetting the volume level every time I loaded up the game. So I added a config option to the ini file.

Will this Pull Request break anything?

It shouldn't but I only tested it on linux.

Suggested Testing Steps

Change the volume setting in the ini file. The range is probably platform dependent, but SDL seems to go from 0 to 128 and it looks like windows probably ranges from 0 to 100. So the ranges aren't that different hopefully.

Copy link
Owner

@snesrev snesrev left a comment

Choose a reason for hiding this comment

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

SDL_MIX_MAXVOLUME goes from 0-128 not 0-100, it needs to be 0-100 in the config file and that should be translated to 128 for SDL. Also is there a way to use less #ifdef

@dagit
Copy link
Author

dagit commented Mar 20, 2023

I refactored it so that the current mixer volume is internally always in the range 0 to 100, but when updating the SDL mixer it scales that value into the range 0 to 128.

I don't really see a way to use less #ifdef here. There's a bit of fundamental complexity due to using two different mixer systems. I've tried to hide that complexity behind the getter and setter functions so that the rest of the code can be written as if there is just one API. I believe that's a pretty decent trade-off and if you want me to do it a different way you'll have to explain what you have in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants