-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Updated VolumeLevel
to use Decibel and Amplitude Ratio Units
#9582
Conversation
VolumeLevel
to use Decibel and Amplitude Ratio Units
This comment got me thinking, I can actually remove the bounds checks entirely. Both variants take a single
So I've adjusted the access and comparison methods to reflect the true statement that the amplitude of a |
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.
Do we really need 2 variants for volume? Unity, Unreal, and Godot all just use decibels. That might just be because they don't have sum types, but the current pr feels a little awkward when you want to do math on the volume.
These are not appropriate due to the possibility of `NaN` values being passed directly into the `enum`.
I think using an enum is a good way to force understanding around what the volume type represents. The An alternative could be to new-type |
/// phase inversion. If `a` and `b` have the same magnitude (`a.abs() == b.abs()`), but opposite | ||
/// sign (`a == -b`), then `Self::Amplitude(a) == Self::Amplitude(b)`, as this phase information | ||
/// is currently ignored. As such, you should use positive values for this variant to avoid confusion. | ||
Amplitude(f32), |
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.
At the risk of some bikeshedding (and probably pedantry), "amplitude" refers to the volume itself, not a specific "unit" -- Linear
would be a better term, as it is the term that is most used for the unitless gain ratio (this applies to the method name line 26 too). I can imagine some users being confused when looking at this variant by itself and not knowing whether we are talking about the linear gain or the logarithmic one (in dB), and having to look up the enum definition to clear the confusion.
@@ -22,34 +21,12 @@ impl Default for Volume { | |||
|
|||
impl Volume { | |||
/// Create a new volume level relative to the global volume. | |||
pub fn new_relative(volume: f32) -> Self { | |||
Self::Relative(VolumeLevel::new(volume)) | |||
pub fn new_relative(amplitude: f32) -> Self { |
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.
It's been kept as-is to not introduce breaking changes, but I do think it would be better to have new_relative
and new_absolute
use the VolumeLevel
. Or we can deprecate these ones and introduce new methods (meaning 4 methods total for the cartesian product of (relative,absolute)x(amplitude,decibel)
?
# Objective - Prework for reviving #9582. ## Solution - Move the two types to volume.rs and made it compile. - Also `#[reflect(Debug)]` on `Volume` while I'm here. ## Testing - Ran example locally. - Rely on CI.
Objective
Solution
AudioSinkPlayback
trait
andVolumeLevel
type.VolumeLevel
with anenum
representing either the amplitude ratio or decibel level. Names and types were chosen to be consistent with 3rd party crates likeKira
, and the internally used types in the rest ofbevy_audio
Changelog
VolumeLevel
into its own filevolume_level.rs
to reduce clutter inaudio.rs
.VolumeLevel
, as both variantsAmplitude
andDecibels
are isomorphic and internally possess both properties.amplitude()
anddecibels()
methods to get the desired unit type fromVolumeLevel
directly without amatch
statement.debug_assert!
statements to ensure calculated values are within their appropriate bounds.Amplitude
andDecibels
, with a truth table obtained from WikipediaVolumeLevel
type, or explicitly request an amplitude which is then wrapped in aVolumeLevel::Amplitude
where appropriate.VolumeLevel::get
andVolumeLevel::new
due to ambiguity in the unit to return.Migration Guide
VolumeLevel::new(volume)
->VolumeLevel::Amplitude(volume)
VolumeLevel::get()
->VolumeLevel::amplitude()
AudioSinkPlayback::volume()
->AudioSinkPlayback::volume().amplitude()
AudioSinkPlayback::set_volume(volume)
->AudioSinkPlayback::set_volume(VolumeLevel::Amplitude(volume))