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

[Merged by Bors] - Audio control - play, pause, volume, speed, loop #3948

Closed
wants to merge 8 commits into from

Conversation

mockersf
Copy link
Member

Objective

  • Add ways to control how audio is played

Solution

  • playing a sound will return a (weak) handle to an asset that can be used to control playback
  • if the asset is dropped, it will detach the sink (same behaviour as now)

@alice-i-cecile alice-i-cecile added A-Audio Sounds playback and modification C-Feature A new feature, making something new possible labels Feb 14, 2022
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 14, 2022
@mockersf mockersf removed the S-Needs-Triage This issue needs to be labelled label Feb 14, 2022
Co-Authored-By: Alice Cecile <alice.i.cecile@gmail.com>
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.

Real audio functionality! In bevy_audio! Very basic still, but this should give us the bare bones needed to do anything useful with it.

Code quality is solid, and this is sorely needed. We don't have the bandwidth to tackle a bevy_kira_audio integration right now, so we should be making bevy_audio incrementally better.

Unsurprisingly, a few nits to clean up.

examples/audio/audio_control.rs Show resolved Hide resolved
examples/audio/audio_control.rs Show resolved Hide resolved
mockersf and others added 3 commits February 14, 2022 20:23
crates/bevy_audio/src/audio_output.rs Show resolved Hide resolved
crates/bevy_audio/src/audio_output.rs Outdated Show resolved Hide resolved
crates/bevy_audio/src/audio_output.rs Outdated Show resolved Hide resolved
crates/bevy_audio/src/audio_output.rs Show resolved Hide resolved
examples/audio/audio_control.rs Outdated Show resolved Hide resolved
Co-Authored-By: Charles <IceSentry@users.noreply.github.com>
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 14, 2022
Comment on lines +114 to 125
impl<Source> fmt::Debug for AudioToPlay<Source>
where
Source: Asset + Decodable,
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("AudioToPlay")
.field("sink_handle", &self.sink_handle)
.field("source_handle", &self.source_handle)
.field("repeat", &self.repeat)
.finish()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

To me this looks like a manual impl of Debug that should be identical to the derive.
Am i missing something?

Copy link
Member Author

@mockersf mockersf Feb 16, 2022

Choose a reason for hiding this comment

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

the automatic derive needs Source to be Debug, but not a manual impl
... I'm not quite sure the debug impl is actually useful though, I added it to keep existing functionality

Comment on lines +101 to +119
/// ```
/// # use bevy_ecs::system::{Local, Res};
/// # use bevy_asset::{Assets, Handle};
/// # use bevy_audio::AudioSink;
/// // Execution of this system should be controlled by a state or input,
/// // otherwise it would just toggle between play and pause every frame.
/// fn pause(
/// audio_sinks: Res<Assets<AudioSink>>,
/// music_controller: Local<Handle<AudioSink>>,
/// ) {
/// if let Some(sink) = audio_sinks.get(&*music_controller) {
/// if sink.is_paused() {
/// sink.play()
/// } else {
/// sink.pause()
/// }
/// }
/// }
/// ```
Copy link
Contributor

Choose a reason for hiding this comment

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

This example is missing the assertion that the fn is a system.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that it's needed: the only thing new in this system is the kind of asset, but systems with those parameters are already tested elsewhere. and if AudioSink is not a valid asset, it would still fail to compile

@wackbyte
Copy link
Contributor

thank you, i was just looking for this today

@alice-i-cecile alice-i-cecile added this to the Bevy 0.7 milestone Feb 24, 2022
@cart
Copy link
Member

cart commented Mar 1, 2022

Nice! Relatively uncontroversial improvements. The "weak handle" returned by audio.play() is a regrettable UX hiccup, but I can't think of a way to resolve that without significant changes:

  1. We cant replace Handle<AudioSink> with Arc<AudioSink> because we can't create a Sink from an Audio reference alone (because audio may not have loaded yet).
  2. We can't create strong handles without mutable access to Assets<AudioSink>

I have some thoughts about asset system changes that might make better UX possible here, but I'm not quite ready to open up that conversation. I think this is the best we can do for now. Well done!

@cart
Copy link
Member

cart commented Mar 1, 2022

bors r+

bors bot pushed a commit that referenced this pull request Mar 1, 2022
# Objective

- Add ways to control how audio is played

## Solution

- playing a sound will return a (weak) handle to an asset that can be used to control playback
- if the asset is dropped, it will detach the sink (same behaviour as now)
@bors bors bot changed the title Audio control - play, pause, volume, speed, loop [Merged by Bors] - Audio control - play, pause, volume, speed, loop Mar 1, 2022
@bors bors bot closed this Mar 1, 2022
rparrett pushed a commit to rparrett/bevy that referenced this pull request Mar 4, 2022
# Objective

- Add ways to control how audio is played

## Solution

- playing a sound will return a (weak) handle to an asset that can be used to control playback
- if the asset is dropped, it will detach the sink (same behaviour as now)
robsws pushed a commit to robsws/bevy that referenced this pull request Mar 4, 2022
# Objective

- Add ways to control how audio is played

## Solution

- playing a sound will return a (weak) handle to an asset that can be used to control playback
- if the asset is dropped, it will detach the sink (same behaviour as now)
kurtkuehnert pushed a commit to kurtkuehnert/bevy that referenced this pull request Mar 6, 2022
# Objective

- Add ways to control how audio is played

## Solution

- playing a sound will return a (weak) handle to an asset that can be used to control playback
- if the asset is dropped, it will detach the sink (same behaviour as now)
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-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

6 participants