Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 123 additions & 0 deletions usermods/PS_Comet/PS_Comet.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
#include "wled.h"
#include "FXparticleSystem.h"

unsigned long nextCometCreationTime = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.


#define FX_FALLBACK_STATIC { SEGMENT.fill(SEGCOLOR(0)); return; }
// Use UINT32_MAX - 1 for the "no comet" case so we can add 1 later and not have it overflow
#define NULL_INDEX UINT32_MAX - 1

///////////////////////
// Effect Function //
///////////////////////

void mode_pscomet() {
ParticleSystem2D *PartSys = nullptr;
uint32_t i;

if (SEGMENT.call == 0) { // Initialization
// Try to allocate one comet for every column
if (!initParticleSystem2D(PartSys, SEGMENT.vWidth())) {
FX_FALLBACK_STATIC; // Allocation failed or not 2D
}
PartSys->setMotionBlur(170); // Enable motion blur
PartSys->setParticleSize(0); // Allow small comets to be a single pixel wide
}
else {
PartSys = reinterpret_cast<ParticleSystem2D *>(SEGENV.data); // If not first call, use existing data
}
if (PartSys == nullptr || SEGMENT.vHeight() < 2 || SEGMENT.vWidth() < 2) {
FX_FALLBACK_STATIC;
}

PartSys->updateSystem(); // Update system properties (dimensions and data pointers)

auto has_fallen_off_screen = [PartSys](uint32_t particleIndex) {
return particleIndex < PartSys->numSources
? PartSys->sources[particleIndex].source.y < PartSys->maxY * -1
: true;
};

// This will be SEGMENT.vWidth() unless the particle system had insufficient memory
uint32_t numComets = PartSys->numSources;
// Pick a random column for a new comet to spawn, but reset it to null if it's not time yet or there's already a
// comet nearby
uint32_t chosenIndex = hw_random(numComets);
if (
strip.now < nextCometCreationTime
|| !has_fallen_off_screen(chosenIndex - 1)
|| !has_fallen_off_screen(chosenIndex)
|| !has_fallen_off_screen(chosenIndex + 1)
) {
chosenIndex = NULL_INDEX;
} else {
uint16_t cometFrequencyDelay = 2040 - (SEGMENT.intensity << 3);
nextCometCreationTime = strip.now + cometFrequencyDelay + hw_random16(cometFrequencyDelay);
Comment on lines +54 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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;
Comment on lines +57 to +61
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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.

uint8_t fallingSpeed = 1 + (SEGMENT.speed >> 2);

// Update the comets
for (i = 0; i < numComets; i++) {
auto& source = PartSys->sources[i];
auto& sourceParticle = source.source;

if (!has_fallen_off_screen(i)) {
// Active comets fall downwards and emit flames
sourceParticle.y -= fallingSpeed;
source.vy = (SEGMENT.speed >> 5) - fallingSpeed; // Emitting speed (upwards)
PartSys->flameEmit(PartSys->sources[i]);
continue;
}

bool isChosenComet = i == chosenIndex;
bool isChosenSideComet =
canLargeCometSpawn &&
(i == chosenIndex - 1 || i == chosenIndex + 1);

// Chosen comets respawn at the top
if (isChosenComet || isChosenSideComet) {
// Map the comet index into an output pixel index
sourceParticle.x = i * PartSys->maxX / (SEGMENT.vWidth() - 1);
// Spawn a bit above the top to avoid popping into view
sourceParticle.y = PartSys->maxY + (2 * fallingSpeed);
if (isChosenComet) {
// Slider 4 controls comet length via particle lifetime and fire intensity adjustments
source.maxLife = 16 + (SEGMENT.custom2 >> 2);
source.minLife = source.maxLife >> 1;
sourceParticle.ttl = 16 - (SEGMENT.custom2 >> 4);
} else {
// Side comets have fixed length
source.maxLife = 18;
source.minLife = 14;
sourceParticle.ttl = 16;
// Shift side comets up by 1 pixel
sourceParticle.y += 2 * PartSys->maxY / (SEGMENT.vHeight() - 1);
}
}
}

// Slider 4 controls comet length via particle lifetime and fire intensity adjustments
PartSys->updateFire(max(255U - SEGMENT.custom2, 45U));
}
static const char _data_FX_MODE_PSCOMET[] PROGMEM = "PS Comet@Falling Speed,Comet Frequency,Large Comet Probability,Comet Length;;!;2;pal=35,sx=128,ix=255,c1=32,c2=128";

/////////////////////
// UserMod Class //
/////////////////////

class PSCometUsermod : public Usermod {
public:
void setup() override {
strip.addEffect(255, &mode_pscomet, _data_FX_MODE_PSCOMET);
}

void loop() override {}
};

static PSCometUsermod ps_comet;
REGISTER_USERMOD(ps_comet);
25 changes: 25 additions & 0 deletions usermods/PS_Comet/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
## Description

A 2D falling comet effect similar to "Matrix" but with a fire particle simulation to enhance the comet trail visuals. Works with custom color palettes, defaulting to "Fire". Supports "small" and "large" comets which are 1px and 3px wide respectively.

Demo: [https://imgur.com/a/i1v5WAy](https://imgur.com/a/i1v5WAy)

## Installation

To activate the usermod, add the following line to your platformio_override.ini
```ini
custom_usermods = ps_comet
```
Or if you are already using a usermod, append ps_comet to the list
```ini
custom_usermods = audioreactive ps_comet
```

You should now see "PS Comet" appear in your effect list.

## Parameters

1. **Falling Speed** sets how fast the comets fall
2. **Comet Frequency** determines how many comets are on screen at a time
3. **Large Comet Probability** determines how often large 3px wide comets spawn
4. **Comet Length** sets how far comet trails stretch vertically
4 changes: 4 additions & 0 deletions usermods/PS_Comet/library.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "PS Comet",
"build": { "libArchive": false }
}