Avoid panic if there is no audio device present #341
Conversation
karroffel
added
A-Audio
| let sink = Sink::new(&self.device); | ||
| sink.append(Decoder::new(Cursor::new(audio_source.clone())).unwrap()); | ||
| sink.detach(); | ||
| if let Some(device) = &self.device { |
There was a problem hiding this comment.
I believe this should be handled at a higher level, currently this would hide the reason audio playback isn't occurring. Possibly instead of using rodio::default_output_device, it could add a resource for a selected audio device from rodio::devices, and use that here instead? Then the app developer could handle which audio device to select (or pass it to the user), register a default audio source if the developer didn't select one (so would mimic current behavior), and could display an error up front so its not hidden
There was a problem hiding this comment.
I thought about that when writing this, I was considering this as a quick fix to the default behavior so that someone just trying out the engine doesn't immediately have a panic. I assumed that it would be changed at some point when the audio engine functionality is more fleshed out. Nonetheless the rodio API is changing completely in the next version, currently in master (see #310), so now I'm not sure about this change as it might be best to just wait for that to drop. @cart any thoughts? I might just close this PR if its not needed.
There was a problem hiding this comment.
Hmm short term I think its probably best to wait. Medium term once we adapt to the new rodio api, I think we need a way to gracefully handle the following cases:
- no audio device present (no crashes, playing sounds results in no output)
- audio device changes (no crashes, as soon as new device is ready audio switches to that device).
There was a problem hiding this comment.
Maybe we should add something like a "null audio device".
There was a problem hiding this comment.
That was my thought as well, I'll close this PR for now, that way the initial issue will stay open. #310 has an initial implementation of the new API, could move the conversation over there.
Fixes #316