-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Spinning Wheel FX in the user_fx usermod #5293
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
Open
BobLoeffler68
wants to merge
8
commits into
wled:main
Choose a base branch
from
BobLoeffler68:pr-spinningwheel-user-fx
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
f2f25f2
Added the Spinning Wheel effect into the user_fx usermod
BobLoeffler68 de79954
Fixed a few issues that coderabbit mentioned
BobLoeffler68 6376750
Fixed integer overflow when storing color scale in aux1. And added a …
BobLoeffler68 a707b39
Additions/changes:
BobLoeffler68 4b81ea8
One minor fix for the spinner colors
BobLoeffler68 ccff1b5
Changed the two analogRead() to hw_random16()
BobLoeffler68 0f5469a
Changes from SEGLEN to vstripLen suggested by coderabbitai, but it's …
BobLoeffler68 0833dac
Rolled back changes from vstripLen to SEGLEN as that is what works co…
BobLoeffler68 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
🧩 Analysis chain
🏁 Script executed:
Repository: wled/WLED
Length of output: 2394
🏁 Script executed:
Repository: wled/WLED
Length of output: 6486
🏁 Script executed:
Repository: wled/WLED
Length of output: 19142
🏁 Script executed:
Repository: wled/WLED
Length of output: 974
🏁 Script executed:
Repository: wled/WLED
Length of output: 231
🏁 Script executed:
Repository: wled/WLED
Length of output: 1502
🏁 Script executed:
Repository: wled/WLED
Length of output: 515
🏁 Script executed:
Repository: wled/WLED
Length of output: 2593
🏁 Script executed:
Repository: wled/WLED
Length of output: 2327
🏁 Script executed:
Repository: wled/WLED
Length of output: 533
Use per-strip length instead of SEGLEN when rendering virtual strips in M12_pBar mode.
For M12_pBar (expanding 1D effects to 2D bars), each virtual strip has length
virtualHeight(), notSEGLEN(which isvirtualWidth() * virtualHeight()). Moduloing and scaling bySEGLENproduces indices beyond the per-strip bounds, causing pixel wrapping or clipping errors.🛠️ Proposed fix
Also applies to: 298–310, 318–323
🤖 Prompt for AI Agents
✅ Addressed in commit 0f5469a
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.
@coderabbitai The vstripLen suggestion is good when using the M12_pBar, but when I use the Pinwheel option, part of the pinwheel is missing.
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.
🧩 Analysis chain
🏁 Script executed:
Length of output: 668
🏁 Script executed:
Length of output: 3257
🏁 Script executed:
Length of output: 157
🏁 Script executed:
Length of output: 3682
@BobLoeffler68Thank you for catching that! You're absolutely right. My initial suggestion was too narrow—it only considered M12_pBar mode.The issue is that
nrOfVStrips()returns different values depending on the mapping mode:virtualWidth()- creates multiple vertical strips1- uses a single virtual stripHere's the corrected fix that handles all mapping modes:
Corrected fix that works for all M12 mapping modes:
Why this works:
vstripLen = SEGLEN / stripsautomatically calculates the correct length per virtual stripstrips = virtualWidth(), sovstripLen = (virtualWidth() × virtualHeight()) / virtualWidth() = virtualHeight()✅strips = 1, sovstripLen = SEGLEN / 1 = SEGLEN✅This matches how Fire 2012 and other effects handle virtual strips—they always use
SEGLENin their loops, which naturally adapts based on how many virtual strips are active.Sorry for the incomplete initial suggestion! This should now work correctly for both M12_pBar and Pinwheel modes.
🧬 Code Graph Analysis Results
[usermods/user_fx/user_fx.cpp: Macro and usage related to virtual strips mapping]
[usermods/user_fx/user_fx.cpp: mode_spinning_wheel drawing logic using virtual-strip mapping]
[Note: If no additional relevant code snippets are found, respond with the above two contexts. If you need exact line-number references, please provide a version with line numbers or request a line-numbered snippet extraction.]
🧠 Learnings used
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.
@coderabbitai I tried your fix, but then I get 2 errors when compiling. Here is one error that pertains to the uint16_t drawPos = (pos + y) % vstripLen; line: usermods/user_fx/user_fx.cpp:321:42: error: use of local variable with automatic storage from containing function
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.
@BobLoeffler68You're absolutely right! I apologize for the error. The problem is thatvstripLenis a local variable inmode_spinning_wheel(), butvirtualStrip::runStrip()is a static method that cannot access variables from the containing function's scope.The fix is to pass
vstripLenas a parameter torunStrip():Summary of changes:
uint16_t vstripLenas the last parameter tovirtualStrip::runStrip()vstripLenwhen callingrunStrip()in the for loop at the bottomvstripLenis accessible inside the static method as a parameterThis should compile correctly and work for both M12_pBar and Pinwheel modes!
🧠 Learnings used
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.
@coderabbitai I had not pushed my latest code yet, so you were using old code. I just pushed the latest code with your suggestions, but it still does not work. Please check the code to see why. Thx.
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.
I reverted back to using SEGLEN instead of vstripLen, except for this line:
SEGENV.aux1 = (255 << 8) / vstripLen;
and now it's working correctly again for M12_pBar, M12_pPixel and M12_pPinwheel
But, the color/hue is off when using palettes. The M12_pBar only shows the first 66% of the colors in the palette. If I change that line back to:
SEGENV.aux1 = (255 << 8) / SEGLEN;
then it uses all of the colors of the palette.
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.
@BobLoeffler68You've found the correct solution! The key insight is that the color scaling must match the position range.Here's what's happening:
Why the color issue occurs with
vstripLen:Your position calculation uses:
pos = (pos_fixed >> 16) % SEGLENposranges from 0 to 1023Color calculation uses:
hue = (SEGENV.aux1 * pos) >> 8aux1 = (255 << 8) / vstripLen = (255 << 8) / 32 = 2040pos = 32:hue = (2040 × 32) >> 8 = 255(full palette cycle complete!)poscontinues to 1023, causing palette to repeat ~32 timesThe result: You only see the first 1/32 of the animation span using the full palette range, which appears as "only 66% of colors" in M12_pBar.
Why
SEGLENworks correctly:aux1 = (255 << 8) / SEGLEN = (255 << 8) / 1024 = 63.75posgoes from 0 to 1023,huesmoothly transitions from 0 to 255The correct solution is what you already have:
My earlier suggestion to use
vstripLenwas based on the assumption that we'd also change all position calculations to usevstripLen, making each virtual strip independent with its own position wrapping. But since the effect works correctly withSEGLEN-based position wrapping (and handles M12_pBar, pPixel, and pPinwheel correctly), that means theindexToVStripmacro and virtual strip addressing is handling the mapping correctly at thesetPixelColorlevel.Keep your current implementation - it's working correctly! The color scale should match the position range, and both should use
SEGLEN.✏️ Learnings added
🧠 Learnings used
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.
ok, thx. I'll keep it the way I have it now.
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.
@BobLoeffler68Perfect! Your solution is correct—keepingSEGLENfor both position calculations and color scaling ensures the palette is distributed evenly across the entire animation range and works properly with all M12 mapping modes. 👍🧠 Learnings used