Conversation
Ring-buffer delay with linear interpolation, one-pole smoothing on delay time (50 ms) to prevent clicks, and feedback capped at 1.0. Also generates StageType::ALL from the stage_registry! macro so the add-stage picker stays in sync automatically.
There was a problem hiding this comment.
Pull request overview
This PR implements a ring-buffer delay stage (echo/slapback effect) for the rustortion guitar amp simulator, closing issue #25. It follows the established stage_registry! macro pattern and hooks into the existing GUI, audio engine, and i18n systems with minimal boilerplate.
Changes:
- New
DelayStageDSP struct with ring buffer, one-pole smoothed delay time, and linear interpolation - New
DelayConfig/DelayMessageGUI layer with sliders for delay time (0–2000ms), feedback (0–1), and dry/wet mix (0–100%) StageType::ALLderived from the macro to replace a manually-maintainedSTAGE_TYPESlist in the pick list; i18n keys added to both EN and ZH_CN
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/amp/stages/delay.rs |
Core DSP: ring buffer, fractional delay, one-pole smoothing, parameter get/set, and 6 unit tests |
src/gui/stages/delay.rs |
GUI config/message/view for delay stage with three labeled sliders |
src/gui/stages/mod.rs |
Adds StageType::ALL to the macro and registers the Delay variant |
src/gui/components/control.rs |
Switches pick list from a static array to StageType::ALL |
src/amp/stages/mod.rs |
Declares the new delay module |
src/i18n/mod.rs |
Adds stage_delay, delay_time, feedback, and dry_wet keys to both EN and ZH_CN translations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let read_pos = self.write_pos as f32 - self.delay_samples_smoothed + buf_len as f32; | ||
| let read_idx = read_pos as usize % buf_len; | ||
| let frac = read_pos.fract(); | ||
| let next_idx = (read_idx + 1) % buf_len; | ||
|
|
||
| let delayed = (1.0 - frac).mul_add(self.buffer[read_idx], frac * self.buffer[next_idx]); |
There was a problem hiding this comment.
When delay_ms is 0.0, delay_samples_smoothed converges to 0.0. In process(), the read position becomes:
read_pos = write_pos as f32 - 0.0 + buf_len as f32 = write_pos + buf_len
read_idx = (write_pos + buf_len) % buf_len = write_pos
Since the read happens before the write at self.write_pos, buffer[write_pos] holds the value written buf_len samples ago, not the current input. When mix > 0 and delay_ms = 0, the wet output is therefore a full-buffer-length old signal (initially silence, but after enough audio passes through, it starts reintroducing buf_len-old audio instead of silence or the current sample). This is incorrect behavior for a "zero delay" setting.
A common fix is to clamp delay_samples_smoothed to a minimum of 1.0 sample, or to add a special case for when delay is zero to bypass the buffer read entirely. Alternatively, the read position for a 1-sample delay would be read_pos = write_pos - 1 + buf_len, reading the sample written at the previous write step, which is the correct 1-sample delay.
| // Should not panic with zero delay time | ||
| for _ in 0..100 { | ||
| let _ = delay.process(1.0); |
There was a problem hiding this comment.
The zero_delay_time test only verifies that processing does not panic, but does not assert correctness of the output. With mix = 1.0 and delay_ms = 0.0, the output should ideally be the current input (or silence), but due to the read-position behavior described above, it actually outputs a buf_len-old sample. The test should also assert what the expected output is to catch regressions and to make the intended behavior explicit.
| // Should not panic with zero delay time | |
| for _ in 0..100 { | |
| let _ = delay.process(1.0); | |
| // With zero delay time and mix = 1.0, the current implementation | |
| // reads from a position that, for the first N samples, still | |
| // contains the initial silence in the buffer. Make this explicit | |
| // and assert that the output is (approximately) zero, not just | |
| // that processing does not panic. | |
| for i in 0..100 { | |
| let out = delay.process(1.0); | |
| assert!( | |
| out.abs() < 1e-6, | |
| "Expected silence for zero delay time at sample {}, got {}", | |
| i, | |
| out | |
| ); |
- Fix f32 precision loss: split read position into integer/fractional parts so interpolation works correctly at high sample rates (192kHz with 16x oversampling = 6M+ buffer indices, beyond f32 mantissa) - Cap feedback at 0.95 to prevent unbounded signal growth - Clamp constructor inputs to valid ranges - Clamp delay to minimum 1 sample to avoid reading stale buffer data when delay_ms = 0 - Flush denormals in feedback loop to prevent RT CPU spikes - Add tests: feedback bounded, constructor clamping, zero delay wet, wet-only dry leak, high sample rate interpolation, parameter change mid-processing - Tighten existing test thresholds
Summary
StageType::ALLgenerated fromstage_registry!macro so the add-stage picker stays in sync automaticallyCloses #25
Test plan
make lintpasses (zero warnings, clippy pedantic+nursery)make testpasses (6 new unit tests: dry passthrough, wet impulse, feedback decay, parameter validation, zero-time edge case, get parameters)