Feat: Segment masks #324
Open
Feat: Segment masks #324
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
📝 WalkthroughWalkthroughAdds WLEDMM per-segment masking: new segment mask state and API, segmask JSON loading/enumeration, UI controls, JSON (de)serialization, and masking checks integrated into 1D/2D rendering and initialization. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend UI
participant App as WLED App
participant Seg as Segment
participant FS as Filesystem
participant Render as Renderer
UI->>App: setMask(seg, id)
App->>App: requestJson({seg:{id,mask}})
App->>Seg: deserializeSegment() with "mask"
Seg->>Seg: setMask(maskId)
Seg->>FS: open segmask{maskId}.json
FS-->>Seg: return JSON (w,h,inv,mask bits)
Seg->>Seg: parse, validate, allocate _mask, set _maskValid
Seg-->>App: mask applied (state updated)
App->>UI: update UI state
Note over Render,Seg: During render loop
Render->>Seg: setPixelColorXY(x,y,color)
Seg->>Seg: maskAllowsXY(x,y)?
alt allowed
Seg->>Seg: apply color
else masked
Seg-->>Seg: skip pixel
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 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
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: 2
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)
94-103: Mask reload path missing after copy/assignment.Copy ctor/assignment null out
_mask(and_maskValid) but preservemaskId/maskInvertvia memcpy. When rendering,maskAllows()checksif (!_mask || !_maskValid) return true;, so copied segments silently skip mask application even thoughmaskIdremains set. There is no automatic reload path—the effect must manually callsetMask(maskId)after copying, ormaskIdshould be cleared.
🤖 Fix all issues with AI agents
In `@wled00/FX_fcn.cpp`:
- Around line 253-260: _maskValid is only set once when loading the mask and can
become stale if virtual geometry changes; update the validity check by
recomputing it whenever geometry may change (for example at start of setUp() and
startFrame() or immediately before any mask application routine). Locate where
_maskValid is used (e.g., in FX_fcn::_maskValid checks and any applyMask/paint
routines) and replace the single-time assignment with a runtime check that
compares _maskW/_maskH to calc_virtualWidth()/calc_virtualHeight() (or reassign
_maskValid at the start of setUp()/startFrame()) so the mask validity reflects
current virtual geometry. Ensure no behavioral change besides recomputing
validity.
- Around line 150-176: The clearMask()/setMask() flow can free or replace _mask
while maskAllows() may be reading it on the other core; to fix, serialize the
pointer swap/free with the renderer by acquiring the same mutex used by the
render loop (e.g., segmentMux) around the critical section in Segment::clearMask
and Segment::setMask (before freeing or assigning _mask and updating
_maskLen/_maskW/_maskH/_maskValid/maskId) or alternatively wait for the strip to
be idle using the existing idle/wait API before changing the buffer; locate and
protect the critical sections in Segment::clearMask, Segment::setMask and
anywhere maskAllows reads _mask to prevent use-after-free.
🧹 Nitpick comments (2)
wled00/json.cpp (1)
364-372: Prefer an explicit clear path whenmaskis 0.
This makes intent obvious and avoids any unintended file load ifsetMask(0)isn’t a no-op.♻️ Proposed tweak
if (elem.containsKey("mask")) { // WLEDMM segment mask id int maskVal = elem["mask"] | 0; if (maskVal < 0) maskVal = 0; uint8_t maskId = constrain(maskVal, 0, WLED_MAX_SEGMASKS-1); - if (maskId != seg.maskId || (maskId != 0 && !seg.hasMask())) seg.setMask(maskId); + if (maskId == 0) { + if (seg.hasMask()) seg.clearMask(); + } else if (maskId != seg.maskId || !seg.hasMask()) { + seg.setMask(maskId); + } } if (elem.containsKey("minv")) { // WLEDMM segment mask invert seg.maskInvert = elem["minv"] | seg.maskInvert; }wled00/FX.h (1)
725-741: Consider adding[[gnu::hot]]attribute for hot-path optimization.The mask accessor logic is correct:
- Fail-open design (returns
truewhen mask is absent/invalid) prevents black pixels on error- Bounds checking prevents buffer overread
- Bit-packing/unpacking is efficient and correct (LSB-first)
Since
maskAllows()andmaskAllowsXY()are called for every pixel during rendering, consider adding the[[gnu::hot]]attribute (likeprogress()at line 692 andcurrentBri()at line 700) to hint the compiler for optimization.♻️ Suggested optimization
- inline bool hasMask(void) const { return _mask != nullptr; } // WLEDMM - inline bool maskAllows(uint16_t i) const { // WLEDMM + [[gnu::hot]] inline bool hasMask(void) const { return _mask != nullptr; } // WLEDMM + [[gnu::hot]] inline bool maskAllows(uint16_t i) const { // WLEDMM if (!_mask || !_maskValid) return true; if (size_t(i) >= _maskLen) return false; // WLEDMM: bit-packed mask (LSB-first): byte = i>>3, bit = i&7 bool bit = (_mask[i >> 3] >> (i & 7)) & 0x01; return maskInvert ? !bit : bit; } - inline bool maskAllowsXY(int x, int y) const { // WLEDMM + [[gnu::hot]] inline bool maskAllowsXY(int x, int y) const { // WLEDMM if (!_mask || !_maskValid) return true; if (x < 0 || y < 0) return false; size_t idx = size_t(x) + (size_t(y) * _maskW); if (idx >= _maskLen) return false; // WLEDMM: row-major (x + y*w), bit-packed mask (LSB-first in each byte) bool bit = (_mask[idx >> 3] >> (idx & 7)) & 0x01; return maskInvert ? !bit : bit; }
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Overview
I'm designing a fancy sign for a craft fair booth with an engraved sheet of acrylic in front. In order to properly light it, I would like to define segments for various shapes in the engraved sheet such that I can apply different effects to each area. Currently, support for this is limited to ledmaps (only one across all segments), jMaps (1D -> 2D translation only), and grouping (only works for specific designs)
This PR adds support for clipping masks per segment, defined in virtual LED space. This enables overlapping segments to render distinct effects/palettes while preserving normal segment geometry and mappings.
Summary of changes
How it works
Segmask.json
Masks are defined by 1/0 values in a list similar to ledmapN.json files:
{"w":16,"h":32,"mask":[0,0,0,1,1,1,0,0,0,0,...,0,1,1],"inv":false}Other Notes
I'd appreciate confirmation that this works on something other than the ESP32-S3 boards I have on hand
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.