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

Migrate to rodio 0.12 using thread local resources #692

Merged
merged 7 commits into from
Oct 20, 2020
Merged

Migrate to rodio 0.12 using thread local resources #692

merged 7 commits into from
Oct 20, 2020

Conversation

Dash-L
Copy link
Contributor

@Dash-L Dash-L commented Oct 16, 2020

Checklist

I confirm that I have done the following (if applicable):

  • Added a changelog entry
  • Added respective unit tests (no unit tests added, should I add some?)

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.

@memoryruins memoryruins added A-Audio Sounds playback and modification dependencies labels Oct 17, 2020
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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.

This should be good to go after cleaning up the unused code and resolving merge conflicts

.finish()
}
}
// Removed the `Debug` impl because `AudioOutput` no longer stores any relevent info
Copy link
Member

Choose a reason for hiding this comment

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

lets remove this commented out code

@Dash-L
Copy link
Contributor Author

Dash-L commented Oct 20, 2020

I have currently severely messed this up but I can't do anymore tonight, I will try to fix it sometime tomorrow.

@cart
Copy link
Member

cart commented Oct 20, 2020

No worries!

@Dash-L
Copy link
Contributor Author

Dash-L commented Oct 20, 2020

So I believe everything is good, but there are 7 commits and not all of them are even mine, should I:

  • Do nothing, this is fine?
  • Get rid of all but the last commit with git-revert(?) or something?
  • Close this PR and open a new one with just one commit?
  • Something else entierly?

@MichaelHills
Copy link
Contributor

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.

@Dash-L
Copy link
Contributor Author

Dash-L commented Oct 20, 2020

Ok great

@cart
Copy link
Member

cart commented Oct 20, 2020

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.

@cart cart merged commit 0dbba3e into bevyengine:master Oct 20, 2020
@cart cart mentioned this pull request Oct 20, 2020
@Dash-L Dash-L deleted the rodio-update branch October 20, 2020 20:49
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
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants