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 independent features: a core FX "Xmas Twinkle" (per-LED twinkle state, skewed timing helpers, effect metadata and registration) and a new usermod "Elastic Collisions" (fixed-point sphere physics, soft-circle rendering, collision response, lifetime/regeneration, README and library.json). Also adds an FX ID and increments MODE_COUNT. ChangesXmas Twinkle FX (core effect)
Elastic Collisions usermod
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
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.
|
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 |
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. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
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.
There was a problem hiding this comment.
@NPAssoc I'll ask the rabbit to double-check if the new fixed point math still has any flaws - fixed point can be a real beast ;-)
@coderabbitai please double-check all fixed point calculations (nfixed type, and intermediate steps) to verify that fixed point math works as expected, and fixed point resolution is kept where needed. Also look for undefined behaviour in bitshift ops (compiler-defined behaviour is acceptable, all code is compiled with gcc on 32bit architectures only).
There was a problem hiding this comment.
@softhack007 Here is a detailed audit of all fixed-point operations in Elastic_Collisions.cpp.
🔴 Critical – Wrong operator precedence in smoothstep
// Line: int t = clamp((x - edge0 << 8) / (edge1 - edge0), 0, 1 << 8);Because << has higher precedence than -, this parses as:
x - (edge0 << 8) // ← actual evaluationinstead of the intended:
(x - edge0) << 8 // ← what was meantWhen x is anywhere inside the [edge0, edge1] range, the numerator becomes a large negative number, so t is clamped to 0 every time and the smooth anti-aliasing transition degenerates to a hard cutoff. Fix:
int t = clamp(((x - edge0) << 8) / (edge1 - edge0), 0, 1 << 8);🔴 Significant – fixed64Sqrt parameter type mismatch causes silent zero returns
fixedSqrt builds a uint64_t scaled value and passes it to fixed64Sqrt(int64_t n):
static nfixed fixedSqrt(nfixed x) {
uint64_t n = (uint64_t)x << SPHERE_PREC_SHIFT; // uint64_t
return fixed64Sqrt(n); // ← implicitly cast to int64_t
}
static nfixed fixed64Sqrt(int64_t n) { // signed parameter
if (n <= 0) return 0; // ← fires if MSB was set
...
}If the upper bit of n is set after the shift (possible for x values with integer parts ≥ 32768, or if x itself is negative), the implicit conversion makes n appear negative to the function, which then returns 0 silently. Fix by changing the parameter to uint64_t:
static nfixed fixed64Sqrt(uint64_t n) {
if (n == 0) return 0;
...
}Additionally, fixedSqrt should guard against negative input before casting:
static nfixed fixedSqrt(nfixed x) {
if (x <= 0) return 0;
uint64_t n = (uint64_t)x << SPHERE_PREC_SHIFT;
return fixed64Sqrt(n);
}🔴 Significant – 2 * m2 / 2 * m1 overflow in elastic collision formula
nfixed v1n_final = fixedDiv(fixedMult(v1n, m1 - m2) + fixedMult(2 * m2, v2n), m1 + m2);
nfixed v2n_final = fixedDiv(fixedMult(v2n, m2 - m1) + fixedMult(2 * m1, v1n), m1 + m2);m1 and m2 are Q16.16 int32_t. For the largest spheres (radius ≈ 30 LEDs), mass() ≈ 30³ × 65536 ≈ 1.77 × 10⁹ – within INT32_MAX ≈ 2.147 × 10⁹, but 2 * m2 pushes it to 3.54 × 10⁹, overflowing int32_t. m1 + m2 also overflows in that regime. Fix using int64_t intermediate arithmetic:
nfixed v1n_final = fixedDiv(
fixedMult(v1n, m1 - m2) + (nfixed)((int64_t)2 * m2 * v2n >> SPHERE_PREC_SHIFT),
m1 + m2); // m1+m2 should also be widened if radii are large🟡 Moderate – mass() headroom is narrow; density > 1 overflows
nfixed mass() { return fixedMult(fixedMult(fixedMult(radius, radius), radius), density()); }At radius = 30 (Q16.16), radius³ ≈ 1.77 × 10⁹ (within INT32_MAX). But _density is 1 << 16 by default and setDensity allows any value – calling mass() with density > ~1.2 silently overflows. Either cap density or widen mass() to return int64_t.
🟡 Moderate – Division by zero in fixedDiv / smoothstep
fixedDiv(a, 0) performs (int64_t)a << 16 / 0 – undefined behaviour (integer division by zero). smoothstep calls it with (edge1 - edge0) as the divisor; if the sphere radius is less than half a pixel, edge0 == edge1 after the >> 8 truncation and this divides by zero. A simple guard fixes it:
static nfixed fixedDiv(nfixed a, nfixed b) {
if (b == 0) return 0; // or return max/min depending on context
return ((int64_t)a << SPHERE_PREC_SHIFT) / b;
}🟢 Minor – Mixed integer/floating-point comparisons
Two places compare nfixed (int32_t) against 0.0 (double), which triggers an implicit integer→double conversion:
if (length >= dist || length == 0.0) // enforceMinDist
if (alpha > 0.0) // drawMeThese should use integer zero to avoid the implicit conversion and any potential FPU usage on targets without an FPU:
if (length >= dist || length == 0)
if (alpha > 0)✅ Operations that are correct
| Operation | Assessment |
|---|---|
fixedMult: (int64_t)a * b >> 16 |
Correctly widens before multiply; arithmetic right shift on signed 64-bit is acceptable under GCC |
fixedDist intermediate sum |
In practice coordinates ≤ 300 << 16 ≈ 2²⁴; squared sum ≈ 2⁴⁹, well within int64_t |
smoothstep result precision (once precedence is fixed) |
At t=128→result=32768 (0.5 in Q16.16) ✓; at t=256→result=65536 (1.0) ✓ |
Right shifts on signed int32_t/int64_t throughout |
Implementation-defined but GCC guarantees arithmetic (sign-extending) shift on all ESP32 targets ✓ |
skewedRandom fixed-point (RAND_PREC_SHIFT 10) |
Internal Q22.10 used only as an intermediate and scaled back; no truncation issue |
sliderToSpeed Q16.16 quadratic |
64-bit intermediates used; result fits in int32_t for slider 0–255 ✓ |
Summary of issues by severity:
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | 🔴 Critical | smoothstep |
Operator precedence: x - edge0 << 8 → (x - edge0) << 8 |
| 2 | 🔴 Significant | fixed64Sqrt / fixedSqrt |
uint64_t → int64_t conversion silently returns 0 for large inputs |
| 3 | 🔴 Significant | handleCollision |
2 * m2, 2 * m1, m1 + m2 overflow int32_t for large spheres |
| 4 | 🟡 Moderate | mass() |
Overflow when density > ~1.2 or near-max radius |
| 5 | 🟡 Moderate | fixedDiv |
No guard against divide-by-zero (reachable via smoothstep) |
| 6 | 🟢 Minor | enforceMinDist, drawMe |
== 0.0 / > 0.0 should be == 0 / > 0 |
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
There was a problem hiding this comment.
CodeRabbit is wrong! << has lower precedent in C++ than +/-, not higher. I checked the original C++ volume by Bjarne Stroustrup. I'm fixing the accidental f.p comparisons.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I agree about the operator precedencies - the official reference https://en.cppreference.com/c/language/operator_precedence says the same as good-old Bjarne stated 40 years ago ;-)
Still, it would be good for maintenance if you add some braces around longer expressions, so people reading your code don't draw wrong conclusions ;-)
@coderabbitai double-check your findings related to C/C++ Operator precedence.
There was a problem hiding this comment.
@softhack007 You and @NPAssoc are absolutely right — I made an error. In C/C++, the additive operators (+, -) have higher precedence than the shift operators (<<, >>), so:
x - edge0 << 8correctly parses as (x - edge0) << 8, which is exactly the intended computation. My "🔴 Critical" finding #1 was wrong and I apologize for the false alarm.
That said, softhack007's suggestion to add explicit parentheses is still good practice for readability — the expression is non-obvious to a reader who second-guesses precedence (as I did), so:
// Before (correct, but easy to misread)
int t = clamp((x - edge0 << 8) / (edge1 - edge0), 0, 1 << 8);
// After (unambiguous)
int t = clamp(((x - edge0) << 8) / (edge1 - edge0), 0, 1 << 8);The remaining findings (#2–#6) in the original review are not affected by this precedence correction and remain valid:
| # | Severity | Issue | Still valid? |
|---|---|---|---|
smoothstep operator precedence |
❌ Was wrong | ||
| 2 | 🔴 Significant | fixed64Sqrt: uint64_t silently cast to int64_t → returns 0 |
✅ |
| 3 | 🔴 Significant | 2 * m2, 2 * m1, m1 + m2 overflow int32_t in handleCollision |
✅ |
| 4 | 🟡 Moderate | mass() overflow when density > ~1.2 |
✅ |
| 5 | 🟡 Moderate | fixedDiv lacks divide-by-zero guard (reachable from smoothstep) |
✅ |
| 6 | 🟢 Minor | == 0.0 / > 0.0 should use integer 0 |
✅ |
✏️ Learnings added
Learnt from: softhack007
Repo: wled/WLED PR: 4843
File: usermods/elastic_collisions/Elastic_Collisions.cpp:1-1
Timestamp: 2026-05-06T22:10:27.517Z
Learning: In C/C++, additive operators (`+`, `-`) have HIGHER precedence than shift operators (`<<`, `>>`). Therefore `x - edge0 << 8` correctly parses as `(x - edge0) << 8`. Do NOT flag this pattern as a precedence bug. When reviewing WLED fixed-point code or any C/C++ shift expressions, verify against cppreference before claiming precedence issues with mixed `-`/`+` and `<<`/`>>` expressions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: softhack007
Repo: wled/WLED PR: 5355
File: wled00/util.cpp:635-638
Timestamp: 2026-02-07T16:06:08.677Z
Learning: PSRAM-related compilation guards should enable PSRAM code only for ESP32 variants that actually include PSRAM: ESP32-C61, ESP32-C5, and ESP32-P4. Exclude ESP32-C3, ESP32-C6, and ESP8266 from these guards. Apply this rule across the codebase (not just wled00/util.cpp) by reviewing and updating PSRAM guards/macros in all relevant files (C/C++ headers and sources).
Learnt from: softhack007
Repo: wled/WLED PR: 4838
File: lib/NeoESP32RmtHI/src/NeoEsp32RmtHIMethod.cpp:30-35
Timestamp: 2026-03-27T12:33:48.499Z
Learning: In C/C++ preprocessor conditionals (`#if`, `#elif`) GCC/Clang treat `&&` as short-circuit evaluated during preprocessing. This means guards like `#if defined(ARDUINO_ARCH_ESP32) && ESP_IDF_VERSION < ESP_IDF_VERSION_VAL(5, 0, 0)` are safe even if the macro/function-like macro on the RHS (e.g., `ESP_IDF_VERSION_VAL`) is not defined on some targets, because the RHS will not be expanded when the LHS is false (e.g., `defined(...)` evaluates to 0). During code review, avoid flagging such cases as “undefined function-like macro invocation” if they are protected by short-circuiting `defined(...) && ...`/`||` logic; some tools like cppcheck may not model this and can produce false positives. Also, don’t suggest refactoring that moves ESP32-specific includes/headers (e.g., `esp_idf_version.h`) outside of these guarded preprocessor blocks, since that will break targets (e.g., ESP8266) where the headers don’t exist.
| for (int lY = lowY; lY < highY; lY++) { | ||
| for (int lX = lowX; lX < highX; lX++) { | ||
| // LED pixel center in high-resolution space | ||
| const nfixed halfPixel = 1 << (SPHERE_PREC_SHIFT - 1); |
There was a problem hiding this comment.
indentation seems broken here
There was a problem hiding this comment.
Double checked. I am moving a couple of open braces, but indentation is correct.
| } | ||
|
|
||
| // If it is time to do something. | ||
| if (millis() > SEGMENT.step) { |
There was a problem hiding this comment.
in general, effects should prefer strip.now over millis(); strip.now can be synchronized over several WLED effect instances. If you want to keep millis(), add short comment why strip.now can't be used here.
|
|
||
| // Check for collisions with other spheres. | ||
| for (int i = 0; i < (SEGMENT.aux0 & SPHERES_ALLOCATED); ++i) | ||
| for (int j = i + 1; j < (SEGMENT.aux0 & SPHERES_ALLOCATED); ++j) |
There was a problem hiding this comment.
you might gain some speed by pre-calculating SEGMENT.aux0 & SPHERES_ALLOCATED before the loop, and storing this into a local variable; the result will not change inside the nested loop.
There was a problem hiding this comment.
Optimizing compilers should pull that out of the loop automatically.
There was a problem hiding this comment.
in theory - yes they should. but in practice, sadly it usually does not happen.
One reason is that SEGMENT is accessing a global object member via pointers. We are in a multi-threaded OS, so the compiler cannot prove that SEGMENT.aux0 has not changed without seeing the whole program - the expression might be re-evaluated in each loop iteration (we are building WLED with -Os not with -O2).
|
just a general comment: on controllers with an FPU float calculations are about 10x faster than 64bit calculations, on S2 and C3 without an FPU 64bit it's the other way around. |
And the ESP32-S3 has a multiply-add instruction that runs at twice the clock rate of the rest of the core. Ah, "the mind reels." Ideally, one would like to see the actual generated code. You can do that in VSCode, but it is not straightforward. I'm sure there are a few more "Gotchas" in there. (I'm a former CPU designer.) By the way, check out th upcoming ESP32-S31. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
usermods/elastic_collisions/Elastic_Collisions.cpp (1)
35-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
PROGMEMtable is read via normal indexing inskewedRandom().
dmPercentagesis declared inPROGMEM, butskewedRandom()uses directpArray[index]reads. On ESP8266 this can read incorrect data; usepgm_read_byte_near()for all table accesses.♻️ Suggested fix
- 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]; + const uint8_t bucket = pgm_read_byte_near(pArray + index); + int32_t t = ((rand100 - cumulativePercentage) << RAND_PREC_SHIFT) / bucket;#!/bin/bash # Verify PROGMEM tables passed into skewedRandom and read style. rg -nP -C2 'skewedRandom\s*\(|PROGMEM|pgm_read_byte' usermods/elastic_collisions/Elastic_Collisions.cppAlso applies to: 407-407
🤖 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 35 - 42, skewedRandom currently reads values from a PROGMEM table (e.g., dmPercentages) using direct indexing (pArray[index]); on ESP8266 this yields wrong data — change every read of pArray[...] inside skewedRandom (including the loop condition and the later uses for division and interpolation where pArray[index] is used) to use pgm_read_byte_near(pArray + index) (or the appropriate pgm_read_* for the stored type) so cumulativePercentage and t use the correct values; update all references in the function (loop, t calculation, and any other pArray accesses) to use the pgm_read wrapper while keeping RAND_PREC_SHIFT and index logic unchanged.
🧹 Nitpick comments (1)
usermods/elastic_collisions/Elastic_Collisions.cpp (1)
150-150: ⚡ Quick winUse flash-stored debug string for embedded target.
Prefer
DEBUG_PRINTLN(F("..."))here to avoid keeping this literal in RAM on ESP8266-class targets.♻️ Suggested fix
- DEBUG_PRINTLN("At 0 `#1`"); + DEBUG_PRINTLN(F("At 0 `#1`"));As per coding guidelines: "Use
F("string")for string constants in C++ to save RAM on ESP8266".🤖 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 150, Replace the RAM-resident debug literal with a flash-stored string by changing the DEBUG_PRINTLN call that prints "At 0 `#1`" to use the F() macro (i.e., use DEBUG_PRINTLN(F("At 0 `#1`"))) so the string is stored in flash on ESP8266-class targets; locate the DEBUG_PRINTLN invocation in Elastic_Collisions.cpp and update it accordingly.
🤖 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.
Duplicate comments:
In `@usermods/elastic_collisions/Elastic_Collisions.cpp`:
- Around line 35-42: skewedRandom currently reads values from a PROGMEM table
(e.g., dmPercentages) using direct indexing (pArray[index]); on ESP8266 this
yields wrong data — change every read of pArray[...] inside skewedRandom
(including the loop condition and the later uses for division and interpolation
where pArray[index] is used) to use pgm_read_byte_near(pArray + index) (or the
appropriate pgm_read_* for the stored type) so cumulativePercentage and t use
the correct values; update all references in the function (loop, t calculation,
and any other pArray accesses) to use the pgm_read wrapper while keeping
RAND_PREC_SHIFT and index logic unchanged.
---
Nitpick comments:
In `@usermods/elastic_collisions/Elastic_Collisions.cpp`:
- Line 150: Replace the RAM-resident debug literal with a flash-stored string by
changing the DEBUG_PRINTLN call that prints "At 0 `#1`" to use the F() macro
(i.e., use DEBUG_PRINTLN(F("At 0 `#1`"))) so the string is stored in flash on
ESP8266-class targets; locate the DEBUG_PRINTLN invocation in
Elastic_Collisions.cpp and update it accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9e68d03-a60e-4c36-9924-bfc73295c7b7
📒 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 (2)
- usermods/elastic_collisions/library.json
- usermods/elastic_collisions/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/FX.h
- wled00/FX.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
usermods/elastic_collisions/Elastic_Collisions.cpp (1)
35-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
pArrayis in PROGMEM but is read with raw indexing — broken on ESP8266.The single caller passes the
static const uint8_t PROGMEM dmPercentages[20]defined at line 407. On ESP8266, PROGMEM data must be accessed withpgm_read_byte_near(...); direct dereference (pArray[index]) returns garbage from the wrong address space. (On ESP32 it currently works only because flash is memory-mapped.)♻️ Suggested fix
- 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++; } - - // Calculate linear interpolation - int32_t t = ((rand100 - cumulativePercentage) << RAND_PREC_SHIFT) / pArray[index]; + // Calculate linear interpolation + int32_t t = ((rand100 - cumulativePercentage) << RAND_PREC_SHIFT) / pgm_read_byte_near(pArray + index);🤖 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 35 - 41, The code reads pArray (a PROGMEM array — e.g., dmPercentages) using raw indexing (pArray[index]) which fails on ESP8266; change all reads of pArray[index] (used in the while loop and the linear interpolation calculation for t) to use pgm_read_byte_near(pArray + index) (or the equivalent PROGMEM read helper) and cast to an integer type as needed, and add the appropriate include for pgm_read_byte_near; ensure cumulativePercentage and the divisor in the t calculation use the value read via pgm_read_byte_near rather than raw dereference so the logic works on ESP8266 and other architectures where flash is not memory-mapped.
🧹 Nitpick comments (4)
usermods/elastic_collisions/Elastic_Collisions.cpp (4)
150-150: 💤 Low valueWrap the debug literal in
F(...)to keep it out of RAM.
DEBUG_PRINTLN("At 0#1")will land in.rodata/RAM on ESP8266. Per project conventions, debug strings should use theF("...")(orPSTR(...)with the_Pvariant) macro so they live in flash.♻️ Suggested fix
- DEBUG_PRINTLN("At 0 `#1`"); + DEBUG_PRINTLN(F("ElasticCollisions: degenerate length"));As per coding guidelines: "Use
F("string")for string constants in C++ to save RAM on ESP8266".🤖 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 150, The debug string literal passed to DEBUG_PRINTLN is stored in RAM; update the call for flash storage by wrapping the literal with the F() macro (i.e., change the DEBUG_PRINTLN invocation that contains "At 0 `#1`" to use F("At 0 `#1`")). Locate the DEBUG_PRINTLN call in Elastic_Collisions.cpp (the DEBUG_PRINTLN("At 0 `#1`") invocation) and replace the argument with F("At 0 `#1`") so the string lives in flash per project conventions.
128-131: ⚡ Quick win
areSpheresCollidingshould take its argument byconst &and beconst.The earlier review thread retracted this on the assumption you had switched to a pointer, but the signature is still pass-by-value and called twice per collision check (lines 465 and 507). Each invocation copies all six
nfixedmembers +colorIdx. Pass byconst &and mark the methodconst.♻️ Suggested fix
- bool areSpheresColliding(MBSphere sp) { - nfixed dist = fixedDist(sp.x - this->x, sp.y - this->y); - return dist <= this->radius + sp.radius; - } + bool areSpheresColliding(const MBSphere &sp) const { + nfixed dist = fixedDist(sp.x - this->x, sp.y - this->y); + return dist <= this->radius + sp.radius; + }As per coding guidelines: "Prefer
const &for read-only function parameters in C++" and "Mark getter/query methodsconstin C++".🤖 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 128 - 131, Change the method signature of areSpheresColliding(MBSphere sp) to take the sphere by const reference (const MBSphere& sp) and mark the method itself as const (areSpheresColliding(...) const) so it doesn’t copy MBSphere on each call; keep the body using fixedDist(sp.x - this->x, sp.y - this->y) and return dist <= this->radius + sp.radius unchanged. This reduces copies of MBSphere’s nfixed/colorIdx members and correctly marks the method as a query.
295-297: 💤 Low valueHoist the per-iteration constant out of the inner pixel loop.
halfPixelis a compile-time constant andlX << SPHERE_PREC_SHIFTonly depends on the inner index, buthalfPixelis recomputed on every inner-loop iteration. Lift it to function/class scope (or aconstexprat file scope) so it doesn't add an instruction per pixel on ESP32-C3/S2.♻️ Suggested fix
- for (int lY = lowY; lY < highY; lY++) { - for (int lX = lowX; lX < highX; lX++) { - // LED pixel center in high-resolution space - const nfixed halfPixel = 1 << (SPHERE_PREC_SHIFT - 1); - nfixed pixelX = (lX << SPHERE_PREC_SHIFT) + halfPixel; - nfixed pixelY = (lY << SPHERE_PREC_SHIFT) + halfPixel; + constexpr nfixed halfPixel = 1 << (SPHERE_PREC_SHIFT - 1); + for (int lY = lowY; lY < highY; lY++) { + const nfixed pixelY = (lY << SPHERE_PREC_SHIFT) + halfPixel; + for (int lX = lowX; lX < highX; lX++) { + const nfixed pixelX = (lX << SPHERE_PREC_SHIFT) + halfPixel;🤖 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 295 - 297, Hoist the per-iteration constant halfPixel out of the inner pixel loop: make halfPixel a constexpr (or static const) computed from SPHERE_PREC_SHIFT at file or function scope instead of re-evaluating "const nfixed halfPixel = 1 << (SPHERE_PREC_SHIFT - 1);" inside the loop that computes pixelX and pixelY (where pixelX = (lX << SPHERE_PREC_SHIFT) + halfPixel and pixelY = (lY << SPHERE_PREC_SHIFT) + halfPixel). This removes the repeated instruction per pixel on ESP32 targets while keeping the existing use of lX and lY and the SPHERE_PREC_SHIFT symbol unchanged.
304-306: 💤 Low valueDrop the dangling
+ 0and the commented-out experiments.
+ 0on line 304 is a leftover from a tuning experiment, and lines 303/305/306 are dead commented code. Cleaning these up makes future readers' lives easier and removes a noise source for static analysis.♻️ Suggested fix
- // Compute anti-aliasing weight - // float alpha = RGBEffect::clamp(1.0f - RGBEffect::smoothstep(FLOAT_IT(edge0), FLOAT_IT(edge1), dist), 0.0f, 1.0f); - nfixed alpha = clamp((1 << SPHERE_PREC_SHIFT) - smoothstep(edge0, edge1, dist), 0, 1 << SPHERE_PREC_SHIFT) + 0; - // nfixed alpha = clamp((1 << SPHERE_PREC_SHIFT) - smoothstep(edge0, edge1, dist), 1 << (SPHERE_PREC_SHIFT - 2), 1 << SPHERE_PREC_SHIFT); - // alpha = 1 << SPHERE_PREC_SHIFT; + // Compute anti-aliasing weight + nfixed alpha = clamp((1 << SPHERE_PREC_SHIFT) - smoothstep(edge0, edge1, dist), + 0, 1 << SPHERE_PREC_SHIFT);🤖 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 304 - 306, Remove the leftover "+ 0" and the three commented experimental lines to clean up the alpha calculation; update the line initializing variable alpha (the expression using clamp, (1 << SPHERE_PREC_SHIFT), and smoothstep(edge0, edge1, dist)) to be the single, final assignment without trailing "+ 0" and delete the commented alternatives so only the intended nfixed alpha = clamp((1 << SPHERE_PREC_SHIFT) - smoothstep(edge0, edge1, dist), 0, 1 << SPHERE_PREC_SHIFT); remains (refer to symbol names alpha, clamp, SPHERE_PREC_SHIFT, smoothstep, edge0, edge1, dist).
🤖 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 173-180: The while loop that retries when dist == 0 can spin
forever; in the loop that increments nx and recomputes dist via fixedDist(nx,
ny) (using SPHERE_PREC_SHIFT), add a hard iteration cap (e.g.,
MAX_ZERO_DIST_TRIES) and break out if reached, falling back to the existing
degenerate handling (leave objects slightly separated or treat as non-colliding)
and log or mark the bail-out; ensure the cap constant is defined and used inside
the loop so nx/ny adjustments still occur up to the limit and fixedDist is not
called indefinitely.
- Around line 28-45: Mark the file-local helper functions as static to avoid
leaking symbols and ODR collisions: change the linkage of skewedRandom,
countLeadingZeros64, elasticLifetime, and sliderToSpeed so they are
declared/defined with static at file scope; this confines each function to the
translation unit and prevents multiple-definition/linker conflicts with
similarly named helpers in other compilation units.
---
Duplicate comments:
In `@usermods/elastic_collisions/Elastic_Collisions.cpp`:
- Around line 35-41: The code reads pArray (a PROGMEM array — e.g.,
dmPercentages) using raw indexing (pArray[index]) which fails on ESP8266; change
all reads of pArray[index] (used in the while loop and the linear interpolation
calculation for t) to use pgm_read_byte_near(pArray + index) (or the equivalent
PROGMEM read helper) and cast to an integer type as needed, and add the
appropriate include for pgm_read_byte_near; ensure cumulativePercentage and the
divisor in the t calculation use the value read via pgm_read_byte_near rather
than raw dereference so the logic works on ESP8266 and other architectures where
flash is not memory-mapped.
---
Nitpick comments:
In `@usermods/elastic_collisions/Elastic_Collisions.cpp`:
- Line 150: The debug string literal passed to DEBUG_PRINTLN is stored in RAM;
update the call for flash storage by wrapping the literal with the F() macro
(i.e., change the DEBUG_PRINTLN invocation that contains "At 0 `#1`" to use F("At
0 `#1`")). Locate the DEBUG_PRINTLN call in Elastic_Collisions.cpp (the
DEBUG_PRINTLN("At 0 `#1`") invocation) and replace the argument with F("At 0 `#1`")
so the string lives in flash per project conventions.
- Around line 128-131: Change the method signature of
areSpheresColliding(MBSphere sp) to take the sphere by const reference (const
MBSphere& sp) and mark the method itself as const (areSpheresColliding(...)
const) so it doesn’t copy MBSphere on each call; keep the body using
fixedDist(sp.x - this->x, sp.y - this->y) and return dist <= this->radius +
sp.radius unchanged. This reduces copies of MBSphere’s nfixed/colorIdx members
and correctly marks the method as a query.
- Around line 295-297: Hoist the per-iteration constant halfPixel out of the
inner pixel loop: make halfPixel a constexpr (or static const) computed from
SPHERE_PREC_SHIFT at file or function scope instead of re-evaluating "const
nfixed halfPixel = 1 << (SPHERE_PREC_SHIFT - 1);" inside the loop that computes
pixelX and pixelY (where pixelX = (lX << SPHERE_PREC_SHIFT) + halfPixel and
pixelY = (lY << SPHERE_PREC_SHIFT) + halfPixel). This removes the repeated
instruction per pixel on ESP32 targets while keeping the existing use of lX and
lY and the SPHERE_PREC_SHIFT symbol unchanged.
- Around line 304-306: Remove the leftover "+ 0" and the three commented
experimental lines to clean up the alpha calculation; update the line
initializing variable alpha (the expression using clamp, (1 <<
SPHERE_PREC_SHIFT), and smoothstep(edge0, edge1, dist)) to be the single, final
assignment without trailing "+ 0" and delete the commented alternatives so only
the intended nfixed alpha = clamp((1 << SPHERE_PREC_SHIFT) - smoothstep(edge0,
edge1, dist), 0, 1 << SPHERE_PREC_SHIFT); remains (refer to symbol names alpha,
clamp, SPHERE_PREC_SHIFT, smoothstep, edge0, edge1, dist).
🪄 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: 8be2e991-01f5-447b-a0c5-26e37999e2ed
📒 Files selected for processing (2)
usermods/elastic_collisions/Elastic_Collisions.cppwled00/FX.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/FX.cpp
| 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]; | ||
| int32_t result = ((index << RAND_PREC_SHIFT) + t) * 100 / pArraySize >> RAND_PREC_SHIFT; | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Mark file-scope helpers static to avoid leaking symbols and ODR collisions.
skewedRandom, countLeadingZeros64, elasticLifetime, and sliderToSpeed are all defined at file scope without static. WLED's "Xmas Twinkle" PR introduces a similarly named skewedRandom/skewedTime family in wled00/FX.cpp; both ending up in the same translation-unit-set produces a multiple-definition link error (or, worse, silent ODR violation since the two implementations differ). Per the project guidelines, prefer static for file-local helpers in usermods.
♻️ Suggested fix
-int32_t skewedRandom( uint8_t rand100, ...) {
+static int32_t skewedRandom(uint8_t rand100, ...) {
@@
-int countLeadingZeros64(uint64_t x)
+static int countLeadingZeros64(uint64_t x)
@@
-uint16_t elasticLifetime() {
+static uint16_t elasticLifetime() {
@@
-nfixed sliderToSpeed(uint8_t slider) {
+static nfixed sliderToSpeed(uint8_t slider) {As per coding guidelines: "Prefer static for file-local helpers/locals to avoid global namespace pollution".
📝 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.
| 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]; | |
| int32_t result = ((index << RAND_PREC_SHIFT) + t) * 100 / pArraySize >> RAND_PREC_SHIFT; | |
| return result; | |
| } | |
| static 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]; | |
| int32_t result = ((index << RAND_PREC_SHIFT) + t) * 100 / pArraySize >> RAND_PREC_SHIFT; | |
| return result; | |
| } |
🤖 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 28 - 45,
Mark the file-local helper functions as static to avoid leaking symbols and ODR
collisions: change the linkage of skewedRandom, countLeadingZeros64,
elasticLifetime, and sliderToSpeed so they are declared/defined with static at
file scope; this confines each function to the translation unit and prevents
multiple-definition/linker conflicts with similarly named helpers in other
compilation units.
| while (dist == 0) { | ||
| // handle gracefully | ||
| // DEBUG_PRINTLN("Two objects on top of each other!"); | ||
|
|
||
| x += 1 << (SPHERE_PREC_SHIFT -2); | ||
| nx += 1 << (SPHERE_PREC_SHIFT -2); | ||
| dist = fixedDist(nx, ny); | ||
| } |
There was a problem hiding this comment.
Unbounded while (dist == 0) loop — add an iteration cap.
If the displacement stays below the Q16.16 quantum (e.g., both spheres at the exact same coordinate with nx == ny == 0), nx += 1 << (SPHERE_PREC_SHIFT-2) does drive nx non-zero — but fixedDist then squares values and right-shifts; with floating-point/precision edge cases or future code changes this is the kind of loop that will silently hang the FX task. Cap the iterations and bail out, keeping the safe degenerate handling.
♻️ Suggested fix
- while (dist == 0) {
- // handle gracefully
- // DEBUG_PRINTLN("Two objects on top of each other!");
-
- x += 1 << (SPHERE_PREC_SHIFT -2);
- nx += 1 << (SPHERE_PREC_SHIFT -2);
- dist = fixedDist(nx, ny);
- }
+ for (uint8_t guard = 0; dist == 0 && guard < 8; ++guard) {
+ // handle gracefully: nudge along x to break the degeneracy
+ x += 1 << (SPHERE_PREC_SHIFT - 2);
+ nx += 1 << (SPHERE_PREC_SHIFT - 2);
+ dist = fixedDist(nx, ny);
+ }
+ if (dist == 0) return; // give up; caller will retry next frame🤖 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 173 - 180,
The while loop that retries when dist == 0 can spin forever; in the loop that
increments nx and recomputes dist via fixedDist(nx, ny) (using
SPHERE_PREC_SHIFT), add a hard iteration cap (e.g., MAX_ZERO_DIST_TRIES) and
break out if reached, falling back to the existing degenerate handling (leave
objects slightly separated or treat as non-colliding) and log or mark the
bail-out; ensure the cap constant is defined and used inside the loop so nx/ny
adjustments still occur up to the limit and fixedDist is not called
indefinitely.
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
Documentation
Chores