-
-
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 - Prework for reviving bevyengine#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 - Prework for reviving bevyengine#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 - Allow users to configure volume using decibels by changing the `Volume` type from newtyping an `f32` to an enum with `Linear` and `Decibels` variants. - Fixes #9507. - Alternative reworked version of closed #9582. ## Solution Compared to #9582, this PR has the following main differences: 1. It uses the term "linear scale" instead of "amplitude" per https://github.com/bevyengine/bevy/pull/9582/files#r1513529491. 2. Supports `ops` for doing `Volume` arithmetic. Can add two volumes, e.g. to increase/decrease the current volume. Can multiply two volumes, e.g. to get the “effective” volume of an audio source considering global volume. [requested and blessed on Discord]: https://discord.com/channels/691052431525675048/749430447326625812/1318272597003341867 ## Testing - Ran `cargo run --example soundtrack`. - Ran `cargo run --example audio_control`. - Ran `cargo run --example spatial_audio_2d`. - Ran `cargo run --example spatial_audio_3d`. - Ran `cargo run --example pitch`. - Ran `cargo run --example decodable`. - Ran `cargo run --example audio`. --- ## Migration Guide Audio volume can now be configured using decibel values, as well as using linear scale values. To enable this, some types and functions in `bevy_audio` have changed. - `Volume` is now an enum with `Linear` and `Decibels` variants. Before: ```rust let v = Volume(1.0); ``` After: ```rust let volume = Volume::Linear(1.0); let volume = Volume::Decibels(0.0); // or now you can deal with decibels if you prefer ``` - `Volume::ZERO` has been renamed to the more semantically correct `Volume::SILENT` because `Volume` now supports decibels and "zero volume" in decibels actually means "normal volume". - The `AudioSinkPlayback` trait's volume-related methods now deal with `Volume` types rather than `f32`s. `AudioSinkPlayback::volume()` now returns a `Volume` rather than an `f32`. `AudioSinkPlayback::set_volume` now receives a `Volume` rather than an `f32`. This affects the `AudioSink` and `SpatialAudioSink` implementations of the trait. The previous `f32` values are equivalent to the volume converted to linear scale so the `Volume:: Linear` variant should be used to migrate between `f32`s and `Volume`. - The `GlobalVolume::new` function now receives a `Volume` instead of an `f32`. --------- Co-authored-by: Zachary Harrold <[email protected]>
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))