Add 3D cube topology support and wrapping animations#5604
Conversation
WalkthroughAdds a new M12_Cube 2D mapping with cube-coordinate wrapping, integrates it into pixel read/write paths, persists the isCube flag in config, and exposes UI controls plus a preset generator to create a 6-face cube panel layout. ChangesCube Mapping Feature
Sequence Diagram(s)(omitted — change is primarily a mapping mode with localized coordinate transformation and UI/config plumbing; a sequence diagram is not required.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 6
🧹 Nitpick comments (3)
wled00/FX.h (1)
1008-1014: 💤 Low valueConsider packing
isCubeinto the adjacent bitfield struct.
isMatrixis currently a fullboolfor legacy reasons, but the very next member is a bit-packed struct holdingautoSegments,correctWB, andcctFromRgb. AddingisCubeas a 1-bit member there (alongside those flags) would save one byte inWS2812FXwithout changing semantics and keeps cube state colocated with other "strip mode" flags.♻️ Proposed refactor
bool isMatrix; - bool isCube; struct { bool autoSegments : 1; bool correctWB : 1; bool cctFromRgb : 1; + bool isCube : 1; };The corresponding initializer in the constructor (line 831) and the
cfg.cpp/set.cpp/xml.cppaccessors that read/writestrip.isCubewill continue to work unchanged.🤖 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 1008 - 1014, Replace the standalone bool isCube with a 1-bit field inside the adjacent anonymous bitfield so it becomes packed with autoSegments, correctWB and cctFromRgb (e.g., add "bool isCube : 1;" to that struct and remove the separate "bool isCube;" declaration); leave isMatrix as-is and keep the same member name isCube so existing constructor initialization and cfg.cpp / set.cpp / xml.cpp accessors continue to work unchanged.wled00/FX_2Dfcn.cpp (2)
188-239: 💤 Low valueIf this block is AI-generated, wrap it with the required
// AI:markers.Per the repository coding guidelines, "Embed larger blocks of AI-generated code within
// AI: below section was generated by an AI...// AI: endcomments". The cube wrapping logic (with its case-by-case "Top-Left gap", "Bottom-Right gap", etc. comments) looks like a candidate for that convention; if any portion of it was generated by an AI assistant, please wrap the corresponding range accordingly.As per coding guidelines: "Mark AI-generated code blocks with
// AI: below section was generated by an AI/// AI: endcomments."🤖 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_2Dfcn.cpp` around lines 188 - 239, The Segment::_applyCubeWrapping function contains a multi-line cube-wrapping implementation that may be AI-generated; if so, wrap that AI-produced region with the repository's required markers: insert a comment "// AI: below section was generated by an AI" immediately before the start of the generated block inside Segment::_applyCubeWrapping and a matching "// AI: end" immediately after the end of that block, preserving existing comments and indentation and ensuring the markers enclose the entire logic from the initial if-check through the final bounds check so the function remains syntactically unchanged.
188-188: ⚡ Quick winConsider removing
IRAM_ATTR_YNfrom_applyCubeWrappingto clarify intent, or rely onWLED_SAVE_IRAMflag for IRAM budget protection.The concern about _applyCubeWrapping's size (~50 lines, multi-branch cube wrapping logic) is valid—it's more complex than the thin wrappers at setPixelColorXY and getPixelColorXY (5–6 lines each). However, WLED_SAVE_IRAM is already enabled in platformio.ini with a comment noting it prevents linker errors, which disables IRAM placement for all three functions by default. This global flag effectively mitigates the IRAM budget concern. For clarity and maintainability, consider either (1) unconditionally removing
IRAM_ATTR_YNfrom_applyCubeWrappingto signal that cube wrapping is not a hot path, or (2) document that the global flag serves as the safety valve. The current state works but relies on the config-level flag rather than explicit code intent.🤖 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_2Dfcn.cpp` at line 188, The function Segment::_applyCubeWrapping currently has the IRAM_ATTR_YN attribute; remove that attribute from the function signature to clarify intent that this larger, multi-branch helper is not a hot IRAM-must function (i.e., change "void IRAM_ATTR_YN Segment::_applyCubeWrapping(...)" to just "void Segment::_applyCubeWrapping(...)"), or if you prefer to keep it, add a short comment on top of the function referencing WLED_SAVE_IRAM in platformio.ini as the intended global IRAM protection so reviewers know the IRAM placement is controlled centrally; update the declaration and any forward declarations accordingly and rebuild to ensure no linker/placement regressions.
🤖 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 `@wled00/data/settings_2D.htm`:
- Around line 122-147: genCube() blindly assumes 6 panels exist and writes
Sf.P0..P5 fields even when addPanel() may stop early due to a lower maxPanels,
causing runtime errors; fix by determining the actual number of panels to create
(e.g., let count = Math.min(6, maxPanels) or by running addPanel until it
returns false and counting successes), set Sf.MPC.value = count, call addPanel()
exactly count times, and only write Sf.P{i}X/P{i}Y and
Sf["P"+i+"W"]/Sf["P"+i+"H"] for i in [0..count-1] (leave or clear remaining
fields as appropriate) so genCube(), addPanel(), Sf.MPC and the P0..P5
assignments stay consistent.
In `@wled00/FX_2Dfcn.cpp`:
- Around line 188-190: The hot-path cost comes from calling
Segment::_applyCubeWrapping out-of-line on every pixel; change callers
Segment::setPixelColorXY and Segment::getPixelColorXY to inline the cheap
is-cube check (e.g. if (strip.isCube) _applyCubeWrapping(x,y);) and move the
heavier dimension validation into _applyCubeWrapping itself; inside
_applyCubeWrapping cache n = vHeight() / 3 (and derived 2*n, 4*n as needed)
instead of recomputing vHeight()/3 multiple times, and optionally add a
precomputed boolean (updated when geometry or strip.isCube changes) that encodes
"cube layout valid" so per-pixel code only does a single flag check.
- Line 190: The dimension check in the conditional using strip.isCube and
vWidth/vHeight allows non-multiples-of-3 heights due to integer truncation;
update the guard in FX_2Dfcn.cpp so it returns early unless strip.isCube is true
AND vHeight() is divisible by 3 and vWidth() equals (vHeight()/3)*4 (i.e. add
vHeight() % 3 == 0 into the condition), ensuring the remapping logic that
assumes vHeight() == 3*n only runs for valid cube-net dimensions.
- Around line 236-239: The final unconditional clamps for x and y in the
wrapping logic (the lines that force x into [0,4*n) and y into [0,3*n)) should
be removed so out-of-net coordinates remain out of range and are dropped by the
caller’s unsigned-range check; locate the bounds-clamping code in FX_2Dfcn.cpp
(the block that does "if (x < 0) x = 0; else if (x >= 4 * n) x = 4 * n - 1;" and
the analogous y line) and delete those two clamp statements (or replace them
with a no-op/early return that leaves x/y unchanged) so stray writes are not
snapped to the nearest face edge and are instead discarded by the caller.
- Around line 188-239: The cube-wrapping function _applyCubeWrapping currently
only checks strip.isCube and global aspect ratio, so per-segment mapping1D2D_t
(M12_Cube) is ignored; gate the remap by checking the segment's mapping
selection before applying the transform. Update Segment::_applyCubeWrapping to
early-return unless both strip.isCube and this->mapping1D2D == M12_Cube (or the
segment-level flag/name you use for per-segment mapping), or alternatively
remove M12_Cube from mapping1D2D_t and the UI if cube behavior is truly
strip-wide; refer to symbols _applyCubeWrapping, strip.isCube, mapping1D2D_t,
M12_Cube and any segment field like mapping1D2D or map1D2D to locate the change.
Ensure tests/usage paths that expect flat 2D semantics for segments not set to
M12_Cube remain unchanged.
In `@wled00/FX.h`:
- Line 550: The current cube-wrapping logic in _applyCubeWrapping (declared in
FX.h and implemented in FX_2Dfcn.cpp) ignores the per-segment map1D2D setting
and instead uses strip.isCube and aspect checks (vWidth(), vHeight()), which
makes the per-segment enum value M12_Cube ineffective; fix this by gating the
cube remapping so it only runs when the segment's map1D2D == M12_Cube (or, if
cube wrapping is meant to be a strip-global feature, remove M12_Cube from the
per-segment enum and related UI option), updating the implementation in
FX_2Dfcn.cpp to consult the Segment::map1D2D before applying cube wrapping and
leaving strip.isCube/aspect checks as supplementary conditions.
---
Nitpick comments:
In `@wled00/FX_2Dfcn.cpp`:
- Around line 188-239: The Segment::_applyCubeWrapping function contains a
multi-line cube-wrapping implementation that may be AI-generated; if so, wrap
that AI-produced region with the repository's required markers: insert a comment
"// AI: below section was generated by an AI" immediately before the start of
the generated block inside Segment::_applyCubeWrapping and a matching "// AI:
end" immediately after the end of that block, preserving existing comments and
indentation and ensuring the markers enclose the entire logic from the initial
if-check through the final bounds check so the function remains syntactically
unchanged.
- Line 188: The function Segment::_applyCubeWrapping currently has the
IRAM_ATTR_YN attribute; remove that attribute from the function signature to
clarify intent that this larger, multi-branch helper is not a hot IRAM-must
function (i.e., change "void IRAM_ATTR_YN Segment::_applyCubeWrapping(...)" to
just "void Segment::_applyCubeWrapping(...)"), or if you prefer to keep it, add
a short comment on top of the function referencing WLED_SAVE_IRAM in
platformio.ini as the intended global IRAM protection so reviewers know the IRAM
placement is controlled centrally; update the declaration and any forward
declarations accordingly and rebuild to ensure no linker/placement regressions.
In `@wled00/FX.h`:
- Around line 1008-1014: Replace the standalone bool isCube with a 1-bit field
inside the adjacent anonymous bitfield so it becomes packed with autoSegments,
correctWB and cctFromRgb (e.g., add "bool isCube : 1;" to that struct and remove
the separate "bool isCube;" declaration); leave isMatrix as-is and keep the same
member name isCube so existing constructor initialization and cfg.cpp / set.cpp
/ xml.cpp accessors continue to work unchanged.
🪄 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: 78a5cdf2-3fe7-4e00-9062-7cad3f9f2d15
📒 Files selected for processing (7)
wled00/FX.hwled00/FX_2Dfcn.cppwled00/cfg.cppwled00/data/index.jswled00/data/settings_2D.htmwled00/set.cppwled00/xml.cpp
… gating/markers, and improve genCube robustness
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@wled00/FX_2Dfcn.cpp`:
- Around line 211-239: The fallback mapping for the back-column gap (when y < n
and fx is not 0/1/2) leaves x in [3*n,4*n) (a gap) instead of remapping into the
Top face span [n,2*n), and there's no symmetric fx==3 branch for the Bottom
side; update the fallback in the "Moving UP" block so the back-gap maps into the
Top face coordinates (use x = 2*n - 1 - u and y = n - 1 - v or equivalent to
place x into [n,2*n) and y into [0,n)), and add a matching branch in the "Moving
DOWN" block (for fx == 3) that maps the back-column gap into the Bottom face
span [n,2*n) (mirror the coordinate transforms using u/v and n so x falls into
[n,2*n) and y into [2*n,3*n) as appropriate); refer to variables fx, x, y, u, v,
and n to locate and adjust the transformations.
🪄 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: 61b6b891-3532-4380-bd54-6ce65af733a3
📒 Files selected for processing (3)
wled00/FX.hwled00/FX_2Dfcn.cppwled00/data/settings_2D.htm
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/data/settings_2D.htm
- wled00/FX.h
@palatter just an idea - can you also support 5-panel cubes (bottom panel missing because invisible) and 3- or 4-panel cubes (backfaces missing)? "Different problem" would be an acceptable answer - I just remember from the past that some users succeeded with these "lightweight cube" variants by directly applying a custom |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@wled00/FX_2Dfcn.cpp`:
- Around line 188-223: The fx != 1 checks in Segment::applyCubeWrapping silently
remap out-of-range x values (fx == -1 or fx >= 4) instead of letting the caller
drop them; change both occurrences inside the fy == 0 and fy == 2 branches to
test fx == 3 (the intended "Back" gap) so only the legitimate back-gap case is
remapped and true out-of-range coordinates fall through to the caller's bounds
check; update the two branches that currently say "fx != 1" to "fx == 3"
(keeping the same remap assignments to x and y).
🪄 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: 8fee6f00-0e9d-4ffa-8ff0-7a264721cbc3
📒 Files selected for processing (2)
wled00/FX.hwled00/FX_2Dfcn.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/FX.h
| void Segment::applyCubeWrapping(int &x, int &y) const | ||
| { | ||
| // AI: below section was generated by an AI | ||
| if (!strip.isCube || map1D2D != M12_Cube) return; | ||
| const uint16_t vH = vHeight(); | ||
| const uint16_t vW = vWidth(); | ||
| if (vH % 3 != 0 || vW != (vH / 3) * 4) return; | ||
|
|
||
| int n = vH / 3; | ||
| int fx = (x >= 0) ? (x / n) : -1; | ||
| int fy = (y >= 0) ? (y / n) : -1; | ||
| int u = (x % n + n) % n; | ||
| int v = (y % n + n) % n; | ||
|
|
||
| if (fy < 0 || fy > 2) { // Above Top or Below Bottom | ||
| x = 4 * n - 1 - u; | ||
| y = 2 * n - 1 - v; | ||
| } else if (fy == 0) { | ||
| if (fx == 0) { // Top-Left gap -> maps to Top face left edge | ||
| x = n + (n - 1 - v); y = u; | ||
| } else if (fx == 2) { // Top-Right gap -> maps to Top face right edge | ||
| x = n + v; y = n - 1 - u; | ||
| } else if (fx != 1) { // fx == 3 (Top-Back gap) -> maps to Top face top edge | ||
| x = 2 * n - 1 - u; y = n - 1 - v; | ||
| } | ||
| } else if (fy == 2) { | ||
| if (fx == 0) { // Bottom-Left gap -> maps to Bottom face left edge | ||
| x = n + v; y = 3 * n - 1 - u; | ||
| } else if (fx == 2) { // Bottom-Right gap -> maps to Bottom face right edge | ||
| x = 2 * n - 1 - v; y = 2 * n + u; | ||
| } else if (fx != 1) { // fx == 3 (Bottom-Back gap) -> maps to Bottom face bottom edge | ||
| x = 2 * n - 1 - u; y = 3 * n - 1 - v; | ||
| } | ||
| } | ||
| // AI: end | ||
| } |
There was a problem hiding this comment.
fx != 1 branches silently remap out-of-range x instead of letting the caller drop it.
Inside the fy == 0 and fy == 2 blocks, the third branch uses fx != 1 with a comment indicating the intent is fx == 3 (Top/Bottom-Back gap). However, fx can also be -1 (when x < 0) or >= 4 (when x >= 4*n), because nothing earlier in the function bounds x. For those cases, this branch still fires and remaps the coordinate onto a valid Top/Bottom face cell:
x = -1, y in [0, n)→fx = -1, hitsfx != 1, mapped onto top face's left column.x >= 4n, y in [0, n)→fx >= 4, hitsfx != 1, mapped onto top face.- Symmetric cases for
fy == 2.
This contradicts the design that out-of-cube-net coordinates should remain out of range so the caller's (unsigned)x >= vWidth() check discards them (the same rationale used to remove the earlier [0,4n) x [0,3n) clamp). Effects that occasionally probe just off the segment edges with cube mode active may paint stray pixels onto Top/Bottom face borders.
Tightening to an exact fx == 3 keeps the back-gap fix but lets truly out-of-range writes/reads fall through to the caller's bounds check:
🛠️ Proposed fix
} else if (fy == 0) {
if (fx == 0) { // Top-Left gap -> maps to Top face left edge
x = n + (n - 1 - v); y = u;
} else if (fx == 2) { // Top-Right gap -> maps to Top face right edge
x = n + v; y = n - 1 - u;
- } else if (fx != 1) { // fx == 3 (Top-Back gap) -> maps to Top face top edge
+ } else if (fx == 3) { // Top-Back gap -> maps to Top face top edge
x = 2 * n - 1 - u; y = n - 1 - v;
}
} else if (fy == 2) {
if (fx == 0) { // Bottom-Left gap -> maps to Bottom face left edge
x = n + v; y = 3 * n - 1 - u;
} else if (fx == 2) { // Bottom-Right gap -> maps to Bottom face right edge
x = 2 * n - 1 - v; y = 2 * n + u;
- } else if (fx != 1) { // fx == 3 (Bottom-Back gap) -> maps to Bottom face bottom edge
+ } else if (fx == 3) { // Bottom-Back gap -> maps to Bottom face bottom edge
x = 2 * n - 1 - u; y = 3 * n - 1 - v;
}
}🤖 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_2Dfcn.cpp` around lines 188 - 223, The fx != 1 checks in
Segment::applyCubeWrapping silently remap out-of-range x values (fx == -1 or fx
>= 4) instead of letting the caller drop them; change both occurrences inside
the fy == 0 and fy == 2 branches to test fx == 3 (the intended "Back" gap) so
only the legitimate back-gap case is remapped and true out-of-range coordinates
fall through to the caller's bounds check; update the two branches that
currently say "fx != 1" to "fx == 3" (keeping the same remap assignments to x
and y).
|
@palatter general question: would it be more efficient maybe to pre-calculate the complete mapping into customMappingTable[] (a.k.a. "ledmap"), instead of re-calculating for each pixel being painted? |
That's a good idea! I'll investigate that. I think it would be trade off of cpu vs memory and likely add more complexity. |
|
I concur with @softhack007 -- an ledmap calculator that doesn't require any logic changes is the best approach. Ideally, it could be implemented entirely in JS in your own repo, and integrated for optional download via the wled-web-tools repository. Basically you make a standalone .htm page, and the web tools repo lets users download it to their WLED install via the Pixelforge system UI (the hammer icon). |
|
Possibly related: an xls sheet that calculates a ledmap.json for some cubes. |
3 participants
This PR adds native support for 6-panel 3D cubes to WLED.
Key changes:
applyCubeWrappinginFX_2Dfcn.cppto allow 2D animations to flow seamlessly across all 6 faces of a cube.This enables users with 3D-printed cubes or similar 6-face geometry to use the full library of WLED 2D effects with correct physical behavior.
Summary by CodeRabbit