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

custom rodio source for audio #145

Merged
merged 3 commits into from
Sep 8, 2020

Conversation

tarkah
Copy link
Contributor

@tarkah tarkah commented Aug 12, 2020

This is very much an early draft and in need of feedback.

I've made AudioOuput able to play anything Decodable. Decodable has been implemented on AudioSource and is the concrete type used for creating the AudioOutput in the AudioPlugin. Decodable outputs a Decoder which must implement rodio::Source<Item = rodio::Sample> + Send + Sync to be accepted by AudioOutput.

The downside right now is if someone created their own Decodable, they'd need to setup their own AudioOutput on that type and redo a bunch of the stuff the AudioPlugin sets up on behalf of the user. I've done this at the following test with my Atra3+ decoder and it works. https://github.com/tarkah/bevy-test-custom-audio. Also, users have to specify AudioOutput<AudioSource> out of the box, which is less than ideal, though we could type alias something I'm sure.

I couldn't get dynamic dispatching working in the way I was hoping since rodio::Sample in the associated type for rodio::Source is Sized. I have no idea if it's possible to have a single AudioOutput with a dynamic rodio::Source input without having to define separate instances over the concrete rodio::Source types. I was hoping to be able to do a Box<dyn Decodable> type setup but it just doesn't work. I'm also a little out of my depth here so maybe I am missing something, or maybe I am just tackling this API change completely wrong. Anyway, your feedback would be appreciated!

@tarkah
Copy link
Contributor Author

tarkah commented Aug 12, 2020

Also, I understand if this is super low priority, so no rush on this at all!

@karroffel karroffel added A-Audio Sounds playback and modification C-Feature A new feature, making something new possible labels Aug 12, 2020
@multun
Copy link
Contributor

multun commented Aug 19, 2020

Can you rebase this on master? Format checking was enabled, and the CI doesn't yet check for it at the commit you're at

@tarkah
Copy link
Contributor Author

tarkah commented Aug 19, 2020

Rebased

@multun
Copy link
Contributor

multun commented Aug 19, 2020

Awesome, thanks! (the failure isn't yours)

@tarkah
Copy link
Contributor Author

tarkah commented Aug 19, 2020

No problem! I noticed your PR to fix the formatting issue.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

In general this seem like a reasonable change. It does introduce a bit of complexity and I dont really understand the cases where you would need new Decodables / rodio::Sources. Could you elaborate on that a bit?

crates/bevy_audio/src/audio_output.rs Outdated Show resolved Hide resolved
examples/audio/audio.rs Outdated Show resolved Hide resolved
@tarkah
Copy link
Contributor Author

tarkah commented Aug 25, 2020

@cart Thanks for taking some time to look at this. I understand this is an edge case, as most people are going to be using MP3, WAV, Vorbis or Flac sources that are supported by the rodio::Decoder. However, if someone wanted to play some other codec, this entire audio module would not work for them. For example, needing to play .at3 that my atrac3-plus decoder handles if you wanted to make an open source port of a PS Vita game using this library.

Since the rodio::Source trait is what enables all the awesome stuff rodio can do with sound, it just seems like a good idea to be able to support playing any rodio::Source and not just what can be decoded by rodio::Decoder.

Anyway, I'm still not in love with where I've landed on this API and I think it's reasonable to hold off on this until we are 100% sure it's a reasonable addition to the audio module. In the meantime I'll go ahead and rebase / make the changes you suggested.

@cart cart marked this pull request as ready for review September 8, 2020 20:36
@cart
Copy link
Member

cart commented Sep 8, 2020

I think this is in a good spot to merge. Thanks for your work here!

@cart cart merged commit 2667c24 into bevyengine:master Sep 8, 2020
@tarkah
Copy link
Contributor Author

tarkah commented Sep 8, 2020

Great, thanks @cart!

mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
support custom rodio source for audio
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
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants