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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Add feature flag to enable wasm for bevy_audio #2397

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Add feature flag to enable wasm for bevy_audio #2397

wants to merge 3 commits into from

Conversation

Ixentus
Copy link
Contributor

@Ixentus Ixentus commented Jun 26, 2021 •
edited
Loading

Exposes Rodio feature flag to enable WASM support.

Note that mp3 doesn't currently work on wasm.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 26, 2021
mockersf
mockersf reviewed Jun 26, 2021
View reviewed changes
docs/cargo_features.md Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added A-Audio Sounds playback and modification C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Jun 27, 2021
MinerSebas
MinerSebas reviewed Jun 27, 2021
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@mockersf mockersf added the O-Web Specific to web (WASM) builds label Jun 27, 2021
@Ixentus @MinerSebas
Update Cargo.toml
242ed35 
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
@Ixentus
Copy link
Contributor Author

Ixentus commented Jul 13, 2021

Is there anything left to do before merging this? I've been happily listening to audio on web for weeks now :)

@mockersf
Copy link
Member

it needs another approval 🙂

NiklasEi
NiklasEi approved these changes Jul 13, 2021
View reviewed changes
Copy link
Member

@NiklasEi NiklasEi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thank you.

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 13, 2021
@mockersf
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jul 14, 2021
@Ixentus
Add feature flag to enable wasm for bevy_audio (#2397)
d80303d 
Exposes Rodio feature flag to enable WASM support.

Note that mp3 doesn't currently work on wasm.
@cart
Copy link
Member

cart commented Jul 14, 2021

Is there a reason to not enable this by default? Rather than adding another cargo feature, maybe we should just selectively enable the "rodio/wasm-bindgen" when building bevy_audio for wasm?

@mockersf
Copy link
Member

cargo doesn't support per target feature: rust-lang/cargo#1197

@cart
Copy link
Member

cart commented Jul 14, 2021

Ah yeah. Maybe we should start considering a move to "feature resolver v2" now that its stable?

@bors
Copy link
Contributor

bors bot commented Jul 14, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add feature flag to enable wasm for bevy_audio [Merged by Bors] - Add feature flag to enable wasm for bevy_audio Jul 14, 2021
@bors bors bot closed this Jul 14, 2021
@mockersf
Copy link
Member

mockersf commented Jul 14, 2021 •
edited
Loading

Ah yeah. Maybe we should start considering a move to "feature resolver v2" now that its stable?

The resolver v2 is not something we can enable for consumers of Bevy, they would have to enable it in their project. We should mention it in all the docs, and it's something additional to do when starting with Bevy... and all existing projects would need to update

@Ixentus Ixentus deleted the wasm_audio branch July 14, 2021 05:13
@cart
Copy link
Member

cart commented Jul 14, 2021

Arg thats annoying. I guess we'll just need to wait for Rust 2021 to make it the default.

@mockersf mockersf mentioned this pull request Jul 14, 2021
Closed
1 task
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
@Ixentus @ostwilkens
Add feature flag to enable wasm for bevy_audio (bevyengine#2397)
411611e 
Exposes Rodio feature flag to enable WASM support.

Note that mp3 doesn't currently work on wasm.
@dman-os dman-os mentioned this pull request Aug 15, 2021
Open
@Ixentus Ixentus mentioned this pull request Oct 21, 2021
Closed
bors bot pushed a commit that referenced this pull request Nov 10, 2021
@Ixentus
Remove wasm audio feature flag for 2021 (#3000)
771c5f5 
- Requires #2997 
- Removes `wasm_audio` feature as discussed in #2397
- Closes only task in #2479 

Open questions:
Should we enable wasm audio by default or only when building for wasm using `cfg`?
Maybe there should be a global wasm feature for bevy?
bors bot pushed a commit that referenced this pull request Nov 10, 2021
@Ixentus
Remove wasm audio feature flag for 2021 (#3000)
d98c2fe 
- Requires #2997 
- Removes `wasm_audio` feature as discussed in #2397
- Closes only task in #2479 

Open questions:
Should we enable wasm audio by default or only when building for wasm using `cfg`?
Maybe there should be a global wasm feature for bevy?
bors bot pushed a commit that referenced this pull request Nov 11, 2021
@Ixentus
Remove wasm audio feature flag for 2021 (#3000)
36394ad 
- Requires #2997 
- Removes `wasm_audio` feature as discussed in #2397
- Closes only task in #2479 

Open questions:
Should we enable wasm audio by default or only when building for wasm using `cfg`?
Maybe there should be a global wasm feature for bevy?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MinerSebas MinerSebas MinerSebas left review comments

@mockersf mockersf mockersf approved these changes

@NiklasEi NiklasEi NiklasEi approved these changes

Assignees
No one assigned
Labels
A-Audio Sounds playback and modification C-Feature A new feature, making something new possible O-Web Specific to web (WASM) builds S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Audio
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Ixentus @mockersf @cart @NiklasEi @MinerSebas @alice-i-cecile