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

[forbjok]

Fixes #850

[NathanSWard]

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)

[mockersf]

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...

[NathanSWard]

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.

[MinerSebas]

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>

[NathanSWard]

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 :)

[NathanSWard]

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.

[Pauan]

@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.

[mockersf]

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?

[Pauan]

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.

[cart]

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).

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (nightly, ubuntu-latest)
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• check-doc
• check-markdown-links
• check-missing-examples-in-docs
• markdownlint
• run-examples