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

[Merged by Bors] - Fix Bevy crashing if no audio device is found #2269

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Fix Bevy crashing if no audio device is found #2269

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 21 additions & 11 deletions crates/bevy_audio/src/audio_output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::{Audio, AudioSource, Decodable};
use bevy_asset::{Asset, Assets};
use bevy_ecs::world::World;
use bevy_utils::tracing::warn;
use rodio::{OutputStream, OutputStreamHandle, Sink};
use std::marker::PhantomData;

Expand All @@ -9,8 +10,8 @@ pub struct AudioOutput<P = AudioSource>
where
P: Decodable,
{
_stream: OutputStream,
stream_handle: OutputStreamHandle,
_stream: Option<OutputStream>,
stream_handle: Option<OutputStreamHandle>,
phantom: PhantomData<P>,
}

Expand All @@ -19,12 +20,19 @@ where
P: Decodable,
{
fn default() -> Self {
let (stream, stream_handle) = OutputStream::try_default().unwrap();

Self {
_stream: stream,
stream_handle,
phantom: PhantomData,
if let Ok((stream, stream_handle)) = OutputStream::try_default() {
Self {
_stream: Some(stream),
stream_handle: Some(stream_handle),
phantom: PhantomData,
}
} else {
warn!("No audio device found.");
Self {
_stream: None,
stream_handle: None,
phantom: PhantomData,
}
}
}
}
Expand All @@ -36,9 +44,11 @@ where
<<P as Decodable>::Decoder as Iterator>::Item: rodio::Sample + Send + Sync,
{
fn play_source(&self, audio_source: &P) {
let sink = Sink::try_new(&self.stream_handle).unwrap();
sink.append(audio_source.decoder());
sink.detach();
if let Some(stream_handle) = &self.stream_handle {
let sink = Sink::try_new(&stream_handle).unwrap();
sink.append(audio_source.decoder());
sink.detach();
}
Comment on lines 46 to +51
Copy link
Contributor

@NathanSWard NathanSWard May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If you try to play_source when there is no audio device, I don't like the behavior of silentely succeeding.
I would expect this to either

  1. error!
  2. panic!
  3. have this return like Result<(), ()> (or some other error type)

Copy link
Member

Choose a reason for hiding this comment

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

From the player point of view, an error or panic are bad. "ok I know I don't have a audio device, don't crash and don't yell at me!". There's already a warn on the creation, re-logging it every time a sound play doesn't add much...

Copy link
Contributor

@NathanSWard NathanSWard May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That's fair.
However, I have the counter argument that if you don't have an audio device, and therefore the stream_handle is None, if you are trying to play an audio_source, that's incorrect. Calling these functions if the type itself is not constructed successfully and then "working" seems a little off. 😅
It's fine if you try to create the AudioSource and that fails, (and it doesn't panic)
however, if you actually try to play audio with that audio source, that probably shouldn't work.

Ultimately I'm fine with this PR as is though.
I think it's just important to consider how silently succeeding can be harmful. 😃

Arguable, we should remove the default implementation, and instead do something like:

impl<P> AudioOutput<P> {
    pub fn new() -> Option<Self>;
}

However, I don't think this is actually possible, since it needs Default to be initialized as a resource.

Copy link
Contributor

Choose a reason for hiding this comment

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

While init_resource requires default (actually FromWorld, but that has a blanket implementation for types that implement default), insert_resource doesn't have that bound.

An alternative is that AudioOutput is just never inserted as a Resource, and systems need to query for Option<Res<AudioOutput>> instead of Res<AudioOutput>

Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative is that AudioOutput is just never inserted as a Resource, and systems need to query for Option<Res> instead of Res

Something like this would be my preferred solution :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think something also to consider for right now is that the current standing of bevy_audio is not the best.
It'll mostly likely get replaced in the future via bevy_kira_audio so I think right now, it should be our goal to make the built-in functionality as simple and friendly as possible, as the API will likely get scrapped and reworked completely.

Copy link

Choose a reason for hiding this comment

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

@NathanSWard That's a completely fair position, and if it eventually gets done Correctly™ then that's fine, and I agree that a proper solution should be more general (for all resources/systems), so it's okay to punt it to a later PR.

But there's a big difference between, "this is hacky and we know it's hacky and we'll do it correctly later" and "this is a good design which we should keep". I'm advocating for the first, not the second.

Copy link
Member

@mockersf mockersf May 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I can also see plenty of situations where it is mandatory to have audio, or where the programmer wants to handle audio failure in a specific way. Using Option to represent a missing resource or component is correct.

Just wondering, and very much out of topic, sorry about that... I pretty much always play with no sound because of reasons. Should the volume being off be detected as a failure then?

Copy link

Choose a reason for hiding this comment

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

Should the volume being off be detected as a failure then?

There's two basic designs:

  1. Treat it as not a failure, and have a separate is_muted method.
  2. Return a Result which has an enum that describes the various failures that can happen, with Muted being one of the reasons.

Either design is fine, they just make different tradeoffs. Personally I would go with the first one.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with the idea that users shouldn't need to care about this (most of the time). I personally don't think the spirit of rust is "whenever you have a chance to require explicit error handling ... do it". I think in rust, just like every other language, you should make a pragmatic call based on the context.

Ex: we could make everyone explicitly handle the case where they try to spawn a sprite, but a render device isn't present by making spawn().insert(Sprite) return a RenderDeviceNotAvailable error. But I would argue that doing that would "get in the way" more often than not. A missing render device is a corner case. If every api involving "rendering things" required checking for render device existence, users would have a "very bad time". And when its not there, its generally because the user intended it to be that way (ex; they are running in a headless environment). This is why we have a "headless render backend". We handle this "internally" so when someone tries using sprites, they don't need to think about render devices / account for them being missing.

I would hazard a guess that in the 99.X% case, when users play an audio clip, they don't want to care about audio device existence via if let / unwrap. Therefore we should have an api that optimizes for that use case. We should still expose "device existence information" to users that care about it. But we constantly make choices to hide information / assume a certain "happy path" state on behalf of our users in the interest of ergonomics and a "pleasant programming experience". This feels like a case where we should do that.

I'm happy to continue this discussion, but I think the current impl is a solid short term solution to a nasty problem. And it has the benefit of not degrading UX for a common use case (and requiring if let Some(audio_device) for all audio operation definitely degrades UX for a common use case).

}

fn try_play_queued(&self, audio_sources: &Assets<P>, audio: &mut Audio<P>) {
Expand Down