Arc Expand Optimizations #4994
Arc Expand Optimizations #4994
Conversation
Faster algorithm without floats, multiplication or division. No more missing pixels. More accurate gPC.
WalkthroughReplaces M12_pArc 2D arc rendering with an integer Bresenham/Tony Barrera circle algorithm, adjusts diagonal length rounding, updates setPixelColor/getPixelColor logic, adds structural braces, and refines initial/symmetric point handling in wled00/FX_fcn.cpp. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX_fcn.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX_fcn.cpp
🧬 Code graph analysis (1)
wled00/FX_fcn.cpp (2)
wled00/wled_math.cpp (2)
sqrt32_bw(225-246)sqrt32_bw(225-225)wled00/FX_2Dfcn.cpp (4)
setPixelColorXY(185-190)setPixelColorXY(185-185)setPixelColorXY(194-232)setPixelColorXY(194-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
| if (i >= vW && i >= vH) { // Barrera's circle algorithm | ||
| int x2 = 0, y2 = i, d = -(i >> 1); | ||
| int validCount = 0; // return 2nd non mirrored pixel if available | ||
| while (x2 <= y2 && validCount < 2) { | ||
| if (y2 < vH && x2 < vW) {x = x2, y = y2; validCount++;} | ||
| else if (y2 < vW && x2 < vH) {x = y2, y = x2; validCount++;} | ||
|
|
||
| if (d <= 0) d += ++x2; else d -= --y2; | ||
| } | ||
| break; |
There was a problem hiding this comment.
Fix getPixelColor guard for pArc (breaks mapping)
setPixelColor() now draws the Barrera circle for every radius i, but getPixelColor() only runs the same loop when i >= vW && i >= vH. On typical panels (e.g. 64×32), all indices i < 64 fall straight through to the M12_pCorner fallback, so callers read back (i,0) even though we actually painted an arc of pixels. Any 1D effect that relies on getPixelColor() (tails, blurs, AA, etc.) will misbehave or flicker because read/write mappings no longer agree.
Please drop that guard (or at least require only i >= min(vW, vH)) and fall back only when the loop fails to find an in-bounds coordinate. For example:
- case M12_pArc:
- if (i >= vW && i >= vH) {
+ case M12_pArc: {
+ int hitCount = 0;
+ int xSel = 0, ySel = 0;
int x2 = 0, y2 = i, d = -(i >> 1);
- int validCount = 0;
- while (x2 <= y2 && validCount < 2) {
- if (y2 < vH && x2 < vW) {x = x2, y = y2; validCount++;}
- else if (y2 < vW && x2 < vH) {x = y2, y = x2; validCount++;}
+ while (x2 <= y2 && hitCount < 2) {
+ if (y2 < vH && x2 < vW) { xSel = x2; ySel = y2; hitCount++; }
+ else if (y2 < vW && x2 < vH) { xSel = y2; ySel = x2; hitCount++; }
if (d <= 0) d += ++x2; else d -= --y2;
}
- break;
- }
+ if (hitCount) { x = xSel; y = ySel; break; }
+ // otherwise fall through
+ }That keeps read/write symmetry for all radii while still falling back when nothing lands inside bounds.
🤖 Prompt for AI Agents
In wled00/FX_fcn.cpp around lines 939 to 948, remove the restrictive guard "if
(i >= vW && i >= vH)" so the Barrera circle search runs for all radii (or change
it to "i >= min(vW, vH)" if you prefer the looser check), and only fall back to
the M12_pCorner mapping when the loop completes without finding any in-bounds
coordinate (i.e., validCount remains zero). Ensure the loop sets x,y on the
first/second valid pixel as before, breaks out once the desired validCount is
reached, and after the loop you perform the fallback only when no valid pixel
was found so read/write mapping stays symmetric.
|
I find this very interesting! Thank you. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9686 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 5010 @blazoncek, you're absolutely right to highlight this! The comparison videos don't clearly demonstrate the behavior on non-square segments, which is particularly important given the technical concern I raised in my review. Specifically, on non-square panels (like the mentioned 32×8 or 64×32), the
This asymmetry could cause visual artifacts in effects that rely on reading back pixel colors (e.g., tails, blurs, anti-aliasing). It would be very helpful if @Brandon502 could provide comparison videos specifically showing non-square segments to validate that:
Testing on a 32×8 segment would be particularly valuable since it has the most extreme aspect ratio mentioned in the PR. 🐰🔍 |
Here's a video of both versions of arc running 64x32 freqmatrix, 64x22 rainbow, and 64x8 dj lights. You can see the hole in the larger sizes on the old version and gPC breaks in the corner. New gPC works similar to old and uses corner when it can. I think there's still a chance that it won't work on the very last pixel due to unneeded rounding on certain sizes. I don't think any effects currently rely on going backwards. I can add condition in sPC that always paints the bottom right pixel when i == vLength() -1 to fix though if necessary. Old.Arc.mp4New.Arc.mp4 |
|
Tip For best results, initiate chat on the files or code changes. @Brandon502, thank you for providing these comparison videos! They clearly demonstrate the improvements, especially:
Regarding the last pixel rounding issue you mentioned: I'd recommend adding the safety check you proposed. Even though no current effects rely on backward traversal, it's good defensive programming to ensure
The check could be as simple as: if (i == vLen - 1) {
setPixelColorXY(vW - 1, vH - 1, col);
return;
}This would guarantee correctness without affecting the performance gains you've achieved (45 → 80 FPS on 64×32 is impressive!). Would you like to add this safety check, or would you prefer to leave it as-is given that no current effects are affected? 🐰 |
Use sPCXYRaw(). Small logic changes.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX_fcn.cpp (1)
684-691: Align maxMappingLength() rounding with virtualLength() for M12_pArcChange maxMappingLength() to use the same rounded‐diagonal formula as virtualLength()’s M12_pArc case to avoid off-by-one under-allocation of PS buffers:
- return max(sqrt32_bw(vH*vH + vW*vW), (uint32_t)getPinwheelLength(vW, vH)); // use diagonal + // Use rounded diagonal to stay ≥ virtualLength() for pArc + return max((uint32_t)((sqrt32_bw((vH*vH + vW*vW) << 2) + 1) >> 1), + (uint32_t)getPinwheelLength(vW, vH));
♻️ Duplicate comments (1)
wled00/FX_fcn.cpp (1)
942-953: Fix read/write mismatch in M12_pArc getPixelColor (drop restrictive guard, add proper fallback)Current guard if (i >= vW && i >= vH) skips the Barrera search for many radii on non-square panels (e.g., 64×32), while setPixelColor always draws via Barrera. This breaks symmetry for effects using getPixelColor (tails/blur/AA), causing artifacts. Also mirror the “last pixel” bottom-right special-case to match setPixelColor.
Apply:
- case M12_pArc: - if (i >= vW && i >= vH) { // Barrera's circle algorithm - x = vW - 1; y = vH - 1; // default to bottom right if not found - int x2 = 0, y2 = i, d = -(i >> 1); - int validCount = 0; // return 2nd non mirrored pixel if available - while (x2 <= y2 && validCount < 2) { - if (y2 < vH && x2 < vW) {x = x2, y = y2; validCount++;} - else if (y2 < vW && x2 < vH) {x = y2, y = x2; validCount++;} - - if (d <= 0) d += ++x2; else d -= --y2; - } - break; - } - // otherwise fallthrough + case M12_pArc: { + // Barrera's circle algorithm; try for all i and fall back only if nothing lands in-bounds + if (i == (int)virtualLength() - 1) { x = vW - 1; y = vH - 1; break; } // keep symmetry with setPixelColor + int x2 = 0, y2 = i, d = -(i >> 1); + int hitCount = 0, xSel = 0, ySel = 0; // pick 2nd non-mirrored point if available + while (x2 <= y2 && hitCount < 2) { + if ((y2 < vH) && (x2 < vW)) { xSel = x2; ySel = y2; hitCount++; } + else if ((y2 < vW) && (x2 < vH)) { xSel = y2; ySel = x2; hitCount++; } + if (d <= 0) d += ++x2; else d -= --y2; + } + if (hitCount) { x = xSel; y = ySel; break; } + // otherwise fallthrough + }
🧹 Nitpick comments (1)
wled00/FX_fcn.cpp (1)
745-753: Barrera set logic looks good; tiny micro-optImplementation is sound and guarded; nice addition of the reference link. Optionally reuse vL (computed at Line 718) to avoid re-calling virtualLength() here:
- else if (i == vLength()-1) setPixelColorXYRaw(vW-1, vH-1, col); // last i always draws bottom right corner + else if (i == vL - 1) setPixelColorXYRaw(vW-1, vH-1, col); // last i always draws bottom right corner
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX_fcn.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX_fcn.cpp
🧠 Learnings (1)
📚 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_fcn.cpp
🧬 Code graph analysis (1)
wled00/FX_fcn.cpp (2)
wled00/wled_math.cpp (2)
sqrt32_bw(225-246)sqrt32_bw(225-225)wled00/FX.h (2)
setPixelColorXYRaw(546-546)vLength(658-658)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- LED output re-arrange by drag & drop - faster 1D arc expansion (by @Brandon502 wled#4994) - FX: consolidate Blink/Strobe & add color time/space swap (wled#4978) - FX: consolidate Running/Saw - replace color_wheel() by color_from_palette() - remove "rainbow" option from settings
2 participants
New Arc algorithm that is more optimized and fixes holes on larger matrices (32x8 64x32 etc). FPS on 64x32 Rainbow went from 45 FPS to 80 FPS on esp32dev hub75 build. Looks slightly different but unnoticeable on most effects. gPC is more accurate and seems to fix the last few effects that still use it.
Videos comparing the new (reversed) and previous version. Left is new, right is old.
Arc.Differences.mp4
Rainbow.Arc.mp4
Summary by CodeRabbit