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

Keep track of floating point volume internally for Sound and Channel #3091

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

Conversation

Matiiss
Copy link
Member

@Matiiss Matiiss commented Sep 5, 2024

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

channel.set_volume(0.1)
print(channel.get_volume())  # prints 0.09375

@Matiiss Matiiss added the mixer pygame.mixer label Sep 5, 2024
@Matiiss Matiiss added this to the 2.5.2 milestone Sep 5, 2024
@Matiiss Matiiss requested a review from a team as a code owner September 5, 2024 21:57
@Starbuck5
Copy link
Member

I don't think we should lie to the users about what their volume actually is.

@Matiiss
Copy link
Member Author

Matiiss commented Sep 6, 2024

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.
For example, suppose you want to increase the volume over a long period of time, you might be tempted to do something like this to say, increase the volume from 0% to 100% over 10 seconds:

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 Rect's x value is when they set it to 2.9 and then they get 2 back and now you have to keep track of that position separately if you want to use floats. Keeping the differences in mind, here the maximum difference is 1, that is 125 times more than what the maximum difference for .get_volume is currently. Now, that was sort of solved by porting FRects, but I don't think we want to have FSound and FChannel and fmusic.

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 volume property to Sound and Channel that will behave as this PR intends to make (get|set)_volume behave. Except then we can't quite do the same with mixer.music.

@damusss
Copy link
Contributor

damusss commented Sep 6, 2024

I don't think it is a lie as matiiss said, but I'm not even sure if anything should be added in the docs, as the SDL3 migration states volume will become a floating point number anyways (at least that's what I grasped by reading it) so it won't be a "lie" anymore saying it's 0-128 will become outdated with SDL3.
image

@Zimzozaur
Copy link

Zimzozaur commented Sep 8, 2024

@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?

@Starbuck5
Copy link
Member

@Starbuck5, don’t you think the getter should return the exact value that the setter sets?

I think the function get_volume should return the actual volume.

I don't think it is a lie as matiiss said, but I'm not even sure if anything should be added in the docs, as the SDL3 migration states volume will become a floating point number anyways (at least that's what I grasped by reading it) so it won't be a "lie" anymore saying it's 0-128 will become outdated with SDL3.

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.

That aside, this is (at least almost) like saying we're lying to the users about what their actual Rect's x value is when they set it to 2.9 and then they get 2 back and now you have to keep track of that position separately if you want to use floats. Keeping the differences in mind, here the maximum difference is 1, that is 125 times more than what the maximum difference for .get_volume is currently. Now, that was sort of solved by porting FRects, but I don't think we want to have FSound and FChannel and fmusic.

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.

I see no harm in making it behave this way, in fact, I see a couple benefits.

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.

@Zimzozaur
Copy link

Zimzozaur commented Sep 9, 2024

I want the Python function to return the actual C state

Why not both? Why not having 2 methods like get_C_volume and get_Python_volume (do not take names literally).
First of all, docs hide API details and why should remember the multiplayer (that is not written in docs) instead of adding a second return method that makes it easy to test volume for example.

I am asking this for the next time because you didn't respond, and I would like to understand your point of view.

@MyreMylar
Copy link
Member

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.

@Matiiss
Copy link
Member Author

Matiiss commented Sep 18, 2024

Currently I can list two reasons:

  • Convenience and practicality (is this two reasons?)

    For example, suppose you want to increase the volume over a long period of time, you might be tempted to do something like this to say, increase the volume from 0% to 100% over 10 seconds:

    sound.set_volume(sound.get_volume() + dt * 0.1)

    Well, this simply won't work the way those values are handled currently.

    - Keep track of floating point volume internally for Sound and Channel #3091 (comment)

  • Principle of least surprise (I suppose this is close to having found the behavior "odd or mildly annoying", but it is an important principle either way)
    It's a lot more intuitive to get back the value you set, not some closest float approximation where a 0-128 range is mapped to 0-1 range.

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)

Copy link
Contributor

@bilhox bilhox left a 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) { \
Copy link
Contributor

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 ?

Copy link
Member Author

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.

@bilhox
Copy link
Contributor

bilhox commented Sep 29, 2024

@Matiiss I can see you forgot mixer_music.

@Starbuck5 Starbuck5 removed this from the 2.5.2 milestone Oct 13, 2024
@Matiiss
Copy link
Member Author

Matiiss commented Oct 18, 2024

@Matiiss I can see you forgot mixer_music.

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.

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

Successfully merging this pull request may close these issues.

pygame.mixer.Sound.get_volume returned value is not correct
6 participants