Add ability to mute audio sinks #16813

mgi388 · 2024-12-14T02:24:08Z

Objective

Allow users to mute audio.

fn mute(
    keyboard_input: Res<ButtonInput<KeyCode>>,
    mut sink: Single<&mut AudioSink, With<MyMusic>>,
) {
    if keyboard_input.just_pressed(KeyCode::KeyM) {
        sink.toggle_mute();
    }
}

I want to be able to press, say, M and mute all my audio. I want this for dev, but I'm sure it's a useful player setting as well.
Muting is different to pausing—I don't want to pause my sounds, I want them to keep playing but with no volume. For example if I have background music playing which is made up of 5 tracks, I want to be able to temporarily mute my background music, and if I unmute at, say, track 4, I want to play track 4 rather than have had everything paused and still be on the first track.
I want to be able to continue to control the volume of my audio even when muted. Like in the example, if I have muted my audio but I use the volume up/down controls, I want Bevy to remember those volume changes so that when I unmute, the volume corresponds to that.

Solution

Add methods to audio to allow muting, unmuting and toggling muting.
To preserve the user's intended volume, each sink needs to keep track of a "managed volume".
I checked rodio and I don't see any built in support for doing this, so I added it to bevy_audio.
I'm interested to hear if this is a good idea or a bad idea. To me, this API looks nice and looks usable, but I'm aware it involves some changes to the existing API and now also requires mutable access in some places compared to before.
I'm also aware of work on Better Audio, but I'm hoping that if this change isn't too wild it might be a useful addition considering we don't really know when we'll eventually get better audio.

Testing

Update and run the example: cargo run --example audio_control
Run the example: cargo run --example soundtrack
Update and run the example: cargo run --example spatial_audio_3d
Add unit tests.

Showcase

See 2 changed examples that show how you can mute an audio sink and a spatial audio sink.

Migration Guide

The AudioSinkPlayback trait now has 4 new methods to allow you to mute audio sinks: is_muted, mute, unmute and toggle_mute. You can use these methods on bevy_audio's AudioSink and SpatialAudioSink components to manage the sink's mute state.
AudioSinkPlayback's set_volume method now takes a mutable reference instead of an immutable one. Update your code which calls set_volume on AudioSink and SpatialAudioSink components to take a mutable reference. E.g.:

Before:

fn increase_volume(sink: Single<&AudioSink>) {
    sink.set_volume(sink.volume() + 0.1);
}

After:

fn increase_volume(mut sink: Single<&mut AudioSink>) {
    let current_volume = sink.volume();
    sink.set_volume(current_volume + 0.1);
}

The PlaybackSettings component now has a muted field which you can use to spawn your audio in a muted state. PlaybackSettings also now has a helper method muted which you can use when building the component. E.g.:

commands.spawn((
    // ...
    AudioPlayer::new(asset_server.load("sounds/Windless Slopes.ogg")),
    PlaybackSettings::LOOP.with_spatial(true).muted(),
));

alice-i-cecile · 2024-12-14T05:52:26Z

Changes like this definitely shouldn't be blocked on the Better Audio work :)

SolarLiner

I really like this! This packs quite a lot of punch for such a small change, and I like the design!

I've left a few nitpicks, but otherwise LGTM!

crates/bevy_audio/src/sinks.rs

Co-authored-by: Nathan Graule <[email protected]>

- There is no output device in CI for this test to pass. - There is no equivalent `SpatialSink::new_idle` in rodio. - For now it's probably best to just rely on unit tests for `AudioSink` and code review for `SpatialAudioSink`. - Might be able to see if a `SpatialSink::try_new_idle` could be added to rodio, but that's for another change.

examples/audio/spatial_audio_3d.rs

rparrett · 2024-12-14T14:25:37Z

What's the reasoning for managed_volume being an Option? At a glance, shuffling the volume setting in and out of the option seems like an unnecessary bit of complexity.

edit: reworded

SolarLiner · 2024-12-14T14:28:40Z

Since there's no native support for sink muting in rodio, this is like the least complex alternative to me. Since if we set the sink output to zero, we lose the original volume, we then have to store it somewhere. The Option<f32> for the volumes makes intuitive sense, at least to me.

rparrett · 2024-12-14T14:38:07Z

I get the need to store it separately, just unclear on Option<f32> vs f32.

SolarLiner · 2024-12-14T14:59:49Z

I get the need to store it separately, just unclear on Option<f32> vs f32.

Ah I didn't get your original question then. Option makes sense because it also makes explicit that the managed volume is sometimes not available, or temporary. It makes less of a sense to me to make it non-optional and have to mutate it everywhere we also mutate the sink's volume, which also increases complexity, and more bug-prone.

rparrett · 2024-12-14T15:24:59Z

Ah I didn't get your original question then. Option makes sense because it also makes explicit that the managed volume is sometimes not available, or temporary.

I feel like "managed volume being Some" is sort of a clunky way to represent that the sink is in a muted state vs something like

volume: f32
muted: bool

IMO, "the volume the user wants when the sink is unmuted" is conceptually not a temporary thing.

But that detail is hidden from users, so not something I feel the need to fuss over. 👍

rparrett

It seems like PlaybackSettings should get a corresponding muted field so that AudioPlayer can be spawned in a muted state.

Right now, if an app author wanted to, for example, persist and restore a user's "muted" preference, implementing that would be tricky for them.

mgi388 · 2024-12-15T02:15:12Z

@rparrett thanks for the review and suggestion to update PlaybackSettings.

Re:

IMO, "the volume the user wants when the sink is unmuted" is conceptually not a temporary thing.

The volume the user wants when the sink is unmuted is whatever is returned from self.sink.volume(), so the concept is not actually temporary, it's just returned from a different place (the underlying sink).

I feel the Option makes more sense. If we're muted, we need to manage the volume for the user, so we have Some(volume). If we're unmuted, we don't want to manage any extra internal state and want to just defer to self.sink.volume(), so we have None.

Supposing we used two fields for this per your suggestion, then if we're unmuted, we're unnecessarily tracking the volume in our own volume field and in the sink's volume, and that feels a little brittle. Plus it just doesn't seem necessary since we don't need that state around (as above, we just defer to whatever self.sink.volume() returns anyway).

# Objective - #16813 added the ability to mute sinks and added a new method `toggle_mute()`. - Leaving `toggle()` as is creates inconsistency and a bit of confusion about what is being toggled. ## Solution - Rename `toggle()` to `toggle_playback()`. - The choice to use the `_playback` suffix was easy because the method comment was already telling us what is being toggled: `Toggles playback of the sink.` - [Raised in Discord] and got the OK from Alice. [Raised in Discord]: https://discord.com/channels/691052431525675048/749430447326625812/1318000355824504905 ## Testing - I ran the example and also updated the instruction text to make it clear `Space` is toggling the playback not just pausing. - I added a unit test for `toggle_playback()` because why not. --- ## Showcase Example instructions: <img width="292" alt="image" src="https://github.com/user-attachments/assets/585c36c6-c4d7-428b-acbe-a92f3a37b460" /> ## Migration Guide - `AudioSinkPlayback`'s `toggle` method has been renamed to `toggle_playback`. This was done to create consistency with the `toggle_mute` method added in #16813. Change instances of `toggle` to `toggle_playback`. E.g.: Before: ```rust fn pause(keyboard_input: Res<ButtonInput<KeyCode>>, sink: Single<&AudioSink>) { if keyboard_input.just_pressed(KeyCode::Space) { sink.toggle(); } } ``` After: ```rust fn pause(keyboard_input: Res<ButtonInput<KeyCode>>, sink: Single<&AudioSink>) { if keyboard_input.just_pressed(KeyCode::Space) { sink.toggle_playback(); } } ```

# Objective - Allow users to mute audio. ```rust fn mute( keyboard_input: Res<ButtonInput<KeyCode>>, mut sink: Single<&mut AudioSink, With<MyMusic>>, ) { if keyboard_input.just_pressed(KeyCode::KeyM) { sink.toggle_mute(); } } ``` - I want to be able to press, say, `M` and mute all my audio. I want this for dev, but I'm sure it's a useful player setting as well. - Muting is different to pausing—I don't want to pause my sounds, I want them to keep playing but with no volume. For example if I have background music playing which is made up of 5 tracks, I want to be able to temporarily mute my background music, and if I unmute at, say, track 4, I want to play track 4 rather than have had everything paused and still be on the first track. - I want to be able to continue to control the volume of my audio even when muted. Like in the example, if I have muted my audio but I use the volume up/down controls, I want Bevy to remember those volume changes so that when I unmute, the volume corresponds to that. ## Solution - Add methods to audio to allow muting, unmuting and toggling muting. - To preserve the user's intended volume, each sink needs to keep track of a "managed volume". - I checked `rodio` and I don't see any built in support for doing this, so I added it to `bevy_audio`. - I'm interested to hear if this is a good idea or a bad idea. To me, this API looks nice and looks usable, but I'm aware it involves some changes to the existing API and now also requires mutable access in some places compared to before. - I'm also aware of work on *Better Audio*, but I'm hoping that if this change isn't too wild it might be a useful addition considering we don't really know when we'll eventually get better audio. ## Testing - Update and run the example: `cargo run --example audio_control` - Run the example: `cargo run --example soundtrack` - Update and run the example: `cargo run --example spatial_audio_3d` - Add unit tests. --- ## Showcase See 2 changed examples that show how you can mute an audio sink and a spatial audio sink. ## Migration Guide - The `AudioSinkPlayback` trait now has 4 new methods to allow you to mute audio sinks: `is_muted`, `mute`, `unmute` and `toggle_mute`. You can use these methods on `bevy_audio`'s `AudioSink` and `SpatialAudioSink` components to manage the sink's mute state. - `AudioSinkPlayback`'s `set_volume` method now takes a mutable reference instead of an immutable one. Update your code which calls `set_volume` on `AudioSink` and `SpatialAudioSink` components to take a mutable reference. E.g.: Before: ```rust fn increase_volume(sink: Single<&AudioSink>) { sink.set_volume(sink.volume() + 0.1); } ``` After: ```rust fn increase_volume(mut sink: Single<&mut AudioSink>) { let current_volume = sink.volume(); sink.set_volume(current_volume + 0.1); } ``` - The `PlaybackSettings` component now has a `muted` field which you can use to spawn your audio in a muted state. `PlaybackSettings` also now has a helper method `muted` which you can use when building the component. E.g.: ```rust commands.spawn(( // ... AudioPlayer::new(asset_server.load("sounds/Windless Slopes.ogg")), PlaybackSettings::LOOP.with_spatial(true).muted(), )); ``` --------- Co-authored-by: Nathan Graule <[email protected]>

# Objective - bevyengine#16813 added the ability to mute sinks and added a new method `toggle_mute()`. - Leaving `toggle()` as is creates inconsistency and a bit of confusion about what is being toggled. ## Solution - Rename `toggle()` to `toggle_playback()`. - The choice to use the `_playback` suffix was easy because the method comment was already telling us what is being toggled: `Toggles playback of the sink.` - [Raised in Discord] and got the OK from Alice. [Raised in Discord]: https://discord.com/channels/691052431525675048/749430447326625812/1318000355824504905 ## Testing - I ran the example and also updated the instruction text to make it clear `Space` is toggling the playback not just pausing. - I added a unit test for `toggle_playback()` because why not. --- ## Showcase Example instructions: <img width="292" alt="image" src="https://github.com/user-attachments/assets/585c36c6-c4d7-428b-acbe-a92f3a37b460" /> ## Migration Guide - `AudioSinkPlayback`'s `toggle` method has been renamed to `toggle_playback`. This was done to create consistency with the `toggle_mute` method added in bevyengine#16813. Change instances of `toggle` to `toggle_playback`. E.g.: Before: ```rust fn pause(keyboard_input: Res<ButtonInput<KeyCode>>, sink: Single<&AudioSink>) { if keyboard_input.just_pressed(KeyCode::Space) { sink.toggle(); } } ``` After: ```rust fn pause(keyboard_input: Res<ButtonInput<KeyCode>>, sink: Single<&AudioSink>) { if keyboard_input.just_pressed(KeyCode::Space) { sink.toggle_playback(); } } ```

# Objective - Allow users to mute audio. ```rust fn mute( keyboard_input: Res<ButtonInput<KeyCode>>, mut sink: Single<&mut AudioSink, With<MyMusic>>, ) { if keyboard_input.just_pressed(KeyCode::KeyM) { sink.toggle_mute(); } } ``` - I want to be able to press, say, `M` and mute all my audio. I want this for dev, but I'm sure it's a useful player setting as well. - Muting is different to pausing—I don't want to pause my sounds, I want them to keep playing but with no volume. For example if I have background music playing which is made up of 5 tracks, I want to be able to temporarily mute my background music, and if I unmute at, say, track 4, I want to play track 4 rather than have had everything paused and still be on the first track. - I want to be able to continue to control the volume of my audio even when muted. Like in the example, if I have muted my audio but I use the volume up/down controls, I want Bevy to remember those volume changes so that when I unmute, the volume corresponds to that. ## Solution - Add methods to audio to allow muting, unmuting and toggling muting. - To preserve the user's intended volume, each sink needs to keep track of a "managed volume". - I checked `rodio` and I don't see any built in support for doing this, so I added it to `bevy_audio`. - I'm interested to hear if this is a good idea or a bad idea. To me, this API looks nice and looks usable, but I'm aware it involves some changes to the existing API and now also requires mutable access in some places compared to before. - I'm also aware of work on *Better Audio*, but I'm hoping that if this change isn't too wild it might be a useful addition considering we don't really know when we'll eventually get better audio. ## Testing - Update and run the example: `cargo run --example audio_control` - Run the example: `cargo run --example soundtrack` - Update and run the example: `cargo run --example spatial_audio_3d` - Add unit tests. --- ## Showcase See 2 changed examples that show how you can mute an audio sink and a spatial audio sink. ## Migration Guide - The `AudioSinkPlayback` trait now has 4 new methods to allow you to mute audio sinks: `is_muted`, `mute`, `unmute` and `toggle_mute`. You can use these methods on `bevy_audio`'s `AudioSink` and `SpatialAudioSink` components to manage the sink's mute state. - `AudioSinkPlayback`'s `set_volume` method now takes a mutable reference instead of an immutable one. Update your code which calls `set_volume` on `AudioSink` and `SpatialAudioSink` components to take a mutable reference. E.g.: Before: ```rust fn increase_volume(sink: Single<&AudioSink>) { sink.set_volume(sink.volume() + 0.1); } ``` After: ```rust fn increase_volume(mut sink: Single<&mut AudioSink>) { let current_volume = sink.volume(); sink.set_volume(current_volume + 0.1); } ``` - The `PlaybackSettings` component now has a `muted` field which you can use to spawn your audio in a muted state. `PlaybackSettings` also now has a helper method `muted` which you can use when building the component. E.g.: ```rust commands.spawn(( // ... AudioPlayer::new(asset_server.load("sounds/Windless Slopes.ogg")), PlaybackSettings::LOOP.with_spatial(true).muted(), )); ``` --------- Co-authored-by: Nathan Graule <[email protected]>

# Objective - bevyengine#16813 added the ability to mute sinks and added a new method `toggle_mute()`. - Leaving `toggle()` as is creates inconsistency and a bit of confusion about what is being toggled. ## Solution - Rename `toggle()` to `toggle_playback()`. - The choice to use the `_playback` suffix was easy because the method comment was already telling us what is being toggled: `Toggles playback of the sink.` - [Raised in Discord] and got the OK from Alice. [Raised in Discord]: https://discord.com/channels/691052431525675048/749430447326625812/1318000355824504905 ## Testing - I ran the example and also updated the instruction text to make it clear `Space` is toggling the playback not just pausing. - I added a unit test for `toggle_playback()` because why not. --- ## Showcase Example instructions: <img width="292" alt="image" src="https://github.com/user-attachments/assets/585c36c6-c4d7-428b-acbe-a92f3a37b460" /> ## Migration Guide - `AudioSinkPlayback`'s `toggle` method has been renamed to `toggle_playback`. This was done to create consistency with the `toggle_mute` method added in bevyengine#16813. Change instances of `toggle` to `toggle_playback`. E.g.: Before: ```rust fn pause(keyboard_input: Res<ButtonInput<KeyCode>>, sink: Single<&AudioSink>) { if keyboard_input.just_pressed(KeyCode::Space) { sink.toggle(); } } ``` After: ```rust fn pause(keyboard_input: Res<ButtonInput<KeyCode>>, sink: Single<&AudioSink>) { if keyboard_input.just_pressed(KeyCode::Space) { sink.toggle_playback(); } } ```

Add ability to mute audio sinks

fc22fa2

mgi388 force-pushed the mute-audio-sink branch from 0821fe6 to fc22fa2 Compare December 14, 2024 02:48

alice-i-cecile added C-Feature A new feature, making something new possible A-Audio Sounds playback and modification S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 14, 2024

alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Dec 14, 2024

SolarLiner suggested changes Dec 14, 2024

View reviewed changes

crates/bevy_audio/src/sinks.rs Outdated Show resolved Hide resolved

crates/bevy_audio/src/sinks.rs Outdated Show resolved Hide resolved

crates/bevy_audio/src/sinks.rs Outdated Show resolved Hide resolved

crates/bevy_audio/src/sinks.rs Outdated Show resolved Hide resolved

mgi388 and others added 6 commits December 14, 2024 21:00

Apply suggestions from code review

21cd987

Co-authored-by: Nathan Graule <[email protected]>

Add missed volume() change for SpatialAudioSink

36c8138

Add unit tests

712e705

lint

3d92e90

add mute system to spatial audio 3d example as well

da352c3

rparrett reviewed Dec 14, 2024

View reviewed changes

examples/audio/spatial_audio_3d.rs Show resolved Hide resolved

SolarLiner approved these changes Dec 14, 2024

View reviewed changes

rparrett reviewed Dec 14, 2024

View reviewed changes

mgi388 added 3 commits December 15, 2024 12:55

Add muted field to PlaybackSettings

0c3574e

Add example instructions to audio_control

41d9de9

Add M: Toggle Mute to instructions in spatial_audio_3d

634bd1d

mgi388 added 3 commits December 15, 2024 13:17

lint

7b2d3b4

lint

ed0841e

Update disable-audio.patch

7496839

Tweak some docs

bb63fea

mgi388 requested review from rparrett and SolarLiner December 15, 2024 03:11

rparrett approved these changes Dec 15, 2024

View reviewed changes

rparrett added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Dec 15, 2024

SolarLiner approved these changes Dec 15, 2024

View reviewed changes

alice-i-cecile 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 Dec 15, 2024

alice-i-cecile added this pull request to the merge queue Dec 15, 2024

Merged via the queue into bevyengine:main with commit 7749c99 Dec 15, 2024
30 checks passed

mgi388 deleted the mute-audio-sink branch December 15, 2024 22:29

mgi388 mentioned this pull request Dec 16, 2024

Rename AudioSinkPlayback::toggle to toggle_playback #16837

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to mute audio sinks #16813

Add ability to mute audio sinks #16813

mgi388 commented Dec 14, 2024 •

edited

Loading

alice-i-cecile commented Dec 14, 2024

SolarLiner left a comment

rparrett commented Dec 14, 2024 •

edited

Loading

SolarLiner commented Dec 14, 2024

rparrett commented Dec 14, 2024

SolarLiner commented Dec 14, 2024

rparrett commented Dec 14, 2024

rparrett left a comment

mgi388 commented Dec 15, 2024 •

edited

Loading

Add ability to mute audio sinks #16813

Add ability to mute audio sinks #16813

Conversation

mgi388 commented Dec 14, 2024 • edited Loading

Objective

Solution

Testing

Showcase

Migration Guide

alice-i-cecile commented Dec 14, 2024

SolarLiner left a comment

Choose a reason for hiding this comment

rparrett commented Dec 14, 2024 • edited Loading

SolarLiner commented Dec 14, 2024

rparrett commented Dec 14, 2024

SolarLiner commented Dec 14, 2024

rparrett commented Dec 14, 2024

rparrett left a comment

Choose a reason for hiding this comment

mgi388 commented Dec 15, 2024 • edited Loading

mgi388 commented Dec 14, 2024 •

edited

Loading

rparrett commented Dec 14, 2024 •

edited

Loading

mgi388 commented Dec 15, 2024 •

edited

Loading