Use decibel scaling for volume control #6490

myQwil · 2025-01-13T01:57:10Z

Meant to work alongside ppy/osu#31493

peppy

I think we can make this direction work.

peppy · 2025-01-13T04:00:40Z

osu.Framework/Game.cs

+            Audio.Volume.Scale();
+            Audio.VolumeSample.Scale();
+            Audio.VolumeTrack.Scale();


This shouldn't be required. It will happen on BindWith.

peppy · 2025-01-13T04:03:10Z

osu.Framework/Audio/VolumeScaler.cs

+
+namespace osu.Framework.Audio
+{
+    public class VolumeScaler


This needs to be renamed to BindableVolume or similar. VolumeScaler reads really bad as a class name.

Also please xmldoc this whole class. Every public property and the class itself.

Also probably seal it. I don't think a framework consumer should be attempting to make a custom implementation of this ever.

peppy · 2025-01-13T04:03:35Z

osu.Framework/Audio/VolumeScaler.cs

+        private const double ln_ten = 2.302585092994045684017991454684364208;
+        private const double k = ln_ten / 20;
+
+        public readonly BindableNumber<double> Real;


Maybe call these Linear and Logarithmic?

this "real" one looks like it should not be externally mutable. should either be IBindableNumber<double>, or if you're gonna keep it mutable, it should be synced both ways (changing "real" should also change "scaled")

this "real" one looks like it should not be externally mutable

I was hoping it would remain mutable so that we aren't deciding on a developers' behalf which they choose to use. Both may have valid use cases, depending on how you're audio engineeringing.

sure, as long as it's not possible to break the instances of this into complete nonsense by setting both to random values and putting it in a completely invalid state

osu.Framework/Audio/VolumeScaler.cs

bdach · 2025-01-13T09:53:12Z

osu.Framework/Audio/VolumeScaler.cs

+        private const double ln_ten = 2.302585092994045684017991454684364208;
+        private const double k = ln_ten / 20;
+
+        public readonly BindableNumber<double> Real;


this "real" one looks like it should not be externally mutable. should either be IBindableNumber<double>, or if you're gonna keep it mutable, it should be synced both ways (changing "real" should also change "scaled")

Co-authored-by: Bartłomiej Dach <[email protected]>

`ln_ten` was really just there to clarify what the numeric literal represented.

bdach · 2025-01-13T12:19:33Z

osu.Framework/Audio/VolumeScaler.cs

@@ -11,8 +11,7 @@ public class VolumeScaler
        public const double MIN = -60;
        public const double STEP = 0.5;

-        private static readonly double ln_ten = Math.Log(10);
-        private const double k = ln_ten / 20;
+        private const double k = Math.Log(10) / 20;


constant declarations must use compile-time constants on the rhs of the assignment. this can't work with const, it must be static readonly. are you even compiling this code?

I kinda still want k to be const. I guess that's why I went with the numeric literal to begin with.

const will get embedded into the IL code wherever it's used, whereas static readonly requires a memory lookup each time it's accessed.

please actually profile this and get back to me. i am 99% confident this will not be measurable. i prefer non-obfuscated code over pedantically marginal performance 'gains'.

If you like, I can change the name of the variable from ln_ten to something that's more readable.

But if the code is decently readable either way, why use the slower version, regardless of how pedantically marginal the difference may be?

there is no solution with the const that i would consider more readable than just writing out what the number is

but i also have no time nor patience to continue this discussion

Make is static readonly or close the PR.

Certainly. I apologize if I came across as confrontational. I'm just trying to have a discussion about your coding philosophies and the reasons behind them.

It's pretty simple: storing one value to memory that is going to take up 8 bytes is not a concern and shouldn't lead to discussion. By using it we don't have to figure out what a random constant decimal number is and check whether it's correct.

I think I see what you're saying. I figured if I just gave the variable a good enough name, describing what it's supposed to represent, then everything would be fine, but at the end of the day, there's still this big ugly numeric literal sitting in the middle of the code and it's a bit unsightly.

`ln_ten` needs to also be const in order to be assigned to `k`

bdach · 2025-01-13T14:12:46Z

osu.Framework/Audio/BindableVolume.cs

+        public double Value
+        {
+            get => Real.Value;
+            set => Scaled.Value = realToScaled(value);
+        }


in what world is it sane to have a property return values on a different scale than the one used to set the value

it should not be the case that after you set a property's value, you read it back and get a completely different number. that is nonsensical

I should probably be able to get rid of that. I think at first, I wasn't as explicit about whether I was using the real value or the scaled value.

Okay, so it does sorta make sense. We don't want to assign directly to Real. Instead, we want to assign to Scaled only, and whenever we do, that triggers a BindValueChanged function, and that's where Real gets assigned. So assigning to Scaled effectively assigns to both of them.

Real doesn't have an equivalent BindValueChanged function as that would result in an infinite loop.

Also, instead of assigning directly to Scaled, sometimes we want to assign a linear value, but it has to go through Scaled first, so the setter's job is to first translate the linear value into a logarithmic value.

I'll probably still get rid of the getter, as that is a bit confusing, and I think it's just there for convenience sake, but it's better to be explicit.

Getter can be replaced with explicit calls to Real Setter should instead be a function with a clearer name.

peppy · 2025-01-14T04:57:34Z

Any reason this was closed? I'd genuinely hope you aren't that opposed to the const discussion.

myQwil added 2 commits January 12, 2025 20:13

Use volume scaling

f0a489f

Display volume in decibel format

ef5a892

pull-request-size bot added the size/M label Jan 13, 2025

myQwil mentioned this pull request Jan 13, 2025

Use decibel scaling for volume control ppy/osu#31493

Open

peppy self-requested a review January 13, 2025 04:04

peppy reviewed Jan 13, 2025

View reviewed changes

bdach requested changes Jan 13, 2025

View reviewed changes

myQwil and others added 3 commits January 13, 2025 06:59

Use Math.Log(10) instead of the numeric literal

41ec0a8

Co-authored-by: Bartłomiej Dach <[email protected]>

Further simplify the assigning of k

38e105d

`ln_ten` was really just there to clarify what the numeric literal represented.

Rename VolumeScaler to BindableVolume

2b61198

bdach reviewed Jan 13, 2025

View reviewed changes

Bring back ln_ten as a numeric literal

998f687

`ln_ten` needs to also be const in order to be assigned to `k`

bdach requested changes Jan 13, 2025

View reviewed changes

myQwil added 2 commits January 13, 2025 11:22

Make BindableVolume a sealed class

370a49c

Remove BindableVolume.Value

590d1d0

Getter can be replaced with explicit calls to Real Setter should instead be a function with a clearer name.

myQwil closed this Jan 13, 2025

Make k a static readonly so we can use Math.Log(10)

8ea57e8

myQwil reopened this Jan 14, 2025

Rename Real/Scaled to Linear/Decibel

844989f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use decibel scaling for volume control #6490

Use decibel scaling for volume control #6490

myQwil commented Jan 13, 2025 •

edited

Loading

peppy left a comment

peppy Jan 13, 2025

peppy Jan 13, 2025

peppy Jan 13, 2025

peppy Jan 13, 2025

peppy Jan 13, 2025

bdach Jan 13, 2025

peppy Jan 13, 2025

bdach Jan 13, 2025

bdach Jan 13, 2025

bdach Jan 13, 2025

myQwil Jan 13, 2025

myQwil Jan 13, 2025

bdach Jan 13, 2025 •

edited

Loading

myQwil Jan 13, 2025 •

edited

Loading

bdach Jan 13, 2025

peppy Jan 13, 2025

myQwil Jan 13, 2025

peppy Jan 14, 2025

myQwil Jan 14, 2025

bdach Jan 13, 2025

myQwil Jan 13, 2025

myQwil Jan 13, 2025 •

edited

Loading

peppy commented Jan 14, 2025

Use decibel scaling for volume control #6490

Are you sure you want to change the base?

Use decibel scaling for volume control #6490

Conversation

myQwil commented Jan 13, 2025 • edited Loading

peppy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

myQwil Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myQwil Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

peppy commented Jan 14, 2025

myQwil commented Jan 13, 2025 •

edited

Loading

bdach Jan 13, 2025 •

edited

Loading

myQwil Jan 13, 2025 •

edited

Loading

myQwil Jan 13, 2025 •

edited

Loading