Add IR Cabinets#53
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds impulse response (IR) cabinet simulation functionality to the amplifier simulation system.
- Implements a new
IrCabinetmodule with zero-latency head processing and partitioned FFT tail convolution - Integrates IR cabinet into the audio processing pipeline with bypass and gain controls
- Adds GUI components for IR selection, bypass toggle, and gain adjustment
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sim/mod.rs | Exposes the new ir_cabinet module |
| src/sim/ir_cabinet.rs | Core IR cabinet implementation with real-time convolution processing |
| src/io/processor.rs | Integrates IR cabinet into audio processing pipeline |
| src/io/manager.rs | Adds IR cabinet control methods for external communication |
| src/gui/messages.rs | Defines IR cabinet GUI message types |
| src/gui/components/mod.rs | Exposes new IR cabinet control component |
| src/gui/components/ir_cabinet_control.rs | GUI component for IR cabinet controls |
| src/gui/app.rs | Integrates IR cabinet controls into main application |
| presets/*.json | Updates preset gain levels |
| Cargo.toml | Adds FFT dependencies for IR processing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| use hound::WavReader; | ||
| use log::{debug, info, warn}; | ||
| use realfft::{ComplexToReal, RealFftPlanner, RealToComplex}; | ||
| use rustfft::num_complex::Complex; // or `use num_complex::Complex;` |
There was a problem hiding this comment.
The comment '// or use num_complex::Complex;' should be removed as it's unclear and potentially confusing. The actual import being used should be clear without alternatives mentioned in comments.
| use rustfft::num_complex::Complex; // or `use num_complex::Complex;` | |
| use rustfft::num_complex::Complex; |
| { | ||
| let g = 0.9 / max; | ||
| for s in &mut truncated { | ||
| *s *= g; |
There was a problem hiding this comment.
[nitpick] The let-else pattern with && condition creates unclear control flow. Consider restructuring this into a more readable if-let pattern or separate the condition check.
| *s *= g; | |
| }) { | |
| if max > 0.0 { | |
| let g = 0.9 / max; | |
| for s in &mut truncated { | |
| *s *= g; | |
| } |
| // Real FFT | ||
| self.r2c | ||
| .process(&mut time_block, &mut freq_block) | ||
| .expect("realfft forward failed"); |
There was a problem hiding this comment.
The error message should be more descriptive and include context about what operation was being performed when the FFT failed.
| .expect("realfft forward failed"); | |
| .with_context(|| format!( | |
| "realfft forward failed in partition_ir_tail: partition {}, samples {}..{}, tail_samples.len()={}", | |
| p, start, end, tail_samples.len() | |
| ))?; |
| &mut self.freq_scratch, | ||
| &mut self.r2c_scratch, | ||
| ) | ||
| .expect("realfft forward failed"); |
There was a problem hiding this comment.
The error message should be more descriptive and include context about what operation was being performed when the FFT failed.
| .expect("realfft forward failed"); | |
| .with_context(|| format!( | |
| "realfft forward failed in process_tail_partition: in_base={}, FFT_BLOCK_SIZE={}, time_scratch[0]={}, freq_scratch.len()={}", | |
| self.in_base, | |
| FFT_BLOCK_SIZE, | |
| self.time_scratch.get(0).copied().unwrap_or_default(), | |
| self.freq_scratch.len() | |
| ))?; |
| &mut self.time_scratch, | ||
| &mut self.c2r_scratch, | ||
| ) | ||
| .expect("realfft inverse failed"); |
There was a problem hiding this comment.
The error message should be more descriptive and include context about what operation was being performed when the FFT failed.
| .expect("realfft inverse failed"); | |
| .expect(&format!( | |
| "realfft inverse failed: hist_head={}, history_len={}, tail_partitions_len={}", | |
| self.hist_head, | |
| self.history.len(), | |
| ir.tail_partitions.len() | |
| )); |
| if let Some(ref mut cab) = self.ir_cabinet | ||
| && let Some(name) = ir_name | ||
| { | ||
| if let Err(e) = cab.set_ir_by_name(&name) { |
There was a problem hiding this comment.
[nitpick] The nested if-let pattern creates deep indentation and unclear control flow. Consider using pattern matching or early returns to flatten the structure.
| if let Err(e) = cab.set_ir_by_name(&name) { | |
| if let (Some(ref mut cab), Some(name)) = (&mut self.ir_cabinet, ir_name.as_ref()) { | |
| if let Err(e) = cab.set_ir_by_name(name) { |
Closes #21