-
-
Notifications
You must be signed in to change notification settings - Fork 4k
spots effect accuracy improvements #5341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
* avoid gaps on left/light side of strip, by using higher accuracy zoneLength * prevent overflows * ensure that zones calculation are performed in full 32bit * fix wrong slider name in "spots fade"
WalkthroughRefactors Spots Fade rendering math to use fixed-point/32-bit arithmetic, clamps zone counts, adjusts per-zone position and indexing, tightens brightness masking, and updates the Spots Fade metadata label from "Spread" to "Speed". Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/FX.cpp`:
- Around line 2955-2970: The current computation of zoneLen8 uses zones >> 3
which collapses 9–15 to 1 and can make (zones * zoneLen8) >> 3 exceed SEGLEN
causing offset underflow and huge indices; fix by using a true 1/8 fixed‑point
scale (avoid shifts on zones) — compute zoneLen8 as (SEGLEN * 8) / zones (using
a wider integer type to avoid overflow) and compute offset the same way ((SEGLEN
- ((zones * zoneLen8) >> 3)) >> 1); also replace the magic literal 8 with a
named constant like ZONE_FP_SCALE to clarify intent and prevent future errors
(symbols: zoneLen8, offset, zones, SEGLEN).
* use named constants for fixed point scaling * remove buggy "SEGLEN / (zones >>3)" shortcut * remove an unnecessary casts to uint32_t
DedeHai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally an improvement, there are two issues (one introduced, one unsolved):
- on small segments, the threshold can be too high, turning off all spots. something like this fixes that but may introduce "breaking changes" I did only a brief test:
uint16_t maxWave = triwave16(((zoneLen - 1) * 0xFFFF) / zoneLen);
threshold = min(threshold, uint16_t(maxWave - 1)); // guarantee threshold is always reached (prevent "no spots" on short strips)to check: use a segment with 15 LEDs, turn speed slider down and intensity slider up.
- the newly introduced issue is a classic integer math rounding problem: while the spots now spread better, they distribute unevenly. I faced the same dilemma in "PS chase" and used more elaborate code to get both a full spread distribution as well as an even spacing.
| return spots_base(tr); | ||
| } | ||
| static const char _data_FX_MODE_SPOTS_FADE[] PROGMEM = "Spots Fade@Spread,Width,,,,,Overlay;!,!;!"; | ||
| static const char _data_FX_MODE_SPOTS_FADE[] PROGMEM = "Spots Fade@Speed,Width,,,,,Overlay;!,!;!"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while you are at it: rename "Width" to "Density" for both variants?
@DedeHai thanks for your comments 😃 Yes unevenly distributed gaps are a new side-effect. At second look the nested loops with offset re-calcution seem unnecessary, we could also just repeat the tri-wave pattern until the end of strip is reached. Just that the effect might not be centered any more for long "spread" values. I'll think about a better implementation. Edit: maybe this is better
Edit2: 🤔 actually offset calculation is the only "tricky" thing left, because the effect should be centered no matter how long or short the strip really is... |
|
I need to check that code but in general for long er strips with 100 LEDs+ you will not notice a 1 pixel offset, unless its coarse and you want it aligned to furniture but I guess that is more of an edge case. What is much more noticeable is an irregular pattern. Edit: with 24V strips and "zones" now coming up more and more, low LED-count segments are not uncommon (I have 64 on mine) |
A few improvement for the spots effect, especially relevant for longer strips:
Summary by CodeRabbit
Bug Fixes
Documentation