Skip to content

First pass of review comments#105

Draft
atlv24 wants to merge 1 commit intoBillyDM:mainfrom
atlv24:ad/review
Draft

First pass of review comments#105
atlv24 wants to merge 1 commit intoBillyDM:mainfrom
atlv24:ad/review

Conversation

@atlv24
Copy link
Contributor

@atlv24 atlv24 commented Feb 19, 2026

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

// this can be used to only recalculate them every few frames.
//
// TODO: use core::hint::cold_path() once that stabilizes
// TODO: instead, we can extract a function and use #[cold] + #[inline(always)] today
Copy link
Owner

Choose a reason for hiding this comment

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

Oh interesting! I might have to try that.

Comment on lines +228 to +231
# 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

&mut channels[0].as_mut()[start_frame_in_channels..start_frame_in_channels + samples];

ch.copy_from_slice(interleaved);
// TODO: should this silence check use epsilon
Copy link
Owner

Choose a reason for hiding this comment

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

Oh yeah, that would be a handy feature to optimize microphone inputs. I'll add that!

}
}

// TODO: method duplication
Copy link
Owner

Choose a reason for hiding this comment

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

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 is_buffer_silent(buffer: &[f32], amp_epsilon: f32) -> bool {
let mut silent = true;
for &s in buffer.iter() {
// TODO: check if this autovectorizes
Copy link
Owner

Choose a reason for hiding this comment

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

It appears to auto-vectorize https://rust.godbolt.org/z/xGen4r59a

pub fn try_debug(&mut self, message: &str) -> Result<(), RealtimeLogError> {
#[cfg(debug_assertions)]
{
// TODO: code duplication
Copy link
Owner

Choose a reason for hiding this comment

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

What is the duplication here?

/// Each channel slice will have a length of [`ProcInfo::frames`].
///
/// These buffers may contain junk data.
// TODO: This is UB; having references to uninitialized data is not allowed. Use MaybeUninit or raw pointers.
Copy link
Owner

Choose a reason for hiding this comment

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

Oh, by "junk data" I just meant that it can contain data from previous process loops (this is a very common thing in the audio processing world, clearing the buffers for every node would be a lot of extra processing overhead). The buffers are all initialized to zero before being sent to the audio thread, so there is no safety concern here. I'll make that clearer in the docs.

]
# 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
Copy link
Owner

Choose a reason for hiding this comment

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

I agree

]

[workspace.dependencies]
# if we upstream firewheel to bevy, these should become deps on bevy_log
Copy link
Owner

Choose a reason for hiding this comment

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

I agree

A mid-level open source audio graph engine for games and other applications, written in Rust.

This crate can be used as-is or as a base for other higher-level audio engines. (Think of it like [wgpu](https://wgpu.rs/) but for audio).
NOTE: I disagree that this is like wgpu for audio. This is a step higher than wgpu, since it provides DSP infrastructure. This is more akin
Copy link
Owner

Choose a reason for hiding this comment

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

Agreed, I should change that.


// TODO: PLEASE FIX ME:
//
// TODO: investigate CPAL
Copy link
Owner

Choose a reason for hiding this comment

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

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.
Copy link
Owner

@BillyDM BillyDM Feb 19, 2026

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants