Brush walker fx - a matrix-inspired colorful 2D effect#5452
Brush walker fx - a matrix-inspired colorful 2D effect#5452
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a 2D "Brush Walker" matrix effect and an audio‑reactive variant, with per‑segment persistent Walker state (up to 32), spawn/conflict logic, timed stepping with fading/trails and palette stepping, and registers both effects with new PROGMEM metadata. (49 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (1)
usermods/user_fx/user_fx.cpp (1)
1403-1491: Extract the shared walker loop before these modes drift further.Both modes duplicate allocation, init, timing, fade, and render/update logic. The only real difference is the spawn predicate, and the bug above is already an example of the copies diverging.
Also applies to: 1504-1607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/user_fx/user_fx.cpp` around lines 1403 - 1491, The brush walker logic in mode_brushwalker duplicates allocation, initialization, timing, fade, spawn and update/render loops (see BrushWalker, SEGENV, SEGMENT, BrushWalkerCountActive, BrushWalkerTrySpawn), causing drift/bugs between modes; extract shared behavior into a small helper API: create AllocateOrInitWalkers(SEGENV, BrushWalker*, count) to handle SEGENV.allocateData and zero/init walkers, a ShouldStep(SEGENV, interval) timing helper to manage SEGENV.step, a CommonFade(SEGMENT, fadeRate) call, and a WalkersStepRender(BrushWalker*, count, cols, rows, palStep, SEGMENT, SEGCOLOR) that does palette/primary color selection, setPixelColorXY, movement, colorIndex increment and deactivation; modify mode_brushwalker and the other duplicated mode to call these helpers and only supply the spawn predicate (e.g., use BrushWalkerTrySpawn or alternate spawn logic) and per-mode parameters like spawnChance, maxWalkers, fadeRate, palStep and interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1569-1570: The code reads the peak flag from the wrong u_data
index; change the access in user_fx.cpp where isPeak is set (uint8_t isPeak =
*(uint8_t *)um_data->u_data[2];) to read from u_data[3] instead so the AR
variant uses the actual samplePeak flag (i.e., use um_data->u_data[3] when
dereferencing the peak byte).
---
Nitpick comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1403-1491: The brush walker logic in mode_brushwalker duplicates
allocation, initialization, timing, fade, spawn and update/render loops (see
BrushWalker, SEGENV, SEGMENT, BrushWalkerCountActive, BrushWalkerTrySpawn),
causing drift/bugs between modes; extract shared behavior into a small helper
API: create AllocateOrInitWalkers(SEGENV, BrushWalker*, count) to handle
SEGENV.allocateData and zero/init walkers, a ShouldStep(SEGENV, interval) timing
helper to manage SEGENV.step, a CommonFade(SEGMENT, fadeRate) call, and a
WalkersStepRender(BrushWalker*, count, cols, rows, palStep, SEGMENT, SEGCOLOR)
that does palette/primary color selection, setPixelColorXY, movement, colorIndex
increment and deactivation; modify mode_brushwalker and the other duplicated
mode to call these helpers and only supply the spawn predicate (e.g., use
BrushWalkerTrySpawn or alternate spawn logic) and per-mode parameters like
spawnChance, maxWalkers, fadeRate, palStep and interval.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a5def1e0-168b-4cc8-b4d1-cf91716aa8e0
📒 Files selected for processing (2)
platformio.iniusermods/user_fx/user_fx.cpp
There was a problem hiding this comment.
please undo change in platformio.ini, these are not related to your PR.
| strip.addEffect(255, &mode_ants, _data_FX_MODE_ANTS); | ||
| strip.addEffect(255, &mode_morsecode, _data_FX_MODE_MORSECODE); | ||
|
|
||
There was a problem hiding this comment.
this whitespace change should be reverted.
usermods/user_fx/user_fx.cpp
Outdated
|
For code readability, please adjust your coding style for opening/closing brackets. for example, use if (SEGMENT.palette > 0) {
c = SEGMENT.color_from_palette(w.colorIndex, false, PALETTE_SOLID_WRAP, 0);
} else {
c = SEGCOLOR(0);
}or even simpler if (SEGMENT.palette > 0) c = SEGMENT.color_from_palette(w.colorIndex, false, PALETTE_SOLID_WRAP, 0);
else c = SEGCOLOR(0);https://github.com/wled/WLED/tree/main?tab=contributing-ov-file#code-style |
|
Thanks for the quick replies! I've already made some fixes, and can hopefully convince VSC to not mess up the styles again while I work my way down the todo list. |
|
I ran you FX quickly yesterday, looks very much like the "matrix" FX to me but I only did a quick test. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
usermods/user_fx/user_fx.cpp (2)
1405-1411:⚠️ Potential issue | 🟠 MajorRead
samplePeakfromu_data[3].The AR spawn gate still dereferences
u_data[2], so it is reacting to the wrong usermod payload instead of the peak flag.🔧 Minimal fix
- if (um_data && - (*(uint8_t*)um_data->u_data[2] || hw_random8() < sensitivity)) + if (um_data && + (((*(uint8_t*)um_data->u_data[3]) != 0) || hw_random8() < sensitivity)) shouldSpawn = true;Based on learnings,
samplePeakshould be read as(*(uint8_t*)um_data->u_data[3]) != 0to stay compatible with AudioReactive's untypedum_datapayload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/user_fx/user_fx.cpp` around lines 1405 - 1411, The spawn gate is reading the wrong payload byte (u_data[2]) from the AudioReactive usermod; change the condition that checks the peak flag to read samplePeak from (*(uint8_t*)um_data->u_data[3]) != 0 instead of using u_data[2] (keep the existing cast to uint8_t and the same null-check flow around UsermodManager::getUMData, simulateSound, and shouldSpawn so the only change is replacing the u_data index to 3 when evaluating the peak).
1376-1379:⚠️ Potential issue | 🟡 MinorUse
FX_FALLBACK_STATICon the error exits.The unsupported-2D branch currently renders
SEGCOLOR(1), and the OOM path just returns. That makes Brush Walker behave differently from the other user FX and can leave stale pixels on allocation failure.🔁 Minimal fix
- if (!strip.isMatrix || !SEGMENT.is2D()) { - SEGMENT.fill(SEGCOLOR(1)); - return; - } + if (!strip.isMatrix || !SEGMENT.is2D()) FX_FALLBACK_STATIC; ... - if (!SEGENV.allocateData(sizeof(Walker) * absoluteMaxWalkers)) return; + if (!SEGENV.allocateData(sizeof(Walker) * absoluteMaxWalkers)) FX_FALLBACK_STATIC;Also applies to: 1386-1386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/user_fx/user_fx.cpp` around lines 1376 - 1379, The unsupported-2D and OOM error exits in Brush Walker currently render SEGCOLOR(1) or simply return, which can leave stale pixels; replace both early-exit paths to use FX_FALLBACK_STATIC so they match other user FX behavior: when !strip.isMatrix or !SEGMENT.is2D() and on any allocation/oom failure in the Brush Walker code paths, invoke FX_FALLBACK_STATIC (rather than SEGCOLOR/return) to perform the standardized fallback cleanup and exit; update the branches around the checks that reference strip.isMatrix, SEGMENT.is2D(), and any allocation failure returns to call FX_FALLBACK_STATIC.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1383-1387: You take a pointer to SEGENV.data in walkers before
ensuring the buffer exists or is reallocated; move the
reinterpret_cast<Walker*>(SEGENV.data) to after the allocation check so that
walkers is obtained only once allocateData(sizeof(Walker) * absoluteMaxWalkers)
has succeeded (and handle the failure by returning). Specifically, call
SEGENV.allocateData(...) first, then set Walker* walkers =
reinterpret_cast<Walker*>(SEGENV.data), and only then run the reset loop over
absoluteMaxWalkers to avoid dereferencing a stale or null pointer.
---
Duplicate comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1405-1411: The spawn gate is reading the wrong payload byte
(u_data[2]) from the AudioReactive usermod; change the condition that checks the
peak flag to read samplePeak from (*(uint8_t*)um_data->u_data[3]) != 0 instead
of using u_data[2] (keep the existing cast to uint8_t and the same null-check
flow around UsermodManager::getUMData, simulateSound, and shouldSpawn so the
only change is replacing the u_data index to 3 when evaluating the peak).
- Around line 1376-1379: The unsupported-2D and OOM error exits in Brush Walker
currently render SEGCOLOR(1) or simply return, which can leave stale pixels;
replace both early-exit paths to use FX_FALLBACK_STATIC so they match other user
FX behavior: when !strip.isMatrix or !SEGMENT.is2D() and on any allocation/oom
failure in the Brush Walker code paths, invoke FX_FALLBACK_STATIC (rather than
SEGCOLOR/return) to perform the standardized fallback cleanup and exit; update
the branches around the checks that reference strip.isMatrix, SEGMENT.is2D(),
and any allocation failure returns to call FX_FALLBACK_STATIC.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d6d0c21-f0ab-4f07-afad-af46c07f89e1
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
…hibitor to increase rhythm response
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/user_fx/user_fx.cpp`:
- Line 1379: The local uint32_t triggerGate is reset each frame, so replace the
local variable with a persistent slot (e.g., read/write SEGENV.aux0 or
SEGENV.aux1) to hold the "next allowed spawn time"; change occurrences that
compare SEGENV.step > triggerGate to read uint32_t triggerGate =
(uint32_t)SEGENV.aux0 (or aux1), and when allowing a spawn update SEGENV.aux0 =
(double)(SEGENV.step + spawnInterval) (or store the timestamp you need) so the
rate-limiting persists across frames; apply this same change to the other
instance noted (around the second occurrence referenced in the comment).
- Around line 1367-1372: The retry loop that calls
walkers[freeSlot].makeCandidate(...) can overwrite a previously successful
activation because makeCandidate resets active and a later retry may set active
back to false; fix this in the for (uint8_t retry = 0; retry < 2; retry++) loop
by exiting the loop immediately after a successful activation (i.e., after
walkers[freeSlot].active = true) so subsequent makeCandidate calls cannot
overwrite the valid spawn—add a break from the retry loop when hasConflict(...)
returns false and active is set.
- Around line 1382-1386: The allocation of segment data is currently gated by
SEGENV.call == 0 which makes future calls crash if SEGENV.data was freed; call
SEGENV.allocateData(sizeof(Walker) * absoluteMaxWalkers) unconditionally before
using SEGENV.data so allocateData can no-op when already sized or reallocate
when needed, then reinterpret_cast SEGENV.data to Walker* (Walker* walkers) and
handle a failed allocation via FX_FALLBACK_STATIC as before (keep the
FX_FALLBACK_STATIC path if allocateData returns false).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2c21c6a8-f5f0-4581-bfb5-36013f41b6da
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
3 participants
The "Brush Walker" is a 2D effect that moves dots across the grid leaving a (fading, optional) trail. It is similar to the Matrix movie title / the matrix effect in WLED, but it picks directions randomly from four sides and uses the current palette to pick a random color, slowly changing the palette position of the next color as the dot moves. The rate/stepping of palette index change is configurable. The function requests storage for up to 32 dots from SEGENV, each dot having X/Y, dX/dY, active flag and a palette index.
The "Brush Walker AR" triggers new walkers by checking the data supplied by audioreactive peak detection, plus an adjustable "base noise" of walkers to keep the display from going completely blank.
I mainly ported this over from an old project I did in Arduino and ESP8266 in 2019, because I found no other effect with this look yet. I used VSC AI assistance and did some cleanup and added comments afterwards because AI isn't quite there yet :-)
Tested on ESP32 dev board and ESP32C3 Supermini board, with I2S microphone directly and audio over UDP, driving several small WS2812B 8x8 grids and a 16x36 SK6812 furniture installation.
Summary by CodeRabbit