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

Make DynamicMixer and OutputStream generic #547

Closed
wants to merge 3 commits into from

Conversation

eira-fransham
Copy link

This commit adds a new trait, Mixer, and refactors some of the code around generic mixers in order to support it. This is to make more of the code reusable without rewriting - specifically, to support a feature that I would like to implement in bevy_audio where audio sources can send audio to an intermediate mixer. The motivation for this was to add a global limiter to all game audio, so that loud sounds did not cause clipping on the master bus, but another possible usecase would be to split in-game audio and menu audio into separate "worlds" and apply, for example, a low-pass filter while in the pause screen.

This commit adds a new trait, `Mixer`, and refactors some of the
code around generic mixers in order to support it. This is to
make more of the code reusable without rewriting - specifically,
to support a feature that I would like to implement in
`bevy_audio` where audio sources can send audio to an
intermediate mixer. The motivation for this was to add a global
limiter to all game audio, so that loud sounds did not cause
clipping on the master bus, but another possible usecase would
be to split in-game audio and menu audio into separate "worlds"
and apply, for example, a low-pass filter while in the pause
screen.
@eira-fransham
Copy link
Author

It looks like the test failures are due to unrelated code, and are because of the addition of redundant import checks to unused_imports.

eira-fransham added a commit to eira-fransham/bevy that referenced this pull request Mar 5, 2024
This commit adds a new component, `AudioTarget`. This component allows marking sources to
be played via a specific `OutputStreamHandle` instead of the global one. At time of
writing, changing the audio target will result in restarting the source, as `rodio`
provides no way to get the inner source queue for a `Sink`. This is already the case for
the current version of Bevy when a component's `AudioSink` is removed, however, so I
think that this behavior is acceptable for now.

This allows users to specifically target certain audio devices, and when [rodio#547](RustAudio/rodio#547)
lands it will allow users to use intermediate mixers - e.g. to add global or per-item-set
effects such as reverb.
@eira-fransham
Copy link
Author

A more-minimal version of this PR would just make OutputStream generic, which would still allow reusing rodio's mixer without adding an actual interface around creating your own.

Copy link
Contributor

@hamirmahal hamirmahal left a comment

Choose a reason for hiding this comment

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

It looks like the test failures are due to unrelated code, and are because of the addition of redundant import checks to unused_imports.

The test failures should be gone on the main branch now because of #554.

@dvdsk
Copy link
Collaborator

dvdsk commented Mar 25, 2024

Could you rebase this on main? (or merge main into it). Then you'll get the fixed (thanks @hamirmahal) test suite.

@hamirmahal
Copy link
Contributor

You're welcome, @dvdsk!

@dvdsk dvdsk added the waiting for rebase PR needs to be rebased before it can be reviewed/merged label Mar 28, 2024
@dvdsk dvdsk force-pushed the master branch 3 times, most recently from b10961c to a7f67b3 Compare October 10, 2024 00:14
@dvdsk dvdsk force-pushed the master branch 4 times, most recently from 0ee0413 to 073fdb1 Compare November 8, 2024 00:55
@dvdsk
Copy link
Collaborator

dvdsk commented Nov 24, 2024

I'm going to close this given we just redesigned the mixer & stream and the code has diverged a lot now. During the design we discussed this use-case and decided to not make the mixer generic for now.

The performance and API tradeoffs where unclear and the use-cases are already possible by adding a Source to the mixer that is itself a new special mixer.

That reduced the use-case of this PR to optimization.

Once we have a better idea how the new dynamic source API will look & how the new Player will work designing this will be easier.

Feel free to re-open but the re-base is going to be hellish and its probably better to start again.

@dvdsk dvdsk closed this Nov 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for rebase PR needs to be rebased before it can be reviewed/merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants