-
Notifications
You must be signed in to change notification settings - Fork 425
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
Use decibel scaling for volume control #6490
base: master
Are you sure you want to change the base?
Changes from 2 commits
f0a489f
ef5a892
41ec0a8
38e105d
2b61198
998f687
370a49c
590d1d0
8ea57e8
844989f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence. | ||
// See the LICENCE file in the repository root for full licence text. | ||
|
||
using System; | ||
using osu.Framework.Bindables; | ||
|
||
namespace osu.Framework.Audio | ||
{ | ||
public class VolumeScaler | ||
{ | ||
public const double MIN = -60; | ||
public const double STEP = 0.5; | ||
|
||
private const double ln_ten = 2.302585092994045684017991454684364208; | ||
myQwil marked this conversation as resolved.
Show resolved
Hide resolved
|
||
private const double k = ln_ten / 20; | ||
|
||
public readonly BindableNumber<double> Real; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe call these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this "real" one looks like it should not be externally mutable. should either be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
public readonly BindableNumber<double> Scaled = new BindableNumber<double>(1) | ||
{ | ||
MinValue = MIN, | ||
MaxValue = 0, | ||
Precision = STEP, | ||
}; | ||
|
||
private double scaledToReal(double x) => x <= MIN ? 0 : Math.Exp(k * x); | ||
|
||
private double realToScaled(double x) => x <= 0 ? MIN : Math.Log(x) / k; | ||
|
||
public VolumeScaler(BindableNumber<double>? real = null) | ||
{ | ||
Real = real ?? new BindableNumber<double>(1) { MinValue = 0, MaxValue = 1 }; | ||
Scaled.BindValueChanged(x => Real.Value = scaledToReal(x.NewValue)); | ||
} | ||
|
||
public double Value | ||
{ | ||
get => Real.Value; | ||
set => Scaled.Value = realToScaled(value); | ||
} | ||
|
||
public void Scale() | ||
{ | ||
Scaled.Value = realToScaled(Real.Value); | ||
Scaled.Default = realToScaled(Real.Default); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -173,9 +173,13 @@ private void load(FrameworkConfigManager config) | |
|
||
// attach our bindables to the audio subsystem. | ||
config.BindWith(FrameworkSetting.AudioDevice, Audio.AudioDevice); | ||
config.BindWith(FrameworkSetting.VolumeUniversal, Audio.Volume); | ||
config.BindWith(FrameworkSetting.VolumeEffect, Audio.VolumeSample); | ||
config.BindWith(FrameworkSetting.VolumeMusic, Audio.VolumeTrack); | ||
config.BindWith(FrameworkSetting.VolumeUniversal, Audio.Volume.Real); | ||
config.BindWith(FrameworkSetting.VolumeEffect, Audio.VolumeSample.Real); | ||
config.BindWith(FrameworkSetting.VolumeMusic, Audio.VolumeTrack.Real); | ||
|
||
Audio.Volume.Scale(); | ||
Audio.VolumeSample.Scale(); | ||
Audio.VolumeTrack.Scale(); | ||
Comment on lines
+180
to
+182
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't be required. It will happen on |
||
|
||
Shaders = new ShaderManager(Host.Renderer, new NamespacedResourceStore<byte[]>(Resources, @"Shaders")); | ||
dependencies.Cache(Shaders); | ||
|
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.
This needs to be renamed to
BindableVolume
or similar.VolumeScaler
reads really bad as a class name.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.
Also please xmldoc this whole class. Every public property and the class itself.
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.
Also probably seal it. I don't think a framework consumer should be attempting to make a custom implementation of this ever.