feat: implement N-input dynamic mixer with per-channel gain routing#277
Merged
MohitBareja16 merged 10 commits intoMay 30, 2026
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 48 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds dynamic 2–8 input mixer support with per-input gains: graph stores per-pin gains and exposes pin add/remove/restore and gain APIs; executor compiles/apply weighted sums and supports runtime gain updates; engine enqueues SetMixerGain commands; GUI exposes pin management, gain sliders (MIDI-mappable), and integration tests validate behavior. ChangesN-Input Mixer Node Implementation
Sequence Diagram(s)sequenceDiagram
participant GUI as PedalBoard UI
participant Engine as AudioEngine
participant Executor as AudioGraphExecutor
GUI->>Engine: push_mixer_gain_change(node_id,pin_index,gain)
Engine->>Executor: drain_gain_commands() forwards SetMixerGain -> update_mixer_gain(node_id,pin_index,gain)
Executor->>Executor: update execution_plan_.input_sources[].gain
Executor->>Executor: process() applies gain * input_buffer (unity fast-path when gain==1.0)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
github-actions
Bot
added
Audio Processing
Contributor
🌍 Cross-Platform Coverage ReportPlatforms: Linux + macOS + Windows ✅ Coverage meets threshold: 78.0% >= 60% Full Merged Coverage Summary |
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/audio/engine/audio_graph.cpp (1)
17-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp mixer input count at creation time.
Line 18 currently accepts any positive
num_inputs, which allows creating mixers outside the 2–8 contract.Proposed fix
- int inputs_to_create = (num_inputs > 0) ? num_inputs : 2; + const int requested_inputs = (num_inputs > 0) ? num_inputs : 2; + const int inputs_to_create = std::clamp(requested_inputs, 2, 8);🤖 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/engine/audio_graph.cpp` around lines 17 - 23, The mixer/merge-sum creation uses num_inputs unchecked, allowing values outside the 2–8 contract; change the calculation of inputs_to_create in the NodeRoutingType::Mixer and NodeRoutingType::MergeSum branch to clamp num_inputs into the range [2,8] (e.g. use std::clamp or manual min/max) before allocating input_pin_ids, input_gains and assigning output_pin_ids; ensure the rest of the block (node.input_pin_ids.push_back(next_id_++), node.input_gains.assign(...), node.output_pin_ids.push_back(...)) uses this clamped inputs_to_create value.
🧹 Nitpick comments (2)
src/gui/pedalboard/pedal_board_chain.cpp (2)
343-347: 💤 Low valueEmpty
ifblocks can be simplified.The return values from
render_remove_mapping_itemandrender_learn_menu_itemare checked but not used. Either remove the conditionals or add a comment if the return values will be used later.♻️ Suggested simplification
if (gui_midi_) { - if (gui_midi_->render_remove_mapping_item(effect_name, - param_name)) { - } - if (gui_midi_->render_learn_menu_item(effect_name, param_name)) { - } + gui_midi_->render_remove_mapping_item(effect_name, param_name); + gui_midi_->render_learn_menu_item(effect_name, param_name);🤖 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/pedalboard/pedal_board_chain.cpp` around lines 343 - 347, The two empty conditionals around gui_midi_->render_remove_mapping_item(effect_name, param_name) and gui_midi_->render_learn_menu_item(effect_name, param_name) should be simplified: remove the surrounding if(...) { } wrappers and call the methods directly (or if the boolean return is intentionally significant, replace the empty bodies with a brief comment like // return value intentionally ignored). Update the calls to gui_midi_->render_remove_mapping_item(effect_name, param_name) and gui_midi_->render_learn_menu_item(effect_name, param_name) accordingly to eliminate the empty if blocks.
546-547: ⚡ Quick winRemove debug printf statement.
This debug output will spam the console during normal wire drag-and-drop operations and should be removed before merging.
🧹 Remove debug code
if (dist_sq < pow(15.0f * ui_state.zoom, 2) && ImGui::IsMouseReleased(ImGuiMouseButton_Left)) { - printf("MANUAL DROP HOVER DETECTED! src: %d, dest: %d\n", - ui_state.active_src_pin_id, pin_id); if (ui_state.active_src_pin_id != -1) { history_.execute(std::make_unique<AddGraphLinkCommand>( engine_, ui_state.active_src_pin_id, pin_id));🤖 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/pedalboard/pedal_board_chain.cpp` around lines 546 - 547, Remove the debug printf that prints "MANUAL DROP HOVER DETECTED!" — delete the printf call that references ui_state.active_src_pin_id and pin_id in pedal_board_chain.cpp (the manual drop hover detection block) so it no longer spams the console; if you want to keep conditional logging, replace it with a proper debug/log macro (e.g., LOG_DEBUG) guarded by the project's debug build flag instead of leaving the raw printf.
🤖 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/engine/audio_engine_process.cpp`:
- Around line 294-299: The SetMixerGain command is currently processed inside
drain_commands() while holding effect_mutex_ (via effect_mutex_.try_lock()),
causing mixer updates to stall under contention; move handling of
AudioCommand::SetMixerGain out of the mutex-gated switch and into the lock-free
drain/command-processing path (the same place non-blocking commands are handled)
so calls to audio_shadow_executor_->update_mixer_gain(cmd.effect_index,
cmd.param_index, cmd.value) execute without acquiring effect_mutex_; update the
switch/dispatch in drain_commands() to skip SetMixerGain and add equivalent
handling in the lock-free/try-lock-failed path (or the drain_lock_free_*
routine) and ensure no other state requiring effect_mutex_ is touched when
processing this command.
In `@src/audio/engine/audio_graph_executor.cpp`:
- Around line 175-185: The update_mixer_gain implementation currently updates
any matching node and accepts raw gain; change
AudioGraphExecutor::update_mixer_gain to first verify the matched
execution_plan_ step is a mixer target (e.g., check a mixer marker such as
step.node_type == NodeType::Mixer or step.is_mixer) and only then iterate
step.input_sources, matching src.pin_index; when assigning src.gain, clamp the
incoming gain to the allowed range 0.0f–2.0f (e.g., std::clamp(gain, 0.0f,
2.0f)) and then break after handling the mixer step. Ensure you reference
step.node_id, step.input_sources, src.pin_index, and src.gain in the updated
logic.
In `@src/audio/engine/audio_graph.cpp`:
- Around line 361-374: In AudioGraph::remove_input_pin, protect against
input_gains/pop_back UB by verifying and reconciling sizes before calling
node.input_gains.pop_back(): check node.input_gains.size() is at least
node.input_pin_ids.size() (or non-empty) and if it is smaller, resize
node.input_gains to match node.input_pin_ids (filling with a sensible default
gain) or skip popping so sizes remain consistent; update the logic around
node.input_pin_ids.pop_back() / node.input_gains.pop_back() to ensure both
containers stay synchronized (references: AudioGraph::remove_input_pin,
node.input_pin_ids, node.input_gains, links_).
In `@src/gui/pedalboard/pedal_board_chain.cpp`:
- Around line 693-694: The draft wire control points use fixed offsets (ImVec2
cp1/ cp2) which don't respect zoom; modify the offsets from 45.0f to scale by
the current zoom (use ui_state.zoom) so cp1 and cp2 compute as p1.x + 45.0f *
ui_state.zoom and mouse_pos.x - 45.0f * ui_state.zoom (same pattern used by the
completed cable code around the completed-cable drawing logic) to keep draft
spline visuals consistent at different zoom levels.
In `@src/midi/midi_manager_mapping.cpp`:
- Around line 240-246: The code in midi_manager_mapping.cpp currently parses
mapping.param_name.substr(5) without verifying the parameter name prefix, which
can mis-assign mixer inputs; update the block that checks mapping.effect_name
("Mixer_") and node_id to first validate that mapping.param_name begins with the
expected "Gain " prefix (e.g., mapping.param_name.find("Gain ") == 0 or
starts_with("Gain ")), only then attempt to stoi the substring to produce
pin_idx; keep the existing try/catch around stoi and only proceed to the mixer
update when both node_id and the validated pin_idx are valid.
In `@tests/integration/test_audio_graph.cpp`:
- Around line 1012-1053: Add assertions to test gain clamping and lock-free
runtime updates: after setting initial gains with
graph.set_mixer_input_gain(mixer, ..) include calls that set out-of-range values
(e.g., -0.5f and 3.0f) and then rebuild/compile and check the effective summed
output via executor.process matches the clamped values (0.0 and 2.0
respectively). Then test dynamic updates by calling
executor.update_mixer_gain(mixer, index, newGain) after executor.compile(graph)
and before executor.process(...) and assert the processed output reflects the
new gain (e.g., 0.25 + 1.0 + 2.0). Use the existing input_audio/output_audio and
ASSERT_TRUE with std::abs(...) < 0.001f to validate results.
- Around line 987-1010: Extend the audio_graph_mixer_n_inputs test to cover the
pin-management constraints: after creating the Mixer node
(audio_graph_mixer_n_inputs / AudioGraph graph / node->input_pin_ids) add a
Source node via add_node and connect its output to the Mixer’s last input pin
using add_link (use graph.get_nodes() to find the Source output_pin_ids), then
assert that graph.remove_input_pin(mixer) returns false when that pin has an
active connection; next, assert the maximum limit by calling
graph.add_input_pin(mixer) up to 8 inputs and then assert the 9th add_input_pin
returns false; finally remove pins down to 2 and assert that removing below the
minimum (remove_input_pin) returns false, using node->input_pin_ids size checks
around each operation.
---
Outside diff comments:
In `@src/audio/engine/audio_graph.cpp`:
- Around line 17-23: The mixer/merge-sum creation uses num_inputs unchecked,
allowing values outside the 2–8 contract; change the calculation of
inputs_to_create in the NodeRoutingType::Mixer and NodeRoutingType::MergeSum
branch to clamp num_inputs into the range [2,8] (e.g. use std::clamp or manual
min/max) before allocating input_pin_ids, input_gains and assigning
output_pin_ids; ensure the rest of the block
(node.input_pin_ids.push_back(next_id_++), node.input_gains.assign(...),
node.output_pin_ids.push_back(...)) uses this clamped inputs_to_create value.
---
Nitpick comments:
In `@src/gui/pedalboard/pedal_board_chain.cpp`:
- Around line 343-347: The two empty conditionals around
gui_midi_->render_remove_mapping_item(effect_name, param_name) and
gui_midi_->render_learn_menu_item(effect_name, param_name) should be simplified:
remove the surrounding if(...) { } wrappers and call the methods directly (or if
the boolean return is intentionally significant, replace the empty bodies with a
brief comment like // return value intentionally ignored). Update the calls to
gui_midi_->render_remove_mapping_item(effect_name, param_name) and
gui_midi_->render_learn_menu_item(effect_name, param_name) accordingly to
eliminate the empty if blocks.
- Around line 546-547: Remove the debug printf that prints "MANUAL DROP HOVER
DETECTED!" — delete the printf call that references ui_state.active_src_pin_id
and pin_id in pedal_board_chain.cpp (the manual drop hover detection block) so
it no longer spams the console; if you want to keep conditional logging, replace
it with a proper debug/log macro (e.g., LOG_DEBUG) guarded by the project's
debug build flag instead of leaving the raw printf.
🪄 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: 0840b6a8-6050-4eb5-bf5c-f8094faae077
📒 Files selected for processing (15)
src/audio/engine/audio_engine.hsrc/audio/engine/audio_engine_api.cppsrc/audio/engine/audio_engine_process.cppsrc/audio/engine/audio_graph.cppsrc/audio/engine/audio_graph.hsrc/audio/engine/audio_graph_executor.cppsrc/audio/engine/audio_graph_executor.hsrc/audio/utils/spsc_queue.hsrc/gui/commands/command_graph.hsrc/gui/pedalboard/pedal_board_chain.cppsrc/gui/pedalboard/pedal_board_menu.cppsrc/midi/midi_manager_mapping.cpptests/integration/test_audio_graph.cpptests/integration/test_clipboard_preset.cpptests/integration/test_json_serialization.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/engine/audio_graph.cpp`:
- Around line 395-409: The restore_input_pin function risks signed/unsigned and
out-of-bounds issues: change the index handling to compare a non-negative index
safely against node.input_pin_ids.size() (cast to size_t or use a local size_t
idx = static_cast<size_t>(index) after checking index >= 0), and before
inserting into node.input_gains ensure node.input_gains.size() >=
node.input_pin_ids.size() (or at least >= idx) by resizing or appending default
gain values so that inserting at input_gains.begin() + idx is safe; keep the
existing fallback push_back logic and the next_id_ update (next_id_ = pin_id +
1) unchanged.
In `@src/gui/commands/command_graph.h`:
- Around line 99-104: The loop that finds the matching input pin (iterating
n->input_pin_ids) reads n->input_gains[i] without ensuring i <
n->input_gains.size(), which can cause undefined behavior; update the code in
the block that sets idx and gain (the variables idx, gain and the loop over
n->input_pin_ids comparing to link.dest_pin_id) to check bounds before accessing
input_gains (e.g., only assign gain = n->input_gains[i] if i <
n->input_gains.size(), otherwise use a safe default like 1.0f or skip that entry
and continue), so the code never reads out-of-range from n->input_gains.
- Around line 236-241: The loop that finds the input pin index reads
node->input_gains[i] without bounds-checking, which can be out-of-range; update
the block that sets auto_removed_pin_index and auto_removed_pin_gain (the loop
iterating node->input_pin_ids and the variables
auto_removed_pin_index/auto_removed_pin_gain) to guard access into
node->input_gains by checking i < node->input_gains.size() before reading, and
if the gain is missing set a safe default (or leave auto_removed_pin_gain
unchanged) so you mirror the same safety fix used in RemoveGraphNodeCommand.
🪄 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: 779cbde6-0f2a-4076-9237-1dccc9bc0510
📒 Files selected for processing (5)
src/audio/engine/audio_graph.cppsrc/audio/engine/audio_graph.hsrc/gui/commands/command_graph.hsrc/gui/pedalboard/pedal_board_chain.cppsrc/gui/pedalboard/pedal_board_menu.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/audio/engine/audio_graph.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/audio/engine/audio_engine_process.cpp (1)
246-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
SetMixerGaincan be silently dropped when it is not at queue head.If a non-gain command is at the front,
drain_gain_commands()stops early. Later,drain_commands()popsSetMixerGainbut does nothing (Line 301), so the gain update is lost.Proposed fix (preserve updates in mutex path as fallback)
case AudioCommand::SetMixerGain: - // Skip SetMixerGain in the mutex-gated path, it is handled lock-free + // Fallback: apply if this command reached mutex path + if (audio_shadow_executor_) { + audio_shadow_executor_->update_mixer_gain( + cmd.effect_index, cmd.param_index, cmd.value); + } break;Also applies to: 301-303
🤖 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/engine/audio_engine_process.cpp` around lines 246 - 263, drain_gain_commands currently breaks when a non-gain command sits at the queue head, allowing a later pop in drain_commands to discard SetMixerGain updates; fix by adding a fallback path in drain_commands to handle AudioCommand::SetMixerGain when it is popped there: after popping the command in drain_commands, detect cmd.type == AudioCommand::SetMixerGain and call audio_shadow_executor_->update_mixer_gain(cmd.effect_index, cmd.param_index, cmd.value) (guarded by the same mutex/nullable check used elsewhere for audio_shadow_executor_), so mixer gain updates are preserved even if they weren't processed in drain_gain_commands; keep existing handling for SetInputGain/SetOutputGain in drain_gain_commands unchanged.
🤖 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.
Outside diff comments:
In `@src/audio/engine/audio_engine_process.cpp`:
- Around line 246-263: drain_gain_commands currently breaks when a non-gain
command sits at the queue head, allowing a later pop in drain_commands to
discard SetMixerGain updates; fix by adding a fallback path in drain_commands to
handle AudioCommand::SetMixerGain when it is popped there: after popping the
command in drain_commands, detect cmd.type == AudioCommand::SetMixerGain and
call audio_shadow_executor_->update_mixer_gain(cmd.effect_index,
cmd.param_index, cmd.value) (guarded by the same mutex/nullable check used
elsewhere for audio_shadow_executor_), so mixer gain updates are preserved even
if they weren't processed in drain_gain_commands; keep existing handling for
SetInputGain/SetOutputGain in drain_gain_commands unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c31dfe06-ec25-486c-bd93-26beffbcb66b
📒 Files selected for processing (6)
src/audio/engine/audio_engine_process.cppsrc/audio/engine/audio_graph.cppsrc/audio/engine/audio_graph_executor.cppsrc/gui/pedalboard/pedal_board_chain.cppsrc/midi/midi_manager_mapping.cpptests/integration/test_audio_graph.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/integration/test_audio_graph.cpp
- src/audio/engine/audio_graph_executor.cpp
- src/audio/engine/audio_graph.cpp
- src/gui/pedalboard/pedal_board_chain.cpp
sudip-mondal-2002
requested changes
May 29, 2026
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
Owner
There was a problem hiding this comment.
LGTM but need similar splitter as well
sudip-mondal-2002
previously approved these changes
May 29, 2026
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
Owner
There was a problem hiding this comment.
The changes are too big to properly go through, based on a cursory reading, LGTM
Comment on lines
+346
to
+369
| bool AudioGraph::add_input_pin(int node_id) { | ||
| for (auto &node : nodes_) { | ||
| if (node.id == node_id && node.routing_type == NodeRoutingType::Mixer) { | ||
| if (node.input_pin_ids.size() < 8) { | ||
| node.input_pin_ids.push_back(next_id_++); | ||
| node.input_gains.push_back(1.0f); | ||
| // Do not necessarily need to rebuild topology if we just added an unconnected pin | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| bool AudioGraph::remove_input_pin(int node_id, int pin_id) { | ||
| for (auto &node : nodes_) { | ||
| if (node.id == node_id && (node.routing_type == NodeRoutingType::Mixer || node.routing_type == NodeRoutingType::MergeSum)) { | ||
| if (node.input_pin_ids.size() > 2) { | ||
| if (node.input_gains.size() < node.input_pin_ids.size()) { | ||
| node.input_gains.resize(node.input_pin_ids.size(), 1.0f); | ||
| } | ||
| int index_to_remove = -1; | ||
| if (pin_id == -1) { |
There was a problem hiding this comment.
There is a big refactor needed
the nodes should not be handled like this
there should be
class Inputnode: Node {
add_to_graph(Graph g){}
}
We need to restructure our codebase to proper OOP style
I dont like this functional programmings
Will take up that as refactoring task. No need to worry
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/engine/audio_graph.cpp`:
- Around line 482-483: The signed/unsigned comparison in the insertion check
should be changed to use a size_t like the analogous restore_input_pin: convert
or replace the signed variable index with a size_t (e.g., size_t idx =
static_cast<size_t>(index)) and then use idx <= node.output_pin_ids.size() in
the condition before calling node.output_pin_ids.insert(... begin() + idx, ...).
Update the surrounding condition that currently reads "index >= 0 && index <=
node.output_pin_ids.size()" to use the new size_t variable to avoid
-Wsign-compare and match restore_input_pin's approach.
🪄 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: 025b234a-0edb-4038-a8cb-799913a282d2
📒 Files selected for processing (6)
src/audio/engine/audio_graph.cppsrc/audio/engine/audio_graph.hsrc/gui/commands/command_graph.hsrc/gui/pedalboard/pedal_board_chain.cppsrc/gui/pedalboard/pedal_board_menu.cpptests/ui/test_pedal_board_menu.cpp
✅ Files skipped from review due to trivial changes (1)
- tests/ui/test_pedal_board_menu.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/audio/engine/audio_graph.h
- src/gui/pedalboard/pedal_board_menu.cpp
- src/gui/pedalboard/pedal_board_chain.cpp
Comment on lines
+482
to
+483
| if (index >= 0 && index <= node.output_pin_ids.size()) { | ||
| node.output_pin_ids.insert(node.output_pin_ids.begin() + index, pin_id); |
Contributor
There was a problem hiding this comment.
Use a size_t comparison to match restore_input_pin.
index <= node.output_pin_ids.size() is a signed/unsigned comparison (-Wsign-compare). The analogous restore_input_pin was already converted to a size_t form; mirror it here for consistency.
🛡️ Proposed fix
- if (index >= 0 && index <= node.output_pin_ids.size()) {
- node.output_pin_ids.insert(node.output_pin_ids.begin() + index, pin_id);
+ if (index >= 0 && static_cast<size_t>(index) <= node.output_pin_ids.size()) {
+ node.output_pin_ids.insert(node.output_pin_ids.begin() + static_cast<size_t>(index), pin_id);
} else {📝 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
| if (index >= 0 && index <= node.output_pin_ids.size()) { | |
| node.output_pin_ids.insert(node.output_pin_ids.begin() + index, pin_id); | |
| if (index >= 0 && static_cast<size_t>(index) <= node.output_pin_ids.size()) { | |
| node.output_pin_ids.insert(node.output_pin_ids.begin() + static_cast<size_t>(index), pin_id); |
🤖 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/engine/audio_graph.cpp` around lines 482 - 483, The signed/unsigned
comparison in the insertion check should be changed to use a size_t like the
analogous restore_input_pin: convert or replace the signed variable index with a
size_t (e.g., size_t idx = static_cast<size_t>(index)) and then use idx <=
node.output_pin_ids.size() in the condition before calling
node.output_pin_ids.insert(... begin() + idx, ...). Update the surrounding
condition that currently reads "index >= 0 && index <=
node.output_pin_ids.size()" to use the new size_t variable to avoid
-Wsign-compare and match restore_input_pin's approach.
sudip-mondal-2002
approved these changes
May 29, 2026
MohitBareja16
added
type:performance
Merged
Merged
12 tasks
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?
This PR upgrades the existing 2-input Mixer node into a fully dynamic N-input Mixer (supporting 2 to 8 input pins). It allows users to build complex parallel routing topologies, such as blending a clean amp, a distorted amp, and a direct signal simultaneously. It also introduces independent, MIDI-learnable gain faders for each channel, fully integrated with the lock-free SPSC command queue to ensure glitch-free updates on the real-time audio thread.
Related Issue
Fixes #147
Type of Change
How Was This Tested?
make && ./build/amplitron-tests(Gain Fader)sliders while passing audio to verify zero-glitch volume scaling.Checklist
./amplitron-tests)std::mutex::lock()on the hot path)Screenshots / Demo
Summary by CodeRabbit
New Features
Behavior
Tests