Skip to content

Conversation

@ewowi
Copy link
Collaborator

@ewowi ewowi commented Dec 23, 2025

Summary by CodeRabbit

  • Version Update

    • Released version 0.7.1
  • Improvements

    • Reoriented many effects to a column-major layout for improved visual alignment and consistency.
    • Fireworks and several motion effects updated for more accurate column-targeting and per-column behavior.
  • Behavior Changes

    • Blurring now defaults to column-wise operation (can be targeted).
    • Audio rings: rings slider removed; ring count is now derived from layer dimensions.

✏️ Tip: You can customize this high-level summary in your review settings.

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!
@coderabbitai
Copy link

coderabbitai bot commented Dec 23, 2025

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)
Validation error: Unrecognized key(s) in object: 'review', 'context'
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Walkthrough

Bumps app version/date, changes VirtualLayer::blur1d to accept an optional x parameter and run along layer->size.y (column-scoped), and reorients many effects/modifiers to a column-major (x-major) coordinate convention with added per-column state where applicable.

Changes

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 removes nrOfRings member and computes rings at runtime from layer->size.y.
Effects — WLED suite
src/MoonLight/Nodes/Effects/E_WLED.h
Broad axis reorientation: many effects (BouncingBalls, Tetrix, Rain, Drip, DJLight, Flow, Fireworks, etc.) switched primary iteration to size.x, coordinate writes adapted to Coord3D(...), per-column state added (e.g., Spark* drops, nrOfDrops) with destructors and onSizeChanged() reallocations; mode_fireworks() signature gained uint16_t x parameter.
Modifiers
src/MoonLight/Nodes/Modifiers/M_MoonLight.h
Axis swaps in modifiers: CircleModifier now maps distance to y, PinwheelModifier petal assignment flipped in 3D branch, RippleYZModifier loop order and source pixel swapped (x/y inversion); minor whitespace edits in TransposeModifier.

Sequence Diagram(s)

(No sequence diagram generated — changes are axis/structure reorientations and per-column state updates without a new multi-component sequential flow that warrants visualization.)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • Lots of effects #77 — Overlaps on axis/orientation changes and VirtualLayer/effects updates; likely touches the same mapping and effect logic.
  • V070 preps #79 — Related axis handling fixes for BouncingBalls/Tetrix onSizeChanged; directly relevant to per-column/axis reorientation.
  • Board presets p4 #74 — Touched VirtualLayer and build metadata; potentially related to the blur1d signature and platformio.ini version/date changes.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 1d-effect-y-axis

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8dfbc0a and f492075.

📒 Files selected for processing (1)
  • src/MoonLight/Nodes/Effects/E_WLED.h
🧰 Additional context used
🧠 Learnings (5)
📚 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
📚 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/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
🔇 Additional comments (8)
src/MoonLight/Nodes/Effects/E_WLED.h (8)

28-95: LGTM: BouncingBallsEffect correctly converted to per-column structure.

The effect now allocates one ball array per column (layer->size.x), with proper bounds checking at line 59 using MIN(layer->size.x, ballsSize). Each column's balls bounce independently using the correct coordinate mapping Coord3D(x, pos).


1017-1062: Verify: Past review comment about bounds checking may still apply.

The past review comment suggested using MIN(layer->size.x, nrOfDrops) in the loop condition (similar to BouncingBallsEffect at line 59). Currently line 1017 uses x < nrOfDrops without the MIN check. While nrOfDrops is set to layer->size.x on successful allocation (line 997), using MIN would be more defensive against edge cases.

🔎 Optional defensive improvement
-    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

1735-1754: LGTM: RainEffect correctly converted to per-column state.

The effect now allocates per-column drop state (Spark* drops) sized to layer->size.x with proper destructor cleanup and initialization. The dimension change from 1D to column-major 2D is well-implemented.


1811-1830: Note: DripEffect dimension changed from learning context.

The retrieved learning mentions dropsSize should be layer->size.y, but after the y-axis conversion in this PR, drops are now correctly allocated per-column (layer->size.x). This is intentional - each column now has its own set of drops falling along the y-axis. The loop at line 1842 correctly iterates x < nrOfDrops where nrOfDrops == layer->size.x.

Based on learnings, but noting the axis conversion changes the expected dimension.


1641-1657: LGTM: FlowEffect correctly converted to vertical (y-axis) flow.

The effect now calculates zones along layer->size.y (lines 1641-1646) and renders at Coord3D(0, pos + led), making it a single-column vertical effect. This aligns with the PR's goal of y-axis orientation.


1661-1719: LGTM: mode_fireworks correctly updated for per-column processing.

The function signature now accepts an x parameter (line 1661), and all coordinate accesses correctly use Coord3D(x, ...) (lines 1671-1672, 1702-1703, 1710-1712). The past review concern about hardcoded column 0 has been resolved.


1964-2010: LGTM: DJLightEffect correctly converted to vertical single-column effect.

The effect now calculates the midpoint from layer->size.y (line 1964) and renders/shifts pixels vertically at Coord3D(0, ...) (lines 2004-2007). The conversion to y-axis orientation is correct.


1938-1941: LGTM: HeartBeatEffect correctly converted to vertical single-column effect.

The effect now iterates over layer->size.y (line 1938) and renders at Coord3D(0, i) (line 1939), correctly implementing vertical heartbeat visualization.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 occasionally

The constrain call has swapped min/max parameters! It should be constrain(value, min, max) but this has constrain(value, max, min). This will always return either layer->size.y - 1 or 0, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0c3733e and f123b8c.

📒 Files selected for processing (5)
  • platformio.ini
  • src/MoonLight/Layers/VirtualLayer.h
  • src/MoonLight/Nodes/Effects/E_MoonLight.h
  • src/MoonLight/Nodes/Effects/E_WLED.h
  • src/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.h
  • src/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.h
  • src/MoonLight/Nodes/Effects/E_MoonLight.h
  • 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
🧬 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 the modifyPosition() 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.y and update all loop bounds to iterate over size.y instead of size.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) and ballsSize is 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.x columns (line 994) and nrOfDrops is 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.x at 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.y applies to the OLD code structure. The NEW per-column approach using size.x is 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), with nrOfDrops set to layer->size.x (line 1823). Each column maintains up to maxNumDrops drops 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 uses Coord3D(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 using Coord3D(x, ...) is correct.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Lines 1774-1777: The pixel shift reads from column 0 (Coord3D(0, ...)) instead of the current column x, causing all columns to display the same shifted pixels.
  2. Line 1783: Resets drops[x].pos instead of drops[x].col when drops[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 nrOfDrops which equals layer->size.x after successful allocation, but if reallocation fails or is incomplete, accesses to drops[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

📥 Commits

Reviewing files that changed from the base of the PR and between f123b8c and 8dfbc0a.

📒 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

@ewowi
Copy link
Collaborator Author

ewowi commented Dec 23, 2025

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Dec 23, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants