Add moving gap to Chase 2 version 2#5507
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe internal Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/FX.cpp`:
- Around line 554-576: SEGENV.aux0 can be out-of-range if period shrinks; after
computing period (based on width/skip/pitch) but before rendering the loop that
uses pos, normalize SEGENV.aux0 to the new period (guarding against period being
zero) by doing something like: if (period > 0 && SEGENV.aux0 >= period)
SEGENV.aux0 %= period; this ensures pos calculations in the for-loop (which
reference SEGENV.aux0) stay correct until the next movement tick updates aux0.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 590a12ed-2b11-4c01-b48f-d27485df7032
📒 Files selected for processing (1)
wled00/FX.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wled00/FX.cpp (1)
554-579:⚠️ Potential issue | 🟡 MinorClamp the saved phase to the new period before rendering.
If Width/Skip is reduced while the effect is already running,
SEGENV.aux0can still be larger than the newperiod. On Line 562 that makes the dividend negative, so%can stay negative and the next frame renders the wrong stripe pattern until Line 578 advances again.Suggested fix
int pitch = skip + 1; int stripe_len = width * pitch - skip; int period = 2 * width * pitch; + int phase = SEGENV.aux0 % period; for (unsigned i = 0; i < SEGLEN; i++) { int col = color3; - int pos = ((int)(i % period) - (int)SEGENV.aux0 + period) % period; + int pos = ((int)(i % period) - phase + period) % period; if (pos < stripe_len && (pos % pitch == 0)) { if (usePalette) color1 = SEGMENT.color_from_palette(i, true, PALETTE_SOLID_WRAP, 0); col = color1; // foreground stripe @@ } if (it != SEGENV.step) { - SEGENV.aux0 = (SEGENV.aux0 + 1) % period; + SEGENV.aux0 = (phase + 1) % period; SEGENV.step = it; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 554 - 579, SEGENV.aux0 can remain >= the new period when width/skip shrink, causing the modulus math for pos to produce negative values and misrender; clamp SEGENV.aux0 to the new period range before rendering by reducing SEGENV.aux0 modulo period (or set to min(SEGENV.aux0, period-1)) right after computing int period and before the for-loop that computes pos, so subsequent uses of SEGENV.aux0 produce valid non-negative positions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wled00/FX.cpp`:
- Around line 554-579: SEGENV.aux0 can remain >= the new period when width/skip
shrink, causing the modulus math for pos to produce negative values and
misrender; clamp SEGENV.aux0 to the new period range before rendering by
reducing SEGENV.aux0 modulo period (or set to min(SEGENV.aux0, period-1)) right
after computing int period and before the for-loop that computes pos, so
subsequent uses of SEGENV.aux0 produce valid non-negative positions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9b82ab2-b472-41b0-a62a-b92fa4c39e67
📒 Files selected for processing (1)
wled00/FX.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wled00/FX.cpp (1)
556-563:⚠️ Potential issue | 🟡 MinorNormalize phase before modulo position math.
Line 562 can produce negative
poswhenSEGENV.aux0is larger than a newly reducedperiod(C++%keeps dividend sign), causing transient wrong rendering until the next tick.🔧 Proposed fix
int pitch = skip + 1; - int stripe_len = width * pitch - skip; + int stripe_len = width * pitch - skip; int period = 2 * width * pitch; + int phase = SEGENV.aux0 % period; for (unsigned i = 0; i < SEGLEN; i++) { uint32_t col = color3; - int pos = ((int)(i % period) - (int)SEGENV.aux0 + period) % period; + int pos = ((int)(i % period) - phase + period) % period; @@ if (it != SEGENV.step) { - SEGENV.aux0 = (SEGENV.aux0 + 1) % period; + SEGENV.aux0 = (phase + 1) % period; SEGENV.step = it; }Also applies to: 578-578
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 556 - 563, Normalize the phase before doing the modulo position math: compute a reducedPhase = (int)SEGENV.aux0 % period (ensure period>0) and then use pos = ((int)(i % period) - reducedPhase + period) % period so the subtraction cannot produce values outside the -period..period range; update the same logic used at the second occurrence around the later pos calculation (the one at line ~578) to use reducedPhase as well. Ensure casts to int are applied consistently.
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
557-557: Use camelCase for new local variable names.
stripe_lenis newly introduced snake_case;stripeLenwould match project C++ naming rules.As per coding guidelines: "Use camelCase for C++ function and variable names".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` at line 557, The new local variable stripe_len should be renamed to camelCase to match project conventions; update the declaration and all its uses to stripeLen (e.g., in FX.cpp where stripe_len is computed as "width * pitch - skip") so references in the surrounding function (the FX rendering/stripe calculation block) use stripeLen consistently and rebuild to ensure no remaining snake_case occurrences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wled00/FX.cpp`:
- Around line 556-563: Normalize the phase before doing the modulo position
math: compute a reducedPhase = (int)SEGENV.aux0 % period (ensure period>0) and
then use pos = ((int)(i % period) - reducedPhase + period) % period so the
subtraction cannot produce values outside the -period..period range; update the
same logic used at the second occurrence around the later pos calculation (the
one at line ~578) to use reducedPhase as well. Ensure casts to int are applied
consistently.
---
Nitpick comments:
In `@wled00/FX.cpp`:
- Line 557: The new local variable stripe_len should be renamed to camelCase to
match project conventions; update the declaration and all its uses to stripeLen
(e.g., in FX.cpp where stripe_len is computed as "width * pitch - skip") so
references in the surrounding function (the FX rendering/stripe calculation
block) use stripeLen consistently and rebuild to ensure no remaining snake_case
occurrences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b5ec2ee-7f59-4970-b501-d34c72ceaa85
📒 Files selected for processing (1)
wled00/FX.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/FX.cpp`:
- Around line 546-573: The function running() references undeclared symbols
usePalette, usePalette2, and it; add explicit parameters or local declarations
to fix this: update running(...) signature to include bool usePalette and bool
usePalette2 (matching the do_palette pattern used by chase()), and declare a
local uint32_t it computed the same way other mode functions do (e.g., uint32_t
it = strip.now / cycleTime or the equivalent timing expression used elsewhere),
then use those variables in the existing logic (leave calls to
SEGMENT.color_from_palette and SEGENV.aux0/step updates as-is).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29022d7e-7091-4185-95f0-1a56f5cc9bc8
📒 Files selected for processing (1)
wled00/FX.cpp
|
I think version2 is better as it has the additional color. |
|
@DedeHai I have renamed "skip" to "gap" in the code and the UI. There is no need for a checkmark. The theater modes work exactly as before. Chase 2 defaults to a gap value of zero. When it is zero, it behaves exactly as the previous version, so previously saved presets will be unchanged. |
RussStringham
changed the title
there is. an existing preset does not use the defaults. please test it. |
2 participants
This PR is an alternate implementation for PR-5504. Only one or the other should be merged. Here is the description:
I have added a new slider to Chase 2 that generates values between 0 and 15. When this slider is at its default value of 0, it behaves exactly like the current version of Chase 2. When the slider is greater than 0, it is treated as a gap value. At any instance in time, the result looks the same as when a gap is specified in the segment definition. However, when the stripe moves by one LED, it moves to the next physical LED, rather than the next lit LED. This results is the gaps rotating as well.
In this PR, the gap color is controled by Cs (color 3) instead of always being black. I have also simplified the running() function so that the theater modes only differ from Chase 2 by the parameters that are passed. There is no special logic in the function to handle theater differently than chase. In this case, the theater BG color is the gap color and the theater width is the gap width.
In the previous PR the gap is called the skip value.
Summary by CodeRabbit
New Features
Refactor