`pygame.mixer.Sound.get_volume` returned value is not correct

[Zimzozaur]

This issue is on MacOS Ventura, Windows 11 and Linux Manjaro, so probably everywhere.

def test():
    import pygame
    pygame.init()
    pygame.mixer.init()
    channel = pygame.mixer.Channel(1)

    channel.set_volume(0.1)
    assert channel.get_volume() == 0.1

test()

If you run this code, the value retuned from get_volume is not 0.1 or even 0.0999 but 0.09375.

[oddbookworm]

I suspect this is actually intended behavior. SDL_mixers docs for channel volume accepts an int between 0 and 128. So 0.1 needs to be normalized to that range. 0.1 * 128 is 12.8. Truncating that yields 12 as the passed parameter. As it turns out, 12/128 is 0.09375. The other option would be to round, but that would still be off because it would be 13/128, which is 0.1015625

[oddbookworm]

The docs could probably do with an update to make it clear that this 129-step volume behavior is the actual behavior

[Zimzozaur]

Return the volume of the channel for the current playing sound in the range of 0.0 to 1.0 (inclusive). This does not take into account stereo separation used byChannel.set_volume(). The Sound object also has its own volume, which is mixed with the channel.

Very misleading.

[Matiiss]

The docs could probably do with an update to make it clear that this 129-step volume behavior is the actual behavior

We could instead keep track of a float volume internally and return that

Suppose I wanted to set the volume based on the current volume and to get the current volume I'd use .get_volume, the calculations would be off for the most part.

[damusss]

The docs could probably do with an update to make it clear that this 129-step volume behavior is the actual behavior

We could instead keep track of a float volume internally and return that

Can we be sure SDL isn't changing the volume internally anywhere for whatever reason?

[Matiiss]

The docs could probably do with an update to make it clear that this 129-step volume behavior is the actual behavior

We could instead keep track of a float volume internally and return that

Can we be sure SDL isn't changing the volume internally anywhere for whatever reason?

yes https://github.com/pygame-community/pygame-ce/blob/b0d45d1dd21722d94805be5bd4ef39858acfbb9f/src_c/mixer.c#L1251

[damusss]

The docs could probably do with an update to make it clear that this 129-step volume behavior is the actual behavior

We could instead keep track of a float volume internally and return that

Can we be sure SDL isn't changing the volume internally anywhere for whatever reason?

yes

https://github.com/pygame-community/pygame-ce/blob/b0d45d1dd21722d94805be5bd4ef39858acfbb9f/src_c/mixer.c#L1251

That's really not what I meant, unless I am missunderstanding, what if the user calls set volume and sets it to 0.5 but then sdl for whatever reason changes its internal value to 0.2, because, uhh driver issues, then we'd be returning 0.5 instead of 0.2.

[Matiiss]

That's really not what I meant, unless I am missunderstanding, what if the user calls set volume and sets it to 0.5 but then sdl for whatever reason changes its internal value to 0.2, because, uhh driver issues, then we'd be returning 0.5 instead of 0.2.

Andrew explained this here already https://github.com/pygame-community/pygame-ce/issues/3090#issuecomment-2329528655

I suspect this is actually intended behavior. SDL_mixers docs for channel volume accepts an int between 0 and 128. So 0.1 needs to be normalized to that range. 0.1 * 128 is 12.8. Truncating that yields 12 as the passed parameter. As it turns out, 12/128 is 0.09375. The other option would be to round, but that would still be off because it would be 13/128, which is 0.1015625

[damusss]

(not very related, but my complaint is very pointless, you can go on to fix it how you please)

[aatle]

The docs should still probably be updated about the 129 intervals, just so users know. E.g., so people know what the quietest volume possible is.

[Starbuck5]

First of all, it's weird that's it 129 values (0-128), rather than 128 values (0-127). Given than 128 values is half of 256 values, it could be represented in 7 bits.

Secondly I don't see this as a bug at all, and I think it's worse to lie to the user about what the volume is.

[aatle]

Secondly I don't see this as a bug at all, and I think it's worse to lie to the user about what the volume is.

If it is desirable to still be able to know the 'real' volume, then in the new implementation it should have its own separate method, which is much less prone to misunderstanding compared to the current implementation. (Note that the new proposed implementation currently cannot give the real volume, though 129 intervals would be sufficient in most cases.)

[damusss]

Secondly I don't see this as a bug at all, and I think it's worse to lie to the user about what the volume is.

If it is desirable to still be able to know the 'real' volume, then in the new implementation it should have its own separate method, which is much less prone to misunderstanding compared to the current implementation. (Note that the new proposed implementation currently cannot give the real volume, though 129 intervals would be sufficient in most cases.)

A separate method for something like that sounds overkill to me. SDL3 will change the volume to a float meaning this proposed fix will become the real volume soon enough.

[Matiiss]

A separate method for something like that sounds overkill to me. SDL3 will change the volume to a float meaning this proposed fix will become the real volume soon enough.

It's not exactly a terrible idea, we could then introduce volume properties to Channel and Sound and make it more Pythonic, but I think for now just keeping track of the floating point value provided by the user is the most practical solution with the least changes to the existing API. I don't think this should cause any backwards compatibility issues either.

[oddbookworm]

I agree with @Starbuck5 that we shouldn't lie to the user about what the actual volume is. Changing the existing behavior is likely not going to happen because it would break backcompat. I'm personally in favor of a docs update to clarify what actually happens when the user requests a specific volume

[aatle]

I agree with @Matiiss that storing a float may be better, for various reasons. (Also see Matiiss's comment in the PR.)

This new behavior is, in my opinion, not 'lying' to the user. It is more of a lie to not meet user expectations than it is to not match technical reality. It is better to make the unintuitive behavior (real volume) opt-in rather than opt-out. That is, make the intuitive (and probably more practical) behavior the "default". Technical users who know about and want the real volume may manually switch away from the default, rather than having average users to opt out of a behavior that they might not even know about. Also, the method to opt-in is much easier than the method to opt-out.

The current behavior is unpythonic and prone to various errors, especially since the methods seem very basic at a glance. Anyone who deals with these methods must know and remember that set_volume() will actually implicitly round to the nearest 1/128th, or else many problems may crop up. Simple methods like these should not carry any special behavior that necessitates extra information from the docs. Permanent warnings aren't great. (Also, are there any other getters/setters with similar behavior in pygame?)

Possible bugs include...

- volume can decrease faster than volume increases - rate of change implicitly decreased by rounding, possibly to 0 - rate of change may differ with FPS (because delta time) - only workaround to avoid all this requires storing an extra float value with the channel/sound object, inconvenient - more prone to stale data, compared to built-in solution

It might not completely break backwards compatibility because this behavior is not documented in pygame. It may be considered an implementation detail or subject to change because one should not totally rely on undocumented special behavior. Though these changes will have effects.

[Starbuck5]

A separate method for something like that sounds overkill to me. SDL3 will change the volume to a float meaning this proposed fix will become the real volume soon enough.

SDL3_mixer has no plans that I can see to change this thing to a float. I said that on the PR as well but also posting here because we're having 2 separate discussions on the same topic.

The current behavior is unpythonic and prone to various errors, especially since the methods seem very basic at a glance. Anyone who deals with these methods must know and remember that set_volume() will actually implicitly round to the nearest 1/128th, or else many problems may crop up.

No, they'll need to remember get_volume is not smoothly continuous, and only if they're using it to implement a lerp or a transformation across multiple volumes.

Has anyone else ever raised an issue about this before?

Simple methods like these should not carry any special behavior that necessitates extra information from the docs. Permanent warnings aren't great. (Also, are there any other getters/setters with similar behavior in pygame?)

Almost all pygame methods I know of return the actual stored values rather than faking it, even when that behavior has foibles, like Rects. One exception that comes to mind is mouse.get_cursor, where we store the state ourselves since the state getter was removed in SDL2.

[Starbuck5]

So if SDL is moving towards float volume for their API, maybe we should lobby SDL_mixer to do the same in the SDL3 transition period. Haven’t done the research on that myself yet.

[aatle]

No, they'll need to remember get_volume is not smoothly continuous, and only if they're using it to implement a lerp or a transformation across multiple volumes.

Yes, only a part of the mixers users would experience issues for this, but a nonzero number. Even if the issue is rare, that is not a reason really to keep the current behavior.

The main thing is that I'm not that convinced on why we shouldn't change the implementation for its cons. The API should reflect usage and not the low-level technicalities in the implementation that the user doesn't need to know about (as long as there are no significant problems with the abstraction). (I see no use cases that would really benefit from restricting the volume to intervals, apart from uncommon, technical places.) As I've said, the intuitive behavior should be 'default', and the technical stuff should be separate/opt-in.

I think one of the problems with the current implementation is that it is halfway between reality/internals (integer volumes) and the intuitive abstraction (float volumes). Pygame should go all the way on the float interface to avoid the problems of the implementation mismatch.

[aatle]

As I understand from my research, the reason why volume is an integer 0-128 is because it is used on mainly integer-sample audio formats, to multiply or whatever. In SDL, the new float volume actually uses all of the precision for float audio formats, and rounds for integer formats (idk about SDL_mixer implementation, looks a lot different and it stores the volume).