1D effects x to y-axis conversion #81
Conversation
Back end ======== - blur1d: add optional column - MoonLight effects: RingEffect, Audiorings effect - WLED effects: Bouncing balls, tetrix: swap x,y - WLED effects: Rain: y-axis + random over x-axis (still a bug ...) - WLED effects: Drip swap xy and inverse y - WLED effects: DJLightEffect swap xy - Modifier: circle, Pinwheel1D, RippleYZModifier takes y-axis!
|
Caution Review failedThe pull request is closed. Note
|
| Cohort / File(s) | Summary |
|---|---|
Configuration platformio.ini |
APP_VERSION -> 0.7.1, APP_DATE -> 20251223 (build_flag metadata bump). |
Core Layer src/MoonLight/Layers/VirtualLayer.h |
void blur1d(fract8) → void blur1d(fract8, uint16_t x = 0); iteration changed to walk size.y and use Coord3D(x,i) (column-scoped blur). |
Effects — MoonLight src/MoonLight/Nodes/Effects/E_MoonLight.h |
Ring effects remapped to y-axis (Coord3D(0, ring)); RingRandomFlowEffect uses layer->size.y for hue arrays/loops; AudioRingsEffect removed nrOfRings member and computes rings from layer->size.y at runtime. |
Effects — WLED suite src/MoonLight/Nodes/Effects/E_WLED.h |
Wide axis realignment: many effects (BouncingBalls, Tetrix, Rain, Drip, DJLight, Flow, Fireworks, etc.) switched primary iteration to size.x; coordinate writes updated to Coord3D(...); per-column state introduced (e.g., Spark* drops, nrOfDrops) with destructors and onSizeChanged() reallocations; mode_fireworks() gained uint16_t x parameter. |
Modifiers src/MoonLight/Nodes/Modifiers/M_MoonLight.h |
Axis swaps in modifiers: CircleModifier maps distance to y instead of x; PinwheelModifier petal axis swapped in 3D branch; RippleYZModifier loop order and source pixel swapped (x/y inversion); minor whitespace edits in TransposeModifier. |
Effects — FastLED src/MoonLight/Nodes/Effects/E_FastLED.h |
RainbowEffect::loop glitter index changed from random16(layer->size.x) to random16(layer->nrOfLights) (uses total lights count). |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- V070 preps #79 — Similar axis/dimension changes affecting effects and onSizeChanged logic.
- Lots of effects #77 — Overlaps on VirtualLayer/effects axis/orientation updates.
- Board presets p4 #74 — Touched
VirtualLayerpublic API and build metadata; potentially related toblur1dand version bump.
Poem
I'm a jittery rabbit in the light,
I hop through columns, not rows tonight,
I blur a whisker at Coord3D(x,i) — so neat,
New rings and drops find their proper seat,
Hop, code, hop 🐇✨
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 20.45% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The pull request title accurately describes the main objective of the changeset, which involves converting 1D effects from x-axis to y-axis orientation across multiple files. |
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonLight/Nodes/Effects/E_FastLED.h
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 help to get the list of available commands and usage tips.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonLight/Nodes/Effects/E_WLED.h (1)
1844-1899: DripEffect: Verify all Coord3D usage and boundary conditions.The outer loop (line 1844) iterates over columns, and the inner loop (line 1845) iterates over drops per column. All LED access uses
Coord3D(x, ...)which is correct for per-column state.However, line 1878 has a concerning comment and constraint:
uint16_t pos = constrain(drops[x][j].pos + i, layer->size.y - 1, 0); // this is BAD, returns a pos >= layer->size.x occasionallyThe constrain call has swapped min/max parameters! It should be
constrain(value, min, max)but this hasconstrain(value, max, min). This will always return eitherlayer->size.y - 1or0, which is likely not the intent.🔎 Proposed fix
for (int i = 1; i < 7 - drops[x][j].colIndex; i++) { // some minor math so we don't expand bouncing droplets - uint16_t pos = constrain(drops[x][j].pos + i, layer->size.y - 1, 0); // this is BAD, returns a pos >= layer->size.x occasionally + uint16_t pos = constrain(drops[x][j].pos + i, 0, layer->size.y - 1); // constrain to valid y range layer->setRGB(Coord3D(x, pos), blend(CRGB::Black, dropColor, drops[x][j].col / i)); // spread pixel with fade while falling }
🧹 Nitpick comments (1)
src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
1609-1615: AudioRingsEffect: nrOfRings now dynamically computed, user control removed.The nrOfRings slider control is commented out (line 1611), and the value is now computed dynamically as
MAX(layer->size.y, 2)in the loop (line 1615). This makes the number of rings adapt to the layer height automatically but removes user configurability.Is this removal of user control intentional? If not, consider keeping the slider but using it as a maximum/multiplier rather than commenting it out entirely.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
platformio.inisrc/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Nodes/Effects/E_WLED.hsrc/MoonLight/Nodes/Modifiers/M_MoonLight.h
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-07T14:16:03.228Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-07T14:16:03.228Z
Learning: VirtualLayer mappingTableSize is uint16_t with a maximum of 65535 LEDs. If a user defines a space where size.x * size.y * size.z exceeds 65535, the code accepts the overflow and performs mapping partially. This is an explicit design decision balancing performance and memory constraints.
Applied to files:
src/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Nodes/Effects/E_MoonLight.h
📚 Learning: 2025-12-18T15:15:57.828Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.828Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Modifiers/M_MoonLight.hsrc/MoonLight/Nodes/Effects/E_MoonLight.hsrc/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-18T15:59:30.556Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:59:30.556Z
Learning: In DripEffect (E_WLED.h), the bounce fails because the damped velocity is too weak to keep the drop at pos > 0 for multiple frames. When the drop returns to pos <= 0 while still in the bouncing state (colIndex == bouncing), it immediately resets to init (line 1663-1664) before any visible bounce occurs. The bounce velocity must be strong enough (divide by 1.1 or less, or use minimal damping like *= 0.9) so the drop stays airborne long enough for a visible bounce arc before the eventual second ground hit triggers the reset.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-18T15:46:48.319Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:46:48.319Z
Learning: In DripEffect (E_WLED.h), the bounce damping at line 1669 needs to be `/1.5` or less (not `/4` or `/2`) to create a visible bounce effect. The continuous gravity application every frame (line 1652) requires a strong initial bounce velocity to overcome it. Using `/1.5` gives approximately 67% energy return, making the bounce visible and lasting for ~33 frames.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-18T15:30:02.486Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:30:02.486Z
Learning: In DripEffect (E_WLED.h), the bounce at line 1669 uses heavy damping (`vel = -vel / 4`), which makes the bounce too weak to be visible. Reducing the damping factor from 4 to 2 (or lower) makes the bounce more pronounced and visible.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
🧬 Code graph analysis (2)
src/MoonLight/Nodes/Modifiers/M_MoonLight.h (1)
src/MoonBase/Utilities.h (1)
Coord3D(151-163)
src/MoonLight/Nodes/Effects/E_WLED.h (2)
src/MoonLight/Nodes/Effects/E_MoonLight.h (18)
dim(17-17)dim(45-45)dim(108-108)dim(141-141)dim(155-155)dim(191-191)dim(307-307)dim(338-338)dim(376-376)dim(450-450)dim(498-498)dim(567-567)dim(663-663)dim(966-966)dim(1202-1202)dim(1267-1267)onSizeChanged(1210-1216)onSizeChanged(1578-1584)src/MoonLight/Nodes/Effects/E_MoonModules.h (1)
onSizeChanged(183-199)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (13)
platformio.ini (1)
58-59: Version and date bump looks good.The patch version increment (0.7.0 → 0.7.1) is appropriate for the refactoring changes across effects and modifiers described in the PR. Date format is consistent with the existing convention.
src/MoonLight/Layers/VirtualLayer.h (1)
214-229: LGTM! Column-scoped blur implementation is correct.The blur1d function now operates on a vertical column (specified by x parameter with default 0), iterating over size.y and using Coord3D(x, i) for all LED access. The carryover logic and blur calculations are preserved correctly from the original implementation.
src/MoonLight/Nodes/Modifiers/M_MoonLight.h (2)
236-242: RippleYZModifier loop order inverted - verify correctness.The 1D->2D ripple traversal changed from iterating y-outer/x-inner with source
(x, y-1)to iterating x-outer/y-inner with source(x-1, y). This fundamentally changes the ripple direction from vertical-to-horizontal to horizontal-to-vertical propagation. The comment update reflects this, but ensure this matches the intended behavior for effects using this modifier.The axis inversion appears intentional and consistent with the PR's goal of reorienting 1D effects to the y-axis.
149-150: The PinwheelModifier 1D axis assignment is correct and aligns with the codebase. The git history confirms this was an intentional refactor in v0.7.1 ("1D effects x to y-axis conversion"), where 1D effects were standardized to use the y-axis dimension. The size assignment (size.x = 1; size.y = petals) is consistent with how other 1D-to-2D/3D modifiers like CircleModifier and RippleYZModifier handle axis expansion, and themodifyPosition()logic correctly remaps the petal index to the x-axis with y fixed at 0, matching the VirtualLayer rendering pattern for 1D effects.src/MoonLight/Nodes/Effects/E_MoonLight.h (2)
1561-1562: LGTM! Ring addressing correctly updated for 1D-on-y-axis.The setRing method now uses
Coord3D(0, ring)to place rings along the y-axis in a 1D-y interpretation, which is consistent with the PR's goal of converting 1D effects to use the y-axis.
1578-1592: LGTM! Hue allocation and loop bounds correctly aligned with size.y.The changes correctly allocate hue memory based on
layer->size.yand update all loop bounds to iterate oversize.yinstead ofsize.x. This matches the learned constraint that arrays must be allocated according to the dimension they index.src/MoonLight/Nodes/Effects/E_WLED.h (7)
34-42: LGTM! BouncingBallsEffect correctly updated for per-column state.The balls array is now allocated per column (
layer->size.x) andballsSizeis set accordingly. This aligns with the new column-based approach where each x-coordinate maintains its own ball state.
59-94: Verify loop bounds match ballsSize to prevent out-of-bounds access.The outer loop iterates
x < MIN(layer->size.x, ballsSize)(line 59), which correctly guards against accessing unallocated memory if reallocation failed. The ball position calculation and rendering logic look correct for the per-column approach.
993-1006: LGTM! TetrixEffect drops allocation aligned with per-column state.The drops array is correctly allocated for
layer->size.xcolumns (line 994) andnrOfDropsis set to match (line 997). Each column will maintain its own drop state, which is consistent with the 2D axis realignment.
1737-1756: RainEffect: Per-column drops allocation is correct for new axis orientation.The drops array is now allocated per column (
layer->size.xat line 1743) instead of per row, which aligns with the PR's goal of having each column maintain its own drop state. The initialization loop (lines 1748-1752) correctly iterates over x and initializes each column's drop.Note: The retrieved learning about
dropsSize = layer->size.yapplies to the OLD code structure. The NEW per-column approach usingsize.xis correct for this refactoring.
1818-1832: DripEffect: Per-column drops allocation is correct.The drops array is correctly allocated as
[layer->size.x][maxNumDrops](line 1819), withnrOfDropsset tolayer->size.x(line 1823). Each column maintains up tomaxNumDropsdrops independently. The initialization loop (lines 1824-1828) correctly iterates over both dimensions.
1965-2010: DJLightEffect: mid calculation uses size.y - verify this aligns with effect intent.Line 1966 calculates
mid = layer->size.y / 2, and the effect usesCoord3D(0, mid)for pixel access (line 2006) and shifts within column 0 (lines 2008-2009). This changes the effect from operating horizontally across x to operating vertically along y. Verify this is the intended behavior for the "DJLight" effect.The change appears consistent with the PR's goal of reorienting 1D effects to the y-axis.
1663-1721: mode_fireworks signature verified—all callers correctly pass x parameter.The function signature change to include the x parameter is correctly implemented. RainEffect (the only caller at line 1789) properly passes x as the loop variable from the
for (int x = 0; x < nrOfDrops; x++)loop at line 1762, and all LED access usingCoord3D(x, ...)is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/MoonLight/Nodes/Effects/E_WLED.h (2)
1774-1783: Critical: Column-shift logic reads from wrong column and resets wrong field.This is the same issue flagged in the previous review. Two problems remain:
- Lines 1774-1777: The pixel shift reads from column 0 (
Coord3D(0, ...)) instead of the current columnx, causing all columns to display the same shifted pixels.- Line 1783: Resets
drops[x].posinstead ofdrops[x].colwhendrops[x].col <= 0.🔎 Proposed fix
{ // shift all leds down - CRGB ctemp = layer->getRGB(Coord3D(x, 0)); + CRGB ctemp = layer->getRGB(Coord3D(x, 0)); for (int i = 0; i < layer->size.y - 1; i++) { - layer->setRGB(Coord3D(x, layer->size.y - 1 - i), layer->getRGB(Coord3D(0, layer->size.y - 1 - (i + 1)))); + layer->setRGB(Coord3D(x, layer->size.y - 1 - i), layer->getRGB(Coord3D(x, layer->size.y - 1 - (i + 1)))); } layer->setRGB(Coord3D(x, 0), ctemp); // wrap around drops[x].pos++; // increase spark index drops[x].col++; } if (drops[x].pos <= 0) drops[x].pos = UINT16_MAX; // reset previous spark position - if (drops[x].col <= 0) drops[x].pos = UINT16_MAX; // reset previous spark position + if (drops[x].col <= 0) drops[x].col = UINT16_MAX; // reset previous spark position if (drops[x].pos >= layer->size.y) drops[x].pos = 0; // ignore
1017-1019: Verify drops array bounds in TetrixEffect loop.This issue was flagged in the previous review. The loop iterates over
nrOfDropswhich equalslayer->size.xafter successful allocation, but if reallocation fails or is incomplete, accesses todrops[x]could exceed the allocated size.Consider using the defensive pattern from BouncingBallsEffect at line 59:
for (int x = 0; x < MIN(layer->size.x, nrOfDrops); x++) {🔎 Proposed fix
- for (int x = 0; x < nrOfDrops; x++) { + for (int x = 0; x < MIN(layer->size.x, nrOfDrops); x++) { if (drops[x].step == 0) { // init brick
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonLight/Nodes/Effects/E_WLED.h
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-18T15:15:57.828Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:15:57.828Z
Learning: In DripEffect (E_WLED.h), dropsSize must be set to layer->size.y (not layer->size.x) in onSizeChanged(), because the drops array is allocated as [layer->size.y][maxNumDrops]. Using layer->size.x causes out-of-bounds access when iterating over y-coordinates.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-18T15:59:30.556Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:59:30.556Z
Learning: In DripEffect (E_WLED.h), the bounce fails because the damped velocity is too weak to keep the drop at pos > 0 for multiple frames. When the drop returns to pos <= 0 while still in the bouncing state (colIndex == bouncing), it immediately resets to init (line 1663-1664) before any visible bounce occurs. The bounce velocity must be strong enough (divide by 1.1 or less, or use minimal damping like *= 0.9) so the drop stays airborne long enough for a visible bounce arc before the eventual second ground hit triggers the reset.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-18T15:46:48.319Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:46:48.319Z
Learning: In DripEffect (E_WLED.h), the bounce damping at line 1669 needs to be `/1.5` or less (not `/4` or `/2`) to create a visible bounce effect. The continuous gravity application every frame (line 1652) requires a strong initial bounce velocity to overcome it. Using `/1.5` gives approximately 67% energy return, making the bounce visible and lasting for ~33 frames.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
📚 Learning: 2025-12-18T15:30:02.486Z
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-18T15:30:02.486Z
Learning: In DripEffect (E_WLED.h), the bounce at line 1669 uses heavy damping (`vel = -vel / 4`), which makes the bounce too weak to be visible. Reducing the damping factor from 4 to 2 (or lower) makes the bounce more pronounced and visible.
Applied to files:
src/MoonLight/Nodes/Effects/E_WLED.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary by CodeRabbit
Version Update
Improvements
Behavior Changes
✏️ Tip: You can customize this high-level summary in your review settings.