Add: parallel amp blend graph preset#214
Closed
arywk40-hue wants to merge 4 commits into
Closed
Conversation
Contributor
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds per-input mixer gains with UI sliders and executor support, graph-format preset IO with a bundled "06 Parallel Amp Blend" preset, an AudioEngine graph-replacement API, and tests verifying mixer gain persistence and weighted blending. ChangesGraph Preset Loading and Mixer Gain Blending
Sequence Diagram(s)sequenceDiagram
participant User as User/UI
participant PresetMgr as PresetManager
participant EffectFac as EffectFactory
participant Engine as AudioEngine
participant Graph as AudioGraph
participant Exec as AudioGraphExecutor
User->>PresetMgr: open/load JSON preset file
PresetMgr->>PresetMgr: parse JSON, detect graph-format
PresetMgr->>EffectFac: create Effect instances per node
PresetMgr->>Graph: create nodes, pins, set input_gains/positions
PresetMgr->>Graph: add links (using remapped pin ids)
PresetMgr->>Engine: replace_graph(Graph, effects)
Engine->>Engine: initialize effects, swap graphs under mutex
Engine->>Exec: commit_graph_changes() -> rebuild execution plan (reads input_gains)
Exec->>Exec: use recorded per-connection gains during processing
PresetMgr->>User: report success / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds per-input gain (blend) support for Mixer nodes in the audio graph, persists those gains in the v2 graph preset format, and exposes them in the GUI along with a new example preset.
Changes:
- Add
input_gainstoDSPNode, apply gains during graph execution, and provide an API to set them. - Extend graph preset load/save to roundtrip mixer
input_gains, and add tests validating correct behavior. - Add a new “Parallel Amp Blend” graph preset and document it.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_preset_manager.cpp | Adds preset loading tests to verify mixer gains and example preset integrity |
| tests/test_audio_graph.cpp | Updates mock graph load/save for input_gains and adds DSP processing test for mixer blend |
| src/preset_manager_io.cpp | Implements v2 graph preset loading (nodes/links/effects) including input_gains |
| src/gui/pedal_board_chain.cpp | Restores node positions from presets and adds mixer gain sliders in the graph UI |
| src/audio/audio_graph_executor.h | Adds per-input gain to executor input sources |
| src/audio/audio_graph_executor.cpp | Applies mixer input gains during summing |
| src/audio/audio_graph.h | Adds input_gains to nodes and a setter API |
| src/audio/audio_graph.cpp | Initializes mixer gains and implements set_node_input_gain |
| src/audio/audio_engine_chain.cpp | Adds AudioEngine::replace_graph to swap in a loaded graph/effects |
| src/audio/audio_engine.h | Declares replace_graph |
| presets/06_Parallel_Amp_Blend.json | Adds a new example graph preset exercising splitter/mixer blend |
| docs/PRESETS.md | Documents the new graph preset and usage tips |
| .gitignore | Un-ignores the new preset file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 2
🧹 Nitpick comments (2)
src/audio/audio_graph_executor.cpp (1)
89-92: ⚡ Quick winClamp mixer gains when building execution input sources.
input_gainsis consumed directly here. Clamping at this boundary keeps execution behavior safe even if graph data was populated from external/legacy sources.Proposed fix
- if (it->routing_type == NodeRoutingType::Mixer && - input_index < it->input_gains.size()) { - gain = it->input_gains[input_index]; + if (it->routing_type == NodeRoutingType::Mixer && + input_index < it->input_gains.size()) { + gain = std::clamp(it->input_gains[input_index], 0.0f, 1.0f); }🤖 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_graph_executor.cpp` around lines 89 - 92, When pulling mixer gain values in the execution-source build (the block checking it->routing_type == NodeRoutingType::Mixer and reading it->input_gains[input_index] into gain), clamp the retrieved value to a safe range (e.g., 0.0f–1.0f) before assigning to gain (use std::clamp or equivalent) so malformed/legacy graph data cannot produce out-of-range gains; update the assignment that reads it->input_gains[input_index] to apply the clamp.src/gui/pedal_board_chain.cpp (1)
203-206: ⚡ Quick winAvoid committing graph topology on every slider delta.
Calling
engine_.commit_graph_changes()for each drag step can cause unnecessary graph recompiles while the user is moving a mixer gain. Stage gain updates continuously, then commit once after edit completion.Proposed change pattern
- if (ImGui::SliderFloat(slider_id.c_str(), &gain, 0.0f, 1.0f, "%.2f")) { - audio_graph.set_node_input_gain(node.id, gain_index, gain); - engine_.commit_graph_changes(); - } + if (ImGui::SliderFloat(slider_id.c_str(), &gain, 0.0f, 1.0f, "%.2f")) { + audio_graph.set_node_input_gain(node.id, gain_index, gain); + } + if (ImGui::IsItemDeactivatedAfterEdit()) { + engine_.commit_graph_changes(); + }🤖 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 203 - 206, SliderFloat currently calls engine_.commit_graph_changes() on every drag step causing repeated graph recompiles; instead call audio_graph.set_node_input_gain(...) continuously as the slider moves, but defer engine_.commit_graph_changes() until the user finishes the edit by checking ImGui item state (e.g. use ImGui::IsItemDeactivatedAfterEdit() or detect transition from ImGui::IsItemActive() to inactive). Concretely: keep the ImGui::SliderFloat and audio_graph.set_node_input_gain(node.id, gain_index, gain) inside the slider block, remove the immediate engine_.commit_graph_changes() call there, and add a one-time commit trigger that runs engine_.commit_graph_changes() when the slider item is deactivated after edit (reference SliderFloat, slider_id, audio_graph.set_node_input_gain, engine_.commit_graph_changes).
🤖 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_chain.cpp`:
- Around line 103-115: The loop that calls fx->set_sample_rate(sample_rate_) and
fx->reset() in AudioEngine::replace_graph mutates Effect instances outside the
effect_mutex_ and can race with the running graph; move the entire for (const
auto& fx : effects) { if (fx) { fx->set_sample_rate(...); fx->reset(); } } loop
inside the same std::lock_guard<std::mutex> lock(effect_mutex_) block that
assigns dummy_effects_ = std::move(effects) and main_graph_ = std::move(graph)
so all effect mutations and the swap occur under effect_mutex_ protection.
In `@src/audio/audio_graph.cpp`:
- Around line 20-21: The mixer node default input gains are set too high
(node.input_gains.push_back(1.0f) twice) causing level doubling; change the
initialization in the mixer node setup to push 0.5f for each input so new mixer
nodes default to a 0.5/0.5 blend (update the code that constructs mixer nodes
where node.input_gains is populated to use 0.5f values instead of 1.0f).
---
Nitpick comments:
In `@src/audio/audio_graph_executor.cpp`:
- Around line 89-92: When pulling mixer gain values in the execution-source
build (the block checking it->routing_type == NodeRoutingType::Mixer and reading
it->input_gains[input_index] into gain), clamp the retrieved value to a safe
range (e.g., 0.0f–1.0f) before assigning to gain (use std::clamp or equivalent)
so malformed/legacy graph data cannot produce out-of-range gains; update the
assignment that reads it->input_gains[input_index] to apply the clamp.
In `@src/gui/pedal_board_chain.cpp`:
- Around line 203-206: SliderFloat currently calls
engine_.commit_graph_changes() on every drag step causing repeated graph
recompiles; instead call audio_graph.set_node_input_gain(...) continuously as
the slider moves, but defer engine_.commit_graph_changes() until the user
finishes the edit by checking ImGui item state (e.g. use
ImGui::IsItemDeactivatedAfterEdit() or detect transition from
ImGui::IsItemActive() to inactive). Concretely: keep the ImGui::SliderFloat and
audio_graph.set_node_input_gain(node.id, gain_index, gain) inside the slider
block, remove the immediate engine_.commit_graph_changes() call there, and add a
one-time commit trigger that runs engine_.commit_graph_changes() when the slider
item is deactivated after edit (reference SliderFloat, slider_id,
audio_graph.set_node_input_gain, engine_.commit_graph_changes).
🪄 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: c84c0324-aa72-4396-bc51-f73b70873aa5
📒 Files selected for processing (13)
.gitignoredocs/PRESETS.mdpresets/06_Parallel_Amp_Blend.jsonsrc/audio/audio_engine.hsrc/audio/audio_engine_chain.cppsrc/audio/audio_graph.cppsrc/audio/audio_graph.hsrc/audio/audio_graph_executor.cppsrc/audio/audio_graph_executor.hsrc/gui/pedal_board_chain.cppsrc/preset_manager_io.cpptests/test_audio_graph.cpptests/test_preset_manager.cpp
arywk40-hue
force-pushed
the
codex/parallel-amp-blend-preset
branch
from
66924ce to
b55bd2d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_audio_graph.cpp (1)
289-320: ⚡ Quick winAssert the full processed buffer in the blend test.
Line 319 checks only sample 0, so per-sample indexing bugs could still pass this test.
Suggested patch
- ASSERT_TRUE(std::abs(output_audio[0] - 0.75f) < 0.0001f); + for (size_t i = 0; i < output_audio.size(); ++i) { + ASSERT_TRUE(std::abs(output_audio[i] - 0.75f) < 0.0001f); + }🤖 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 `@tests/test_audio_graph.cpp` around lines 289 - 320, The test audio_graph_mixer_input_gains_control_blend currently asserts only output_audio[0], which can miss per-sample errors; modify the test after executor.process(...) to check every sample in output_audio (size 64) equals 0.75f within the same tolerance (e.g., loop for i in 0..63 and ASSERT_NEAR(output_audio[i], 0.75f, 0.0001f) or equivalent) so that the entire processed buffer is validated rather than a single sample; reference the test function name, the output_audio vector and executor.process call to locate where to add the loop-based assertions.
🤖 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 `@docs/PRESETS.md`:
- Line 60: The presets list has a numbering conflict: the new preset file
"06_Parallel_Amp_Blend.json" occupies slot 06 while the heading "06 Jet Flanger"
still uses 06; update the heading "06 Jet Flanger" in PRESETS.md to "07 Jet
Flanger" (and adjust any following preset numbers if the file inserts shifted
the sequence) so each preset number is unique and matches the corresponding file
names.
In `@src/gui/pedal_board_chain.cpp`:
- Line 380: Change the condition that enables output-pin drag so graph output
(sink) nodes cannot start drags: where the code currently checks "if
(!node.output_pin_ids.empty())" add a guard that excludes graph-output nodes
(e.g. "&& !node.is_graph_output()" or "&& node.type != NodeType::GraphOutput"
depending on your node API). In other words, detect the graph-output node
identity (use the existing node flag/method that indicates an output/sink node)
and only allow the output-pin drag behavior when the node is not a graph output.
In `@src/preset_manager_io.cpp`:
- Around line 73-77: The code currently static_casts an integer from node_j into
NodeRoutingType without validation; before calling graph.add_node, read the
integer (e.g., int rt = node_j.value("routing_type", 0)), validate it is one of
the defined NodeRoutingType values (either by checking against the explicit enum
members or via a small helper like is_valid_routing_type(int) using
std::underlying_type), and if invalid either log/throw a clear error and
skip/fail node creation; then only perform the static_cast<NodeRoutingType>(rt)
and call graph.add_node(node_name, routing_type, effect). Ensure references to
NodeRoutingType, routing_type, node_j, node_name, and graph.add_node are used to
locate and fix the code.
- Around line 101-110: When remapping serialized pin IDs into pin_map
(processing node_j["input_pin_ids"] and node_j["output_pin_ids"]), detect
duplicates before assignment: for each old_id = old_pins[i].get<int>(), check if
pin_map already contains old_id (using pin_map.find(old_id) or count) and if so
reject the preset remap (throw/return an error or log and abort) instead of
overwriting; otherwise insert the mapping pin_map[old_id] =
new_node->input_pin_ids[i] (or output_pin_ids[i]). Ensure the same
duplicate-check logic is applied in both loops handling new_node->input_pin_ids
and new_node->output_pin_ids so malformed presets cannot silently overwrite
existing entries.
---
Nitpick comments:
In `@tests/test_audio_graph.cpp`:
- Around line 289-320: The test audio_graph_mixer_input_gains_control_blend
currently asserts only output_audio[0], which can miss per-sample errors; modify
the test after executor.process(...) to check every sample in output_audio (size
64) equals 0.75f within the same tolerance (e.g., loop for i in 0..63 and
ASSERT_NEAR(output_audio[i], 0.75f, 0.0001f) or equivalent) so that the entire
processed buffer is validated rather than a single sample; reference the test
function name, the output_audio vector and executor.process call to locate where
to add the loop-based assertions.
🪄 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: c1e3269f-a079-473b-b5a6-5c80224c5abf
📒 Files selected for processing (13)
.gitignoredocs/PRESETS.mdpresets/06_Parallel_Amp_Blend.jsonsrc/audio/audio_engine.hsrc/audio/audio_engine_chain.cppsrc/audio/audio_graph.cppsrc/audio/audio_graph.hsrc/audio/audio_graph_executor.cppsrc/audio/audio_graph_executor.hsrc/gui/pedal_board_chain.cppsrc/preset_manager_io.cpptests/test_audio_graph.cpptests/test_preset_manager.cpp
✅ Files skipped from review due to trivial changes (2)
- src/audio/audio_engine.h
- .gitignore
Contributor
Author
|
@sudip-mondal-2002 hey can u kindly have a look upon my pull request!! |
sudip-mondal-2002
added
type:feature
sudip-mondal-2002
requested changes
May 24, 2026
sudip-mondal-2002
left a comment
sudip-mondal-2002
left a comment
Owner
There was a problem hiding this comment.
Check CI, everything is failing
arywk40-hue
force-pushed
the
codex/parallel-amp-blend-preset
branch
4 times, most recently
from
ba2c159 to
0a9ea68
Compare
Contributor
Code Coverage Report 📊Line Coverage: 78.9% ✅ Coverage meets threshold: 78.9% >= 60% Full Coverage Summary |
Contributor
PR Preview RemovedThe GitHub Pages preview for this PR has been removed because the PR was closed. |
sudip-mondal-2002
requested changes
May 24, 2026
Comment on lines
+140
to
+172
| const char* json = R"JSON( | ||
| { | ||
| "format_version": 2, | ||
| "name": "Graph Load Test", | ||
| "input_gain": 0.6, | ||
| "output_gain": 0.7, | ||
| "nodes": [ | ||
| { | ||
| "id": 1, | ||
| "name": "Input", | ||
| "routing_type": 0, | ||
| "is_graph_input": true, | ||
| "is_graph_output": false, | ||
| "x": 40.0, | ||
| "y": 120.0, | ||
| "input_pin_ids": [2], | ||
| "output_pin_ids": [3] | ||
| }, | ||
| { | ||
| "id": 4, | ||
| "name": "Splitter", | ||
| "routing_type": 1, | ||
| "is_graph_input": false, | ||
| "is_graph_output": false, | ||
| "x": 240.0, | ||
| "y": 120.0, | ||
| "input_pin_ids": [5], | ||
| "output_pin_ids": [6, 7] | ||
| }, | ||
| { | ||
| "id": 8, | ||
| "name": "Mixer", | ||
| "routing_type": 2, |
There was a problem hiding this comment.
Why are we using JSON strings? there is a proper JSON handler framework we use please use that
arywk40-hue
force-pushed
the
codex/parallel-amp-blend-preset
branch
from
0a9ea68 to
fe94fbd
Compare
Contributor
Author
|
@sudip-mondal-2002 now can u check once more |
Collaborator
|
@arywk40-hue Send screenshots/video that 6th preset is working properly |
MohitBareja16
left a comment
MohitBareja16
left a comment
Collaborator
There was a problem hiding this comment.
AI SLOP, close it asap
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
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 graph-format preset,
06_Parallel_Amp_Blend, showcasing parallel amp routing with a Splitter and Mixer. The preset blends a dirty British Crunch path with a clean American path at a default 50/50 mix, then sends the merged signal into shared Reverb.This PR also adds graph preset loading support, Mixer input gain handling, readable saved canvas positions, documentation, and focused tests.
Related Issue
Fixes #153
Type of Change
How Was This Tested?
python3 -m json.tool presets/06_Parallel_Amp_Blend.jsongit diff --check0.5 / 0.5cmakeis not installed in this environment.Checklist
./amplitron-tests)std::mutex::lock()on the hot path)Screenshots / Demo
The UI change adds Mixer A/B gain sliders so users can adjust the clean/dirty blend directly from the Mixer node.