First pass of review comments - #105
Conversation
| # My proposal for the path forward is to have a bevy_firewheel crate in bevy/crates, which houses a copy | ||
| # of the firewheel top level crate and all firewheel subcrates (with names unchanged), as well as the | ||
| # original top level firewheel crate. bevy_firewheel would bring in all the subcrates with bevy integration | ||
| # enabled, and the original top level firewheel crate will be the way to use firewheel without bevy. |
There was a problem hiding this comment.
note that i wrote this on the plane before discussion after posting my writeup, this is still one of the possible plans but is not my #1 anymore
| } | ||
| } | ||
|
|
||
| // TODO: method duplication |
There was a problem hiding this comment.
The reason for the duplication is to have optimized auto-vectorized loops for the most common use cases. Though I suppose those optimized loops could all be in a single function.
| pub fn try_debug(&mut self, message: &str) -> Result<(), RealtimeLogError> { | ||
| #[cfg(debug_assertions)] | ||
| { | ||
| // TODO: code duplication |
| ] | ||
| # Enables the wasm-bindgen feature for the CPAL backend | ||
| wasm-bindgen = ["firewheel-cpal/wasm-bindgen"] | ||
| # NOTE: these glam deps will also be maintenance burden, in an upstream future we should |
| ] | ||
|
|
||
| [workspace.dependencies] | ||
| # if we upstream firewheel to bevy, these should become deps on bevy_log |
|
|
||
| // TODO: PLEASE FIX ME: | ||
| // | ||
| // TODO: investigate CPAL |
There was a problem hiding this comment.
Yeah, I'm at a bit of a loss on this one. Might have to make an issue in the CPAL repository.
| # of the firewheel top level crate and all firewheel subcrates (with names unchanged), as well as the | ||
| # original top level firewheel crate. bevy_firewheel would bring in all the subcrates with bevy integration | ||
| # enabled, and the original top level firewheel crate will be the way to use firewheel without bevy. | ||
| # Versioning will become lockstep with bevy versions for all firewheel crates. |
There was a problem hiding this comment.
We could also consider merging the crates to as few as possible. The only one that really needs to be separate is firewheel-cpal (assuming we keep the audio backend abstraction).
firewheel-core was originally meant to be like a "stable node API" that third party node developers would base on. But if we are planning to upstream Firewheel to Bevy anyway, then I suppose it doesn't make sense to have this stability guarantee anymore.
2 participants
As i can't easily leave comments on already merged code, and I am auditing the entire codebase, the easiest way to leave notes is as comments. I do not intend for this PR to merge; it is just reference. I will be putting up PRs fixing the things ive noted here once the other two PRs i have open merge: #103 #104