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.
WalkthroughAdds two new FX modes: Xmas Twinkle (per-light twinkle system with skewed timing and weighting) and Elastic Collisions (physics-based spheres with collision handling). Introduces supporting structs/classes, helper utilities, metadata strings, and registers both effects. Updates FX.h with two new mode IDs and increments MODE_COUNT. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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.
wled00/FX.cpp
Outdated
| 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.
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 |
|
On Sep 6, 2025, at 1:36 AM, Damian Schneider ***@***.***> wrote:
briefly looked through your code, you may want to read this: #4206 <#4206>
Very cool. Thanks
I see floating point operations/sec. for older chips are pretty grim. I assume those are emulated.
I’m finally going to look at greatly reducing the code load, perhaps by using PS to do it. I didn’t know about static arrays. If I think I’ll need to modify PS, I’ll run it by you.
By the way, I do limit the number of elastic spheres to 32, because they are computationally intensive. Is that too limiting?
I too, have been using Claude & chatGPT to write code, though their recommendations often have to be carefully debugged.
I’ll probably have a question or two. — Nick
P.S. PS Blobs does not show in the Effects list if you don’t have a 2D panel defined.
|
S2 and C3 are newer chip, it all comes down to hardware support: no FPU no good float performance.
if you find the results are not what you wanted, you can put your original effect (even with floats) into the effects usermod, which is the place created exactly for effects that are not well suited for everyone or are more setup specific.
any variable will reside in RAM, if at global scope, they are always in RAM. thats why C(++) has the scope brackets { }
please do, maybe run it by me before spending too much time on it as I tried many things and can tell you if something is doable before its even implemented. In general: things that operate on a per-particle basis, every single instruction counts.
for your effect, no. for a particle system: yes.
They do ok if you know what you want and how to instruct them, but its often more time consuming than just writing the code yourself and yes, you have to carefully check the code and often re-structure and clean it up but they are fantastic to write quick test functions to evaluate something.
its a 2D only effect, PS has two versions: 1D and 2D both with different optimizations to minimize RAM use and maximize speed. I was thinking of writing code to enable "1.5D" rendering for 2D effects i.e. render them to a strip but I found its better to write an effect specifically. I think I wrote over 30 effects, each with many parameters to tune to ones liking. You can for example turn PS Volcano into a "whirlpool" effect by choosing the right parameters. |
(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