Automation: interpolate dynamics at audio engine level - #207
Open
RomanPudashkin wants to merge 2 commits into
Open
Automation: interpolate dynamics at audio engine level#207RomanPudashkin wants to merge 2 commits into
RomanPudashkin wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe PR replaces discrete dynamic-level maps with automation curves. It adds curve evaluation, resampling, ease-based interpolation, and normalized dynamic-level conversion. Envelope evaluation and RPC serialization use the new types. Main-stream playback receives dynamic automation layers, while off-stream playback no longer receives them. FluidSynth, MuseSampler, and VST sequencers resample curves every 30 ms and emit deduplicated dynamic events. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@framework/audio/engine/internal/abstracteventsequencer.h`:
- Line 174: Update the automation evaluation used by the dynamic-level path
around evaluateCurveAt so positions before the first automation point return
curve.begin()->second directly, rather than interpolating with a negative
factor. Preserve normal interpolation for positions at or after the first point
and retain the existing dynamicLevelFromNormalized conversion.
In `@framework/audio/engine/internal/synthesizers/fluidsynth/fluidsequencer.cpp`:
- Around line 121-134: Update the expression-event generation in the layer
resampling loop to map each layerIdx to its target MIDI channel and set that
channel on every generated ControlChange event before inserting it into
destination. Also set channelIdx on the initial expression event in the
corresponding setup path before the note-on event, preserving the existing
note-channel mapping.
In `@framework/mpe/automationpoint.h`:
- Around line 181-187: Update the steps calculation in the resampling loop
around intervalDuration and stepInterval to use ceiling division rather than
truncating integer division, while retaining the minimum of one step. Preserve
the existing interpolation and onSample behavior so each interpolated segment
does not exceed stepInterval.
- Around line 146-157: Update the evaluation logic around findLessOrEqual so
that when it returns curve.end() for a position before the first key,
immediately return curve.begin()->second.outValue before interpolation. Preserve
interpolation for positions at or after an existing key, and add a test covering
evaluation before the first key.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 248196af-e2e7-449b-9406-42fab230e1ac
📒 Files selected for processing (14)
framework/audio/common/automatablevalue.hframework/audio/common/rpc/rpcpacker.hframework/audio/engine/internal/abstracteventsequencer.hframework/audio/engine/internal/nodes/eventaudionode.cppframework/audio/engine/internal/synthesizers/fluidsynth/fluidsequencer.cppframework/audio/engine/internal/synthesizers/fluidsynth/fluidsequencer.hframework/audio/tests/rpcpacker_tests.cppframework/mpe/automationpoint.hframework/mpe/events.hframework/mpe/mpetypes.hframework/musesampler/internal/musesamplersequencer.cppframework/musesampler/internal/musesamplersequencer.hframework/vst/internal/synth/vstsequencer.cppframework/vst/internal/synth/vstsequencer.h
💤 Files with no reviewable changes (1)
- framework/audio/engine/internal/nodes/eventaudionode.cpp
Comment on lines
+121
to
+134
| for (const auto& [layerIdx, curve] : layers) { | ||
| std::optional<int> lastLevel; | ||
|
|
||
| mpe::resampleCurve(curve, STEP_INTERVAL_US, [&](mpe::timestamp_t t, muse::real_t normalized) { | ||
| const int level = expressionLevel(mpe::dynamicLevelFromNormalized(normalized)); | ||
| if (lastLevel == level) { | ||
| return; | ||
| } | ||
| lastLevel = level; | ||
|
|
||
| midi::Event event(Event::Opcode::ControlChange, Event::MessageType::ChannelVoice10); | ||
| event.setIndex(midi::EXPRESSION_CONTROLLER); | ||
| event.setData(level); | ||
| destination[t].emplace_back(std::move(event)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Route expression controls to the note channel.
These controller events do not set a channel. The note-on event at Line 161 sets channelIdx, so expression controls do not affect notes on other channels.
Map each layerIdx to its target MIDI channel when resampling the curve. Set that channel on each generated controller event. Set channelIdx on the initial expression event before the note-on event.
Also applies to: 153-157
🤖 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 `@framework/audio/engine/internal/synthesizers/fluidsynth/fluidsequencer.cpp`
around lines 121 - 134, Update the expression-event generation in the layer
resampling loop to map each layerIdx to its target MIDI channel and set that
channel on every generated ControlChange event before inserting it into
destination. Also set channelIdx on the initial expression event in the
corresponding setup path before the note-on event, preserving the existing
note-channel mapping.
Comment on lines
+181
to
+187
| const Key intervalDuration = next->first - it->first; | ||
| const size_t steps = std::max(size_t(intervalDuration / stepInterval), size_t(1)); | ||
|
|
||
| for (size_t j = 1; j < steps; ++j) { | ||
| const Key t = it->first + intervalDuration * j / steps; | ||
| const real_t normalized = evaluateAt(next->second, it->second.outValue, real_t(j) / real_t(steps)); | ||
| onSample(t, normalized); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use ceiling division for resampling steps.
Line 182 truncates the interval ratio. For a 59 ms segment with a 30 ms interval, steps is 1, so the loop emits no intermediate sample. The output gap becomes 59 ms.
Calculate steps with ceiling division. This ensures that no interpolated segment exceeds stepInterval.
🤖 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 `@framework/mpe/automationpoint.h` around lines 181 - 187, Update the steps
calculation in the resampling loop around intervalDuration and stepInterval to
use ceiling division rather than truncating integer division, while retaining
the minimum of one step. Preserve the existing interpolation and onSample
behavior so each interpolated segment does not exceed stepInterval.
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
1 participant
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.
No description provided.