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

Improve silence detection in MutedNotification #26951

Merged
merged 22 commits into from Feb 16, 2024

Conversation

myQwil
Copy link
Contributor

@myQwil myQwil commented Feb 2, 2024

On entering a PlayerLoader screen, a MutedNotification will offer to restore the volume to an audible level if either the master or music volume level is <= 1%.

The problem with this logic is that, since the volume levels are multiplied together, 1% of 100% will be exactly the same volume level as 10% of 10%, or 2% of 50%, but only the first scenario would prompt a notification.

Instead, we should be checking their aggregate volume to see if it's below a certain threshold, and then raising the volume levels to reach a specific aggregate amount.

Instead of checking the master and music volumes separately to
see if they're <= 1%, check the aggregate of the two volumes to
see if it's <= -60 dB.

When muted notification is activated, restore the aggregate volume
level to -30 dB.
osu.Game/Screens/Play/PlayerLoader.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Play/PlayerLoader.cs Outdated Show resolved Hide resolved
@frenzibyte
Copy link
Member

The logic seems fine. I don't understand why the target volume has been lowered down to 10%. I brought it back to 50% as I don't believe it's relevant to the intent of this PR given by the title and description.

frenzibyte
frenzibyte previously approved these changes Feb 15, 2024
@myQwil
Copy link
Contributor Author

myQwil commented Feb 15, 2024

Float casts aren't vital, there were just these small inaccuracies that would start to pop up that were kinda bugging me. That probably won't be an issue though if the target volume was brought back up to 50%, since 0.5 is a power of 2, so there's no mantissa to worry about.

Actually, never mind, it's not the target volume but rather the silence threshold that's being compared against. Again, it's probably fine either way.

@myQwil
Copy link
Contributor Author

myQwil commented Feb 15, 2024

Aggregate volume needs to be <= 0.01 in order to trigger the silence threshold. 0.6 * 0.15 = 0.09.

On second thought, you should probably put the float casts back in because 0.1 * 0.1 is going to result in a value that's just a hair above 0.01. If you were to test against those volume levels, the notification wouldn't get triggered and the test would fail. Float casting, on the other hand, results in a value slightly less than 0.01.

double x = 0.1;
double y = 0.1;
double agg = (float)(x * y);
Console.WriteLine($"{x * y}\n{agg}");
Console.WriteLine($"{x * y <= 0.01}\n{agg <= 0.01}");

output:

0.010000000000000002
0.009999999776482582
False
True

auto-merge was automatically disabled February 15, 2024 23:56

Head branch was pushed to by a user without write access

@bdach
Copy link
Collaborator

bdach commented Feb 16, 2024

you should probably put the float casts back in because 0.1 * 0.1 is going to result in a value that's just a hair above 0.01

Precision.AlmostBigger() is for those sorts of usages. Not "float casts".

audioManager.Volume.Value = 0.5f;
if (audioManager.VolumeTrack.Value <= volume_requirement)
audioManager.VolumeTrack.Value = 0.5f;
if (aggregateVolumeTrack <= volume_requirement)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This is all so black box to a user. I'd still prefer the old hard cut increases, then let the user adjust further.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds fine, I agree it's a touch more complicated/arbitrary but wasn't sure if it warrants a discussion.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Feb 16, 2024
@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 16, 2024
@frenzibyte frenzibyte merged commit 4bbb1bc into ppy:master Feb 16, 2024
10 of 11 checks passed
@myQwil myQwil deleted the mute_detection branch February 20, 2024 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants