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

Support decibels in bevy_audio::Volume #17605

Merged
merged 12 commits into from
Feb 10, 2025

Conversation

mgi388
Copy link
Contributor

@mgi388 mgi388 commented Jan 29, 2025

Objective

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.

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:

let v = Volume(1.0);

After:

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 f32s. 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 f32s and Volume.
  • The GlobalVolume::new function now receives a Volume instead of an f32.

@alice-i-cecile alice-i-cecile added A-Audio Sounds playback and modification C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Contentious There are nontrivial implications that should be thought through S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 30, 2025
@mgi388 mgi388 changed the title WIP: Support decibels in bevy_audio::Volume Support decibels in bevy_audio::Volume Jan 31, 2025
@mgi388 mgi388 marked this pull request as ready for review January 31, 2025 03:00
Copy link
Contributor

@SolarLiner SolarLiner left a comment

Choose a reason for hiding this comment

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

Maybe I went into the territory of bikeshedding, but I have some comments on this, which maybe warrants a new pass at the PR.

@alice-i-cecile alice-i-cecile added D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 31, 2025
@BenjaminBrienen
Copy link
Contributor

I think the points in the previous review should be addressed. I agree with the problems they pointed out.

@mgi388 mgi388 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 3, 2025
@mgi388
Copy link
Contributor Author

mgi388 commented Feb 8, 2025

@SolarLiner

  • PLAT again and let me know what you think.
  • See newest commit messages for changelog.
  • I tested the spatial 3d example and also proved that the multiplication of the source volume combined with the global volume's linear value is the same as it was without using ops::mul.
  • BTW I don't know if the methods should be called to_linear / to_decibels or as_linear/as_decibels.
  • I need to update the PR desc (and migration guide) before merging, but I'll do that at the end once there's an impending approval.

@mgi388 mgi388 requested a review from SolarLiner February 8, 2025 13:48
@mgi388 mgi388 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Feb 9, 2025
@mgi388 mgi388 requested a review from SolarLiner February 10, 2025 09:46
@SolarLiner SolarLiner added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 10, 2025
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Very nicely made, and really clear, excellent docs. This is something that I think will be kept around even after swapping backends.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 10, 2025
Merged via the queue into bevyengine:main with commit 2660ddc Feb 10, 2025
30 checks passed
@mgi388 mgi388 deleted the volume-decibels branch February 10, 2025 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Audio Sounds playback and modification C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Expose both linear and logarithmic volume settings
4 participants