Fix orbit read guard sample clamping#67
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request refactors the calculation of guardSamples in OrbitDelayCore::processChannelFast to simplify the clamping logic. Feedback indicates that the current implementation may still cause issues with small delay buffers where the guard zone becomes disproportionately large, potentially breaking delay functionality. A code suggestion was provided to ensure the lower bound of the clamp is adjusted based on the buffer size to maintain robustness.
| const float guardSamples = clampf(maxGuardBySize, | ||
| kOrbitReadGuardMinSamples, | ||
| kOrbitReadGuardMaxSamples); |
There was a problem hiding this comment.
The current clamping logic forces guardSamples to a minimum of kOrbitReadGuardMinSamples (16.0f). For small delay buffers where maxGuardBySize (25% of the buffer) is less than 16, this results in a guard zone that is too large relative to the buffer size. For example, if delaySize is 32, guardSamples becomes 16, which causes the read pointer to be constantly reset because the condition distanceForward < 16 || distanceForward > 16 (lines 641-642) will be true for almost all positions. This effectively breaks the delay functionality for small buffers.
To ensure robustness, the lower bound of the clamp should not exceed maxGuardBySize.
| const float guardSamples = clampf(maxGuardBySize, | |
| kOrbitReadGuardMinSamples, | |
| kOrbitReadGuardMaxSamples); | |
| const float guardSamples = clampf(maxGuardBySize, | |
| (maxGuardBySize < kOrbitReadGuardMinSamples) ? maxGuardBySize : kOrbitReadGuardMinSamples, | |
| kOrbitReadGuardMaxSamples); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c573f004a
ℹ️ 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); |
There was a problem hiding this comment.
Cap guardSamples by delay size for small buffers
guardSamples is now floored to kOrbitReadGuardMinSamples (16) even when the attached delay buffer is smaller than 64 samples, so in orbit mode the check distanceForward < guardSamples || distanceForward > (delaySize - guardSamples) becomes effectively always true for small buffers (e.g. size 32, and for size <32 it is strictly always true). That forces readPos to writePos + guardSamples every sample, collapsing modulation and creating a read/write spacing that no longer scales with buffer size. The previous code reduced guard for small buffers, so this is a behavior regression for valid attachBuffers(..., size>=4) configurations.
Useful? React with 👍 / 👎.
Motivation
clampfthat inverted min/max and forcedguardSamplestokOrbitReadGuardMinSamples, preventing it from growing with buffer size.Description
clampf(maxGuardBySize, kOrbitReadGuardMinSamples, kOrbitReadGuardMaxSamples)incore/src/orbit_delay_core.cppwhile leavingwrapPosFloat,readPos,writePos, and the distance checks unchanged.Testing
cmake .. -DBUILD_TESTING=ON && cmake --build .and ran tests withctest --output-on-failure, and all tests passed (5/5).Codex Task