Add reverse smear toggle with dedicated reverse allpass chain#79
Conversation
|
Warning Review limit reached
More reviews will be available in 20 minutes and 53 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ 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 |
![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 introduces a "reverse smear" feature to the orbit delay effect, adding a C API wrapper, atomic parameter handling, and processing logic using four-stage allpass filters in the accidental reverse read mode. The feedback recommends refactoring the hardcoded array sizes using a static constexpr member to improve maintainability and avoid duplication. Additionally, it suggests resetting the reverse smear stages when the feature is toggled to prevent audible clicks or pops from stale audio buffers.
| AllpassStage reverseSmearStagesL_[4]; | ||
| AllpassStage reverseSmearStagesR_[4]; |
There was a problem hiding this comment.
Using a hardcoded magic number 4 for the array size of reverseSmearStagesL_ and reverseSmearStagesR_ introduces a maintainability risk and code duplication.
We can define kReverseSmearStages as a static constexpr member of OrbitDelayCore and use it to declare the array sizes. This keeps the array sizes synchronized with the DSP implementation.
| AllpassStage reverseSmearStagesL_[4]; | |
| AllpassStage reverseSmearStagesR_[4]; | |
| static constexpr uint32_t kReverseSmearStages = 4u; | |
| AllpassStage reverseSmearStagesL_[kReverseSmearStages]; | |
| AllpassStage reverseSmearStagesR_[kReverseSmearStages]; |
| } | ||
| }; | ||
|
|
||
| constexpr uint32_t kReverseSmearStages = 4u; |
There was a problem hiding this comment.
Instead of hardcoding 4u here, we can reference the kReverseSmearStages constant defined in OrbitDelayCore to avoid duplication and ensure consistency across the header and source files.
| constexpr uint32_t kReverseSmearStages = 4u; | |
| constexpr uint32_t kReverseSmearStages = OrbitDelayCore::kReverseSmearStages; |
| diffuserDirty_ = true; | ||
| } | ||
| dcBlockEnabled_ = nextDcBlock; | ||
| reverseSmearEnabled_ = nextReverseSmearEnabled; |
There was a problem hiding this comment.
When reverseSmearEnabled_ is toggled, the internal reverseSmearStagesL_ and reverseSmearStagesR_ are not reset. This can cause stale audio samples remaining in the allpass buffers to be output as a sudden burst or cause audible clicks/pops when the reverse smear is re-enabled.
To ensure a smooth transition and consistent behavior (similar to how shimmerModeEnabled_ resets the diffusers), we should reset the reverse smear stages when reverseSmearEnabled_ is toggled.
if (reverseSmearEnabled_ != nextReverseSmearEnabled) {
reverseSmearEnabled_ = nextReverseSmearEnabled;
for (uint32_t i = 0; i < kReverseSmearStages; ++i) {
reverseSmearStagesL_[i].reset();
reverseSmearStagesR_[i].reset();
}
}![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: 29381baccd
ℹ️ 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".
| bool orbit_set_reverse_smear_enabled(OrbitDelayHandle* handle, bool enabled) { | ||
| if (handle == nullptr) { | ||
| return false; | ||
| } | ||
| handle->core.setReverseSmearEnabled(enabled); |
There was a problem hiding this comment.
Add the corresponding WASM bridge/export for this new C API setter; currently only native C callers can toggle reverse smear. The WASM target uses explicit wrappers and an explicit EXPORTED_FUNCTIONS list (targets/wasm/orbit_delay_wasm.cpp, targets/wasm/CMakeLists.txt), and there is no orbit_wasm_set_reverse_smear_enabled entry, so web/demo/worklet clients cannot access the feature introduced here.
Useful? React with 👍 / 👎.
1 participant
Motivation
Description
void setReverseSmearEnabled(bool enabled)and internal flagsreverseSmearEnabled_andpendingReverseSmearEnabled_to integrate the parameter into the existing pending/applied parameter flow.reverseSmearStagesL_/reverseSmearStagesR_) with fixed delays and gains and helper functionsinitReverseSmearStagesandprocessReverseSmearincore/src/orbit_delay_core.cpp.resetand applied the reverse chain only after obtaining the wet reverse signal and only whenreadMode == AccidentalReverseandreverseSmearEnabled_is true; the shimmer diffuser remains unchanged and is not reused.bool orbit_set_reverse_smear_enabled(OrbitDelayHandle* handle, bool enabled)incore/include/orbit_delay_c_api.h/core/src/orbit_delay_c_api.cpp.Testing
cmake -S . -B build && cmake --build build -j2which completed successfully.ctest --test-dir build --output-on-failureand all tests passed (5/5 tests passed).Codex Task