fix: forward parameter changes to live stages, eliminate rebuild clicks#217
Merged
fix: forward parameter changes to live stages, eliminate rebuild clicks#217
Conversation
d627948 to
92f110f
Compare
…hain Eliminates audible clicks caused by full AmplifierChain rebuilds on every parameter change. Float params are debounced and forwarded via SetParameter messages. Enum params trigger single-stage replacement. Structural changes (add/remove/reorder) are sent immediately. Full rebuilds reserved for preset switches and oversampling changes.
92f110f to
928e372
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the audio/GUI interaction to avoid full DSP chain rebuilds on every parameter tweak by forwarding parameter updates to live stages, while also moving IR loading and object deallocation off the JACK RT thread.
Changes:
- Add engine messages for live parameter updates and structural chain mutations (add/remove/swap/replace stage) to preserve unaffected stage state.
- Introduce RT-safe “drop handles” (
RtDropHandle,ConvolverDropHandle) and an IR background load service to keep allocation/deallocation and IR prep off the RT callback. - Update GUI stage configs to return
ParamUpdateso float params are forwarded and enum-like changes trigger single-stage rebuilds.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/engine.rs | Updates Engine::new call sites and adds integration test for SetParameter live updates. |
| src/ir/mod.rs | Exposes new ir::load_service module. |
| src/ir/load_service.rs | Adds background IR load/cache service + convolver retirement channel. |
| src/ir/cabinet.rs | Refactors IrCabinet to accept ready-to-swap Convolver instances instead of loading WAVs directly. |
| src/gui/stages/mod.rs | Introduces ParamUpdate and changes StageConfig::apply to return parameter-update intent. |
| src/gui/stages/preamp.rs | Returns ParamUpdate (forward floats; rebuild on clipper change). |
| src/gui/stages/compressor.rs | Returns ParamUpdate for forwarding compressor params. |
| src/gui/stages/tonestack.rs | Returns ParamUpdate (forward floats; rebuild on model change). |
| src/gui/stages/poweramp.rs | Returns ParamUpdate (forward floats; rebuild on amp type change). |
| src/gui/stages/level.rs | Returns ParamUpdate for forwarding gain changes. |
| src/gui/stages/noise_gate.rs | Returns ParamUpdate for forwarding noise gate params. |
| src/gui/stages/multiband_saturator.rs | Returns ParamUpdate for forwarding band params. |
| src/gui/stages/delay.rs | Returns ParamUpdate for forwarding delay params. |
| src/gui/stages/reverb.rs | Returns ParamUpdate for forwarding reverb params. |
| src/gui/stages/eq.rs | Forwards per-band EQ updates via static band-name table. |
| src/gui/handlers/preset.rs | Avoids cloning preset list by returning &[String] and adds lookup by name. |
| src/gui/app.rs | Replaces “dirty chain rebuild” with debounced dirty-param flushing + single-stage rebuilds for enum changes; wires IR preloading/loading requests. |
| src/audio/rt_drop.rs | Adds generic RT-safe disposal channel (handle + receiver). |
| src/audio/mod.rs | Registers new rt_drop module. |
| src/audio/manager.rs | Wires IR loader/service, convolver retirement, and RT drop thread; changes IR cabinet construction. |
| src/audio/engine.rs | Adds new engine messages, drains message queue per callback, and retires old chains/stages/convolvers off RT. |
| src/amp/chain.rs | Adds parameter forwarding and chain mutation helpers + unit tests. |
| docs/superpowers/specs/2026-03-14-bug10-parameter-forwarding-design.md | Design spec for parameter forwarding + RT-safe disposal. |
| docs/superpowers/plans/2026-03-14-bug10-parameter-forwarding.md | Implementation plan documenting the change sequence. |
| benches/engine.rs | Updates benchmark engine construction for new Engine::new signature. |
| benches/common/mod.rs | Updates IR cabinet bench helpers to build/swaps convolvers directly. |
Comments suppressed due to low confidence (3)
src/audio/engine.rs:341
EngineHandle::sendusestry_send, so when the bounded channel fills, messages are silently dropped (only logged). Dropping structural messages (AddStage/RemoveStage/SwapStages/ReplaceStage) orSetAmpChaincan desync GUI vs engine state. Consider makingsendblocking (send) or returning aResultso callers can retry; alternatively keeptry_sendonly for high-rateSetParameterupdates and ensure non-droppable messages cannot be lost.
impl EngineHandle {
pub fn send(&self, message: EngineMessage) {
self.engine_sender.try_send(message).unwrap_or_else(|e| {
error!("Failed to send engine message: {e}");
});
}
src/gui/app.rs:93
- In
boot,if let Some(ir_name) = preset.ir_name { ... }movesir_nameout ofpreset, butpresetis used later (e.g.,preset.name,preset.stages). This will fail to compile unlessir_nameis cloned/borrowed. Usepreset.ir_name.clone()orpreset.ir_name.as_deref()(and clone only when needed) to avoid partially movingpreset.
if let Some(ir_name) = preset.ir_name {
ir_cabinet_control.set_selected_ir(Some(ir_name.clone()));
audio_manager.request_ir_load(&ir_name);
} else if let Some(first_ir) = ir_cabinet_control.get_selected_ir() {
src/audio/engine.rs:250
SwapIrConvolverdrops the receivedPreparedIr(and itsConvolver) on the audio thread whenself.ir_cabinetisNone(theif let Some(ref mut cab)guard just skips the swap). If this message can ever be sent in that configuration, the convolver deallocation defeats the RT-safety goal. Consider explicitly retiring/dropping the convolver viaconvolver_drop(orrt_drop) when there is no cabinet to accept it.
EngineMessage::SwapIrConvolver(prepared) => {
if let Some(ref mut cab) = self.ir_cabinet {
debug!("IR convolver swapped: {}", prepared.name);
let old = cab.swap_convolver(prepared.convolver);
self.convolver_drop.retire(old);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SetParameterengine messages instead of rebuilding the entireAmplifierChainon every knob turn — eliminates audible clicks caused by DSP state loss every 100msAddStage,RemoveStage,SwapStages,ReplaceStage) sent immediately to the engine, preserving all unaffected stages' internal stateRtDropHandlefor generic RT-safe object disposal — old chains and replaced stages are deallocated on a background thread, never on the JACK RT callbackReplaceStage— only the affected stage loses stateTest plan
make test)make lint) under strict clippy flagsengine_set_parameter_updates_live_stageverifies parameter forwarding end-to-endAmplifierChainmutation methods (7 tests) andRtDropHandle(2 tests)