PoC Rotate Segment #5202
Open
PoC Rotate Segment #5202
Conversation
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
New rotate effect based on copy that rotates a segment's 2D pixel data with optional zoom, wrap, and mirror wrap features.
Contributor
WalkthroughAdded a new "Rotate Segment" effect: a mode implementation with zoom/flip/shear and inverse-mapping sampling, its PROGMEM metadata descriptor, registration via addEffect(), and a new FX mode constant with MODE_COUNT incremented. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)wled00/**/*.cpp📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (12)📓 Common learnings📚 Learning: 2025-08-26T11:51:21.817ZApplied to files:
📚 Learning: 2025-06-15T09:59:52.720ZApplied to files:
📚 Learning: 2025-09-02T19:42:57.475ZApplied to files:
📚 Learning: 2025-11-16T19:40:46.260ZApplied to files:
📚 Learning: 2025-10-20T09:41:50.374ZApplied to files:
📚 Learning: 2025-11-27T06:33:11.436ZApplied to files:
📚 Learning: 2025-08-31T03:38:14.114ZApplied to files:
📚 Learning: 2025-11-14T05:48:44.673ZApplied to files:
📚 Learning: 2025-06-07T15:58:42.579ZApplied to files:
📚 Learning: 2025-04-24T09:31:06.879ZApplied to files:
📚 Learning: 2025-11-14T13:37:11.994ZApplied to files:
🧬 Code graph analysis (1)wled00/FX.cpp (1)
⏰ 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). (20)
🔇 Additional comments (2)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FX.cpp(1 hunks)wled00/FX.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/FX.h
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (12)
📓 Common learnings
Learnt from: freakintoddles2
Repo: wled/WLED PR: 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”.
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
Repo: wled/WLED PR: 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
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 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.hwled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 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.hwled00/FX.cpp
📚 Learning: 2025-08-31T03:38:14.114Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 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.hwled00/FX.cpp
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/FX.h
📚 Learning: 2025-11-16T19:40:46.260Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4926
File: wled00/FX.cpp:4727-4730
Timestamp: 2025-11-16T19:40:46.260Z
Learning: WLED AuroraWave (wled00/FX.cpp): wave_start and wave_end intentionally use int16_t; segments longer than 32k LEDs are not supported (bounded by MAX_LEDS), so widening to 32-bit is unnecessary.
Applied to files:
wled00/FX.hwled00/FX.cpp
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/FX.h
📚 Learning: 2025-11-27T06:33:11.436Z
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Applied to files:
wled00/FX.hwled00/FX.cpp
📚 Learning: 2025-10-20T09:41:50.374Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4995
File: wled00/FX.cpp:5209-5212
Timestamp: 2025-10-20T09:41:50.374Z
Learning: In WLED 2D effects, SEGMENT.length() returns the physical pixel count (upper bound), while SEG_W * SEG_H can be smaller depending on mapping (mirroring/grouping). For per-cell state buffers (e.g., mode_2Dgameoflife in wled00/FX.cpp), allocate using SEGMENT.length() to ensure sufficient capacity and avoid reallocations, while bounding indexing by cols*rows.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-11-14T05:48:44.673Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5040
File: wled00/image_loader.cpp:84-96
Timestamp: 2025-11-14T05:48:44.673Z
Learning: In WLED (wled00/FX_2Dfcn.cpp), the Segment::setPixelColorXY() function performs internal bounds checking against vWidth() and vHeight(), returning early if coordinates are out of bounds. No additional guards are needed when calling this function, even in upscaling loops where coordinates might exceed segment dimensions.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (1)
usermods/usermod_v2_animartrix/usermod_v2_animartrix.cpp (1)
SEGENV(64-76)
⏰ 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). (20)
- 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 (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
Comment on lines
+382
to
+385
|
|
||
| #define FX_MODE_ROTATE 218 | ||
|
|
||
| #define MODE_COUNT 219 |
Contributor
There was a problem hiding this comment.
Mode ID and count are correctly assigned.
The new FX_MODE_ROTATE constant (218) correctly follows the previous highest mode ID (217), and MODE_COUNT is properly updated to 219, following the established convention where MODE_COUNT represents the highest mode ID + 1.
Verify that FX.cpp includes the implementation:
mode_rotate_segment()function_data_FX_MODE_ROTATEdescriptor- Mode registration referencing ID 218
🤖 Prompt for AI Agents
In wled00/FX.h around lines 382 to 385, MODE_COUNT and FX_MODE_ROTATE are added
but FX.cpp must be updated accordingly: implement a mode_rotate_segment()
function, add a _data_FX_MODE_ROTATE descriptor object for the mode, and
register the mode using ID 218 in the mode registration table/list; ensure the
function name, descriptor variable name, and registration entry match the
FX_MODE_ROTATE constant (218) exactly and follow the same patterns and parameter
ordering used by other modes in FX.cpp.
Contributor
|
To clarify what @Brandon502 referred to when mentioning "segment options": My personal view is that "effect" (or, if you prefer, function that will paint pixels) should have no idea about other segments (or canvas if you prefer) other than the one it is supposed to paint on (encapsulation). This unfortunately prevents "effects" like segment copy or segment zoom or segment rotate (as it could only copy/zoom/rotate its segment/canvas). However if we move up in object hierarchy and allow segments to interact (within We already have segment property for 1D -> 2D expansion ( This is all theoretical and may not be easily achieved in practice, I agree (I needed 1 whole year to come up with segment blending that actually works). It may also overwhelm MCUs with small amounts of RAM like S2 and ESP8266 as JSON API will increase. However I think idea is worth exploring. |
Member
|
Hi @Brandon502 nice to see how an old idea gets a second chance with your PR 😃 it looks like you used the "shear rotation" from Moonlight, instead of my older "rotate each pixel coordinate" idea. I was always fighting with gaps/holes in the rotated pixels, does shear rotation work better? I'm (again) very busy with non-wled matters, but I'll take the time to review your code once I find a bit of time ✌️ |
Contributor
Author
Shear rotation shouldn't produce any holes outside of the corners when rotated which wrap can fix. This method is an inverse of Moonlight version. Instead of shearing the pixel to the destination, the destination is inverse sheared to the source location. It's already pretty efficient but I'm sure it can be improved further. |
Combined early return if not 2D / valid source
blazoncek
added a commit
to blazoncek/WLED
that referenced
this pull request
Dec 19, 2025
Contributor
|
@Brandon502 @DedeHai @softhack007 I've been considering zoom and rotate as a segment property (in a similar way as 1D->2D expansion is) and have come to the following extension proposal:
I have prepared POC which is available in my fork (includes UI). |
blazoncek
added a commit
to blazoncek/WLED
that referenced
this pull request
Dec 20, 2025
- see wled#5202 - does not include mirroring and always wraps
blazoncek
added a commit
that referenced
this pull request
Dec 23, 2025
- see #5202 for original implementation - does not include mirroring and always wraps - implemented as Segment property - reduced index.js footprint by coalescing several similar functions - slightly optimised push transition logic
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
New rotate effect based on copy that rotates a segment's 2D pixel data with optional zoom, wrap, and mirror wrap features. Inspired by @softhack007 spinning matrix MoonModules#122
This can be implemented as a "utility" effect like copy segment, or @blazoncek mentioned expanding SEGMENT options to include rotate, zoom, etc.
20251216_235446.mp4
20251216_235617.mp4
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.