Enable Hermite reads; add orbit read guard, soft-clip writes, and cadence thresholds#66
Enable Hermite reads; add orbit read guard, soft-clip writes, and cadence thresholds#66ovelhaaa wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR enhances the delay processing engine by enabling conditional Hermite interpolation at compile time, refactoring delay reads to use the new logic, gating parameter updates on audible deltas, introducing guard logic to protect forward read positions, and applying soft-clipping to feedback writes. ChangesDelay read quality and parameter update gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables Hermite interpolation, introduces a read guard to prevent pointer collisions in orbit mode, and adds soft clipping to the feedback loop. It also refines the update logic for lowpass and diffuser parameters using a cadence-based system. The review feedback identifies that the new cadence update thresholds are redundant compared to the immediate update thresholds, potentially preventing parameters from reaching their targets. Additionally, a correction is suggested for the read guard calculation to ensure it remains valid for small delay buffers.
| const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kLowpassCadenceUpdateDeltaHz; | ||
| if (lowpassDirty_ || delta >= kLowpassUpdateDeltaHz || cadenceAndAudible) { |
There was a problem hiding this comment.
The logic for cadence-driven updates is redundant and likely inverted. Currently, kLowpassUpdateDeltaHz is 20.0 and kLowpassCadenceUpdateDeltaHz is 80.0. Since delta >= 80.0 implies delta >= 20.0, the cadenceAndAudible condition is dead code—the filter will update every sample as soon as the delta exceeds 20.0, regardless of the cadence hit.
Furthermore, this change prevents the filter from ever reaching its final target if the smoother stops moving while the delta is less than 20Hz (whereas previously it would sync on the next cadence hit). To achieve the goal of reducing updates while maintaining accuracy, you should update immediately for large changes and on cadence for smaller ones.
| const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kLowpassCadenceUpdateDeltaHz; | |
| if (lowpassDirty_ || delta >= kLowpassUpdateDeltaHz || cadenceAndAudible) { | |
| const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kLowpassUpdateDeltaHz; | |
| if (lowpassDirty_ || delta >= kLowpassCadenceUpdateDeltaHz || cadenceAndAudible) { |
| const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kDiffuserCadenceUpdateDelta; | ||
| if (diffuserDirty_ || delta >= kDiffuserUpdateDelta || cadenceAndAudible) { |
There was a problem hiding this comment.
Similar to the lowpass logic, the cadence check here is redundant because kDiffuserCadenceUpdateDelta (0.03) is greater than kDiffuserUpdateDelta (0.01). The immediate update condition will always trigger before the cadence condition.
| const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kDiffuserCadenceUpdateDelta; | |
| if (diffuserDirty_ || delta >= kDiffuserUpdateDelta || cadenceAndAudible) { | |
| const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kDiffuserUpdateDelta; | |
| if (diffuserDirty_ || delta >= kDiffuserCadenceUpdateDelta || cadenceAndAudible) { |
| #endif | ||
| const float writePos = static_cast<float>(delay.writePos); | ||
| const float maxGuardBySize = 0.25f * delaySize; | ||
| const float guardSamples = clampf(maxGuardBySize, kOrbitReadGuardMinSamples, kOrbitReadGuardMaxSamples); |
There was a problem hiding this comment.
The guardSamples calculation floors at kOrbitReadGuardMinSamples (16), which can exceed maxGuardBySize (25% of the buffer) for small delay lines. If guardSamples becomes too large relative to delaySize, the read pointer may become stuck or the guard condition may always be true. The clamp should ensure the guard does not exceed the available safety margin.
| const float guardSamples = clampf(maxGuardBySize, kOrbitReadGuardMinSamples, kOrbitReadGuardMaxSamples); | |
| const float guardSamples = clampf(kOrbitReadGuardMaxSamples, kOrbitReadGuardMinSamples, maxGuardBySize); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34541b6e76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const float guardSamples = clampf(maxGuardBySize, kOrbitReadGuardMinSamples, kOrbitReadGuardMaxSamples); | ||
| const float distanceForward = wrapPosFloat(readPos - writePos, delaySize, invDelaySize); | ||
| if (distanceForward < guardSamples || distanceForward > (delaySize - guardSamples)) { |
There was a problem hiding this comment.
Clamp guardSamples so it cannot exceed half the buffer size; with the current fixed minimum of 16 samples, any attached delay buffer smaller than 32 samples has no valid read region (distanceForward < guard or > size-guard is always true), so reads are forced to writePos + guard every sample. Since attachBufferMono/attachBuffers explicitly accept sizes down to 4, this regresses valid configurations by collapsing orbit/offset modulation into a fixed tap.
Useful? React with 👍 / 👎.
| const float delta = std::fabs(clampedToneHz - appliedToneHz_); | ||
| if (lowpassDirty_ || heavyParamCadenceHit_ || delta >= kLowpassUpdateDeltaHz) { | ||
| const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kLowpassCadenceUpdateDeltaHz; | ||
| if (lowpassDirty_ || delta >= kLowpassUpdateDeltaHz || cadenceAndAudible) { |
There was a problem hiding this comment.
Reintroduce a cadence-based fallback for small tone deltas; after this change, if lowpassDirty_ is false and the tone change stays below 20 Hz, maybeApplyLowpassCutoff never applies the new cutoff (delta >= 20 is false and cadenceAndAudible requires delta >= 80). Tone parameter updates do not set lowpassDirty_, so small but intentional adjustments can be ignored indefinitely.
Useful? React with 👍 / 👎.
| const float delta = std::fabs(smoothedSmear - appliedSmear_); | ||
| if (diffuserDirty_ || heavyParamCadenceHit_ || delta >= kDiffuserUpdateDelta) { | ||
| const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kDiffuserCadenceUpdateDelta; | ||
| if (diffuserDirty_ || delta >= kDiffuserUpdateDelta || cadenceAndAudible) { |
There was a problem hiding this comment.
Keep a cadence-triggered path for small smear adjustments; with this condition, once diffuserDirty_ is cleared, smear changes below 0.01 never propagate (delta >= 0.01 is false and the cadence branch now also requires delta >= 0.03). That creates dead zones for fine automation/knob moves that previously converged through cadence updates.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@core/src/orbit_delay_core.cpp`:
- Around line 386-387: The cadence check (heavyParamCadenceHit_ && delta >=
kLowpassCadenceUpdateDeltaHz) is currently masked by the broader delta checks
(delta >= kLowpassUpdateDeltaHz / kDiffuserUpdateDelta), so cadence never
changes behavior; modify the boolean logic so cadence-triggered updates only
fire when the regular update thresholds would NOT already fire — for example,
replace the current condition with something like: if (lowpassDirty_ || delta >=
kLowpassUpdateDeltaHz || (heavyParamCadenceHit_ && delta >=
kLowpassCadenceUpdateDeltaHz && delta < kLowpassUpdateDeltaHz)) and apply the
analogous change for the diffuser branch (use kDiffuserCadenceUpdateDelta and
kDiffuserUpdateDelta / diffuserDirty_), ensuring the cadence guard is combined
with a check that delta is below the broader update threshold so cadence gating
actually takes effect.
- Around line 637-642: The guardSamples value may exceed the actual delaySize
for small buffers (causing reads to collapse onto the write head); ensure
guardSamples is bounded by the buffer feasibility before use by clamping it
against delaySize (e.g., limit to at most delaySize * 0.5f or delaySize - 1
sample equivalent) after computing maxGuardBySize and before comparing to
distanceForward; update the logic around maxGuardBySize/guardSamples (and the
subsequent use with wrapPosFloat, readPos, writePos, invDelaySize) so
guardSamples cannot be larger than the delay buffer allows.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b1b1b21-1089-4aea-af15-7516868f1f48
📒 Files selected for processing (3)
core/CMakeLists.txtcore/include/orbit_delay_core.hcore/src/orbit_delay_core.cpp
| const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kLowpassCadenceUpdateDeltaHz; | ||
| if (lowpassDirty_ || delta >= kLowpassUpdateDeltaHz || cadenceAndAudible) { |
There was a problem hiding this comment.
Cadence thresholds are currently ineffective due to condition dominance
Line 387 and Line 399 already trigger on lower deltas (kLowpassUpdateDeltaHz, kDiffuserUpdateDelta), so the new cadence checks never change behavior (80 >= 20, 0.03 >= 0.01). This defeats the intended cadence gating/perf win.
One concrete way to restore cadence-gated behavior
- const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kLowpassCadenceUpdateDeltaHz;
- if (lowpassDirty_ || delta >= kLowpassUpdateDeltaHz || cadenceAndAudible) {
+ const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kLowpassCadenceUpdateDeltaHz;
+ if (lowpassDirty_ || cadenceAndAudible) {
@@
- const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kDiffuserCadenceUpdateDelta;
- if (diffuserDirty_ || delta >= kDiffuserUpdateDelta || cadenceAndAudible) {
+ const bool cadenceAndAudible = heavyParamCadenceHit_ && delta >= kDiffuserCadenceUpdateDelta;
+ if (diffuserDirty_ || cadenceAndAudible) {Also applies to: 398-399
🤖 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 `@core/src/orbit_delay_core.cpp` around lines 386 - 387, The cadence check
(heavyParamCadenceHit_ && delta >= kLowpassCadenceUpdateDeltaHz) is currently
masked by the broader delta checks (delta >= kLowpassUpdateDeltaHz /
kDiffuserUpdateDelta), so cadence never changes behavior; modify the boolean
logic so cadence-triggered updates only fire when the regular update thresholds
would NOT already fire — for example, replace the current condition with
something like: if (lowpassDirty_ || delta >= kLowpassUpdateDeltaHz ||
(heavyParamCadenceHit_ && delta >= kLowpassCadenceUpdateDeltaHz && delta <
kLowpassUpdateDeltaHz)) and apply the analogous change for the diffuser branch
(use kDiffuserCadenceUpdateDelta and kDiffuserUpdateDelta / diffuserDirty_),
ensuring the cadence guard is combined with a check that delta is below the
broader update threshold so cadence gating actually takes effect.
| const float maxGuardBySize = 0.25f * delaySize; | ||
| const float guardSamples = clampf(maxGuardBySize, kOrbitReadGuardMinSamples, kOrbitReadGuardMaxSamples); | ||
| const float distanceForward = wrapPosFloat(readPos - writePos, delaySize, invDelaySize); | ||
| if (distanceForward < guardSamples || distanceForward > (delaySize - guardSamples)) { | ||
| readPos = wrapPosFloat(writePos + guardSamples, delaySize, invDelaySize); | ||
| } |
There was a problem hiding this comment.
Guard size can collapse reads onto/near write position for small delay buffers
With Line 638 clamped to a minimum of 16 samples, guardSamples can exceed delaySize for valid small buffers, making the clamp branch always trigger and potentially wrapping to the write head. Clamp guard by buffer-size feasibility before use.
Suggested fix
- const float maxGuardBySize = 0.25f * delaySize;
- const float guardSamples = clampf(maxGuardBySize, kOrbitReadGuardMinSamples, kOrbitReadGuardMaxSamples);
+ const float maxGuardBySize = 0.25f * delaySize;
+ const float requestedGuard = clampf(maxGuardBySize, kOrbitReadGuardMinSamples, kOrbitReadGuardMaxSamples);
+ const float maxSafeGuard = clampf(0.5f * (delaySize - 1.0f), 1.0f, kOrbitReadGuardMaxSamples);
+ const float guardSamples = clampf(requestedGuard, 1.0f, maxSafeGuard);📝 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.
| const float maxGuardBySize = 0.25f * delaySize; | |
| const float guardSamples = clampf(maxGuardBySize, kOrbitReadGuardMinSamples, kOrbitReadGuardMaxSamples); | |
| const float distanceForward = wrapPosFloat(readPos - writePos, delaySize, invDelaySize); | |
| if (distanceForward < guardSamples || distanceForward > (delaySize - guardSamples)) { | |
| readPos = wrapPosFloat(writePos + guardSamples, delaySize, invDelaySize); | |
| } | |
| const float maxGuardBySize = 0.25f * delaySize; | |
| const float requestedGuard = clampf(maxGuardBySize, kOrbitReadGuardMinSamples, kOrbitReadGuardMaxSamples); | |
| const float maxSafeGuard = clampf(0.5f * (delaySize - 1.0f), 1.0f, kOrbitReadGuardMaxSamples); | |
| const float guardSamples = clampf(requestedGuard, 1.0f, maxSafeGuard); | |
| const float distanceForward = wrapPosFloat(readPos - writePos, delaySize, invDelaySize); | |
| if (distanceForward < guardSamples || distanceForward > (delaySize - guardSamples)) { | |
| readPos = wrapPosFloat(writePos + guardSamples, delaySize, invDelaySize); | |
| } |
🤖 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 `@core/src/orbit_delay_core.cpp` around lines 637 - 642, The guardSamples value
may exceed the actual delaySize for small buffers (causing reads to collapse
onto the write head); ensure guardSamples is bounded by the buffer feasibility
before use by clamping it against delaySize (e.g., limit to at most delaySize *
0.5f or delaySize - 1 sample equivalent) after computing maxGuardBySize and
before comparing to distanceForward; update the logic around
maxGuardBySize/guardSamples (and the subsequent use with wrapPosFloat, readPos,
writePos, invDelaySize) so guardSamples cannot be larger than the delay buffer
allows.
|
@coderabbitai resolve merge conflicts |
|
❌ An unexpected error occurred while resolving merge conflicts: Invalid tree info - https://docs.github.com/rest/git/trees#create-a-tree |
1 participant
Motivation
Description
ORBIT_DELAY_ENABLE_HERMITE=1compile definition incore/CMakeLists.txtand introducesreadDelayWrappedhelper to select Hermite vs linear reads based on that define.kLowpassCadenceUpdateDeltaHz,kDiffuserCadenceUpdateDelta, andkOrbitReadGuardMinSamples/kOrbitReadGuardMaxSamplesconstants toorbit_delay_core.hand uses them inmaybeApplyLowpassCutoffandmaybeApplyDiffuserAmountto refine cadence-driven updates.processChannelFastclamps/adjustsreadPosto a forward guard zone around the write pointer to avoid near-write reads, routes reads throughreadDelayWrapped, and appliessoftClipPolynomialto buffer writes before callingdelay.write.Testing
CMakeLists.txtand verified compilation with theORBIT_DELAY_ENABLE_HERMITEdefine enabled usingcmakeandcmake --build.ctest, and all tests completed successfully.processChannelto verify no runtime NaNs from the new read-guard and soft-clip code and observed stable outputs.Codex Task
Summary by CodeRabbit
New Features
Bug Fixes