Lots of effects #77
Lots of effects #77
Conversation
Migrated from StarLight, different origins, WLED etc
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
You can disable this status message by setting the Note
|
| Cohort / File(s) | Summary |
|---|---|
Documentation docs/moonlight/overview.md, README.md, docs/index.md |
Emoji presentation tweaks: variation selector added for the lightning emoji (e.g., "⚡️ FastLED origin") in docs/README. |
UI Styling interface/src/lib/components/moonbase/FieldRenderer.svelte |
Increased Coord3D numeric input widths by switching from a calc-based width to a fixed offset; presentation-only. |
Module Effects / Registration src/MoonLight/Modules/ModuleEffects.h |
Switched default layer control index to 0-based (index 0 = Layer 1). Expanded/reordered effect lists and extended addNode/addNodes and name-matching to instantiate many new effects across MoonLight, MoonModules, WLED, and FastLED. |
FastLED Effects src/MoonLight/Nodes/Effects/E_FastLED.h |
Added RainbowEffect class with speed/deltaHue/chanceOfGlitter controls, hue progression and glitter logic. |
MoonLight Effects src/MoonLight/Nodes/Effects/E_MoonLight.h |
Removed prior RainbowEffect; added many new effects and helpers (FireEffect, VUMeterEffect, PixelMapEffect, MarioTestEffect, RingEffect, RingRandomFlowEffect, AudioRingsEffect, etc.), global color tables, isqrt utility, palette-access adjustments, lifecycle hooks, and new public controls/members. |
WLED Effects src/MoonLight/Nodes/Effects/E_WLED.h |
Added public BlackholeEffect and DNAEffect classes with controls and setup/loop implementations; adjusted palette access to use public layerP. |
Sandbox Effects src/MoonLight/Nodes/Effects/E__Sandbox.h |
Removed BlackholeEffect from sandbox; renamed ExampleEffect::name() to "Example". |
Physical Layer Defaults src/MoonLight/Layers/PhysicalLayer.h |
Changed LightsHeader.lightPreset default from UINT8_MAX to 2 (comment notes default GRB). |
Virtual Layer API src/MoonLight/Layers/VirtualLayer.h |
Changed signature void blendColor(Coord3D& position, ...) → void blendColor(Coord3D position, ...) (pass-by-value). |
Nodes Cleanup src/MoonBase/Nodes.cpp |
Removed explicit base destructor call in LiveScriptNode::~LiveScriptNode() to rely on implicit base destruction. |
Driver / Layout / Modifier Names src/MoonLight/Nodes/Drivers/D__Sandbox.h, src/MoonLight/Nodes/Layouts/L__Sandbox.h, src/MoonLight/Nodes/Modifiers/M__Sandbox.h |
Normalized example static name() return values to "Example" for driver/layout/modifier. |
Build Config platformio.ini |
Bumped build flag APP_DATE from 20251216 to 20251217. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
- Areas needing close inspection:
src/MoonLight/Modules/ModuleEffects.h— default index change, effect ordering, factory matching, and addNode/addNodes changes.src/MoonLight/Nodes/Effects/E_MoonLight.h— many new classes, memory allocations, palette usage, lifecycle hooks, and utilities (isqrt, glow).src/MoonLight/Nodes/Effects/E_WLED.h&src/MoonLight/Nodes/Effects/E__Sandbox.h— Blackhole moved/removed; check for duplicate registrations or name collisions.src/MoonLight/Layers/VirtualLayer.h— callers affected by pass-by-value change.src/MoonLight/Layers/PhysicalLayer.h— impact oflightPresetdefault change.
Possibly related PRs
- rename UI components #75 — touches
interface/src/lib/components/moonbase/FieldRenderer.svelte; related to the numeric input width/UI edits. - Board presets p4 #74 — touches
LightsHeader/lightPresetdefaults insrc/MoonLight/Layers/PhysicalLayer.h; related to the default change. - Board presets more presets #71 — modifies module layer-selection logic in
ModuleEffects.h; related to switching to a 0-based default and effect registration changes.
Suggested labels
enhancement
Poem
🐰✨ I hopped into code with a carrot so bright,
New fires and rings that shimmer at night.
Rainbows and pixels now swirl in a row,
Blackholes relocated where curious things go.
Hooray — a rabbit-approved glowing show!
Pre-merge checks and finishing touches
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 22.60% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
|
| Title check | ❓ Inconclusive | The title "Lots of effects" is vague and generic, using a non-descriptive term that doesn't convey meaningful information about the specific changes in the changeset. | Consider using a more specific title that highlights the main changes, such as "Add new visual effects (Fire, VUMeter, PixelMap, etc.) and refactor effect handling" or "Introduce multiple new effects and improve effect module structure". |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
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: 20
🧹 Nitpick comments (4)
src/MoonLight/Nodes/Effects/E_WLED.h (1)
778-783: Missingoverridekeyword ononUpdatemethod.The
onUpdatemethod should have theoverridespecifier to ensure it correctly overrides the base class method.- void onUpdate(const Char<20>& oldValue, const JsonObject& control) { + void onUpdate(const Char<20>& oldValue, const JsonObject& control) override {src/MoonLight/Nodes/Effects/E_FastLED.h (2)
23-26: Missingoverridekeyword onsetup()method.The
setup()method should have theoverridespecifier to match the pattern used inRainbowWithGlitterEffectand other effect classes.- void setup() { + void setup() override {
58-62: Consider usinglayer->nrOfLightsfor 1D effects.The
addGlitterfunction usesrandom16(layer->size.x)which only considers the x dimension. For a 1D effect, usinglayer->nrOfLightswould be more accurate for the full addressable range.void addGlitter(fract8 chanceOfGlitter) { if (random8() < chanceOfGlitter) { - layer->setRGB(random16(layer->size.x), CRGB::White); + layer->setRGB(random16(layer->nrOfLights), CRGB::White); } }src/MoonLight/Nodes/Effects/E_MoonLight.h (1)
1373-1384: Missingoverridekeywords on FireEffect methods.Both
setup()(line 1373) andloop()(line 1384) are missing theoverridespecifier.- void setup() { + void setup() override { addControl(usePalette, "usePalette", "checkbox"); ... } - void loop() { + void loop() override {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/moonlight/overview.md(1 hunks)interface/src/lib/components/moonbase/FieldRenderer.svelte(3 hunks)src/MoonLight/Modules/ModuleEffects.h(7 hunks)src/MoonLight/Nodes/Effects/E_FastLED.h(1 hunks)src/MoonLight/Nodes/Effects/E_MoonLight.h(1 hunks)src/MoonLight/Nodes/Effects/E_WLED.h(3 hunks)src/MoonLight/Nodes/Effects/E__Sandbox.h(0 hunks)
💤 Files with no reviewable changes (1)
- src/MoonLight/Nodes/Effects/E__Sandbox.h
🧰 Additional context used
🧬 Code graph analysis (4)
src/MoonLight/Nodes/Effects/E_FastLED.h (2)
src/MoonBase/NodeManager.h (1)
Node(80-80)src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonLight/Modules/ModuleEffects.h (3)
src/MoonLight/Nodes/Effects/E_FastLED.h (1)
RainbowEffect(14-33)src/MoonLight/Nodes/Effects/E_WLED.h (1)
DJLightEffect(1357-1425)src/MoonBase/Char.h (1)
equalAZaz09(171-184)
src/MoonLight/Nodes/Effects/E_MoonLight.h (2)
src/MoonBase/Nodes.h (2)
Node(66-181)Node(237-237)src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
src/MoonLight/Nodes/Effects/E_WLED.h (3)
src/MoonLight/Nodes/Effects/E__Sandbox.h (2)
onUpdate(35-41)onSizeChanged(33-33)src/MoonBase/Modules/ModuleIO.h (1)
onUpdate(523-547)src/MoonLight/Nodes/Drivers/D__Sandbox.h (1)
onUpdate(31-37)
🪛 markdownlint-cli2 (0.18.1)
docs/moonlight/overview.md
131-131: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ 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 (4)
interface/src/lib/components/moonbase/FieldRenderer.svelte (1)
237-237: LGTM! Width formula now consistent with number inputs.The coord3D inputs now use the same width calculation as the standard number type (line 187), improving UI consistency across numeric input fields.
Also applies to: 246-246, 255-255
src/MoonLight/Modules/ModuleEffects.h (3)
63-64: Good clarification of 0-based indexing.The comment clarifying that index 0 refers to Layer 1 is helpful for maintainability.
76-136: Effect registration looks well-organized.The new effects are properly categorized (MoonLight, WLED, FastLED sections) and follow alphabetical ordering within sections. The registration in
addNodesmatches the instantiation logic inaddNode.
167-275: Effect instantiation matches registration.The
addNodemethod properly handles all newly registered effects with correct class instantiation usingallocMBObject.
docs/moonlight/overview.md
Outdated
| * 🎨 Using palette | ||
| * 🐙 WLED origin | ||
| * 💫 MoonLight origin | ||
| * ⚡: FastLED origin |
There was a problem hiding this comment.
Fix list indentation to match parent list style.
The static analysis tool flagged incorrect indentation. This line uses 4-space indentation but should use 2-space indentation to be consistent with sibling items like "🎨 Using palette" on line 128.
- * ⚡: FastLED origin
+ * ⚡: FastLED origin📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * ⚡: FastLED origin | |
| * ⚡: FastLED origin |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
131-131: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🤖 Prompt for AI Agents
In docs/moonlight/overview.md around line 131, the list item "* ⚡: FastLED
origin" is indented with 4 spaces but should use 2-space indentation to match
the parent list style and sibling items (e.g., "🎨 Using palette" at line 128);
change the leading indentation from 4 spaces to 2 spaces so the item aligns with
the other top-level list entries.
| if (layer->getRGB(Coord3D(j, layer->size.y - 1 - i)) < usePalette ? ColorFromPalette(layerP.palette, n * 23) : colors[n]) { // can only get brighter | ||
| layer->setRGB(Coord3D(j, layer->size.y - 1 - i), usePalette ? ColorFromPalette(layerP.palette, n * 23) : colors[n]); // 23*11 -> within palette range |
There was a problem hiding this comment.
Inconsistent palette access in FireEffect::glow.
Lines 1351-1352 use layerP.palette instead of layer->layerP->palette.
- if (layer->getRGB(Coord3D(j, layer->size.y - 1 - i)) < usePalette ? ColorFromPalette(layerP.palette, n * 23) : colors[n]) { // can only get brighter
- layer->setRGB(Coord3D(j, layer->size.y - 1 - i), usePalette ? ColorFromPalette(layerP.palette, n * 23) : colors[n]); // 23*11 -> within palette range
+ if (layer->getRGB(Coord3D(j, layer->size.y - 1 - i)) < usePalette ? ColorFromPalette(layer->layerP->palette, n * 23) : colors[n]) { // can only get brighter
+ layer->setRGB(Coord3D(j, layer->size.y - 1 - i), usePalette ? ColorFromPalette(layer->layerP->palette, n * 23) : colors[n]); // 23*11 -> within palette range🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Effects/E_MoonLight.h around lines 1351-1352, the code
uses layerP.palette but the correct member is accessed via the layer pointer;
change both occurrences of layerP.palette to layer->layerP->palette, and guard
access by ensuring layer->layerP is non-null before using it (or assert/handle
null) so palette lookup is safe.
| for (int x = 0; x < layer->size.x; ++x) { | ||
| CRGB i = layer->getRGB(Coord3D(x, layer->size.y - 1)); // - 0; //-0 to force conversion to CRGB | ||
| if (i != CRGB::Black) { | ||
| layer->setRGB(Coord3D(x, layer->size.y - 1), usePalette ? ColorFromPalette(layerP.palette, random8()) : colors[random(NCOLORS - 6, NCOLORS - 2)]); |
There was a problem hiding this comment.
Inconsistent palette access in FireEffect::loop.
Line 1414 uses layerP.palette instead of layer->layerP->palette.
- layer->setRGB(Coord3D(x, layer->size.y - 1), usePalette ? ColorFromPalette(layerP.palette, random8()) : colors[random(NCOLORS - 6, NCOLORS - 2)]);
+ layer->setRGB(Coord3D(x, layer->size.y - 1), usePalette ? ColorFromPalette(layer->layerP->palette, random8()) : colors[random(NCOLORS - 6, NCOLORS - 2)]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| layer->setRGB(Coord3D(x, layer->size.y - 1), usePalette ? ColorFromPalette(layerP.palette, random8()) : colors[random(NCOLORS - 6, NCOLORS - 2)]); | |
| layer->setRGB(Coord3D(x, layer->size.y - 1), usePalette ? ColorFromPalette(layer->layerP->palette, random8()) : colors[random(NCOLORS - 6, NCOLORS - 2)]); |
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Effects/E_MoonLight.h around line 1414, the call uses
layerP.palette instead of accessing the palette via the layer object; update the
expression to use layer->layerP->palette so it consistently references the
palette on the layer, and add a brief null-check (or assume non-null) for
layer->layerP before use to avoid potential dereference errors.
| if (sparks[i].pos > 0 && sparks[i].pos < rows) { | ||
| if (!(sparks[i].posX >= 0 && sparks[i].posX < cols)) continue; | ||
| uint16_t prog = sparks[i].col; | ||
| CRGB spColor = ColorFromPalette(layerP.palette, sparks[i].colIndex); |
There was a problem hiding this comment.
Inconsistent palette access in FireworksEffect.
Line 999 uses layerP.palette instead of layer->layerP->palette.
- CRGB spColor = ColorFromPalette(layerP.palette, sparks[i].colIndex);
+ CRGB spColor = ColorFromPalette(layer->layerP->palette, sparks[i].colIndex);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CRGB spColor = ColorFromPalette(layerP.palette, sparks[i].colIndex); | |
| CRGB spColor = ColorFromPalette(layer->layerP->palette, sparks[i].colIndex); |
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Effects/E_WLED.h around line 999, the FireworksEffect
mistakenly accesses the palette via `layerP.palette` instead of using the
instance's pointer; update the access to `layer->layerP->palette` (or the
correct member path `layer->layerP->palette`) so the code consistently reads the
palette from the layer instance rather than an undefined/incorrect `layerP`
variable.
| layer->fill_solid(ColorFromPalette(layerP.palette, -counter)); | ||
|
|
||
| for (int z = 0; z < zones; z++) { | ||
| uint16_t pos = offset + z * zoneLen; | ||
| for (int i = 0; i < zoneLen; i++) { | ||
| uint8_t colorIndex = (i * 255 / zoneLen) - counter; | ||
| uint16_t led = (z & 0x01) ? i : (zoneLen - 1) - i; | ||
| layer->setRGB(pos + led, ColorFromPalette(layerP.palette, colorIndex)); |
There was a problem hiding this comment.
Inconsistent palette access in FlowEffect.
Lines 1104 and 1111 use layerP.palette instead of layer->layerP->palette.
- layer->fill_solid(ColorFromPalette(layerP.palette, -counter));
+ layer->fill_solid(ColorFromPalette(layer->layerP->palette, -counter));
for (int z = 0; z < zones; z++) {
uint16_t pos = offset + z * zoneLen;
for (int i = 0; i < zoneLen; i++) {
uint8_t colorIndex = (i * 255 / zoneLen) - counter;
uint16_t led = (z & 0x01) ? i : (zoneLen - 1) - i;
- layer->setRGB(pos + led, ColorFromPalette(layerP.palette, colorIndex));
+ layer->setRGB(pos + led, ColorFromPalette(layer->layerP->palette, colorIndex));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| layer->fill_solid(ColorFromPalette(layerP.palette, -counter)); | |
| for (int z = 0; z < zones; z++) { | |
| uint16_t pos = offset + z * zoneLen; | |
| for (int i = 0; i < zoneLen; i++) { | |
| uint8_t colorIndex = (i * 255 / zoneLen) - counter; | |
| uint16_t led = (z & 0x01) ? i : (zoneLen - 1) - i; | |
| layer->setRGB(pos + led, ColorFromPalette(layerP.palette, colorIndex)); | |
| layer->fill_solid(ColorFromPalette(layer->layerP->palette, -counter)); | |
| for (int z = 0; z < zones; z++) { | |
| uint16_t pos = offset + z * zoneLen; | |
| for (int i = 0; i < zoneLen; i++) { | |
| uint8_t colorIndex = (i * 255 / zoneLen) - counter; | |
| uint16_t led = (z & 0x01) ? i : (zoneLen - 1) - i; | |
| layer->setRGB(pos + led, ColorFromPalette(layer->layerP->palette, colorIndex)); |
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Effects/E_WLED.h around lines 1104 to 1111, the
FlowEffect uses layerP.palette incorrectly as if it were a direct member;
replace those uses with the pointer-based access layer->layerP->palette to match
surrounding code. Update the two occurrences (line ~1104 and ~1111) to use
layer->layerP->palette and ensure layer->layerP is valid where this code runs
(add a null check before use if one does not already exist).
| static void mode_fireworks(VirtualLayer* layer, uint16_t aux0, uint16_t aux1, uint8_t speed, uint8_t intensity, bool useAudio = false) { | ||
| // fade_out(0); | ||
| layer->fadeToBlackBy(10); | ||
| // if (call == 0) { | ||
| // aux0 = UINT16_MAX; | ||
| // aux1 = UINT16_MAX; | ||
| // } | ||
| bool valid1 = (aux0 < layer->size.x); | ||
| bool valid2 = (aux1 < layer->size.x); | ||
| CRGB sv1 = 0, sv2 = 0; | ||
| if (valid1) sv1 = layer->getRGB(aux0); | ||
| if (valid2) sv2 = layer->getRGB(aux1); | ||
|
|
||
| // WLEDSR | ||
| uint8_t blurAmount = 255 - speed; | ||
| uint8_t my_intensity = 129 - intensity; | ||
| bool addPixels = true; // false -> inhibit new pixels in silence | ||
| int soundColor = -1; // -1 = random color; 0..255 = use as palette index | ||
|
|
||
| // if (useAudio) { | ||
| // if (FFT_MajorPeak < 100) { blurAmount = 254;} // big blobs | ||
| // else { | ||
| // if (FFT_MajorPeak > 3200) { blurAmount = 1;} // small blobs | ||
| // else { // blur + color depends on major frequency | ||
| // float musicIndex = logf(FFT_MajorPeak); // log scaling of peak freq | ||
| // blurAmount = mapff(musicIndex, 4.60, 8.08, 253, 1);// map to blur range (low freq = more blur) | ||
| // blurAmount = constrain(blurAmount, 1, 253); // remove possible "overshot" results | ||
| // soundColor = mapff(musicIndex, 4.6, 8.08, 0, 255); // pick color from frequency | ||
| // } } | ||
| // if (sampleAgc <= 1.0) { // silence -> no new pixels, just blur | ||
| // valid1 = valid2 = false; // do not copy last pixels | ||
| // addPixels = false; | ||
| // blurAmount = 128; | ||
| // } | ||
| // my_intensity = 129 - (speed >> 1); // dirty hack: use "speed" slider value intensity (no idea how to _disable_ the first slider, but show the second one) | ||
| // if (samplePeak == 1) my_intensity -= my_intensity / 4; // inclease intensity at peaks | ||
| // if (samplePeak > 1) my_intensity = my_intensity / 2; // double intensity at main peaks | ||
| // } | ||
| // // WLEDSR end | ||
|
|
||
| layer->blur1d(blurAmount); | ||
| if (valid1) layer->setRGB(aux0, sv1); | ||
| if (valid2) layer->setRGB(aux1, sv2); | ||
|
|
||
| if (addPixels) { // WLEDSR | ||
| for (uint16_t i = 0; i < max(1, layer->size.x / 20); i++) { | ||
| if (random8(my_intensity) == 0) { | ||
| uint16_t index = random(layer->size.x); | ||
| if (soundColor < 0) | ||
| layer->setRGB(index, ColorFromPalette(layerP.palette, random8())); | ||
| else | ||
| layer->setRGB(index, ColorFromPalette(layerP.palette, soundColor + random8(24))); // WLEDSR | ||
| aux1 = aux0; | ||
| aux0 = index; | ||
| } | ||
| } | ||
| } | ||
| // return FRAMETIME; | ||
| } |
There was a problem hiding this comment.
Critical: layerP is not accessible in free function mode_fireworks.
This is a standalone function that receives VirtualLayer* layer as a parameter. The variable layerP is not in scope here. This will cause a compilation error on lines 1166 and 1168.
if (addPixels) { // WLEDSR
for (uint16_t i = 0; i < max(1, layer->size.x / 20); i++) {
if (random8(my_intensity) == 0) {
uint16_t index = random(layer->size.x);
if (soundColor < 0)
- layer->setRGB(index, ColorFromPalette(layerP.palette, random8()));
+ layer->setRGB(index, ColorFromPalette(layer->layerP->palette, random8()));
else
- layer->setRGB(index, ColorFromPalette(layerP.palette, soundColor + random8(24))); // WLEDSR
+ layer->setRGB(index, ColorFromPalette(layer->layerP->palette, soundColor + random8(24))); // WLEDSRCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Effects/E_WLED.h around lines 1117-1175, the free
function mode_fireworks references layerP.palette which is out of scope and
causes a compile error; fix by either (A) replacing layerP.palette with a
palette accessible from the passed VirtualLayer (e.g., layer->palette or
layer->getPalette(), whichever exists in VirtualLayer) or (B) modify the
function signature to accept the correct LayerParams/palette (e.g., add a
LayerParams& layerP or CRGBPalette16 palette parameter) and update all callers
to pass it, then use that palette when calling ColorFromPalette.
src/MoonLight/Nodes/Effects/E_WLED.h
Outdated
| drops[j].vel = 0; // speed | ||
| drops[j].col = sourcedrop; // brightness | ||
| drops[j].colIndex = 1; // drop state (0 init, 1 forming, 2 falling, 5 bouncing) | ||
| drops[j].velX = (uint32_t)ColorFromPalette(layerP.palette, random8()); // random color |
There was a problem hiding this comment.
Inconsistent palette access in DripEffect.
Line 1265 uses layerP.palette instead of layer->layerP->palette.
- drops[j].velX = (uint32_t)ColorFromPalette(layerP.palette, random8()); // random color
+ drops[j].velX = (uint32_t)ColorFromPalette(layer->layerP->palette, random8()); // random color📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| drops[j].velX = (uint32_t)ColorFromPalette(layerP.palette, random8()); // random color | |
| drops[j].velX = (uint32_t)ColorFromPalette(layer->layerP->palette, random8()); // random color |
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Effects/E_WLED.h around line 1265, the DripEffect code
incorrectly accesses the palette via layerP.palette; change this to use the
layer pointer's palette (layer->layerP->palette) so the effect reads the correct
palette from the layer context; update the expression where drops[j].velX is set
to call ColorFromPalette(layer->layerP->palette, random8()) (or equivalent
dereference consistent with surrounding code).
| // setRingFromFtt(leds, 2, 7); // set outer ring to bass | ||
| // setRingFromFtt(leds, 0, 8); // set outer ring to bass | ||
| for (int i = 0; i < layer->size.x; i++) { | ||
| layer->setRGB(i, ColorFromPalette(layerP.palette, ::map(i, 0, layer->size.x, 0, 255), 255 - (bri_lower >> 8))); |
There was a problem hiding this comment.
Inconsistent palette access in HeartBeatEffect.
Line 1352 uses layerP.palette instead of layer->layerP->palette.
- layer->setRGB(i, ColorFromPalette(layerP.palette, ::map(i, 0, layer->size.x, 0, 255), 255 - (bri_lower >> 8)));
+ layer->setRGB(i, ColorFromPalette(layer->layerP->palette, ::map(i, 0, layer->size.x, 0, 255), 255 - (bri_lower >> 8)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| layer->setRGB(i, ColorFromPalette(layerP.palette, ::map(i, 0, layer->size.x, 0, 255), 255 - (bri_lower >> 8))); | |
| layer->setRGB(i, ColorFromPalette(layer->layerP->palette, ::map(i, 0, layer->size.x, 0, 255), 255 - (bri_lower >> 8))); |
🤖 Prompt for AI Agents
In src/MoonLight/Nodes/Effects/E_WLED.h around line 1352, the HeartBeatEffect
uses layerP.palette but should access the palette through the layer pointer;
change the call to use layer->layerP->palette (i.e., replace layerP.palette with
layer->layerP->palette) so the effect consistently reads the palette from the
layer instance.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/MoonLight/Nodes/Effects/E_FastLED.h (2)
46-47: Consider specifying explicit bounds for the chance slider.While the default bounds (0-255) work for a probability value, explicitly setting
minandmaximproves clarity and matches typical probability ranges (e.g., 0-100 for percentage).Apply this diff to make the range explicit:
addControl(glitter, "glitter", "checkbox"); - addControl(chance, "chance", "slider"); + addControl(chance, "chance", "slider", 0, 255);Or use 0-100 for percentage-style control:
addControl(glitter, "glitter", "checkbox"); - addControl(chance, "chance", "slider"); + addControl(chance, "chance", "slider", 0, 100);(If using 0-100, scale it in
addGlitter:addGlitter(chance * 255 / 100))
53-53: Consider exposing rainbow animation parameters as controls.The rainbow animation uses hardcoded values (
millis() / 50for speed,7for deltaHue), whereasRainbowEffectexposes these as user-adjustable controls. This inconsistency limits customization.Consider adding
speedanddeltaHuecontrols similar toRainbowEffectfor a consistent user experience across rainbow-based effects.src/MoonLight/Nodes/Effects/E_MoonLight.h (2)
1598-1598: Fix typo in comment.The comment contains "ruing" which should be "ring".
Apply this diff:
- hue[r] = hue[(r - 1)]; // set this ruing based on the inner + hue[r] = hue[(r - 1)]; // set this ring based on the inner
1332-1332: Consider scoping the global colors array or using a more specific name.The generic name
colorsat file scope could conflict with future additions. Consider making it a static member ofFireEffector renaming it tofireColors.Option 1 - Rename for clarity:
-const uint32_t colors[] = {0x000000, 0x100000, 0x300000, 0x600000, 0x800000, 0xA00000, 0xC02000, 0xC04000, 0xC06000, 0xC08000, 0x807080}; +const uint32_t fireColors[] = {0x000000, 0x100000, 0x300000, 0x600000, 0x800000, 0xA00000, 0xC02000, 0xC04000, 0xC06000, 0xC08000, 0x807080};(Then update references in FireEffect from
colorstofireColors)Option 2 - Move into FireEffect as a static member for better encapsulation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(2 hunks)docs/index.md(2 hunks)docs/moonlight/overview.md(1 hunks)src/MoonLight/Nodes/Effects/E_FastLED.h(1 hunks)src/MoonLight/Nodes/Effects/E_MoonLight.h(8 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/index.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/moonlight/overview.md
🧰 Additional context used
🧬 Code graph analysis (1)
src/MoonLight/Nodes/Effects/E_FastLED.h (2)
src/MoonBase/NodeManager.h (1)
Node(80-80)src/MoonBase/Module.cpp (2)
addControl(361-370)addControl(361-361)
⏰ 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 (3)
src/MoonLight/Nodes/Effects/E_FastLED.h (1)
14-33: LGTM!The hue increment arithmetic is correct. With
speedcapped at 32, the expression(hue += speed * 32) >> 8safely accumulates sub-pixel timing in a uint16_t before converting back to uint8_t for the rainbow fill.src/MoonLight/Nodes/Effects/E_MoonLight.h (2)
1360-1365: LGTM!The integer square root implementation is a classic recursive algorithm and appears correct for the uint32_t range.
1512-1556: LGTM!The Mario bitmap rendering with offset controls and boundary checks is well implemented.
Back End ======== - Remove Node::~Node() - WLED effects: add Tetrix and PacMan
|
@coderabbitai, why do you say Review completed? I added 2 commits (0364c17 and 0dca0ab), can you also review these? |
|
@ewowi, you're absolutely right! I can see that you've added two new commits to the PR. I'll review these changes now. ✅ Actions performedReview triggered.
|
|
@coderabbitai, regarding
Is that still in progress? when do you expect to deliver? |
🧩 Analysis chain🏁 Script executed: Length of output: 20369 @ewowi, I apologize for the delay! I can see from the PR activity that my last review was posted on December 17th for commit Let me trigger a fresh review now to ensure those changes are properly analyzed. ✅ Actions performedReview triggered.
|
|
@coderabbitai, I added 2 new effects in E_WLED.h: PacManEffect and TetrixEffect. Can you review them and look especially at memory management and algorithm used? For the tetrix effect, it is suppose to run multiple 1D Tetris lines (lines on the x-axis), multiple versions on the y-axis, like is done with BouncingBalls effect. For some reason it is only running one line over all the leds |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 81818 🏁 Script executed: Length of output: 57 🏁 Script executed: Length of output: 1075 @ewowi, I've analyzed both effects with a focus on memory management and the algorithm. Here's my detailed review: PacManEffect ✅Memory Management: Excellent
Algorithm: Looks solid overall. The effect tracks PacMan, ghosts, and power dots with proper state management. TetrixEffect
|
|
@coderabbitai, can you check my latest commit especially on reallocMB robustness? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 17446 @ewowi, excellent work on the reallocMB robustness! I've reviewed commit ✅ Robustness Pattern AppliedYou've consistently implemented the correct pattern: Type* newAlloc = reallocMB<Type>(ptr, newSize);
if (newAlloc) {
ptr = newAlloc;
sizeTracking = newSize;
} else {
EXT_LOGE(ML_TAG, "(re)allocate failed"); // keep old allocation
}🎯 Key Improvements by EffectBouncingBallsEffect ✅
GEQEffect ✅
TetrixEffect ✅
OctopusEffect ✅
PacManEffect ✅
🛡️ What This Prevents
💯 Overall AssessmentExcellent robustness! The pattern is:
The TetrixEffect fixes are also spot-on—all instances of 🎉🐰 |
Back end ======= - WLED effects: Tetrix slower - WLED effects: Drip: remove invert (we have modifiers), add multucolumn, color hack/fix
|
@coderabbitai, Drip has some weird colors on pos == 0, see 
Do you know why that is? I know it is related to layer->setRGB(Coord3D(0, y), blend(dropColor, CRGB::Black, drops[y][j].col)); because when I comment it, there is are no colors - but there needs to be a splash of water in the same color as the drip. what is strange is that it happens only for larger y values |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9282 @ewowi, I found the bug! 🎯 Root Cause: Array Bounds IssueLine 1606 is the culprit: dropsSize = layer->size.x; // ❌ WRONG!Here's what's happening:
For example:
When you hit line 1660 for these invalid y values: layer->setRGB(Coord3D(0, y), blend(dropColor, CRGB::Black, drops[y][j].col));
The FixLine 1606 - change to: dropsSize = layer->size.y; // ✅ CORRECT!This matches the allocation size and ensures the loop only accesses valid array indices. Why It Appears Only for Larger Y ValuesBecause only y-indices beyond Let me know once you've fixed it, and I can verify the change! 🐰 ✏️ Learnings added
|
|
@coderabbitai, nice catch, I will fix that but it isn't the problem, as I tested with square matrices (x==y). Those weird colors are the first you see when the effect starts and they stay they sometimes dissappear when the effect is running but are also coming back, please investigate |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13798 @ewowi, excellent point about testing with square matrices! I found the actual bug - it's uninitialized memory in the Root Cause: Uninitialized MemoryLine 1602-1609 allocates the drops array but never initializes it: void onSizeChanged(const Coord3D& prevSize) override {
Spark(*newAlloc)[maxNumDrops] = reallocMB<Spark[maxNumDrops]>(drops, layer->size.y);
if (newAlloc) {
drops = newAlloc;
dropsSize = layer->size.x; // (also needs to be size.y, but separate issue)
} else {
EXT_LOGE(ML_TAG, "(re)allocate drops failed");
}
// ❌ NO INITIALIZATION OF drops ARRAY!
}Here's what happens:
This explains:
The FixInitialize the drops array after allocation in void onSizeChanged(const Coord3D& prevSize) override {
Spark(*newAlloc)[maxNumDrops] = reallocMB<Spark[maxNumDrops]>(drops, layer->size.y);
if (newAlloc) {
drops = newAlloc;
dropsSize = layer->size.y; // ✅ ALSO FIX THIS (was size.x)
// ✅ ADD THIS: Initialize all drops to init state
for (int y = 0; y < layer->size.y; y++) {
for (int j = 0; j < maxNumDrops; j++) {
drops[y][j].colIndex = init; // Set to 0 so loop() will initialize properly
}
}
} else {
EXT_LOGE(ML_TAG, "(re)allocate drops failed");
}
}Alternatively, you can use memset(drops, 0, sizeof(Spark[maxNumDrops]) * layer->size.y);This ensures every drop starts in the 🧠 Learnings used |
|
@coderabbitai, thx that solved the problem. One more thing, I don't see the bouncing happening , see if (drops[y][j].colIndex == falling) { // init bounce what is wrong there? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI need to generate shell scripts to read the file. Let me correct my approach: 🏁 Script executed: Length of output: 11658 @ewowi, I found the bounce issue! 🎯 Root Cause: Over-Damped BounceLine 1669 is the problem: drops[y][j].vel = -drops[y][j].vel / 4; // divide by 4 = heavy damping!Here's what happens:
The FixReduce the damping from if (drops[y][j].colIndex == falling) { // init bounce
drops[y][j].vel = -drops[y][j].vel / 2; // ✅ CHANGED: /2 instead of /4
drops[y][j].pos += drops[y][j].vel;
}This gives the drop twice the bounce velocity, making the bounce visible:
Alternatively, if you want even more dramatic bounces, you can use Let me know if this fixes it! 🐰 ✏️ Learnings added
🧠 Learnings used |
|
@coderabbitai, regarding
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13822 @ewowi, you're absolutely right! The problem is that gravity is too strong relative to the bounce velocity. Here's what's happening: The Physics ProblemWhen the drop bounces (line 1668-1670): drops[y][j].vel = -drops[y][j].vel / 2; // velocity after bounce ≈ +2.5
drops[y][j].pos += drops[y][j].vel; // moves up slightlyBut then every single frame (line 1652): drops[y][j].vel += gravity; // gravity ≈ -0.05 to -0.1 (negative!)So here's what happens frame-by-frame:
The bounce happens, but it's too small and fast to see! After ~20-25 frames (less than a second), it's falling again. The Fix: Much Less DampingLine 1669 - reduce damping from if (drops[y][j].colIndex == falling) { // init bounce
drops[y][j].vel = -drops[y][j].vel / 1.5; // ✅ CHANGED: /1.5 for stronger bounce
drops[y][j].pos += drops[y][j].vel;
}This gives 67% energy return instead of 50%:
Alternatively, for even more dramatic bounce, use drops[y][j].vel = -drops[y][j].vel / 1.2; // 83% energy returnOr for almost elastic bounce, just reverse without damping: drops[y][j].vel = -drops[y][j].vel * 0.9; // 90% energy returnTry ✏️ Learnings added
🧠 Learnings used |
|
@coderabbitai, nope won't help, whatever I try it stays at the bottom, I think it is related to |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 15855 @ewowi, excellent catch! 🎯 You've identified the exact problem! The state machine is indeed the issue. Let me trace through what's happening: The State Machine BugHere's the sequence of events: Frame 1 (First hit):
Frame 2 (Immediate problem):
Frame ~15-20 (Falls back too fast):
The bounce velocity is so weak that the drop can't sustain flight - it immediately falls back to The SolutionThe bounce velocity needs to be dramatically stronger so the drop stays airborne ( Change line 1669 to remove most/all damping: if (drops[y][j].colIndex == falling) { // init bounce
drops[y][j].vel = -drops[y][j].vel / 1.1; // ✅ Almost no damping (90% energy return)
drops[y][j].pos += drops[y][j].vel;
}Or even stronger (no damping at all): drops[y][j].vel = -drops[y][j].vel * 0.95; // 95% energy returnThis way:
The key insight: The drop must stay at Try ✏️ Learnings added
🧠 Learnings used |
|
@coderabbitai, regarding
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 19289 @ewowi, I found it! 🎯🐰 The bounce physics is actually working perfectly - the problem is the bouncing drop is INVISIBLE! The Bug: Missing Pixel RenderingWhen the drop is falling or bouncing with Here's the proof: Line 1658-1661 - Trail rendering: for (int i = 1; i < 7 - drops[y][j].colIndex; i++) { // ← i starts at 1, not 0!
uint16_t pos = constrain(uint16_t(drops[y][j].pos) + i, 0, layer->size.x - 1);
layer->setRGB(Coord3D(pos, y), blend(CRGB::Black, dropColor, drops[y][j].col / i));
}Notice Line 1643 - Main pixel only drawn when forming: if (drops[y][j].colIndex == forming) {
layer->setRGB(Coord3D(drops[y][j].pos, y), blend(CRGB::Black, dropColor, drops[y][j].col));
// ...
}Once the drop starts falling (colIndex = falling) or bouncing (colIndex = bouncing), this code no longer executes, so the main pixel disappears! The FixAdd the main drop pixel rendering inside the if (drops[y][j].colIndex > forming) { // falling
if (drops[y][j].pos > 0) { // fall until end of layer
drops[y][j].pos += drops[y][j].vel;
if (drops[y][j].pos < 0) drops[y][j].pos = 0;
drops[y][j].vel += gravity; // gravity is negative
// ✅ ADD THIS: Draw the main drop pixel
layer->setRGB(Coord3D(uint16_t(drops[y][j].pos), y),
blend(CRGB::Black, dropColor, drops[y][j].col));
for (int i = 1; i < 7 - drops[y][j].colIndex; i++) { // trail pixels
uint16_t pos = constrain(uint16_t(drops[y][j].pos) + i, 0, layer->size.x - 1);
layer->setRGB(Coord3D(pos, y), blend(CRGB::Black, dropColor, drops[y][j].col / i));
}
if (drops[y][j].colIndex > falling) { // during bounce, some water is on the floor
layer->setRGB(Coord3D(0, y), blend(dropColor, CRGB::Black, drops[y][j].col));
}This will make the drop visible during both falling AND bouncing! 🎉 The bounce physics you've implemented is actually correct - the drop was bouncing all along, you just couldn't see it! 🐰 🧠 Learnings used |
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Style
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.