2 Effects: Xmas Twinkle, Elastic Collisions#4843
Conversation
…nimum number of LEDs lit is now 2.
Conflicts: wled00/FX.h wled00/FX_fcn.cpp
…one ends up inside the other. This fix is not perfect, but makes it is rarer and clears it faster.
|
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:
WalkthroughAdds two independent effects: a new core FX "Xmas Twinkle" (per-LED twinkle state, skewed timing helpers, effect registration) and a new usermod "Elastic Collisions" (fixed-point sphere physics, collisions, drawing, lifetime/regeneration, and README/library.json). Also adds FX ID and increments mode count. ChangesXmas Twinkle FX (core effect)
Elastic Collisions usermod
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 4
🧹 Nitpick comments (9)
wled00/FX.h (1)
379-379: MODE_COUNT correctly bumped to highest FX_MODE_ ID + 1MODE_COUNT=220 matches the guidance that it must be (max FX_MODE_ ID) + 1, which is used for bounds/range checks.
To guard future changes, consider a compile-time check to keep MODE_COUNT within uint8_t limits used by WS2812FX internals:
-#define MODE_COUNT 220 +#define MODE_COUNT 220 +static_assert(MODE_COUNT < 256, "MODE_COUNT must fit into uint8_t");wled00/FX.cpp (8)
7894-7917: Return a value on all code paths in elasticLifetime()
elasticLifetime()has no default return, which can trigger a compiler warning. WhileSEGMENT.custom2 >> 5is constrained to [0..7], make the function total by adding a fallback.uint32_t elasticLifetime() { // 8 categories. switch (SEGMENT.custom2 >> 5) // /32 { case 0: return 300; // 15s case 1: return 600; // 30s case 2: return 1200; // 1m case 3: return 2400; // 2m case 4: return 6000; // 5m case 5: return 12000; // 10m case 6: return 36000; // 30m case 7: return 72000; // 1hr } + // Fallback (15s) – defensive default + return 300; }
7714-7720: Avoid unnecessary copying in areSpheresColliding()Passing
MBSphereby value creates a copy every time; it’s not needed and wastes cycles on embedded targets. Make it a const reference and mark the method const.- bool areSpheresColliding(MBSphere sp) + bool areSpheresColliding(const MBSphere& sp) const
7494-7500: Macro names are too generic; consider localizing to avoid collisionsGlobal macros like
MAX_CYCLE,TIME_TO_EVENT, etc., are common identifiers and can collide or confuse with unrelated code. Preferstatic constexprinside a local namespace or at least prefix them, e.g.,XTW_....Example:
-#define MAX_CYCLE 0x001ff800 +#define XTW_MAX_CYCLE 0x001ff800Or:
struct XTwinkleLight { static constexpr uint32_t TwinkleOn = 0x80000000; static constexpr uint32_t TimeToEvent = 0x7fe00000; static constexpr uint32_t TimeToEventShift = 21; // ... };
7551-7565: Typo: “Twiklers”
numTwiklersappears multiple times. Rename tonumTwinklersfor clarity and consistency.- uint16_t numTwiklers = SEGLEN * SEGMENT.intensity / 255; - if (numTwiklers <= 1) - numTwiklers = 2; + uint16_t numTwinklers = SEGLEN * SEGMENT.intensity / 255; + if (numTwinklers <= 1) + numTwinklers = 2; ... - uint16_t dataSize = sizeof(XTwinkleLight) * numTwiklers; + uint16_t dataSize = sizeof(XTwinkleLight) * numTwinklers; ... - for (int i = 0; i < numTwiklers; ++i) + for (int i = 0; i < numTwinklers; ++i) ... - SEGMENT.aux0 = numTwiklers; // Initialized. + SEGMENT.aux0 = numTwinklers; // Initialized.
7643-7661: Sparse-set rendering: optional enhancementCurrently you clear and redraw the entire segment every frame, but only a handful of indices are lit (
inset = i * SEGLEN / numTwinklers). On long strips this wastes fill bandwidth. Consider tracking previous “on” indices and only toggle pixels that changed state for a small win.No changes required; offering as a future optimization.
7962-8016: Sphere initialization: bulletproof the placement and overlap avoidance
- You already avoid overlaps by retrying positions, but
safetyis 100 and failure stops allocation early. Consider falling back to shrinkingradiusslightly for the last few spheres, or increasing retries proportionally tonumSpheres.- Also, prefer initializing
MBSphereusing a direct struct write or a small factory function with validation; repeatedrandom()+ test loops can be tightened.Not blocking; just suggestions to improve robustness on very small displays with many spheres.
7580-7591: Use consistent RNG helperElsewhere in FX.cpp the code uses
hw_random8/16andrandom8/16to keep RNG behavior consistent and, in some places, seedable. Inmode_XmasTwinkle, calls torandom()are mixed in. Consider switching to the established helpers for consistency.Example:
- light->colorIdx = random8(); + light->colorIdx = hw_random8();And similarly for other
random(...)calls.
11376-11378: FX_MODE IDs and MODE_COUNT are correct
BothFX_MODE_XMASTWINKLE(218) andFX_MODE_ELASTICCOLLISIONS(219) are uniquely defined andMODE_COUNTis set to 220 (highest ID + 1), matching WLED convention.Optional cleanup:
- Rename
_data_FXMODE_ELASTICCOLLISIONS→_data_FX_MODE_ELASTICCOLLISIONSto align with the prevailing_data_FX_MODE_*pattern.[optional_refactors_recommended]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
wled00/FX.cpp(2 hunks)wled00/FX.h(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.hwled00/FX.cpp
🔇 Additional comments (1)
wled00/FX.h (1)
377-378: New FX IDs look correct and contiguousFX_MODE_XMASTWINKLE=218 and FX_MODE_ELASTICCOLLISIONS=219 seamlessly extend from the previous highest (217). Naming is consistent with existing conventions.
| const uint16_t pArraySize, | ||
| const float *pArray) | ||
| { | ||
| int index = 0; | ||
| float cumulativePercentage = 0; | ||
|
|
||
| // Find the range in the table based on randomValue. | ||
| while (index < pArraySize - 1 && rand100 >= cumulativePercentage + pArray[index]) { | ||
| cumulativePercentage += pArray[index]; | ||
| index++; | ||
| } | ||
|
|
||
| // Calculate linear interpolation | ||
| float t = (rand100 - cumulativePercentage) / pArray[index]; | ||
| float result = (float(index) + t) * 100.0 / pArraySize; | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Guard against zero-sized percentage bins in skewedRandom()
If the current pArray[index] ends up being zero (e.g., after weighting), the linear interpolation divides by zero. It’s unlikely with your current tables, but the helper should be robust.
// Calculate linear interpolation
- float t = (rand100 - cumulativePercentage) / pArray[index];
- float result = (float(index) + t) * 100.0 / pArraySize;
+ if (pArray[index] <= 0.0f) {
+ // Degenerate bin, snap to the bin start
+ return (index * 100) / pArraySize;
+ }
+ float t = (rand100 - cumulativePercentage) / pArray[index];
+ float result = (float(index) + t) * 100.0f / pArraySize;📝 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.
| const uint16_t pArraySize, | |
| const float *pArray) | |
| { | |
| int index = 0; | |
| float cumulativePercentage = 0; | |
| // Find the range in the table based on randomValue. | |
| while (index < pArraySize - 1 && rand100 >= cumulativePercentage + pArray[index]) { | |
| cumulativePercentage += pArray[index]; | |
| index++; | |
| } | |
| // Calculate linear interpolation | |
| float t = (rand100 - cumulativePercentage) / pArray[index]; | |
| float result = (float(index) + t) * 100.0 / pArraySize; | |
| return result; | |
| } | |
| const uint16_t pArraySize, | |
| const float *pArray) | |
| { | |
| int index = 0; | |
| float cumulativePercentage = 0; | |
| // Find the range in the table based on randomValue. | |
| while (index < pArraySize - 1 && rand100 >= cumulativePercentage + pArray[index]) { | |
| cumulativePercentage += pArray[index]; | |
| index++; | |
| } | |
| // Calculate linear interpolation | |
| if (pArray[index] <= 0.0f) { | |
| // Degenerate bin, snap to the bin start | |
| return (index * 100) / pArraySize; | |
| } | |
| float t = (rand100 - cumulativePercentage) / pArray[index]; | |
| float result = (float(index) + t) * 100.0f / pArraySize; | |
| return result; | |
| } |
| } | ||
| // Put the updated event time back. | ||
| light->twData = (light->twData & ~TIME_TO_EVENT) | (eventTime << TIME_TO_EVENT_SHIFT & TIME_TO_EVENT); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clamp TIME_TO_EVENT to its 10-bit storage width
TIME_TO_EVENT stores only 10 bits. If eventTime ever exceeds 1023 (e.g., due to subtraction timing quirks), the higher bits will be truncated on writeback. Clamp to [0..1023] before packing.
- light->twData = (light->twData & ~TIME_TO_EVENT) | (eventTime << TIME_TO_EVENT_SHIFT & TIME_TO_EVENT);
+ eventTime = std::max<int16_t>(0, std::min<int16_t>(eventTime, 1023));
+ light->twData = (light->twData & ~TIME_TO_EVENT) | ((uint32_t(eventTime) << TIME_TO_EVENT_SHIFT) & TIME_TO_EVENT);📝 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.
| } | |
| // Put the updated event time back. | |
| light->twData = (light->twData & ~TIME_TO_EVENT) | (eventTime << TIME_TO_EVENT_SHIFT & TIME_TO_EVENT); | |
| } | |
| // Put the updated event time back. | |
| eventTime = std::max<int16_t>(0, std::min<int16_t>(eventTime, 1023)); | |
| light->twData = (light->twData & ~TIME_TO_EVENT) | |
| | ((uint32_t(eventTime) << TIME_TO_EVENT_SHIFT) & TIME_TO_EVENT); |
🤖 Prompt for AI Agents
In wled00/FX.cpp around lines 7624 to 7627, the code writes eventTime back into
the 10-bit TIME_TO_EVENT field but does not clamp it, so values >1023 will
overflow; clamp eventTime to the 0..1023 range before packing (e.g., eventTime =
max(0, min(eventTime, 1023)) or an equivalent clamp) and then store it into
light->twData using the existing shift/mask pattern so only the low 10 bits are
written.
| #define FX_MODE_XMASTWINKLE 218 | ||
| #define FX_MODE_ELASTICCOLLISIONS 219 | ||
| #define MODE_COUNT 220 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify FX.cpp registers both new effects with matching IDs and metadata
To avoid out-of-bounds or missing entries (since _modeCount is set from MODE_COUNT), ensure setupEffectData adds entries for both new IDs, and that the functions and metadata strings exist.
Run this script from the repo root:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Confirm mode function definitions exist"
rg -nP -C2 --type=cpp '\buint16_t\s+mode_XmasTwinkle\s*\(' || { echo "Missing mode_XmasTwinkle()"; exit 1; }
rg -nP -C2 --type=cpp '\buint16_t\s+mode_ElasticCollisions\s*\(' || { echo "Missing mode_ElasticCollisions()"; exit 1; }
echo
echo "2) Confirm setupEffectData registers both modes with their IDs"
rg -nP -C2 --type=cpp 'addEffect\s*\(\s*FX_MODE_XMASTWINKLE\s*,\s*mode_XmasTwinkle\s*,' || { echo "Missing addEffect for FX_MODE_XMASTWINKLE"; exit 1; }
rg -nP -C2 --type=cpp 'addEffect\s*\(\s*FX_MODE_ELASTICCOLLISIONS\s*,\s*mode_ElasticCollisions\s*,' || { echo "Missing addEffect for FX_MODE_ELASTICCOLLISIONS"; exit 1; }
echo
echo "3) Confirm metadata tokens exist for UI (names/control data)"
rg -nP -n --type=cpp 'XMASTWINKLE' || { echo "No references to XMASTWINKLE metadata found"; exit 1; }
rg -nP -n --type=cpp 'ELASTICCOLLISIONS' || { echo "No references to ELASTICCOLLISIONS metadata found"; exit 1; }
echo
echo "4) Sanity check: MODE_COUNT is max(FX_MODE_*) + 1"
python - << 'PY'
import re, glob, sys
paths = glob.glob("**/FX.h", recursive=True)
if not paths:
print("FX.h not found"); sys.exit(1)
h = open(paths[0], "r", encoding="utf-8", errors="ignore").read()
ids = [int(m) for m in re.findall(r'#define\s+FX_MODE_[A-Z0-9_]+\s+(\d+)', h)]
mc = re.search(r'#define\s+MODE_COUNT\s+(\d+)', h)
if not ids or not mc: print("Parse failed"); sys.exit(1)
mi = max(ids); mc = int(mc.group(1))
print(f"HIGHEST FX_MODE_ ID: {mi}, MODE_COUNT: {mc}, OK={mc == mi+1}")
if mc != mi+1: sys.exit(2)
PY
echo "All checks passed."Length of output: 1079
Register missing effects in setupEffectData
The verification shows that FX_MODE_XMASTWINKLE isn’t being added in FX.cpp’s setupEffectData (so _modeCount = 220 will be out-of-bounds). Please:
• In FX.cpp, inside setupEffectData, add
addEffect(FX_MODE_XMASTWINKLE, mode_XmasTwinkle, F("Xmas Twinkle"), /*…control data…*/);• Also confirm you have a matching
addEffect(FX_MODE_ELASTICCOLLISIONS, mode_ElasticCollisions, F("Elastic Collisions"), /*…*/);• Update the UI metadata arrays (names, control tokens, descriptions) to include both “Xmas Twinkle” and “Elastic Collisions” entries.
Once added, rerun your sanity checks to ensure MODE_COUNT still equals highest ID + 1.
🤖 Prompt for AI Agents
In wled00/FX.h around lines 377-379, FX_MODE_XMASTWINKLE (218) and
FX_MODE_ELASTICCOLLISIONS (219) are defined but not registered, causing
MODE_COUNT (220) to be out-of-bounds; open FX.cpp and inside setupEffectData add
registrations for both effects using addEffect(FX_MODE_XMASTWINKLE,
mode_XmasTwinkle, F("Xmas Twinkle"), /* control data */) and ensure
addEffect(FX_MODE_ELASTICCOLLISIONS, mode_ElasticCollisions, F("Elastic
Collisions"), /* control data */) is present; also update the UI metadata arrays
(names, control tokens, descriptions) to include entries for "Xmas Twinkle" and
"Elastic Collisions" in the proper indices so arrays align with effect IDs, then
rerun sanity checks to confirm MODE_COUNT == highest ID + 1.
blazoncek
left a comment
blazoncek
left a comment
There was a problem hiding this comment.
It would be beneficial to post a video of expected effect behaviour to easily judge if existing effects behave similarly.
For elastic collision, there is already a similar effect called Blobs. It would be preferred if that one would be updated to handle collisions instead of adding a new one.
For twinkle, there are several other twinkles already in place. Does none of them work similarly?
Other than that, I stopped reviewing when I saw a madness of indentations. Please fix all indentations.
| #define FX_MODE_PS1DSPRINGY 216 | ||
| #define FX_MODE_PARTICLEGALAXY 217 | ||
| #define MODE_COUNT 218 | ||
| #define FX_MODE_XMASTWINKLE 218 |
|
On Aug 16, 2025, at 11:14 AM, Blaž Kristan ***@***.***> wrote:
It would be beneficial to post a video of expected effect behaviour to easily judge if existing effects behave similarly.
I just posted two videos on Discord "# custom-fx" thread. Let me know if there is a better place.
For elastic collision, there is already a similar effect called Blobs. It would be preferred if that one would be updated to handle collisions instead of adding a new one.
I’ll take a look.
For twinkle, there are several other twinkles already in place. Does none of them work similarly?
No, it is quite a bit different. Existing twinkle effects, fade in and out randomly. My effect blinks on and off in the same position. Each light is assigned a fixed color index normally, so it doesn’t change color—much like twinkle lights on a real Christmas tree string. It also simulates the heating and cooling of a real twinkle light bulb. You can adjust the percentage of lights over a segment and their blink rate. A checkbox controls color assignment.
Because of this, the patten of blinking is more “interesting”.
It works well on a single LED.
Other than that, I stopped reviewing when I saw a madness of indentations. Please fix all indentations.
Indentations are fine in my Visual Studio Code, where a tab is only two spaces, but I’ll convert tabs to spaces so tabbing is consistent with any spaces/tab. I’ll get to it tomorrow. :-)
|
|
First off I really like these effects and the effort you put into making them physically accurate. There are a few issues that will prevent them making it into default WLED effects:
When playing around, some sliders were unresponsive by themseles and only changed things wen sliding another one. Needs some refinement. Here are my suggestions:
|
|
Hello DedeHai, et al,
Thank you for the review. Your points are well taken.
I actually wrote these effects over a year ago in my own environment but I had trouble figuring out submission procedures until I had more time—a contract job interfered. I only got back to moving them to WLED recently.
Since a year has gone by, the effect landscape has changed a bit, hence the overlap. Also the Particle System did not exist at the time I wrote Elastic Collisions.
Some of the tabbing problems were a result of the port from my independent project, I’ff fix that up.
On Aug 17, 2025, at 3:29 AM, Damian Schneider ***@***.***> wrote:
DedeHai
left a comment
(wled/WLED#4843)
<#4843 (comment)>
First off I really like these effects and the effort you put into making them physically accurate. There are a few issues that will prevent them making it into default WLED effects:
As already mentioned, very similar effects do already exist: Twinklecat and PS Blobs / PS Pinball
Using floats all over is problematic: on my C3 test setup it drops to 8FPS. For comparison: PS Blobs runs at 65FPS
With careful coding, I could probably get rid of all the floating point. If I want fractional numbers, I could use fractional fixed point with a virtual binary point shifted left. I’ve worked on main frame computers in the very distant past that had no floating point.
Even more intriguing would be to convert Elastic Collisions to use the Particle System. That would eliminate a whole lot of duplicate code. Would there be any documentation of PS beyond the code, by chance?
And last but not least: code size. This PR adds over 4k of flash for just two effects (9k on my C3 test setup!). For comparison: the whole 1D particle system with 15 effects uses 17k. Flash is precious.
Agreed.
When playing around, some sliders were unresponsive by themseles and only changed things wen sliding another one. Needs some refinement.
I could fix that by detecting slider deltas, but having only .aux0, .aux1 and .step to store stuff between invocations, I’d have to do something tricky, unless you have another suggestion or I’m missing something.
Here are my suggestions:
Update Twinklecat with a checkmark and add some of your code to it, keep it as simple as possible
I’ll look at that.
If you want to keep your effects as options in WLED code: move it to the user FX usermod: here the "rules" are less strict as these are not maintained by the devs but by the creators and they are not compiled in by default.
If you feel that these effect algorithms are worthwhile, I’d be willing to put in the time to revise them and/or incorporate them into existing effects in a way that wouldn’t intrude on them.
I’m glad to do the work, but only if there is sufficient interest.
Thank you.
Best regards,
Nick Pisarro
|
unfortunately not but I did try to comment the code and I made many FX that use it so you can take it from whichever PS FX is closest to what you want to do. I also have a trick in at least one of the PS FX to use aux to track all sliders: just sum them all up and store. I can only speak on my behalf and the "Elastic Collisions" effect is just too bulky for what it is and there is PS Blobs which is quite close in overall looks and much more versatile. Except it does not handle mass. |
…nd make their parameters uint8_t, instead of float.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
wled00/FX.cpp (3)
7512-7530: Guard skewedRandom() against zero-sized bins (divide-by-zero)pArray[index] can be 0 after weighting, leading to a division by zero in t. Add a fast return when the selected bin has zero weight. This was raised previously and is still present.
int32_t skewedRandom( uint8_t rand100, const uint8_t pArraySize, const uint8_t *pArray) { int32_t index = 0; int32_t cumulativePercentage = 0; // Find the range in the table based on randomValue. while (index < pArraySize - 1 && rand100 >= cumulativePercentage + pArray[index]) { cumulativePercentage += pArray[index]; index++; } // Calculate linear interpolation - int32_t t = ((rand100 - cumulativePercentage) << RAND_PREC_SHIFT) / pArray[index]; + if (pArray[index] == 0) { + // Degenerate bin, snap to bin start + return (index * 100) / pArraySize; + } + int32_t t = ((rand100 - cumulativePercentage) << RAND_PREC_SHIFT) / pArray[index]; int32_t result = ((index << RAND_PREC_SHIFT) + t) * 100 / pArraySize >> RAND_PREC_SHIFT; return result; }
7605-7636: Clamp TIME_TO_EVENT before packing into 10-bit fieldeventTime can underflow or exceed 10-bit capacity; store only 0..1023 to avoid wrap/surprise long delays. This was previously flagged and remains unresolved.
- // Put the updated event time back. - light->twData = (light->twData & ~TIME_TO_EVENT) | (eventTime << TIME_TO_EVENT_SHIFT & TIME_TO_EVENT); + // Put the updated (clamped) event time back (10-bit field). + if (eventTime < 0) eventTime = 0; + if (eventTime > 1023) eventTime = 1023; + light->twData = (light->twData & ~TIME_TO_EVENT) + | ((uint32_t(eventTime) << TIME_TO_EVENT_SHIFT) & TIME_TO_EVENT);
7791-7817: Degenerate overlap handling: don’t early-return on length==0; separate spheres deterministicallyReturning when centers coincide leaves spheres interpenetrating; handleCollision then has to recover with extra hacks. Nudge apart here using a tiny deterministic offset to prevent divide-by-zero and stabilize resolution.
- if (length >= dist || length == 0.0) - return; // Already long enough, or degenerate point + if (length >= dist) return; // Already long enough + if (length == 0) { + // Pick a stable axis to separate + dx = 1 << (SPHERE_PREC_SHIFT - 8); + dy = 0; + length = fixedDist(dx, dy); + }
🧹 Nitpick comments (7)
wled00/FX.cpp (7)
7579-7586: Prefer hw_random utilities over random() for consistency and portability*WLED uses hw_random8/16 for speed and platform consistency. Replace Arduino random() uses here.
- light->colorIdx = random8(); + light->colorIdx = hw_random8(); ... - int cycleTime = skewedRandom(random(100), pSize, wkgPercentages) * maximumTime / 100 + 20; + int cycleTime = skewedRandom(hw_random8(100), pSize, wkgPercentages) * maximumTime / 100 + 20; ... - light->twData |= random(50, cycleTime) << TIME_TO_EVENT_SHIFT & TIME_TO_EVENT; + light->twData |= ( (hw_random16(uint16_t(max(1,cycleTime-50))) + 50u) << TIME_TO_EVENT_SHIFT ) & TIME_TO_EVENT; ... - if (SEGMENT.check1) - light->colorIdx = random8(); - eventTime += random(10, ((light->twData & MAX_CYCLE) >> MAX_CYCLE_SHIFT) / 3); // turn ON + if (SEGMENT.check1) light->colorIdx = hw_random8(); + eventTime += (hw_random16(uint16_t(max(1, ((light->twData & MAX_CYCLE) >> MAX_CYCLE_SHIFT) / 3 - 10))) + 10);Also applies to: 7613-7619
7780-7784: Avoid copying MBSphere in areSpheresColliding()Passing by value copies the sphere every call (O(n²) loop). Pass by const reference to reduce overhead.
- bool areSpheresColliding(MBSphere sp) + bool areSpheresColliding(const MBSphere& sp)
7829-7836: Remove Serial prints and busy-wait in collision hot pathThe while-loop with Serial.println in handleCollision fires in-frame and can stall effects. Replace with a single epsilon nudge without I/O.
- while (dist == 0) { - // handle gracefully - Serial.println("Two objects on top of each other!"); - x += 1 << (SPHERE_PREC_SHIFT -2); - nx += 1 << (SPHERE_PREC_SHIFT -2); - dist = fixedDist(nx, ny); - } + if (dist == 0) { + // Nudge along Y by tiny epsilon to define a normal + ny = 1 << (SPHERE_PREC_SHIFT - 8); + dist = fixedDist(nx, ny); + }
8165-8173: Clear before draw: prefer SEGMENT.fill(BLACK) for 2D as wellIterating setPixelColor over SEGLEN each frame is fine, but fill(BLACK) is clearer and may be optimized in driver backends.
- for (int i = 0; i < SEGLEN; ++i) - SEGMENT.setPixelColor(i, CRGB::Black); + SEGMENT.fill(BLACK);
11512-11514: Registration names: keep naming consistent_Datum for Elastic Collisions is named data_FXMODE_ELASTICCOLLISIONS (missing underscore after FX), while most entries use data_FX_MODE*. Consider renaming for consistency.
7484-7663: Functional overlap and footprint: consider integrating Xmas Twinkle into TwinklecatGiven reviewer concerns on code size/perf, Xmas Twinkle’s palette-index-stickiness and long/short cycle logic could be an option in Twinklecat (e.g., “Vintage bimetal twinkle” toggle) rather than a new FX. Would simplify maintenance and reduce flash growth.
8035-8208: Elastic Collisions complexity: add a “lite” mode to bound CPU on small MCUsAA circle rasterization plus O(n²) collision checks gets heavy at 30 spheres on ESP32-C3/8266. Add a flag/slider to:
- disable AA (draw hard discs),
- cap collision checks by spatial binning or reduce sphere count at high speeds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wled00/FX.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware files
Files:
wled00/FX.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
PR: wled/WLED#4891
File: wled00/FX.cpp:3333-3349
Timestamp: 2025-08-31T03:38:14.114Z
Learning: WLED PacMan effect (wled00/FX.cpp): Keep pacmancharacters_t position fields as signed int (not int16_t). Maintainer preference (blazoncek) prioritizes avoiding potential overhead/regressions over minor RAM savings. Avoid type shrinking here unless memory pressure is demonstrated.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
PR: wled/WLED#4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
PR: wled/WLED#4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.cpp
🔇 Additional comments (1)
wled00/FX.cpp (1)
7535-7543: Incorrect weighting math in weightPercentages (left term not shifted) — causes biased tablesThe right term is scaled down, but arg1[i]*arg1Factor isn't, so the sum is not normalized. Shift the sum of both weighted terms, not just arg2’s contribution.
void weightPercentages(const uint8_t *arg1, const uint8_t *arg2, const int cnt, const uint32_t factor, // 0.0-1.0 weight given to arg2 << RAND_PREC_SHIFT uint8_t *result) { - uint32_t arg1Factor = (1 << RAND_PREC_SHIFT) - factor; - for (int i = 0; i < cnt; ++i) - result[i] = arg1[i] * arg1Factor + arg2[i] * factor >> RAND_PREC_SHIFT; + const uint32_t arg1Factor = (1u << RAND_PREC_SHIFT) - factor; + for (int i = 0; i < cnt; ++i) { + const uint32_t wsum = uint32_t(arg1[i]) * arg1Factor + uint32_t(arg2[i]) * factor; + result[i] = uint8_t(wsum >> RAND_PREC_SHIFT); + } }⛔ Skipped due to learnings
Learnt from: DedeHai PR: wled/WLED#4798 File: wled00/FX.cpp:7531-7533 Timestamp: 2025-08-26T11:51:21.817Z Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
| nfixed smoothstep(nfixed edge0, nfixed edge1, nfixed x) | ||
| { | ||
| // float t = clamp((x - edge0) / (edge1 - edge0), 0.0f, 1.0f); | ||
| // nfixed t = clamp(fixedDiv(x - edge0, edge1 - edge0), 0, 1 << SPHERE_PREC_SHIFT); | ||
| // return t * t * (3.0f - 2.0f * t); | ||
|
|
||
| // Use a faster divide and multiply using Q24.8 numbers instead of Q16.16. | ||
| edge0 >>= 8; | ||
| edge1 >>= 8; | ||
| x >>= 8; | ||
| int t = clamp((x - edge0 << 8) / (edge1 - edge0), 0, 1 << 8); // Q24.8 | ||
| return (t * t >> 8) * ((3 << 8) - 2 * t); // Result of cubing is Q16.16. | ||
| } |
There was a problem hiding this comment.
smoothstep() returns wrong fixed-point scale (breaks anti-alias). Normalize to Q16.16
Current math mixes Q24.8 and claims Q16.16, producing inconsistent alpha and edge artifacts. Compute t in Q16.16 and keep all ops in Q16.16.
- nfixed smoothstep(nfixed edge0, nfixed edge1, nfixed x)
- {
- // float t = clamp((x - edge0) / (edge1 - edge0), 0.0f, 1.0f);
- // nfixed t = clamp(fixedDiv(x - edge0, edge1 - edge0), 0, 1 << SPHERE_PREC_SHIFT);
- // return t * t * (3.0f - 2.0f * t);
-
- // Use a faster divide and multiply using Q24.8 numbers instead of Q16.16.
- edge0 >>= 8;
- edge1 >>= 8;
- x >>= 8;
- int t = clamp((x - edge0 << 8) / (edge1 - edge0), 0, 1 << 8); // Q24.8
- return (t * t >> 8) * ((3 << 8) - 2 * t); // Result of cubing is Q16.16.
- }
+ nfixed smoothstep(nfixed edge0, nfixed edge1, nfixed x)
+ {
+ // t in Q16.16, clamped to [0,1]
+ if (edge1 <= edge0) return 0;
+ nfixed t = fixedDiv(x - edge0, edge1 - edge0);
+ if (t < 0) t = 0;
+ if (t > (1 << SPHERE_PREC_SHIFT)) t = (1 << SPHERE_PREC_SHIFT);
+ // t*t*(3 - 2*t) in Q16.16
+ nfixed t2 = fixedMult(t, t);
+ nfixed three = (3 << SPHERE_PREC_SHIFT);
+ return fixedMult(t2, three - (t << 1));
+ }📝 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.
| nfixed smoothstep(nfixed edge0, nfixed edge1, nfixed x) | |
| { | |
| // float t = clamp((x - edge0) / (edge1 - edge0), 0.0f, 1.0f); | |
| // nfixed t = clamp(fixedDiv(x - edge0, edge1 - edge0), 0, 1 << SPHERE_PREC_SHIFT); | |
| // return t * t * (3.0f - 2.0f * t); | |
| // Use a faster divide and multiply using Q24.8 numbers instead of Q16.16. | |
| edge0 >>= 8; | |
| edge1 >>= 8; | |
| x >>= 8; | |
| int t = clamp((x - edge0 << 8) / (edge1 - edge0), 0, 1 << 8); // Q24.8 | |
| return (t * t >> 8) * ((3 << 8) - 2 * t); // Result of cubing is Q16.16. | |
| } | |
| nfixed smoothstep(nfixed edge0, nfixed edge1, nfixed x) | |
| { | |
| // t in Q16.16, clamped to [0,1] | |
| if (edge1 <= edge0) return 0; | |
| nfixed t = fixedDiv(x - edge0, edge1 - edge0); | |
| if (t < 0) t = 0; | |
| if (t > (1 << SPHERE_PREC_SHIFT)) t = (1 << SPHERE_PREC_SHIFT); | |
| // Compute t*t*(3 - 2*t) in Q16.16 | |
| nfixed t2 = fixedMult(t, t); | |
| nfixed three = (3 << SPHERE_PREC_SHIFT); | |
| return fixedMult(t2, three - (t << 1)); | |
| } |
|
On Aug 17, 2025, at 7:59 AM, Damian Schneider ***@***.***> wrote:
I also have a trick in at least one of the PS FX to use aux to track all sliders: just sum them all up and store.
I’m already using .aux0 & 1 but there is spare byte there. I’ll xor the sliders. This will work because only one slider changes at a time.
I can only speak on my behalf and the "Elastic Collisions" effect is just too bulky for what it is and there is PS Blobs which is quite close in overall looks and much more versatile. Except it does not handle mass.
The new blinking lights effect could indeed be very interesting on pucks or bullet strings to make them look "old school". Since its very close to twinklecat, extending that FX makes more sense to me then we feel that there are too many FX already in the list plus there is the issue of limited IDs. Both these issues will be addressed in the future but we do not have a good solution just yet.
I’ve been doing a bit of poking around and recoding.
It seems to me that Colortwinkles is the closest to my effect, rather than either TwinkleCat or Fox.
Yes, most of the code is for Elastic Collisions. I’ll look at recoding it to use the PS objects.
Looking around, Xmas Twinkles has about the same code load as other effects.
At a basic level, Elastic Collisions and Blobs are similar, but there are several differences.
• Elastic Collisions uses a fractional coordinate space and anti-alias drawing. This is a major difference. See how smoothly Elastic Collisions particles move, particularly at very low speeds. Blobs movement is very jerky and Blob objects are awkward because the blobs can only have integer sizes and velocities. Blob objects don’t appear to touch before bounding off each other.
• Collisions works on 1D as well as 2D displays. Try Collisions on an LED string. Try a uniformity of 255, which is special.
• Collision objects have mass and momentum. Mass is just radius cubed. Collision object initial velocities are the inverse of size to keep their momentums more equitable.
I have laboriously recoded the effects to use fixed point—ripping out all the floats. I use Q16.16 (32 bit with the binary point in the middle) arithmetic. This had a minor effect on Xmas Twinkles, but it took two weeks to redo Elastic Collisions. While in theory, you can do most calculations just as accurately using fixed point arithmetic as floating point, the bookkeeping necessary to avoid overflows and precision loss is pretty breathtaking. I’m afraid this did nothing to help the code load of Collisions. I also had to include a fast int64 floating point SQRT function.
A radical idea is to replace PS Blobs code with Elastic Collisions code. The differences, which could be adjusted, are:
• Blob size distribution is different.
• How the effects regenerate. Control with a check box?
• You’d gain smooth movement and sizing.
• You’d gain working on a 1D string.
Here is my project list as I see it:
• Catch all slider changes so they affect the effects immediately.
• Use retired effect numbers for any effects I keep standalone.
• Fix column indents in the source code.
• I’ve already ripped out all the floating point, but I have not reduced code size.
• Options for Xmas Twinkle: Add it to Colortwinkles as a checkbox—labeled “Classic", to TwinkleCat or keep it standalone. I really think it is best standalone. It supports 0d, vs. all the other twinkle effects. I believe it is sufficiently different to be its own effect, rather than a ‘hidden’ feature. But it is your call.
• Look at recoding Elastic Collisions using PS objects. This could reduce its code footprint. PS objects would need to support fractional coordinates. It might be possible to “fake it” by multiplying all coordinates by a factor, 256, say and reducing it out in a draw function. Using Q16.16 arithmetic, like I do now, runs into overflow problems when you multiply, but Q24.8 may avoid that. But it would require using int32_t for PSparticle position.
Having the particle systems work with fractional coordinates could open up the possibility of adding still more sophisticated effects. I noticed a trend in making effects more ‘interesting’—that have sophisticated life cycles, like PS Hourglass, rather than just blink and flash.
Hmmm, Maybe you have some ideas here? I’m just throwing thoughts out. I need to understand the PS world more.
• I’ll keep making my changes into this pull request, but if you want, I can eventually merge out my changes with a new PR.
Best regards,
Nick
|
it does, but colortwinkle for example uses 1/3 of your code. You could achieve an almost identical effect with much less code. It is way over-engineered the way it is, there are some things like static arrays that are inacceptable. If you want I can show you what I did with the help of claude.ai to mimic your effect with much less code and no static ram used and I am sure it can be improved upon. I am sorry if I am being picky but we spent countless hours to cut down code size and make as many features fit as possible. Spending 1k of code on a (sophisticated) blink effect is just way too much IMHO. Some of your statements about the PS are just not correct, you should look at it more closely.
so does the PS.
they are jerky on purpose to mimic the old effect. look at ballpit.
nothing wrong with that, size goes from 0-255, velocity is ±127. Its a tradeoff between accuracy, speed and RAM. Try instantiating 1000 of your spherese and see what you get ;) -> Blob objects don’t appear to touch before bounding off each other. that is true, the size calculation is not perfectly accurate and could use improvement but I was not able to make it better without taking away speed, remember: some effects use hundreds of particles.
I did, looks nice. you did not try the PS version I suggested I assume.
I do not doubt that one can do nice things by spending lots of flash and CPU time (and floats).
No. But you could present a PS version of your code. It will take you less than 2 weeks to change a few lines in PS Blobs and put it in a new FX ;) As stated initially, you can add your effect to the user-FX usermod. Adding another "physics engine" just for one effect is just not going to fly. With the PS it will look almost the same and "good enough" for 20% of the code size. p.s. |
|
I briefly looked through your code, you may want to read this: #4206 |
… Elastic Collisions.
… Elastic Collisions.
…solute time in light->timing.nextEvent.
…me' parameter was defined as uint8_t instead of uint16_5, so it was being truncated.
|
Hi DedeHai,
I plugged in your code to try out. I basically like the blinking pattern but I found two serious bugs and an optimization in the code.
• TwinkleLight.timing.nextEvent holds an absolute time, rather than an increment. Even though it is centiseconds, after a few minutes it overflows and the display starts flashing 40 times/second. I now store an increment there, in ms. millis() will also wrap around, eventually, and I handle this too.
• skewedTime() parameter was defined as uint8_t, not uint16_t—too small to hold the parameter passed to it.
‘ skewedTime() has floating point in it. ‘normalized’ can be defined a uint32_t and the order of calculation can be reordered to preserve precision.
And it does need some more tuning, so other than a simpler random generator, I don’t know how much it saves over my version. I’m going to work on making Elastic Collisions a UserMod and then revisit this. I’ll keep you posted.
Nick
… On Mar 24, 2026, at 1:36 AM, Damian Schneider ***@***.***> wrote:
ust for reference https://gist.github.com/DedeHai/bbeaa71fa613610e6a7683f4688a48b5 my suggested simplified version of the twinkle effect.
|
|
My code is just a rough sketch as a suggestion, definitely nothing finished. The main difference is code size and complexity. |
|
OK,
When I revisit it, I will probably use elements from both yours and mine.
… On Apr 23, 2026, at 8:04 AM, Damian Schneider ***@***.***> wrote:
My code is just a rough sketch as a suggestion, definitely nothing finished. The main difference is code size and complexity.
|
…tages' in PROGMEM.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
wled00/FX.h (1)
375-376: MODE_COUNT invariant and ID assignment look correct.
FX_MODE_XMASTWINKLE = 220is the correct next ID. The only numeric gaps in the SR-compat range (169–171) carry comments saying they are reserved for future re-addition of removed SR effects, so they are not available for an unrelated effect.MODE_COUNT = 221 = 220 + 1✓ — invariant holds.- Per the AI summary,
mode_XmasTwinkleand_data_FX_MODE_XMASTWINKLEare now registered insetupEffectData(), resolving the earlier missing-registration concern.One forward-looking note:
_modeCountis typed asuint8_t(line 1041), capping the total effect count at 255. With 221 effects registered, only 34 IDs remain before overflow. No action needed in this PR, but worth tracking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/FX.h` around lines 375 - 376, The MODE_COUNT invariant and ID assignment are correct: ensure FX_MODE_XMASTWINKLE is defined as 220 and MODE_COUNT as 221 (so FX_MODE_XMASTWINKLE + 1 = MODE_COUNT), and confirm that mode_XmasTwinkle and _data_FX_MODE_XMASTWINKLE are registered in setupEffectData(); additionally note that _modeCount is a uint8_t (max 255) so track total effects growth though no code changes are required in this PR.usermods/elastic_collisions/Elastic_Collisions.cpp (3)
300-303: 💤 Low valueDead code in
alphaexpression and stale commented-out lines.The trailing
+ 0on Line 301 has no effect, and Lines 300/302/303 are commented-out alternatives left from the float-based prototype. Worth cleaning up to make the rendering path easier to follow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/elastic_collisions/Elastic_Collisions.cpp` around lines 300 - 303, Remove the dead trailing "+ 0" and stale float-prototype comments from the nfixed alpha assignment to keep the rendering path clean: update the nfixed alpha expression using clamp((1 << SPHERE_PREC_SHIFT) - smoothstep(edge0, edge1, dist), 0, 1 << SPHERE_PREC_SHIFT) (no "+ 0"), and delete the commented-out float and alternative alpha lines so only the fixed-point calculation for alpha remains; ensure references to nfixed alpha, clamp, smoothstep, and SPHERE_PREC_SHIFT are preserved.
142-142: 💤 Low valueComparing
nfixed(int32_t) against float literals.
length == 0.0andalpha > 0.0cause an implicitint32_t → floatconversion on every check. Use integer literals to keep the math purely fixed-point and to avoid the FPU/soft-float tax called out elsewhere in this PR.♻️ Suggested fix
- if (length >= dist || length == 0.0) + if (length >= dist || length == 0) @@ - if (alpha > 0.0) + if (alpha > 0)Also applies to: 314-314
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/elastic_collisions/Elastic_Collisions.cpp` at line 142, Replace float literals with integer/fixed-point literals and do comparisons using the same integer type as nfixed to avoid implicit int→float conversions; specifically, update checks involving length, dist, alpha and nfixed (e.g., the condition using length >= dist || length == 0.0 and any alpha > 0.0 checks) to compare against 0 or the appropriate fixed-point integer constant (or convert values to the fixed-point scale used by nfixed) so the comparisons are purely integer-based and avoid FPU/soft-float conversions.
96-112: 💤 Low valueWrap AI-generated blocks with the project's
// AI:markers.The Newton–Raphson sqrt block (97–112, "curtesy Code Copilot 5") and the
sliderToSpeed()quadratic ("Thank you Claude.ai", 362–377) are flagged in-source as AI-authored. Per project convention they should be delimited so reviewers and future maintainers know the provenance:// AI: below section was generated by an AI ... // AI: endAs per coding guidelines: "Mark AI-generated code blocks with // AI: below section was generated by an AI and // AI: end comments in C++".
Also applies to: 362-377
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/elastic_collisions/Elastic_Collisions.cpp` around lines 96 - 112, The Newton–Raphson sqrt implementation inside fixed64Sqrt and the sliderToSpeed quadratic block are AI-generated and must be wrapped with the project's AI markers; locate the fixed64Sqrt function (the sqrt implementation with the "curtesy Code Copilot 5" comment) and the sliderToSpeed function (the quadratic block labelled "Thank you Claude.ai") and add a leading comment "// AI: below section was generated by an AI" immediately before each block and a trailing comment "// AI: end" immediately after each block so the AI provenance is clearly delimited.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@usermods/elastic_collisions/Elastic_Collisions.cpp`:
- Around line 146-150: Replace raw Serial.println calls in
Elastic_Collisions.cpp (the branch checking "if (length << 1 == 0)" and the
later block around lines 170-178 where "Two objects on top of each other!" is
commented) with the project's debug macro DEBUG_PRINTLN (or remove them
entirely). Locate the collision-handling function that uses the variable length
and change Serial.println("At 0 `#1`") and any other Serial.* debug output to
DEBUG_PRINTLN(...) so the messages are compiled out unless -D WLED_DEBUG is set;
ensure you update all similar Serial prints in that function to use
DEBUG_PRINTF/DEBUG_PRINTLN consistently.
- Line 477: SEGMENT.aux1 is a uint16_t and cannot hold elasticLifetime() values
up to 72000, causing wrap; change the storage or representation so lifetime no
longer overflows: either store the full-cycle count in a larger field (e.g., use
SEGMENT.step if it is a wider integer or add/repurpose a 32-bit field), or
encode the lifetime in scaled units (e.g., seconds/slots so the value <= 65535),
or combine SEGMENT.aux0 and SEGMENT.aux1 into a 32-bit value when
reading/writing the lifetime; update usages around SEGMENT.aux1 assignment and
the code that checks/decrements that lifetime to use the new storage/scale so
the 1hr and 30m cases are represented correctly.
- Around line 421-441: The init loop uses floating literals and divisions when
computing radius, vx and vy; replace those with integer/Q16.16 fixed-point math
to avoid float round-trips: compute random offsets as (random(100) - 50) and use
fixed-point multipliers/shifts instead of -50.0, /5.0, /500.0 and /10; keep
types nfixed and use MBSphere::fixedDiv where needed, e.g., convert constants to
Q16.16 (shift left by SPHERE_PREC_SHIFT or 16) and replace divides by 5, 500 and
10 with multiply-by-inverse-integer or right-shifts appropriate for Q16.16, and
ensure any unsigned casts go via a signed int; update expressions that set
radius, vx and vy (and the branch that sets vx = 1 << (SPHERE_PREC_SHIFT - 1))
to only use integer/shift operations and fixed-point helpers like
MBSphere::fixedDiv.
- Around line 481-484: The loop that clears LEDs using SEGLEN and
SEGMENT.setPixelColor leaves many pixels uncleared on 2D segments; inside the if
(millis() > SEGMENT.step) block replace the manual for-loop over
SEGLEN/SEGMENT.setPixelColor with the segment helper SEGMENT.fill(BLACK) (or
SEGMENT.fill(CRGB::Black) to match existing color constants) so the clear
respects 1D/2D mapping (target symbols: SEGMENT.fill, SEGLEN,
SEGMENT.setPixelColor, if (millis() > SEGMENT.step)).
- Around line 402-405: The local PROGMEM array dmPercentages declared inside
mode_ElasticCollisions is allocated on the stack (so PROGMEM is ineffective on
ESP8266 and causes repeated emission on ESP32); move the array to file/static
scope as a static const uint8_t PROGMEM dmPercentages[] and update
skewedRandom() to read values via pgm_read_byte_near(&dmPercentages[index]) (or
the appropriate pgm_read_* accessor) instead of direct indexing so the table
truly lives in flash and is read correctly on ESP8266/ESP32.
- Around line 530-540: Add the new usermod ID constant to wled00/const.h so the
ElasticCollisionsUsermod is registered per guidelines: after the existing
USERMOD_ID_USER_FX 58 entry add a new define named USERMOD_ID_ELASTIC_COLLISIONS
with the next ID (59) and a comment referencing "Elastic_Collisions.cpp"; this
ties the ElasticCollisionsUsermod/REGISTER_USERMOD and its use of
strip.addEffect(255, &mode_ElasticCollisions, _data_FXMODE_ELASTICCOLLISIONS) to
a unique USERMOD_ID.
- Around line 270-285: The code sets vx and vy to 0 when the sphere is fully
off-screen but still proceeds to clamp coordinates and run the expensive
per-pixel rendering loop; change the logic in the function containing
lowX/highX/lowY/highY so that after detecting the fully-off-screen condition
(lowX > gridW || highX < 0 || lowY > gridH || highY < 0) you set vx = vy = 0 and
immediately return/skip the rest of the rendering work (i.e., do not run the
clamps or the per-pixel loop), ensuring the per-pixel sqrt work is avoided for
off-screen spheres.
In `@usermods/elastic_collisions/README.md`:
- Around line 3-35: Update the README so the Uniformity range is consistent and
fix the typos: change the top description's "Uniformity 0-255" to "Uniformity
0-100%" (match the Parameters section), correct "ranage" to "range" in the
Parameters description for Speed, and change "Regenration" to "Regeneration" in
the Lifetime parameter; also tidy the punctuation in the sentence "You can
control; their Speed (velocity), Number of balls; 1-30, Uniformity ..." to a
clearer form like "You can control their Speed (velocity), Count (1–30),
Uniformity (0–100%), and regeneration time (15 seconds to 1 hour)" so both the
top description and the "Parameters" section read consistently and cleanly.
---
Nitpick comments:
In `@usermods/elastic_collisions/Elastic_Collisions.cpp`:
- Around line 300-303: Remove the dead trailing "+ 0" and stale float-prototype
comments from the nfixed alpha assignment to keep the rendering path clean:
update the nfixed alpha expression using clamp((1 << SPHERE_PREC_SHIFT) -
smoothstep(edge0, edge1, dist), 0, 1 << SPHERE_PREC_SHIFT) (no "+ 0"), and
delete the commented-out float and alternative alpha lines so only the
fixed-point calculation for alpha remains; ensure references to nfixed alpha,
clamp, smoothstep, and SPHERE_PREC_SHIFT are preserved.
- Line 142: Replace float literals with integer/fixed-point literals and do
comparisons using the same integer type as nfixed to avoid implicit int→float
conversions; specifically, update checks involving length, dist, alpha and
nfixed (e.g., the condition using length >= dist || length == 0.0 and any alpha
> 0.0 checks) to compare against 0 or the appropriate fixed-point integer
constant (or convert values to the fixed-point scale used by nfixed) so the
comparisons are purely integer-based and avoid FPU/soft-float conversions.
- Around line 96-112: The Newton–Raphson sqrt implementation inside fixed64Sqrt
and the sliderToSpeed quadratic block are AI-generated and must be wrapped with
the project's AI markers; locate the fixed64Sqrt function (the sqrt
implementation with the "curtesy Code Copilot 5" comment) and the sliderToSpeed
function (the quadratic block labelled "Thank you Claude.ai") and add a leading
comment "// AI: below section was generated by an AI" immediately before each
block and a trailing comment "// AI: end" immediately after each block so the AI
provenance is clearly delimited.
In `@wled00/FX.h`:
- Around line 375-376: The MODE_COUNT invariant and ID assignment are correct:
ensure FX_MODE_XMASTWINKLE is defined as 220 and MODE_COUNT as 221 (so
FX_MODE_XMASTWINKLE + 1 = MODE_COUNT), and confirm that mode_XmasTwinkle and
_data_FX_MODE_XMASTWINKLE are registered in setupEffectData(); additionally note
that _modeCount is a uint8_t (max 255) so track total effects growth though no
code changes are required in this PR.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75f7e311-fe55-4b28-a094-9b9616023b24
📒 Files selected for processing (5)
usermods/elastic_collisions/Elastic_Collisions.cppusermods/elastic_collisions/README.mdusermods/elastic_collisions/library.jsonwled00/FX.cppwled00/FX.h
✅ Files skipped from review due to trivial changes (1)
- usermods/elastic_collisions/library.json
👮 Files not reviewed due to content moderation or server errors (1)
- wled00/FX.cpp
| // Radius distribution. | ||
| const int dmTableSize = 20; | ||
| const uint8_t PROGMEM dmPercentages[20] = {40, 20, 10, 4, 3, 17, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 3}; | ||
|
|
There was a problem hiding this comment.
Local PROGMEM array is wrong on ESP8266 and largely defeats PROGMEM elsewhere.
const uint8_t PROGMEM dmPercentages[20] = { ... } is declared inside mode_ElasticCollisions. On ESP8266, PROGMEM only works for variables with static storage; on a stack/auto variable, the data is not placed in flash and pgm_read_byte_near is required to read it (which skewedRandom() does not do). On ESP32 it happens to work because flash is memory-mapped, but the array is still re-emitted on every call.
Move to file/static scope and read with pgm_read_byte_near(...) in skewedRandom():
♻️ Suggested fix
+static const uint8_t dmPercentages[] PROGMEM = {40, 20, 10, 4, 3, 17, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 3};
+static constexpr uint8_t dmTableSize = sizeof(dmPercentages);
@@ skewedRandom(...)
- while (index < pArraySize - 1 && rand100 >= cumulativePercentage + pArray[index]) {
- cumulativePercentage += pArray[index];
+ while (index < pArraySize - 1 && rand100 >= cumulativePercentage + pgm_read_byte_near(pArray + index)) {
+ cumulativePercentage += pgm_read_byte_near(pArray + index);
index++;
}
- int32_t t = ((rand100 - cumulativePercentage) << RAND_PREC_SHIFT) / pArray[index];
+ int32_t t = ((rand100 - cumulativePercentage) << RAND_PREC_SHIFT) / pgm_read_byte_near(pArray + index);
@@ mode_ElasticCollisions
- const int dmTableSize = 20;
- const uint8_t PROGMEM dmPercentages[20] = {40, 20, 10, 4, 3, 17, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 3};As per coding guidelines: "Store repeated strings as static const char[] PROGMEM in C++ usermod code" (and the same applies to PROGMEM tables — they must have static storage to actually live in flash and be read with pgm_read_* accessors).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usermods/elastic_collisions/Elastic_Collisions.cpp` around lines 402 - 405,
The local PROGMEM array dmPercentages declared inside mode_ElasticCollisions is
allocated on the stack (so PROGMEM is ineffective on ESP8266 and causes repeated
emission on ESP32); move the array to file/static scope as a static const
uint8_t PROGMEM dmPercentages[] and update skewedRandom() to read values via
pgm_read_byte_near(&dmPercentages[index]) (or the appropriate pgm_read_*
accessor) instead of direct indexing so the table truly lives in flash and is
read correctly on ESP8266/ESP32.
| class ElasticCollisionsUsermod : public Usermod { | ||
| public: | ||
| void setup() override { | ||
| strip.addEffect(255, &mode_ElasticCollisions, _data_FXMODE_ELASTICCOLLISIONS); | ||
| } | ||
|
|
||
| void loop() override {} | ||
| }; | ||
|
|
||
| static ElasticCollisionsUsermod elastic_collisions; | ||
| REGISTER_USERMOD(elastic_collisions); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Inspect addEffect signature/semantics for the magic id 255.
ast-grep --pattern 'uint8_t WS2812FX::addEffect($$$) { $$$ }'
rg -nP --type=cpp -C3 '\baddEffect\s*\('
# 2. Confirm a USERMOD_ID for elastic_collisions exists in const.h.
rg -nP -C2 'USERMOD_ID_' wled00/const.h | head -n 80
rg -nP 'ELASTIC|elastic' wled00/const.hRepository: wled/WLED
Length of output: 41491
Add USERMOD_ID_ELASTIC_COLLISIONS to wled00/const.h.
The use of addEffect(255, ...) is correct—255 is the documented sentinel for usermod effects (the function searches for the first available slot). However, per the coding guidelines, new usermods must have an ID entry in wled00/const.h. Add a new line following USERMOD_ID_USER_FX 58:
`#define` USERMOD_ID_ELASTIC_COLLISIONS 59 //Usermod "Elastic_Collisions.cpp"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@usermods/elastic_collisions/Elastic_Collisions.cpp` around lines 530 - 540,
Add the new usermod ID constant to wled00/const.h so the
ElasticCollisionsUsermod is registered per guidelines: after the existing
USERMOD_ID_USER_FX 58 entry add a new define named USERMOD_ID_ELASTIC_COLLISIONS
with the next ID (59) and a comment referencing "Elastic_Collisions.cpp"; this
ties the ElasticCollisionsUsermod/REGISTER_USERMOD and its use of
strip.addEffect(255, &mode_ElasticCollisions, _data_FXMODE_ELASTICCOLLISIONS) to
a unique USERMOD_ID.
|
I've rewritten Xmas Twinkle with suggested and simplified skewed Random number generator, now converted to Fixed Point. I added a new control, "Avg. Duty Cycle" that controls on/off cycle length. See if you like that, and the effect name. Elastic Collisions is now a UserMod as it is acres of code and I'm not sure how to use the Particle System for it, particularly since I do sub-pixel calculations. I also fixed all the code indentation. My changes are in the "ElasticAndTwinkle" branch. I didn't know if I should merge it into the 'main' branch of my PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
wled00/FX.h (2)
375-376:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify
FX_MODE_XMASTWINKLEis registered insetupEffectData()A prior verification found the
addEffect(FX_MODE_XMASTWINKLE, ...)call missing fromFX.cpp. The AI summary claims it is now present, but please confirm — if it's still absent the slot pre-allocated by thefor (i=1; i<_modeCount; i++)loop will remain a_data_RESERVEDplaceholder, and the effect will never appear in the UI.#!/bin/bash # Description: Confirm addEffect is called for FX_MODE_XMASTWINKLE in FX.cpp # and that MODE_COUNT == highest FX_MODE_* ID + 1. echo "=== 1. addEffect registration for FX_MODE_XMASTWINKLE ===" rg -n --type=cpp 'addEffect\s*\(\s*FX_MODE_XMASTWINKLE' || echo "MISSING: addEffect(FX_MODE_XMASTWINKLE, ...) not found" echo "" echo "=== 2. mode_XmasTwinkle definition ===" rg -n --type=cpp 'uint16_t\s+mode_XmasTwinkle\s*\(' || echo "MISSING: mode_XmasTwinkle() not found" echo "" echo "=== 3. Sanity check: MODE_COUNT == max(FX_MODE_*) + 1 ===" python3 - << 'PY' import re, glob, sys paths = glob.glob("**/FX.h", recursive=True) if not paths: print("FX.h not found"); sys.exit(1) h = open(paths[0]).read() ids = [(m.group(1), int(m.group(2))) for m in re.finditer(r'#define\s+(FX_MODE_\w+)\s+(\d+)', h)] mc = re.search(r'#define\s+MODE_COUNT\s+(\d+)', h) if not ids or not mc: print("Parse failed"); sys.exit(1) max_name, max_id = max(ids, key=lambda x: x[1]) mc_val = int(mc.group(1)) print(f"Highest FX_MODE_* : {max_name} = {max_id}") print(f"MODE_COUNT : {mc_val}") print(f"OK (== max+1) : {mc_val == max_id + 1}") if mc_val != max_id + 1: sys.exit(2) PY🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/FX.h` around lines 375 - 376, The review warns FX_MODE_XMASTWINKLE is not registered; check setupEffectData() in FX.cpp and ensure there is an addEffect(FX_MODE_XMASTWINKLE, ...) call to replace the _data_RESERVED slot so the effect appears in the UI; also verify the mode implementation exists as mode_XmasTwinkle() and that MODE_COUNT equals the highest FX_MODE_* value plus one (adjust `#define` MODE_COUNT if needed).
375-376:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGaps 169–171 remain unused — reuse a gap instead of extending ceiling to 220.
IDs 169 (
FX_MODE_2DPOOLNOISE), 170 (FX_MODE_2DTWISTER), and 171 (FX_MODE_2DCAELEMENTATY) are commented out and available. AssigningFX_MODE_XMASTWINKLE = 169(or 170/171) and keepingMODE_COUNTunchanged at 220 would fill a gap rather than grow the table.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/FX.h` around lines 375 - 376, FX_MODE_XMASTWINKLE is currently set to 220 which expands the enum ceiling instead of reusing unused IDs 169–171; change the value of FX_MODE_XMASTWINKLE to one of the available IDs (e.g., set FX_MODE_XMASTWINKLE = 169) and leave MODE_COUNT defined as 221 (or keep MODE_COUNT unchanged at 221 if it already reflects the correct total), ensuring you replace references to FX_MODE_XMASTWINKLE accordingly and avoid adding new higher IDs; reference the defines FX_MODE_XMASTWINKLE, FX_MODE_2DPOOLNOISE, FX_MODE_2DTWISTER, FX_MODE_2DCAELEMENTATY, and MODE_COUNT when making the change.usermods/elastic_collisions/README.md (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the Uniformity range consistent in the intro.
The opening description still says
Uniformity 0-255, but the Parameters section documents the same control as0-100%. Please use one representation consistently so readers do not have to guess whether this is a raw slider value or a percentage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/elastic_collisions/README.md` at line 3, The README's intro and Parameters disagree on the "Uniformity" scale; update the intro sentence in the "Elastic Collisions" description so the Uniformity representation matches the Parameters section (use either "0-100%" or "0-255" consistently). Locate the phrase containing "Uniformity 0-255" in the README and change it to the same format used in the Parameters (e.g., "Uniformity 0-100%") and ensure any mention of "Uniformity" elsewhere (including the phrase "Ball colors are random indices into the current palette") remains consistent with that choice.usermods/elastic_collisions/Elastic_Collisions.cpp (1)
478-479:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
SEGMENT.aux1cannot represent the advertised 1-hour lifetime.
elasticLifetime()returns up to72000, but this state is stored inSEGMENT.aux1, which is only 16-bit in WLED. The 1-hour setting therefore wraps to6464ticks and regenerates after about 5.4 minutes instead of 60. Please move this counter to 32-bit storage or rescale the unit so it never exceeds65535.Also applies to: 513-515
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/elastic_collisions/Elastic_Collisions.cpp` around lines 478 - 479, SEGMENT.aux1 is a 16-bit field and cannot hold the up-to-72000 value returned by elasticLifetime(), causing wraparound; update the code to store the lifetime in a 32-bit field (e.g., add or use a SEGMENT.aux1_32 or convert SEGMENT.aux1's type to uint32_t) or change elasticLifetime() to return a scaled unit that never exceeds 65535, then set SEGMENT.aux1 (or the new 32-bit field) from elasticLifetime() and adjust any reads/writes and usages (including the other occurrences referenced around lines 513-515) to use the new 32-bit storage or scaled units consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@usermods/elastic_collisions/Elastic_Collisions.cpp`:
- Around line 97-109: The Copilot/Claude-generated math routine in the
fixed64Sqrt function (and the other larger AI-authored blocks flagged in the
review) must be wrapped with the repository sentinel comments; insert a starting
comment "// AI: below section was generated by an AI" immediately before the
AI-written block (e.g., before the beginning of fixed64Sqrt's generated code)
and a closing "// AI: end" immediately after the block, and do the same for the
other flagged AI-generated sections so each generated region is clearly
delimited.
---
Duplicate comments:
In `@usermods/elastic_collisions/Elastic_Collisions.cpp`:
- Around line 478-479: SEGMENT.aux1 is a 16-bit field and cannot hold the
up-to-72000 value returned by elasticLifetime(), causing wraparound; update the
code to store the lifetime in a 32-bit field (e.g., add or use a SEGMENT.aux1_32
or convert SEGMENT.aux1's type to uint32_t) or change elasticLifetime() to
return a scaled unit that never exceeds 65535, then set SEGMENT.aux1 (or the new
32-bit field) from elasticLifetime() and adjust any reads/writes and usages
(including the other occurrences referenced around lines 513-515) to use the new
32-bit storage or scaled units consistently.
In `@usermods/elastic_collisions/README.md`:
- Line 3: The README's intro and Parameters disagree on the "Uniformity" scale;
update the intro sentence in the "Elastic Collisions" description so the
Uniformity representation matches the Parameters section (use either "0-100%" or
"0-255" consistently). Locate the phrase containing "Uniformity 0-255" in the
README and change it to the same format used in the Parameters (e.g.,
"Uniformity 0-100%") and ensure any mention of "Uniformity" elsewhere (including
the phrase "Ball colors are random indices into the current palette") remains
consistent with that choice.
In `@wled00/FX.h`:
- Around line 375-376: The review warns FX_MODE_XMASTWINKLE is not registered;
check setupEffectData() in FX.cpp and ensure there is an
addEffect(FX_MODE_XMASTWINKLE, ...) call to replace the _data_RESERVED slot so
the effect appears in the UI; also verify the mode implementation exists as
mode_XmasTwinkle() and that MODE_COUNT equals the highest FX_MODE_* value plus
one (adjust `#define` MODE_COUNT if needed).
- Around line 375-376: FX_MODE_XMASTWINKLE is currently set to 220 which expands
the enum ceiling instead of reusing unused IDs 169–171; change the value of
FX_MODE_XMASTWINKLE to one of the available IDs (e.g., set FX_MODE_XMASTWINKLE =
169) and leave MODE_COUNT defined as 221 (or keep MODE_COUNT unchanged at 221 if
it already reflects the correct total), ensuring you replace references to
FX_MODE_XMASTWINKLE accordingly and avoid adding new higher IDs; reference the
defines FX_MODE_XMASTWINKLE, FX_MODE_2DPOOLNOISE, FX_MODE_2DTWISTER,
FX_MODE_2DCAELEMENTATY, and MODE_COUNT when making the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e8dabe0-b97b-43fd-bb9e-8d181fdc9f13
📒 Files selected for processing (5)
usermods/elastic_collisions/Elastic_Collisions.cppusermods/elastic_collisions/README.mdusermods/elastic_collisions/library.jsonwled00/FX.cppwled00/FX.h
✅ Files skipped from review due to trivial changes (1)
- usermods/elastic_collisions/library.json
👮 Files not reviewed due to content moderation or server errors (1)
- wled00/FX.cpp
|
Could it be that we now have a third "elastic collisions" effect?
|
@NPAssoc maybe you could add this effect to the |
I looked at those effects and they are quite a bit different than "Elastic Collisions". With some work, it might be possible to bend "PS Ballpit" around to work like my effect, but it would take a lot of work. And "Elastic Collisions" works on both 1D and 2D arrays, which Ballpit does not. Give "Elastic Collisions a try and see. |
I'll take a look… |
|
given its complexity, I would say it's a good candidate for its own (out of tree) usermod, user FX is possible too but optional |
|
On May 5, 2026, at 1:07 PM, Frank Möhle ***@***.***> wrote:
@NPAssoc <https://github.com/NPAssoc> this alternate "fast SQRT" should not be necessary
I needed a 64 bit integer square root.
Nick
|
…. Fixed potential overflow. Use better LED erase.
5 participants
(My apologies for not successfully squashing sub commits. It is the first time I've done this.)
Xmas Twinkle is an effect that simulates mid-century Christmas tree twinkle lights that use a bimetallic strip to switch the bulb on and off. Because its contact is kind of sticky, the blinking is random. As the bulb heats up or cools down the blinking rate varies. Each bulb sort of has a mind of its own.
In the effect, you can control the average rate of blinking and the density of blinking LEDS in a segment, i.e. what percentage of the LEDs blink or are off. LEDs are either on or off. Each LED is assigned a random, but unchanging, index into the current color palette. It works great with a C-9 color palette. A Checkbox causes a new color index to be generated each time a bulb turns on.
It looks great on 2D displays, imitating fake computer consoles in 1960s science fiction movies. It also works great flashing a single LED.
This simulation uses a short term random cycle to control each blinking LED, and a long term random number to control its average rate. Every 20 seconds an LED's long term rate is adjusted randomly. A general function, 'skewedRandom()', part of the code but may be useful elsewhere, is used to skew the long term random numbers to emphasize ranges of twinkle times.
The code uses 5 bytes/blinking LED to keep track of the long term time interval, time to next on or off cycle, time to next long term recalculate, and color palette index. Manual bit fields are used to cram all this data into 5 bytes.
Elastic Collisions simulates balls randomly hitting each other and bouncing elastically. Balls also bounce off the edges of a display. You can control, their Speed (velocity), Number of balls; 1-30, Uniformity 0-255, and regeneration time 15 seconds to 1 hour. Ball colors are random indices into the current palette.
Balls have a mass that is the cube of their diameter. Collisions conserve their energy and momentum as per the laws of physics
A Uniformity of 255 is special. The balls are initialized with equal mass in a row and only one moving. When it collides with another ball, all momentum is transferred to the next ball and it stops, much like the swinging ball puzzle in "Professor T".
a few seconds before a regenerating a new set, the walls/ends "collapse" and the balls drift off the display.
It works very well on both 2D and 1D displays.
• The code matches the general coding style of other effects as suggested in the WLED advanced documentation and is well annotated.
• The code uses explicit shifting and masking to get at the saved data, the C++ bit fields did not work for me.
• It was extensively developed standalone in an Arduino environment, then merged into the main branch for more testing. The code is well isolated. There is only one block of code in FX.cpp.
• It has been extensively tested on single 300 LED strip segment and a 40x32 2D LED display. It has never crashed or misbehaved.
• Note this code uses single precision floating point. It may be possible to convert it to all fixed point, and maybe even the particle simulation objects, if the effect is found to be worthwhile.
Summary by CodeRabbit
New Features
Chores