feat: add looper effect with UI integration#116
Merged
sudip-mondal-2002 merged 8 commits intoMay 18, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a Looper pedal: header + implementation with preallocated loop buffers and atomic mailbox, CMake registration, unit tests, ImGui menu and custom pedal UI with transport controls and loop-level slider, and theme/chain layout tweaks. ChangesLooper Pedal Effect and UI Integration
Sequence Diagram(s)sequenceDiagram
participant User as User/ImGui
participant UIReq as Looper Request API
participant Mailbox as Atomic Mailbox
participant Audio as Audio Thread Callback
participant Buffers as Loop Buffers
User->>UIReq: click Record/Play/Overdub/Clear buttons
UIReq->>Mailbox: pending_commands_ |= COMMAND_BIT
Audio->>Mailbox: atomically exchange pending_commands_
Audio->>Audio: apply_pending_commands()
alt Recording
Audio->>Buffers: write input samples
else Playing
Audio->>Buffers: read loop samples and mix
Audio->>Audio: apply crossfade at wrap
else Overdubbing
Audio->>Buffers: accumulate soft-clipped input into buffer
end
Audio->>User: publish_snapshot(state, has_loop, loop_len, playhead)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/gui/pedal_widget_body.cpp (1)
382-387: ⚡ Quick winCoalesce Loop Level undo entries to one command per drag.
commit_param_changeis currently called on every slider tick, which will flood undo history during a single drag. Capture the value on activation and commit once on deactivation (matching your knob/popup coalescing behavior).Proposed fix
- float old_val = level; - if (ImGui::SliderFloat(slider_id, &level, 0.0f, 1.0f, "Loop Level: %.2f")) { - level = clamp(level, 0.0f, 1.0f); - engine_.push_param_change(index_, 0, level); - commit_param_change(0, old_val, level); - } + if (ImGui::IsItemActivated()) { + popup_active_param_index_ = 0; + popup_param_value_before_edit_ = level; + } + if (ImGui::SliderFloat(slider_id, &level, 0.0f, 1.0f, "Loop Level: %.2f")) { + level = clamp(level, 0.0f, 1.0f); + engine_.push_param_change(index_, 0, level); + } + if (ImGui::IsItemDeactivatedAfterEdit() && popup_active_param_index_ == 0) { + commit_param_change(0, popup_param_value_before_edit_, level); + popup_active_param_index_ = -1; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gui/pedal_widget_body.cpp` around lines 382 - 387, The slider currently calls commit_param_change every tick; change it to record the starting value when the slider becomes active and only call commit_param_change once when the user releases the slider. Specifically, on the SliderFloat interaction for slider_id, keep using engine_.push_param_change(index_, 0, level) per-tick, but capture old_val at activation (use ImGui::IsItemActivated/IsItemActive with slider_id or an instance member like saved_old_level and dragging flag) and defer commit_param_change(0, saved_old_level, level) until deactivation (use ImGui::IsItemDeactivated/IsItemDeactivatedAfterChange or check IsItemActive transition); ensure you reset the saved_old_level/dragging state after committing so subsequent drags behave the same.src/audio/effects/looper.cpp (1)
102-109: 💤 Low valueUnreachable branch:
State::Emptycheck is dead code.The early return at line 103 (
if (!has_loop_rt_) return;) means line 106'sState::Emptycondition can never be true when reached, sinceEmptyimplies no loop exists. This branch is unreachable.Consider simplifying:
Suggested cleanup
void Looper::toggle_play_rt() { if (!has_loop_rt_) return; if (state_rt_ == State::Playing || state_rt_ == State::Overdubbing) { state_rt_ = State::Idle; - } else if (state_rt_ == State::Idle || state_rt_ == State::Empty) { + } else if (state_rt_ == State::Idle) { state_rt_ = State::Playing; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/effects/looper.cpp` around lines 102 - 109, The branch in Looper::toggle_play_rt is unreachable because has_loop_rt_ being false causes an early return, so checking State::Empty in the later condition is dead code; update toggle_play_rt to remove State::Empty from the else-if (i.e., change "else if (state_rt_ == State::Idle || state_rt_ == State::Empty)" to just check State::Idle) or, if intended, reorder or adjust the has_loop_rt_ guard so State::Empty can be reached—ensure you modify only Looper::toggle_play_rt and keep references to has_loop_rt_ and state_rt_ consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/audio/effects/looper.cpp`:
- Line 167: The code reads loop_level directly from params_[0].value in
process_core, causing zipper noise; add a one-pole smoothing state (e.g., float
loop_level_smooth_ = 0.80f in the class) and update it each sample inside
process_core by applying a one-pole low-pass toward params_[0].value
(loop_level_smooth_ += coef*(target - loop_level_smooth_)) then use
loop_level_smooth_ wherever loop_level is used; choose a small smoothing
coefficient (derived from desired time constant and sample rate) and ensure the
state is initialized in the header/class so smoothing persists across calls.
---
Nitpick comments:
In `@src/audio/effects/looper.cpp`:
- Around line 102-109: The branch in Looper::toggle_play_rt is unreachable
because has_loop_rt_ being false causes an early return, so checking
State::Empty in the later condition is dead code; update toggle_play_rt to
remove State::Empty from the else-if (i.e., change "else if (state_rt_ ==
State::Idle || state_rt_ == State::Empty)" to just check State::Idle) or, if
intended, reorder or adjust the has_loop_rt_ guard so State::Empty can be
reached—ensure you modify only Looper::toggle_play_rt and keep references to
has_loop_rt_ and state_rt_ consistent.
In `@src/gui/pedal_widget_body.cpp`:
- Around line 382-387: The slider currently calls commit_param_change every
tick; change it to record the starting value when the slider becomes active and
only call commit_param_change once when the user releases the slider.
Specifically, on the SliderFloat interaction for slider_id, keep using
engine_.push_param_change(index_, 0, level) per-tick, but capture old_val at
activation (use ImGui::IsItemActivated/IsItemActive with slider_id or an
instance member like saved_old_level and dragging flag) and defer
commit_param_change(0, saved_old_level, level) until deactivation (use
ImGui::IsItemDeactivated/IsItemDeactivatedAfterChange or check IsItemActive
transition); ensure you reset the saved_old_level/dragging state after
committing so subsequent drags behave the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d311d2a-2b05-4eed-80e6-7a1618acf43e
📒 Files selected for processing (8)
CMakeLists.txtsrc/audio/effects/looper.cppsrc/audio/effects/looper.hsrc/gui/pedal_board_menu.cppsrc/gui/pedal_widget.cppsrc/gui/pedal_widget.hsrc/gui/pedal_widget_body.cppsrc/gui/theme.h
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
sudip-mondal-2002
requested changes
May 18, 2026
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
Owner
There was a problem hiding this comment.
Hi please add test coverage for this pedal
Owner
|
Hi @ionfwsrijan the UI is very inconsistent, can you please fix it? Use AI if needed, you can use antigravity for free 
|
sudip-mondal-2002
added
type:feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/audio/effects/looper.cpp`:
- Around line 66-69: Add a one-pole smoothing state for the crossfade parameter
and use it everywhere the raw params_[1].value is converted to samples (notably
in Looper::crossfade_samples_rt()); replace direct reads of params_[1].value
with the smoothed value when computing ms and xf, update the smoother each audio
block with an appropriate attack/release or coefficient, clamp as before, and
apply the same change to the other locations that read params_[1].value for
sample conversion (the occurrences referenced around the other usage) so all
crossfade conversions use the one-pole filtered parameter.
- Around line 179-189: When recording hits capacity mid-buffer (variables:
record_pos_, cap) and stop_recording_rt_and_play_rt() is called, the remainder
of the current audio callback can still be processed in the playback branch,
causing already-recorded samples to be processed twice; modify the sample loop
in State::Recording (the block using buffer_l_, buffer_r_, left/right,
num_samples, stereo) so that after calling stop_recording_rt_and_play_rt() you
immediately exit the processing callback (e.g., return from the function) or
otherwise ensure no further sample processing occurs in this callback, rather
than just breaking the inner loop—this prevents reprocessing the same samples in
the same callback when playback starts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6654be43-3620-4627-a471-fb99ed57204f
📒 Files selected for processing (4)
CMakeLists.txtsrc/audio/effects/looper.cppsrc/audio/effects/looper.hsrc/gui/pedal_board_menu.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- CMakeLists.txt
- src/audio/effects/looper.h
Comment on lines
+66
to
+69
| int Looper::crossfade_samples_rt() const { | ||
| const float ms = params_[1].value; | ||
| const int xf = static_cast<int>(std::round((ms / 1000.0f) * static_cast<float>(sample_rate_))); | ||
| return std::clamp(xf, 0, std::max(loop_length_ / 2, 0)); |
Contributor
There was a problem hiding this comment.
Smooth the Crossfade parameter before converting it to samples.
params_[1].value is used directly, so abrupt UI changes can still produce boundary artifacts. Add a one-pole smoothed crossfade state and use that in crossfade_samples_rt().
Proposed fix
int Looper::crossfade_samples_rt() const {
- const float ms = params_[1].value;
+ const float ms = crossfade_ms_smoothed_;
const int xf = static_cast<int>(std::round((ms / 1000.0f) * static_cast<float>(sample_rate_)));
return std::clamp(xf, 0, std::max(loop_length_ / 2, 0));
}
void Looper::process_core(float* left, float* right, int num_samples, bool stereo) {
apply_pending_commands();
const float loop_level_target = clamp(params_[0].value, 0.0f, 1.0f);
+ const float crossfade_target_ms = clamp(params_[1].value, 0.0f, 20.0f);
+ crossfade_ms_smoothed_ += crossfade_alpha_ * (crossfade_target_ms - crossfade_ms_smoothed_);
const int cap = max_samples_;As per coding guidelines: "DSP Agents must utilize one-pole filters internally on parameter inputs to prevent audible 'zipper' noise or clicking when parameters change abruptly".
Also applies to: 193-194
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/audio/effects/looper.cpp` around lines 66 - 69, Add a one-pole smoothing
state for the crossfade parameter and use it everywhere the raw params_[1].value
is converted to samples (notably in Looper::crossfade_samples_rt()); replace
direct reads of params_[1].value with the smoothed value when computing ms and
xf, update the smoother each audio block with an appropriate attack/release or
coefficient, clamp as before, and apply the same change to the other locations
that read params_[1].value for sample conversion (the occurrences referenced
around the other usage) so all crossfade conversions use the one-pole filtered
parameter.
Comment on lines
+179
to
+189
| if (state_rt_ == State::Recording) { | ||
| for (int i = 0; i < num_samples; ++i) { | ||
| if (record_pos_ >= cap) { | ||
| stop_recording_rt_and_play_rt(); | ||
| break; | ||
| } | ||
| buffer_l_[record_pos_] = left[i]; | ||
| if (stereo && right) buffer_r_[record_pos_] = right[i]; | ||
| ++record_pos_; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid reprocessing already-recorded samples when recording auto-stops at capacity.
If record_pos_ hits cap mid-buffer, recording breaks and playback then starts from sample index 0, so earlier samples in the same callback are processed twice.
Proposed fix
void Looper::process_core(float* left, float* right, int num_samples, bool stereo) {
apply_pending_commands();
+ int process_from = 0;
@@
if (state_rt_ == State::Recording) {
for (int i = 0; i < num_samples; ++i) {
if (record_pos_ >= cap) {
stop_recording_rt_and_play_rt();
+ process_from = i; // first unprocessed sample in this callback
break;
}
buffer_l_[record_pos_] = left[i];
if (stereo && right) buffer_r_[record_pos_] = right[i];
++record_pos_;
+ process_from = i + 1;
}
}
@@
- for (int i = 0; i < num_samples; ++i) {
+ for (int i = process_from; i < num_samples; ++i) {
...
}Also applies to: 191-194
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/audio/effects/looper.cpp` around lines 179 - 189, When recording hits
capacity mid-buffer (variables: record_pos_, cap) and
stop_recording_rt_and_play_rt() is called, the remainder of the current audio
callback can still be processed in the playback branch, causing already-recorded
samples to be processed twice; modify the sample loop in State::Recording (the
block using buffer_l_, buffer_r_, left/right, num_samples, stereo) so that after
calling stop_recording_rt_and_play_rt() you immediately exit the processing
callback (e.g., return from the function) or otherwise ensure no further sample
processing occurs in this callback, rather than just breaking the inner
loop—this prevents reprocessing the same samples in the same callback when
playback starts.
Contributor
Author
|
@sudip-mondal-2002 Addressed the requested changes:
Thanks for the review — please take another look. |
Owner
|
One small minor issue @ionfwsrijan |
Contributor
Author
|
@sudip-mondal-2002 Fixed it. |
sudip-mondal-2002
added
type:performance
This was referenced May 19, 2026
This was referenced May 28, 2026
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
2 participants
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.
What does this PR do?
Adds a new Looper pedal to Amplitron’s effect chain, including DSP + GUI controls.
Loopereffect with a small state machine (Empty / Stop / Rec / Play / Dub).Related Issue
Fixes #93
Type of Change
How Was This Tested?
./amplitron-tests(not run in this environment; CMake wasn’t available in PATH here)+ Add Pedal→Looper(in the TIME section).Record, play some audio, clickRecordagain to finish and auto-start playback.Overdubto layer additional audio, then toggle it off.Play/Stopto pause/resume playback without clearing the loop.Clearto remove the loop from memory.Checklist
./amplitron-tests)std::mutex::lock()on the hot path)Summary by CodeRabbit
New Features
UI
Theme
Tests
Chores