Clean up setGeometry (main)

[willmmiles]

Reorganize setGeometry to perform all checks first before applying the values. This lets us accurately detect changes before starting a transition.

Forward port from #5474.

Sorry for the noise, I accidentally pushed this directly to main while setting up the branch. :(

Summary by CodeRabbit

• Bug Fixes
  • Improved segment geometry handling by sanitizing/clamping all geometry inputs (including 1D/2D bounds) before updating state, with additional safeguards for invalid configurations.
  • Enhanced “no-change” detection so geometry updates are skipped when nothing relevant changed.
  • Improved render/pixel buffer lifecycle: allocation is performed only when needed, and if allocation fails the segment is cleanly disabled to avoid inconsistent playback/rendering.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 10be7720-096a-4652-b497-f3012e581c04

📥 Commits

Reviewing files that changed from the base of the PR and between 73c2224 and acb9487.

📒 Files selected for processing (1)

• wled00/FX_fcn.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• wled00/FX_fcn.cpp

---

Walkthrough

Segment::setGeometry() now sanitizes geometry inputs before mutating state, expands the unchanged-bounds check, and changes pixel-buffer reallocation so allocation failure disables the segment and falls through to existing cleanup.

Changes

Segment Geometry Refactor

Layer / File(s)	Summary
Input sanitization and bounds check wled00/FX_fcn.cpp	Clamps and normalizes geometry inputs, disables invalid ranges, and evaluates a broader unchanged-bounds condition before mutating state.
State update and buffer reallocation wled00/FX_fcn.cpp	Updates segment state after the early return, reallocates the pixel buffer, and uses the length()==0 cleanup path when allocation fails.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• wled/WLED#5474: Refactors the same Segment::setGeometry() flow with input clamping and a stronger unchanged-bounds check before state mutation.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and directly matches the main change: a cleanup/refactor of setGeometry.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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_fcn.cpp`:
- Around line 466-468: Clamp oversized stop values in the segment handling logic
so 2D matrix segments cannot extend past the matrix domain; the current `i2`
adjustment in `FX_fcn.cpp` allows `stop` to reach `strip.getLengthTotal()` even
when the segment starts in the matrix area, which can make `Segment::length()`
exceed the framebuffer bounds. Update the `i2` clamping branch in this
segment-stop calculation to limit by the segment’s matrix dimensions first, and
only permit total-length stops for trailing 1D segments, using the existing
`Segment` and `strip` checks around the `i2` logic.

🪄 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: 8e5c94a2-e9df-4bbb-8b12-4e9a710fd1a7

📥 Commits

Reviewing files that changed from the base of the PR and between 14709dd and 4e5f3f7.

📒 Files selected for processing (1)

• wled00/FX_fcn.cpp

[DedeHai]

can you explain why one does inherit and the other does not? in what scenario does this even happen? I imagine if loading a preset with oob segment settings?

[willmmiles]

Historical compatibility, ie. that's what the code prior to this PR did. I agree it's a strange semantic. Like so many API quirks I imagine it was an early implementation decision that stuck around. The tweak here was because I misread the original implementation - in the original code, Y was always reset to 0 if invalid, while X keeps the old value.

I ended up with a little test script to validate that the API was consistent with main, except fixing the crash and notification bugs. Sadly there is one incompatibility I left in here as I judged it wasn't worth the effort to fix: if you have a system with a matrix + trailing strip, and you send an update to a segment that was in the trailing strip with a trailing-strip stop, but matrix start index; then the original code would disable the segment, but this code keeps the old start value.

eg. for a system with a 10x10 matrix followed by a 100 LED strip:
original segment: { "start": 120, "len": 60 } (stop = 180)
is sent a bad update: { "start": 2 } (unlike every other value, omitting "len" doesn't keep the length the same, it keeps the stop coord the same...!!!)

The old code would produce:
{ "start": 120, "len": 0 } (disabled)
while the new code will still land at:
{ "start": 120, "len": 60}

[DedeHai]

thanks for the explanation, makes sense to me. Matrix with trailing strip is a bit of a special case I assume not many people use.
could you please update the comment to // Unlike i1, doesn't inherit old value (legacy) for clarity?