-
-
Notifications
You must be signed in to change notification settings - Fork 154
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
Keep track of floating point volume internally for Sound
and Channel
#3091
base: main
Are you sure you want to change the base?
Keep track of floating point volume internally for Sound
and Channel
#3091
Conversation
I don't think we should lie to the users about what their volume actually is. |
I'm unsure how we would be lying here. Obviously the sound is not going to have infinite resolution anyway, so at some point, tiny changes won't actually affect the volume regardless of whether the resolution is a 129 steps or a 1000. This is what I would call "close enough", at no point is the returned volume going to differ from the "actual" volume by more than 0.008. That is a tiny maximum difference and in my opinion the practicality of returning a more accurate value (as in, closer to what it was set to) outweighs any such discrepancies. sound.set_volume(sound.get_volume() + dt * 0.1) Well, this simply won't work the way those values are handled currently. That aside, this is (at least almost) like saying we're lying to the users about what their actual The current behavior is also somewhat less intuitive in my opinion. Sure, we can make a note in the docs about this, but that still violates the intuition of how one might expect it to behave and it is less practical. I see no harm in making it behave this way, in fact, I see a couple benefits. If none of the above is convincing, here's an alternative, we add the note about this behavior in the docs and add a |
@Starbuck5, don’t you think the getter should return the exact value that the setter sets? In this case, the getter returns a different value due to an implementation detail that isn’t mentioned in the docs. Isn’t the point of an API to hide such internals and provide simple methods to access parts of an object without requiring users to consider what’s beneath the API layer? |
I think the function
This is incorrect. Looking at the SDL_mixer headers on their main branch they still very much use MIX_MAX_VOLUME. What you're reading is an SDL changelog, not an SDL_mixer changelog.
I want the Python function to return the actual C state, both here and in Rect. If you applied this PR's logic to Rect each Rect would have a bunch of random stored floats to try to return the value the user "expects" from the Rect instead of the actual value.
Code complexity, a small increase in runtime memory use. @Matiiss you've brought up music several times in this thread but you haven't changed the behavior of music.get_volume, which has the same behavior as the other get_volumes. |
Why not both? Why not having 2 methods like I am asking this for the next time because you didn't respond, and I would like to understand your point of view. |
Looking into human decibel perception it seem generally agreed that we can only distinguish in around 3 decibel increments. AFAICT reasonably expensive sound equipment promises around 130db range which would be about 43 audible increments of volume. Based on that back-of-the-envelope math I don't see much incentive for SDL Mixer to increase from 128 increment precision to floating point in the future. What is the real purpose of us changing what these functions do/return? It is all different types of 'lying' about what is actually changing. Right now we are 'lying' that you can increment volume by values like 0.01 and have it actually change anything. Which it won't either mathematically in SDL Mixer or audibly to anyone listening. The get volume returns neither exactly what SDL Mixer is doing (as it returns a float conversion of the integer internal state), and even if it did those increments themselves are inaudible unless you move them up or down in increments of at least 3 ints. Generally we should probably document the volume functions better with this kind of information so users have an idea of what is happening. If there is a useful, practical reason to change the current 'lying' behaviour to different 'lying' behaviour then please let us know what it is or where this issue came up in programming. AFAICT this is a fairly uncommon issue, though I found one other person noting it on stack overflow where the user didn't seem to know how to round numbers themselves. The default presumption has to be to not change behaviour, even seemingly minor changes like this, unless there is a compelling reason to do so - as no doubt somebody, somewhere is relying on the current implementation. So, please let us know a good 'why?' use case for motivation; beyond that you noticed it and and found it odd or mildly annoying. |
Currently I can list two reasons:
It's highly unlikely someone actually relies on the current behavior, even if someone does, I can't then imagine a way this could break their code. (Assuming their code is relatively reasonable, of course (in before someone hits me with an obscure example)) Also, as mentioned, we wouldn't hear all the increments anyway, so the volume seemingly not changing after incremented by a tiny bit is on par with the proposed changes, where the returned value would not necessarily reflect the actual volume, but the "actual volume" is not that important because even if SDL did use floats, they would have limited precision regarding the actual volume being outputted anyway. I already mentioned this in my previous comment along other points #3091 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a question I want the answer.
Except that, this is a nice job.
// never be raised, it is here simply as a precaution | ||
#define MIXER_VOLUME_DELTA_CHECK(delta) \ | ||
do { \ | ||
if (delta > MIXER_VOLUME_DELTA) { \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit skeptical of this method, comparing 2 floats, especially floats like this, is something we're taught to avoid generally. Why not at least go for 0.0078125
which seems to be an exact float representation, or a greater decimal number with also an exact float representation ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it 0.008
? To add a small margin of error to the exact value of 0.78125
. I do not want this check to be true if for some reason the delta is 0.79125
, because that may or may not (I can't say I know for certain) also happen under normal operation. This check is mainly serving as an internal safeguard/sanity check for when the delta is unexpectedly large.
@Matiiss I can see you forgot |
I didn't forget it, I was planning on covering it in a separate PR, but at this point the future of this idea is uncertain. |
Fixes #3090
This essentially solves the 129-step precision that SDL uses for sounds, in that a user of the
.(set|get)_volume()
API does not have to worry about it or wonder why