Migrate to rodio 0.12 using thread local resources #692
Conversation
CHANGELOG.md
Outdated
| @@ -14,6 +14,10 @@ | |||
| - New methods `Color::rgb_linear` and `Color::rgba_linear` will accept colors already in linear sRGB (the old behavior) | |||
| - Individual color-components must now be accessed through setters and getters: `.r`, `.g`, `.b`, `.a`, `.set_r`, `.set_g`, `.set_b`, `.set_a`, and the corresponding methods with the `*_linear` suffix. | |||
| - Despawning an entity multiple times causes a debug-level log message to be emitted instead of a panic [649] [651] | |||
| - Breaking Change: Migrated to rodio 0.12, this means: | |||
| - All audio must be played on the main thread | |||
There was a problem hiding this comment.
Would it be possible to "schedule" audio from other threads? That way you are not forced to use thread_local_systems to trigger sound effects etc?
There was a problem hiding this comment.
My idea was to use events and have an internal thread_local_system to handle those events, which would also (theoretically) allow pausing and modifying already playing sounds, but I didn't implement that during the initial migration. Should I include that as a commit? Also, do you think events would be a good idea? We could also probably have an AudioScheduler struct if you think that would work better than events.
There was a problem hiding this comment.
I'm not familiar enough with the Bevy architecture of events so can't really comment on events vs struct without seeing code. As for whether we need a solution in this PR, I would lean towards yes as to not break the ergonomics of being able to play audio from any system. It would be good to get some feedback from others though! I was just passing by to use your commit for testing iOS audio.
There was a problem hiding this comment.
Yeah I don't want to require thread local systems to queue up audio and I think we should resolve that before merging. How about something like this:
fn system(asset_server: Res<AssetServer>, audio: Res<Audio>) {
let music = asset_server.load("song.mp3");
audio.play(music);
}Where Audio is the interface most users use to interact with Audio playback + audio configuration. AudioOutput would still be thread-local, but its job is to play queued sounds from Audio.
| .finish() | ||
| } | ||
| } | ||
| // Removed the `Debug` impl because `AudioOutput` no longer stores any relevent info |
There was a problem hiding this comment.
lets remove this commented out code
Updates the requirements on [ahash](https://github.com/tkaitchuck/ahash) to permit the latest version. - [Release notes](https://github.com/tkaitchuck/ahash/releases) - [Commits](https://github.com/tkaitchuck/ahash/commits) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
I have currently severely messed this up but I can't do anymore tonight, I will try to fix it sometime tomorrow. |
|
No worries! |
|
So I believe everything is good, but there are 7 commits and not all of them are even mine, should I:
|
|
Based on my observation of the commit log, this PR will be automatically squashed when merged with the commit message based off the PR description. So sounds like, if code is all good, you don't need to do anything. |
|
Ok great |
|
Yeah looks like we got a dependabot commit in there somehow. We could do an interactive rebase and drop that commit, but @MichaelHills is right, this will all get squashed when we merge so its all basically the same. |
Checklist
I confirm that I have done the following (if applicable):
rodio 0.12 seems to fix a bunch of audio panics and crashes. In its current state audio is a little clunky to use (because audio must be played on the main thread, in a separate system than most of the app logic is going to be in) but it's usable and hopefully can be made better later.