Feat/auto bpm detection sync controls#353
Closed
affanraza84 wants to merge 17 commits into
Closed
Conversation
* ci: Use artifacts to pass coverage data between jobs * ci: ignore missing source files during lcov capture
… Delay BPM unit test
… for delay and chorus
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR integrates tap-tempo state and auto-detect BPM functionality into the master tempo UI controls. It adds a TapTempo member to GuiManager, reworks the metronome control layout from 3 columns to 2, and wires the tempo engine implementation into app and test builds with improved dependency resolution for nlohmann_json. ChangesTempo UI Controls and Tap-Tempo Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
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: 6
🧹 Nitpick comments (2)
src/gui/pedal_board_chain.cpp (1)
102-106: ⚡ Quick winExtract magic number to a named constant.
The multiband compressor width multiplier
2.2fis hardcoded in multiple locations (here and inpedal_widget.cppline 39). Extract this to a named constant intheme.hto improve maintainability.♻️ Proposed fix
In
src/gui/theme.h, add:constexpr float PEDAL_WIDTH_MULTIBAND_MULT = 2.2f; // MultiBand Compressor requires wider layoutThen replace all occurrences of
190.0f * 2.2fwithTheme::PEDAL_WIDTH * Theme::PEDAL_WIDTH_MULTIBAND_MULT:if (existing_node.pedal && std::strcmp(existing_node.pedal->name(), "MultiBand Compressor") == 0) { - width = 190.0f * 2.2f; + width = Theme::PEDAL_WIDTH * Theme::PEDAL_WIDTH_MULTIBAND_MULT;Apply similar changes in
pedal_widget.cppline 39 and line 135 of this file.Also applies to: 131-135
🤖 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_board_chain.cpp` around lines 102 - 106, Extract the magic multiplier 2.2f into a named constant and replace hardcoded uses: add a constexpr like PEDAL_WIDTH_MULTIBAND_MULT (e.g., 2.2f) in theme.h (and expose it via Theme::PEDAL_WIDTH_MULTIBAND_MULT if using a Theme class), then update the width calculations that currently use 190.0f * 2.2f to use Theme::PEDAL_WIDTH * Theme::PEDAL_WIDTH_MULTIBAND_MULT (refer to the width assignment in pedal_board_chain.cpp where existing_node.pedal name() is "MultiBand Compressor" and the corresponding uses in pedal_widget.cpp). Ensure all occurrences (including the other spots noted) are replaced so the multiplier is maintained centrally.src/audio/effects/delay.cpp (1)
31-42: ⚡ Quick winExtract subdivision mapping and param indices into shared constants/helper.
Line 31 and Line 91 duplicate the same subdivision mapping and rely on magic indices (
0/4/5). This is easy to drift when parameter layouts evolve.Suggested refactor
+namespace { +constexpr int k_param_time = 0; +constexpr int k_param_sync = 4; +constexpr int k_param_subdivision = 5; + +float subdivision_multiplier(int subdivision) { + switch (subdivision) { + case 1: return 0.5f; // 1/8 + case 2: return 0.25f; // 1/16 + case 3: return 0.75f; // 1/8 dotted + case 4: return 1.0f / 3.0f; // 1/8 triplet + default: return 1.0f; // 1/4 + } +} +} ... - bool sync = params_[4].value >= 0.5f; + const bool sync = params_[k_param_sync].value >= 0.5f; ... - int subdiv = static_cast<int>(params_[5].value); - if (subdiv == 1) mult = 0.5f; - else if (subdiv == 2) mult = 0.25f; - else if (subdiv == 3) mult = 0.75f; - else if (subdiv == 4) mult = 1.0f / 3.0f; + const int subdiv = static_cast<int>(params_[k_param_subdivision].value); + const float mult = subdivision_multiplier(subdiv); ... - params_[0].value = clamp(target_time, params_[0].min_val, params_[0].max_val); + params_[k_param_time].value = clamp(target_time, params_[k_param_time].min_val, params_[k_param_time].max_val);Also applies to: 91-103
🤖 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/delay.cpp` around lines 31 - 42, Duplicate magic parameter indices (0/4/5) and the subdivision->multiplier mapping are used in delay.cpp; extract them into shared constants and a helper to avoid drift. Define named constants/enum for the parameter indices (e.g., PARAM_TIME, PARAM_SYNC, PARAM_SUBDIV) and implement a single helper function (e.g., getSubdivisionMultiplier(int subdiv) or computeTargetTimeFromSubdiv(float bpm,int subdiv)) that encapsulates the mapping (1->0.5, 2->0.25, 3->0.75, 4->1.0/3.0) and clamping logic using clamp/params_ where needed; replace the duplicated blocks (the current usage around params_[0], params_[4], params_[5] and the later block at lines ~91-103) to call the helper and use the named param constants.
🤖 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/audio_engine_api.cpp`:
- Around line 34-38: Add a direct include of <cmath> at the top of the file so
std::round is guaranteed available; update src/audio/audio_engine_api.cpp to
include <cmath> (so the AudioEngine::set_global_bpm(float) implementation that
calls std::round compiles robustly) and remove reliance on indirect includes.
In `@src/audio/audio_engine_process.cpp`:
- Around line 102-106: The audio callback must not call
tempo_engine_.set_sample_rate(...) because that takes a std::mutex and
reallocates memory; remove that call from the callback (where
metronome_sample_rate_ is compared to sample_rate_) and instead set a flag or
update metronome_sample_rate_ only, then perform the actual
tempo_engine_.set_sample_rate(sample_rate_) on the non-RT control path (for
example inside AudioEngine::set_sample_rate(...) or another stream-stopped
control method) so the reconfiguration happens while the stream is stopped and
off the real-time thread.
In `@src/audio/effects/multiband_compressor.cpp`:
- Around line 64-68: The diff shows calculate_coefficients() is being called for
low_high_pass_filter_ and mid_pass_filter_ even though process() only uses
low_pass_filter_.process() and mid_high_pass_filter_.process(); remove the
wasted filters or wire them into the processing path. Either: 1) delete the
unused members low_high_pass_filter_ and mid_pass_filter_ from the class
(header) and remove their calculate_coefficients(...) calls and reset()
invocations in multiband_compressor.cpp, or 2) if a different crossover topology
was intended, update process() to use low_high_pass_filter_.process() and
mid_pass_filter_.process() (and any downstream gain/route logic) and ensure
their coefficients are computed in calculate_coefficients() and they are reset
in reset(); adjust references to low_pass_filter_ and mid_high_pass_filter_
accordingly so only actually-used filters remain.
In `@src/audio/effects/multiband_compressor.h`:
- Around line 14-45: The struct ButterworthLP uses member names b1 and b2 that
violate the trailing-underscore member naming convention; rename the members to
b1_ and b2_ and update all internal usages (in
ButterworthLP::calculate_coefficients, ButterworthLP::process, and
ButterworthLP::reset) to reference b1_ and b2_, and also update any external
references to ButterworthLP's members across the codebase to the new names to
keep the API consistent.
- Around line 51-82: The ButterworthHP struct violates member naming conventions
by using b1 and b2; rename these members to b1_ and b2_ and update all uses
inside ButterworthHP (calculate_coefficients, process, reset) to the new names
so the assignments and method calls (e.g., b1_.b0 = ..., b2_.process(...),
b1_.reset()) compile and follow the trailing-underscore guideline.
In `@src/gui/pedal_widget_knobs.cpp`:
- Around line 352-357: Capture the existing subdivision value before you mutate
params[5].value so the undo system receives the true "old" value: read and store
float old_val = params[5].value before assigning new_val, then set
params[5].value = new_val, call engine_.push_param_change(index_, 5, new_val),
and call commit_param_change(5, old_val, new_val). Apply the same change to the
other occurrence around the 435-440 block so both commit_param_change calls use
the pre-assignment old value.
---
Nitpick comments:
In `@src/audio/effects/delay.cpp`:
- Around line 31-42: Duplicate magic parameter indices (0/4/5) and the
subdivision->multiplier mapping are used in delay.cpp; extract them into shared
constants and a helper to avoid drift. Define named constants/enum for the
parameter indices (e.g., PARAM_TIME, PARAM_SYNC, PARAM_SUBDIV) and implement a
single helper function (e.g., getSubdivisionMultiplier(int subdiv) or
computeTargetTimeFromSubdiv(float bpm,int subdiv)) that encapsulates the mapping
(1->0.5, 2->0.25, 3->0.75, 4->1.0/3.0) and clamping logic using clamp/params_
where needed; replace the duplicated blocks (the current usage around
params_[0], params_[4], params_[5] and the later block at lines ~91-103) to call
the helper and use the named param constants.
In `@src/gui/pedal_board_chain.cpp`:
- Around line 102-106: Extract the magic multiplier 2.2f into a named constant
and replace hardcoded uses: add a constexpr like PEDAL_WIDTH_MULTIBAND_MULT
(e.g., 2.2f) in theme.h (and expose it via Theme::PEDAL_WIDTH_MULTIBAND_MULT if
using a Theme class), then update the width calculations that currently use
190.0f * 2.2f to use Theme::PEDAL_WIDTH * Theme::PEDAL_WIDTH_MULTIBAND_MULT
(refer to the width assignment in pedal_board_chain.cpp where
existing_node.pedal name() is "MultiBand Compressor" and the corresponding uses
in pedal_widget.cpp). Ensure all occurrences (including the other spots noted)
are replaced so the multiplier is maintained centrally.
🪄 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: a570ae03-1dce-4921-98f4-ab6c8bb91975
📒 Files selected for processing (23)
CMakeLists.txtsrc/audio/audio_engine.cppsrc/audio/audio_engine.hsrc/audio/audio_engine_api.cppsrc/audio/audio_engine_process.cppsrc/audio/effects/chorus.cppsrc/audio/effects/delay.cppsrc/audio/effects/multiband_compressor.cppsrc/audio/effects/multiband_compressor.hsrc/audio/tempo_engine.cppsrc/audio/tempo_engine.hsrc/audio/tempo_math.hsrc/gui/gui_manager.hsrc/gui/gui_manager_frame.cppsrc/gui/pedal_board_chain.cppsrc/gui/pedal_board_menu.cppsrc/gui/pedal_widget.cppsrc/gui/pedal_widget.hsrc/gui/pedal_widget_body.cppsrc/gui/pedal_widget_knobs.cppsrc/gui/theme.htests/test_effects.cpptests/test_tempo.cpp
Comment on lines
102
to
106
| const bool sample_rate_changed = (metronome_sample_rate_ != sample_rate_); | ||
| if (sample_rate_changed) { | ||
| metronome_sample_rate_ = sample_rate_; | ||
| tempo_engine_.set_sample_rate(sample_rate_); | ||
| } |
Contributor
There was a problem hiding this comment.
Move tempo-engine sample-rate reconfiguration out of the audio callback.
Line 105 calls tempo_engine_.set_sample_rate(sample_rate_), and that function takes a standard mutex and reallocates the ring buffer. That is a real-time hazard on the callback thread (xruns/dropouts).
Proposed fix
- const bool sample_rate_changed = (metronome_sample_rate_ != sample_rate_);
+ const bool sample_rate_changed = (metronome_sample_rate_ != sample_rate_);
if (sample_rate_changed) {
metronome_sample_rate_ = sample_rate_;
- tempo_engine_.set_sample_rate(sample_rate_);
}// Apply tempo_engine_.set_sample_rate(rate) on the non-RT control path,
// e.g., in AudioEngine::set_sample_rate(...) while the stream is stopped.Based on learnings: "Never allocate memory, perform disk/console I/O, or block on standard mutexes inside the audio callback loop".
🤖 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/audio_engine_process.cpp` around lines 102 - 106, The audio
callback must not call tempo_engine_.set_sample_rate(...) because that takes a
std::mutex and reallocates memory; remove that call from the callback (where
metronome_sample_rate_ is compared to sample_rate_) and instead set a flag or
update metronome_sample_rate_ only, then perform the actual
tempo_engine_.set_sample_rate(sample_rate_) on the non-RT control path (for
example inside AudioEngine::set_sample_rate(...) or another stream-stopped
control method) so the reconfiguration happens while the stream is stopped and
off the real-time thread.
Comment on lines
+64
to
+68
| low_pass_filter_.calculate_coefficients(f_low, sample_rate_); | ||
| low_high_pass_filter_.calculate_coefficients(f_low, sample_rate_); | ||
|
|
||
| mid_pass_filter_.calculate_coefficients(f_high, sample_rate_); | ||
| mid_high_pass_filter_.calculate_coefficients(f_high, sample_rate_); |
Contributor
There was a problem hiding this comment.
Unused crossover filters waste CPU cycles.
The low_high_pass_filter_ (line 65) and mid_pass_filter_ (line 67) coefficients are computed here but never used in the process() method (lines 113-115), which only calls low_pass_filter_.process() and mid_high_pass_filter_.process(). This wastes CPU cycles on coefficient computation for dead filters.
Either remove the unused filters or complete the implementation if they were intended for a different crossover topology.
♻️ Proposed fix to remove dead code
In the header (multiband_compressor.h), remove the unused filter declarations:
// Crossover filters (LR4 crossovers)
ButterworthLP low_pass_filter_;
- ButterworthHP low_high_pass_filter_;
- ButterworthLP mid_pass_filter_;
ButterworthHP mid_high_pass_filter_;In the implementation, remove their coefficient computation:
low_pass_filter_.calculate_coefficients(f_low, sample_rate_);
- low_high_pass_filter_.calculate_coefficients(f_low, sample_rate_);
-
- mid_pass_filter_.calculate_coefficients(f_high, sample_rate_);
mid_high_pass_filter_.calculate_coefficients(f_high, sample_rate_);And remove their reset calls in reset():
void MultiBandCompressor::reset() {
low_pass_filter_.reset();
- low_high_pass_filter_.reset();
- mid_pass_filter_.reset();
mid_high_pass_filter_.reset();🤖 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/multiband_compressor.cpp` around lines 64 - 68, The diff
shows calculate_coefficients() is being called for low_high_pass_filter_ and
mid_pass_filter_ even though process() only uses low_pass_filter_.process() and
mid_high_pass_filter_.process(); remove the wasted filters or wire them into the
processing path. Either: 1) delete the unused members low_high_pass_filter_ and
mid_pass_filter_ from the class (header) and remove their
calculate_coefficients(...) calls and reset() invocations in
multiband_compressor.cpp, or 2) if a different crossover topology was intended,
update process() to use low_high_pass_filter_.process() and
mid_pass_filter_.process() (and any downstream gain/route logic) and ensure
their coefficients are computed in calculate_coefficients() and they are reset
in reset(); adjust references to low_pass_filter_ and mid_high_pass_filter_
accordingly so only actually-used filters remain.
Comment on lines
+14
to
+45
| struct ButterworthLP { | ||
| Biquad b1, b2; | ||
|
|
||
| void calculate_coefficients(float cutoff, float sample_rate) { | ||
| float w0 = TWO_PI * cutoff / sample_rate; | ||
| float cos_w0 = std::cos(w0); | ||
| float sin_w0 = std::sin(w0); | ||
| float alpha = sin_w0 / (2.0f * 0.70710678f); // Q = 1/sqrt(2) | ||
| float a0 = 1.0f + alpha; | ||
|
|
||
| float b0 = (1.0f - cos_w0) / 2.0f; | ||
| float b1_val = 1.0f - cos_w0; | ||
| float b2_val = (1.0f - cos_w0) / 2.0f; | ||
| float a1 = -2.0f * cos_w0; | ||
| float a2 = 1.0f - alpha; | ||
|
|
||
| b1.b0 = b2.b0 = b0 / a0; | ||
| b1.b1 = b2.b1 = b1_val / a0; | ||
| b1.b2 = b2.b2 = b2_val / a0; | ||
| b1.a1 = b2.a1 = a1 / a0; | ||
| b1.a2 = b2.a2 = a2 / a0; | ||
| } | ||
|
|
||
| float process(float x) { | ||
| return b2.process(b1.process(x)); | ||
| } | ||
|
|
||
| void reset() { | ||
| b1.reset(); | ||
| b2.reset(); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Member variable naming convention violation.
The b1 and b2 members should use trailing underscores (b1_, b2_) to comply with the coding guidelines for class member variables.
📝 Proposed fix
struct ButterworthLP {
- Biquad b1, b2;
+ Biquad b1_, b2_;
void calculate_coefficients(float cutoff, float sample_rate) {
// ... coefficient calculation ...
- b1.b0 = b2.b0 = b0 / a0;
- b1.b1 = b2.b1 = b1_val / a0;
- b1.b2 = b2.b2 = b2_val / a0;
- b1.a1 = b2.a1 = a1 / a0;
- b1.a2 = b2.a2 = a2 / a0;
+ b1_.b0 = b2_.b0 = b0 / a0;
+ b1_.b1 = b2_.b1 = b1_val / a0;
+ b1_.b2 = b2_.b2 = b2_val / a0;
+ b1_.a1 = b2_.a1 = a1 / a0;
+ b1_.a2 = b2_.a2 = a2 / a0;
}
float process(float x) {
- return b2.process(b1.process(x));
+ return b2_.process(b1_.process(x));
}
void reset() {
- b1.reset();
- b2.reset();
+ b1_.reset();
+ b2_.reset();
}
};As per coding guidelines: "Class Member Variables should use lowercase snake_case with a trailing underscore (e.g., current_port_, midi_queue_)."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| struct ButterworthLP { | |
| Biquad b1, b2; | |
| void calculate_coefficients(float cutoff, float sample_rate) { | |
| float w0 = TWO_PI * cutoff / sample_rate; | |
| float cos_w0 = std::cos(w0); | |
| float sin_w0 = std::sin(w0); | |
| float alpha = sin_w0 / (2.0f * 0.70710678f); // Q = 1/sqrt(2) | |
| float a0 = 1.0f + alpha; | |
| float b0 = (1.0f - cos_w0) / 2.0f; | |
| float b1_val = 1.0f - cos_w0; | |
| float b2_val = (1.0f - cos_w0) / 2.0f; | |
| float a1 = -2.0f * cos_w0; | |
| float a2 = 1.0f - alpha; | |
| b1.b0 = b2.b0 = b0 / a0; | |
| b1.b1 = b2.b1 = b1_val / a0; | |
| b1.b2 = b2.b2 = b2_val / a0; | |
| b1.a1 = b2.a1 = a1 / a0; | |
| b1.a2 = b2.a2 = a2 / a0; | |
| } | |
| float process(float x) { | |
| return b2.process(b1.process(x)); | |
| } | |
| void reset() { | |
| b1.reset(); | |
| b2.reset(); | |
| } | |
| }; | |
| struct ButterworthLP { | |
| Biquad b1_, b2_; | |
| void calculate_coefficients(float cutoff, float sample_rate) { | |
| float w0 = TWO_PI * cutoff / sample_rate; | |
| float cos_w0 = std::cos(w0); | |
| float sin_w0 = std::sin(w0); | |
| float alpha = sin_w0 / (2.0f * 0.70710678f); // Q = 1/sqrt(2) | |
| float a0 = 1.0f + alpha; | |
| float b0 = (1.0f - cos_w0) / 2.0f; | |
| float b1_val = 1.0f - cos_w0; | |
| float b2_val = (1.0f - cos_w0) / 2.0f; | |
| float a1 = -2.0f * cos_w0; | |
| float a2 = 1.0f - alpha; | |
| b1_.b0 = b2_.b0 = b0 / a0; | |
| b1_.b1 = b2_.b1 = b1_val / a0; | |
| b1_.b2 = b2_.b2 = b2_val / a0; | |
| b1_.a1 = b2_.a1 = a1 / a0; | |
| b1_.a2 = b2_.a2 = a2 / a0; | |
| } | |
| float process(float x) { | |
| return b2_.process(b1_.process(x)); | |
| } | |
| void reset() { | |
| b1_.reset(); | |
| b2_.reset(); | |
| } | |
| }; |
🤖 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/multiband_compressor.h` around lines 14 - 45, The struct
ButterworthLP uses member names b1 and b2 that violate the trailing-underscore
member naming convention; rename the members to b1_ and b2_ and update all
internal usages (in ButterworthLP::calculate_coefficients,
ButterworthLP::process, and ButterworthLP::reset) to reference b1_ and b2_, and
also update any external references to ButterworthLP's members across the
codebase to the new names to keep the API consistent.
Comment on lines
+51
to
+82
| struct ButterworthHP { | ||
| Biquad b1, b2; | ||
|
|
||
| void calculate_coefficients(float cutoff, float sample_rate) { | ||
| float w0 = TWO_PI * cutoff / sample_rate; | ||
| float cos_w0 = std::cos(w0); | ||
| float sin_w0 = std::sin(w0); | ||
| float alpha = sin_w0 / (2.0f * 0.70710678f); // Q = 1/sqrt(2) | ||
| float a0 = 1.0f + alpha; | ||
|
|
||
| float b0 = (1.0f + cos_w0) / 2.0f; | ||
| float b1_val = -(1.0f + cos_w0); | ||
| float b2_val = (1.0f + cos_w0) / 2.0f; | ||
| float a1 = -2.0f * cos_w0; | ||
| float a2 = 1.0f - alpha; | ||
|
|
||
| b1.b0 = b2.b0 = b0 / a0; | ||
| b1.b1 = b2.b1 = b1_val / a0; | ||
| b1.b2 = b2.b2 = b2_val / a0; | ||
| b1.a1 = b2.a1 = a1 / a0; | ||
| b1.a2 = b2.a2 = a2 / a0; | ||
| } | ||
|
|
||
| float process(float x) { | ||
| return b2.process(b1.process(x)); | ||
| } | ||
|
|
||
| void reset() { | ||
| b1.reset(); | ||
| b2.reset(); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Member variable naming convention violation.
The b1 and b2 members should use trailing underscores (b1_, b2_) to comply with the coding guidelines for class member variables (same issue as ButterworthLP).
📝 Proposed fix
struct ButterworthHP {
- Biquad b1, b2;
+ Biquad b1_, b2_;
// Apply same underscore changes to all usages in calculate_coefficients, process, and reset
};As per coding guidelines: "Class Member Variables should use lowercase snake_case with a trailing underscore."
🤖 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/multiband_compressor.h` around lines 51 - 82, The
ButterworthHP struct violates member naming conventions by using b1 and b2;
rename these members to b1_ and b2_ and update all uses inside ButterworthHP
(calculate_coefficients, process, reset) to the new names so the assignments and
method calls (e.g., b1_.b0 = ..., b2_.process(...), b1_.reset()) compile and
follow the trailing-underscore guideline.
Comment on lines
+352
to
+357
| if (ImGui::Combo(subdiv_lbl, &subdiv, subdiv_items, IM_ARRAYSIZE(subdiv_items))) { | ||
| float new_val = static_cast<float>(subdiv); | ||
| params[5].value = new_val; | ||
| engine_.push_param_change(index_, 5, new_val); | ||
| commit_param_change(5, params[5].value, new_val); | ||
| } |
Contributor
There was a problem hiding this comment.
Capture old subdivision values before assignment for undo commits.
At Line 356 and Line 439, commit_param_change gets the post-update value as the “old” value, so subdivision changes are not correctly undoable.
🔧 Suggested fix
- if (ImGui::Combo(subdiv_lbl, &subdiv, subdiv_items, IM_ARRAYSIZE(subdiv_items))) {
- float new_val = static_cast<float>(subdiv);
- params[5].value = new_val;
- engine_.push_param_change(index_, 5, new_val);
- commit_param_change(5, params[5].value, new_val);
- }
+ if (ImGui::Combo(subdiv_lbl, &subdiv, subdiv_items, IM_ARRAYSIZE(subdiv_items))) {
+ float old_val = params[5].value;
+ float new_val = static_cast<float>(subdiv);
+ params[5].value = new_val;
+ engine_.push_param_change(index_, 5, new_val);
+ commit_param_change(5, old_val, new_val);
+ }
...
- if (ImGui::Combo(subdiv_lbl, &subdiv, subdiv_items, IM_ARRAYSIZE(subdiv_items))) {
- float new_val = static_cast<float>(subdiv);
- params[4].value = new_val;
- engine_.push_param_change(index_, 4, new_val);
- commit_param_change(4, params[4].value, new_val);
- }
+ if (ImGui::Combo(subdiv_lbl, &subdiv, subdiv_items, IM_ARRAYSIZE(subdiv_items))) {
+ float old_val = params[4].value;
+ float new_val = static_cast<float>(subdiv);
+ params[4].value = new_val;
+ engine_.push_param_change(index_, 4, new_val);
+ commit_param_change(4, old_val, new_val);
+ }Also applies to: 435-440
🤖 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_knobs.cpp` around lines 352 - 357, Capture the existing
subdivision value before you mutate params[5].value so the undo system receives
the true "old" value: read and store float old_val = params[5].value before
assigning new_val, then set params[5].value = new_val, call
engine_.push_param_change(index_, 5, new_val), and call commit_param_change(5,
old_val, new_val). Apply the same change to the other occurrence around the
435-440 block so both commit_param_change calls use the pre-assignment old
value.
Collaborator
|
@affanraza84 Fix the merge conflicts |
… and resolve conflicts
Contributor
Author
|
@sudip-mondal-2002 Please close this PR, I will raise a fresh one with no conflicts |
Owner
|
@affanraza84 I have fixed the conflicts no worries |
Contributor
Author
|
Thank you @sudip-mondal-2002 . what should I do now? |
Contributor
🌍 Cross-Platform Coverage ReportPlatforms: Linux + macOS + Windows ✅ Coverage meets threshold: 74.6% >= 60% Full Merged Coverage Summary |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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
3 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.
Automatic BPM Detection & Sync Controls for Delay and Chorus
Overview
This PR introduces a complete tempo synchronization system for Amplitron, featuring automatic BPM detection, tap tempo functionality, global tempo management, and BPM-synchronized Delay and Chorus effects.
The implementation enables users to lock time-based effects to a shared global tempo, improving rhythmic consistency and enhancing the overall performance workflow.
Features Added
Automatic BPM Detection
Implemented a real-time tempo detection engine that analyzes incoming audio and estimates BPM automatically.
Highlights
Tap Tempo System
Added a dedicated tap tempo mechanism for manual tempo entry.
Highlights
Global Tempo Engine
Introduced a centralized tempo management architecture.
Additions
Changes Made
Core Tempo Logic
New Files
tempo_math.hImplemented
TapTempohelper class for:tempo_engine.htempo_engine.cppImplemented
TempoEnginefeaturing:Audio Engine Integration
Modified Files
audio_engine.haudio_engine.cppaudio_engine_process.cppChanges
global_bpm_TempoEngine tempo_engine_Delay Pedal Synchronization
Modified Files
delay.cppdelay.hNew Parameters
Behavior
When Sync is enabled:
Supported divisions include:
Chorus Pedal Synchronization
Modified Files
chorus.cppchorus.hNew Parameters
Behavior
When Sync is enabled:
User Interface Enhancements
Modified File
gui_manager_frame.cppImprovements
Modified File
pedal_widget_knobs.cppImprovements
Custom controls added for Delay and Chorus:
UX Enhancements
Testing
Unit Tests Added
tap_tempo_basicVerifies BPM calculation using consistent tap intervals.
tap_tempo_outliersVerifies trimmed mean outlier rejection.
tap_tempo_inactivity_resetVerifies tap history reset after 4 seconds of inactivity.
tempo_engine_detectionVerifies autocorrelation beat detection using simulated pulse trains.
delay_sync_parameterVerifies delay time synchronization with BPM and subdivisions.
chorus_sync_parameterVerifies chorus modulation synchronization with BPM and subdivisions.
Validation Results
Automated Testing
Benefits
closes issue #142
Summary by CodeRabbit
New Features
UI Improvements