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.

Sign up for GitHub

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

update documentation on AudioSink #9332

Merged
merged 2 commits into from
Aug 3, 2023
Merged

update documentation on AudioSink #9332

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
870c6f1
update documentation on AudioSink
rdrpenguin04 Aug 1, 2023
ab7458d
add documentation to SpatialAudioSink as well
rdrpenguin04 Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions crates/bevy_audio/src/sinks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ pub trait AudioSinkPlayback {
/// Use [`AudioBundle`][crate::AudioBundle] to trigger that to happen.
///
/// You can use this component to modify the playback settings while the audio is playing.
///
/// If this component is removed from an entity, and an [`AudioSource`][crate::AudioSource] is
/// attached to that entity, that [`AudioSource`][crate::AudioSource] will start playing. If
/// that source is unchanged, that translates to the audio restarting.
Copy link
Member

@Selene-Amanita Selene-Amanita Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What does "unchanged" mean? When it's already playing, or when it was playing but paused in the middle?

Why does this happen? Shouldn't the audio stop? Isn't this a bug? Is a new AudioSink re-added?

Does it also happen if there is no PlaybackSettings on this entity, what about if PlaybackSettings::paused is true?

To be honest I'm not sure why the API is this way, I feel like to be consistent with most other Bevy APIs the AudioSink should be part of AudioSourceBundle not added after, and no audio would be played if an entity doesn't have all the components of AudioSourceBundle including AudioSink, but there's maybe something I don't know/am missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AudioSink is created by the audio engine dynamically. This may be a bug, but it's difficult to change, and it's genuinely useful. There's a discussion on the Discord about this in the #audio-dev channel (Reproducer and following discussion: https://canary.discord.com/channels/691052431525675048/749430447326625812/1135716276132642878).

If the AudioSource that was used to start the audio hasn't been removed, and the AudioSink is removed, it's recreated from the same AudioSource, thus being an extremely easy way to restart music. Likewise, if the source is swapped out for another one, this provides a clean way to swap from one song to another instantly.

Copy link
Member

@Selene-Amanita Selene-Amanita Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It makes sense that that's what's happening.

That's a personal opinion and a bit out of scope for this PR but this API (if it's intended to have to remove the AudioSink to restart an audio track or switch track) feels kinda hacky to me.
I would expect:

  • either AudioSink to be added in AudioSourceBundle and play_queued_audio_system would query newly-added AudioSink, or play_queued_audio_system would create AudioSink only for newly added Handle<Source> and PlaybackSettings,
  • a system would check for changed Handle<Source> to recreate the AudioSink if the track changed
  • a method on either AudioSink or PlaybackSettings would allow the track to restart,
  • I would also expect that either PlaybackSettings is deleted when AudioSink is created, or that modifying its fields after would effectively do something

There might be a very good reason for most of it though like not having a one-frame lag in the changes or something.

Anyway, for this PR, I think this comment should clearly mention what's happening, something like:

/// If this component is removed from an entity, and the components from `[`AudioBundle`] are still
/// attached to that entity, a new [`AudioSink`] will be recreated. This will effectively:
/// - reset the playback settings to the values in [`PlaybackSettings`]
/// - restart the audio source from the begining
/// - change the audio source if [`Handle<Source>`](Source) changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is not to explain the implementation details but to explain the effect of them; other parts of the documentation don't explain why the engine does what it does with certain things, so why should this?

Also, the more I think about it, the more natural this API feels. It's odd, but not as odd as it could be.

Copy link
Member

@Selene-Amanita Selene-Amanita Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think when components are added/removed from entities automatically should be documented.

I'm not saying it should be mentioned in the doc of AudioBundle for example, but this is the doc of this component, how it behaves is important.

#[derive(Component)]
pub struct AudioSink {
rdrpenguin04 marked this conversation as resolved.
Show resolved Hide resolved
// This field is an Option in order to allow us to have a safe drop that will detach the sink.
Expand Down Expand Up @@ -128,6 +132,10 @@ impl AudioSinkPlayback for AudioSink {
/// Use [`SpatialAudioBundle`][crate::SpatialAudioBundle] to trigger that to happen.
///
/// You can use this component to modify the playback settings while the audio is playing.
///
/// If this component is removed from an entity, and a [`AudioSource`][crate::AudioSource] is
/// attached to that entity, that [`AudioSource`][crate::AudioSource] will start playing. If
/// that source is unchanged, that translates to the audio restarting.
#[derive(Component)]
pub struct SpatialAudioSink {
// This field is an Option in order to allow us to have a safe drop that will detach the sink.
Expand Down