Introduce comet effect usermod with fire particle system#5347
Introduce comet effect usermod with fire particle system#5347gustebeast wants to merge 3 commits intowled:mainfrom
Conversation
WalkthroughAdds a new PS Comet usermod: a 2D falling-comet particle effect implementation, a README with installation/parameters, and a library manifest. Introduces effect logic, a Usermod class with setup/loop, and registers the usermod with the framework. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 2
🤖 Fix all issues with AI agents
In `@usermods/PS_Comet/PS_Comet.cpp`:
- Around line 23-60: Guard against division-by-zero when computing pixel mapping
and vertical shifts by using a safe denominator (e.g., uint32_t denomW = max(1u,
SEGMENT.vWidth() - 1) and uint32_t denomH = max(1u, SEGMENT.vHeight() - 1)) and
replace any uses of (SEGMENT.vWidth() - 1) / (SEGMENT.vHeight() - 1) with those
safe denominators; fix the 3x->column mapping in mode_pscomet by computing
baseCol = i / 3, offset = (int)(i % 3) - 1 (so center = 0, left = -1, right =
+1), then targetCol = clamp(baseCol + offset, 0u, SEGMENT.vWidth() - 1) and set
sourceParticle.x = targetCol * PartSys->maxX / denomW; similarly compute
sideCometVerticalShift using denomH (uint32_t sideCometVerticalShift = 2 *
PartSys->maxY / denomH) and ensure all divisions use the safe denominators and
clamp results to valid ranges to avoid out-of-bounds writes.
In `@usermods/PS_Comet/README.md`:
- Line 3: Fix the spelling mistake "palletes" to "palettes" in the README
sentence describing color support (the phrase "Works with custom color palletes,
defaulting to "Fire""). Update that phrase to read "Works with custom color
palettes, defaulting to 'Fire'".
🧹 Nitpick comments (1)
usermods/PS_Comet/PS_Comet.cpp (1)
4-4: Avoid global spawn timer to prevent cross‑segment coupling.
nextCometCreationTimeis global, so multiple segments running PS Comet will share a single spawn cadence. Consider storing this per segment (e.g., in SEGENV.aux0/aux1 or a small state struct tied to SEGENV.data) to keep animations independent.
|
looks good in general, I'll add some comments to consider |
usermods/PS_Comet/PS_Comet.cpp
Outdated
There was a problem hiding this comment.
if you enable gravity and enable killoutofbounds and let the PS handle the movement of your sources, you do not need this function and can just check if TTL=0. you can set gravity to 1 for a very slight acceleration only if that is not disturbing your intentions.
another option would be to set the initial TTL accordingly so your sources die once off-screen.
or just do this check inside the FX, or add a lambda function.
usermods/PS_Comet/PS_Comet.cpp
Outdated
There was a problem hiding this comment.
why not make the emit speed handle this? the downside would be that the speed would not react immediately to to a slider change.
|
I gave it a quick spin. Parameters may need some tuning: set all sliders except speed to max and it breaks the effect. |
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/PS_Comet/PS_Comet.cpp`:
- Line 4: The global variable nextCometCreationTime causes all segments to share
one spawn timer; replace this file-scope unsigned long with per-segment storage
by packing a uint32_t timestamp into SEGENV.aux0/aux1 or by creating a small
context struct (containing the ParticleSystem2D* and a uint32_t
nextCometCreationTime) and storing a pointer in SEGENV.data; update the comet
spawn logic (references to nextCometCreationTime) to read/write the per-segment
field from SEGENV.aux0/aux1 or the context in SEGENV.data and initialize that
per-segment storage where the ParticleSystem2D is created/attached.
- Around line 57-61: The current expression for canLargeCometSpawn calls
hw_random8(254) even when no comet will spawn (chosenIndex == NULL_INDEX),
wasting RNG; change the condition order or split it so chosenIndex is validated
first: check chosenIndex != NULL_INDEX (and the edge guards chosenIndex != 0 &&
chosenIndex != numComets - 1) before calling SEGMENT.custom1 > hw_random8(254).
Update the canLargeCometSpawn computation (or add a short if) so hw_random8(254)
is only invoked when chosenIndex is a real column index.
- Around line 54-55: When SEGMENT.intensity can make cometFrequencyDelay zero,
avoid calling hw_random16(0) and prevent scheduling comets every frame: clamp
cometFrequencyDelay to a minimum (e.g., 1) before calling hw_random16 or skip
the random offset when cometFrequencyDelay == 0 and instead add a safe non-zero
fallback to nextCometCreationTime; update the code that computes
cometFrequencyDelay, the hw_random16 call, and the assignment to
nextCometCreationTime to use the clamped/fallback value (references:
cometFrequencyDelay, SEGMENT.intensity, hw_random16, nextCometCreationTime,
strip.now).
| #include "wled.h" | ||
| #include "FXparticleSystem.h" | ||
|
|
||
| unsigned long nextCometCreationTime = 0; |
There was a problem hiding this comment.
nextCometCreationTime is a file-scope global — breaks multi-segment usage
All segments running this effect share the same spawn timer. When one segment creates a comet it sets the timer far into the future, silencing comet creation in every other segment for that duration.
Store the next-creation timestamp per segment, e.g. in SEGENV.aux0/aux1 (packed as a uint32_t) or inside a small context struct held in SEGENV.data alongside the ParticleSystem2D pointer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PS_Comet/PS_Comet.cpp` at line 4, The global variable
nextCometCreationTime causes all segments to share one spawn timer; replace this
file-scope unsigned long with per-segment storage by packing a uint32_t
timestamp into SEGENV.aux0/aux1 or by creating a small context struct
(containing the ParticleSystem2D* and a uint32_t nextCometCreationTime) and
storing a pointer in SEGENV.data; update the comet spawn logic (references to
nextCometCreationTime) to read/write the per-segment field from SEGENV.aux0/aux1
or the context in SEGENV.data and initialize that per-segment storage where the
ParticleSystem2D is created/attached.
| uint16_t cometFrequencyDelay = 2040 - (SEGMENT.intensity << 3); | ||
| nextCometCreationTime = strip.now + cometFrequencyDelay + hw_random16(cometFrequencyDelay); |
There was a problem hiding this comment.
hw_random16(0) UB at maximum intensity — root cause of reported breakage
When SEGMENT.intensity == 255, SEGMENT.intensity << 3 == 2040, making cometFrequencyDelay == 0. hw_random16(0) typically reduces to random16() % 0, which is undefined behaviour (integer division by zero). Even if the implementation short-circuits to 0, nextCometCreationTime stays at strip.now, so a new comet is attempted every frame — this matches the reviewer's observation that maxing out the sliders breaks the effect.
🛠️ Proposed fix
- uint16_t cometFrequencyDelay = 2040 - (SEGMENT.intensity << 3);
- nextCometCreationTime = strip.now + cometFrequencyDelay + hw_random16(cometFrequencyDelay);
+ uint16_t cometFrequencyDelay = max(1U, (unsigned)(2040U - (SEGMENT.intensity << 3)));
+ nextCometCreationTime = strip.now + cometFrequencyDelay + hw_random16(cometFrequencyDelay);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PS_Comet/PS_Comet.cpp` around lines 54 - 55, When SEGMENT.intensity
can make cometFrequencyDelay zero, avoid calling hw_random16(0) and prevent
scheduling comets every frame: clamp cometFrequencyDelay to a minimum (e.g., 1)
before calling hw_random16 or skip the random offset when cometFrequencyDelay ==
0 and instead add a safe non-zero fallback to nextCometCreationTime; update the
code that computes cometFrequencyDelay, the hw_random16 call, and the assignment
to nextCometCreationTime to use the clamped/fallback value (references:
cometFrequencyDelay, SEGMENT.intensity, hw_random16, nextCometCreationTime,
strip.now).
| uint8_t canLargeCometSpawn = | ||
| // Slider 3 determines % of large comets with extra particle sources on their sides | ||
| SEGMENT.custom1 > hw_random8(254) | ||
| && chosenIndex != 0 | ||
| && chosenIndex != numComets - 1; |
There was a problem hiding this comment.
canLargeCometSpawn can be true when chosenIndex == NULL_INDEX
When no comet will spawn (chosenIndex == NULL_INDEX), the edge-guard checks on lines 60–61 still pass (since NULL_INDEX is far outside the valid column range), so hw_random8(254) is consumed and canLargeCometSpawn can be set to true. This is functionally safe — no comet is actually spawned — but it wastes a random draw on every suppressed frame.
🛠️ Proposed fix
uint8_t canLargeCometSpawn =
SEGMENT.custom1 > hw_random8(254)
+ && chosenIndex != NULL_INDEX
&& chosenIndex != 0
&& chosenIndex != numComets - 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/PS_Comet/PS_Comet.cpp` around lines 57 - 61, The current expression
for canLargeCometSpawn calls hw_random8(254) even when no comet will spawn
(chosenIndex == NULL_INDEX), wasting RNG; change the condition order or split it
so chosenIndex is validated first: check chosenIndex != NULL_INDEX (and the edge
guards chosenIndex != 0 && chosenIndex != numComets - 1) before calling
SEGMENT.custom1 > hw_random8(254). Update the canLargeCometSpawn computation (or
add a short if) so hw_random8(254) is only invoked when chosenIndex is a real
column index.
This was due to the code using 3 sources for every column and not cleaning them up properly after falling out of view. With the new implementation it looks normal maxed out. The comet length max makes the large comets appear a bit faint but that's a limitation with the PS system requiring all particle sources to use the same intensity. Ideally the large comets could have their own intensity value since they have their own maxlife/minlife/ttl |
3 participants
A 2D falling comet effect similar to "Matrix" but with a fire particle simulation to enhance the comet trail visuals. Works with custom color palletes, defaulting to "Fire". Supports "small" and "large" comets which are 1px and 3px wide respectively.
Demo: https://imgur.com/a/i1v5WAy
Summary by CodeRabbit
New Features
Documentation