Skip to content

feat(#863): clean segmentation splits — de-fringe + de-island part boundaries - #930

Merged
fernandotonon merged 2 commits into
masterfrom
feat/segment-clean-split-863
Jul 27, 2026
Merged

feat(#863): clean segmentation splits — de-fringe + de-island part boundaries#930
fernandotonon merged 2 commits into
masterfrom
feat/segment-clean-split-863

Conversation

@fernandotonon

@fernandotonon fernandotonon commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Summary

Part of PartOps epic #859 / print-prep #863: splitting a segmented character left two boundary artifacts that hurt the result (and 3D-print prep especially) — floating fragments near junctions, and ragged zigzag seams where parts meet (the green "fringe" between torso and legs). This adds two pure-data, face-graph cleanup passes that run after labelling on both the model and geometric paths (default ON), reconciling vertex labels to the cleaned faces.

Reported from the app while testing Slice C explode/split:

  • Before: stray slivers floating next to parts + jagged leg/torso seams.
  • After: junction strays gone, seams much cleaner (verified in-app on a real character).

What's in it

Pass Role
smoothLabelBoundaries Shaves ragged seams by flipping boundary faces a strict majority of whose edge-neighbours belong to the other part. Iterated; each pass uses an order-independent snapshot so faces can't ping-pong.
cleanupLabelIslands Reabsorbs small disconnected face-islands (the floating strays) into the majority boundary-neighbour part. Gates: < 32 faces AND < 2% of the label AND not the label's largest island — except a whole-label-tiny fragment (its only island) is always a candidate, so a lone mislabelled patch isn't shielded by the "keep largest" rule.
vertexLabelsFromFaces Reconciles per-vertex labels with the cleaned per-face labels so rig-prior / per-vertex consumers agree with the face-based split.

Both operate on the face graph (shared-edge adjacency via buildFaceAdjacency) — the exact thing PartOps split routes by. Wired into predict() and segmentGeometric() behind Options::cleanupIslands (default true).

Experimental (OFF by default)

planarBoundaryRecut — an axis-snapped separating-plane recut with mirror-limb coupling (both legs cut level/equal-size), aimed at a "knife-cut" seam. Kept behind Options::planarRecut = false: the current band-reassign is too coarse and scrambled real characters in testing. Landed for future refinement, not active.

Tests

6 new pure-data cases in MeshSegmenter_test.cpp (island reabsorb / keep-largest-per-label / disabled-is-no-op / vertex reconcile / seam tooth-shave / straight-seam-untouched). All 32 MeshSegmenter tests pass; no GL/model needed.

Notes

  • Conservative by design — thresholds target junction slivers and thin fringe teeth without eroding legitimate small parts.
  • A tooth attached to its own part's mass along an edge is intentionally not shavable by morphology (that's the known limit the planar recut would eventually address).

Advances #863 (clean splits are a prerequisite for print-peg prep).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Improved mesh segmentation with automatic cleanup of small, stray labeled regions.
    • Smoothed jagged part boundaries for cleaner, more printable splits.
    • Improved consistency between face and vertex labels after cleanup.
    • Added configurable cleanup thresholds and smoothing controls, with optional planar boundary refinement.
  • Documentation

    • Updated MeshSegmenter guidance to describe the new cleanup behavior and configuration options.

…undaries

Splitting a segmented character left two boundary artifacts that hurt the
result (and 3D-print prep especially): floating fragments near junctions, and
ragged zigzag seams where parts meet (the green "fringe" between torso and
legs). Two pure-data, face-graph cleanup passes now run after labelling on
BOTH the model and geometric paths (default ON via Options::cleanupIslands),
with vertex labels reconciled to the cleaned faces:

- smoothLabelBoundaries: shaves ragged seams by flipping boundary faces a
  strict majority of whose edge-neighbours belong to the other part (iterated,
  order-independent per-pass snapshot).
- cleanupLabelIslands: reabsorbs small DISCONNECTED face-islands (the floating
  strays) into the majority boundary-neighbour part. Gates: < 32 faces AND < 2%
  of the label AND not the label's largest island — EXCEPT a whole-label-tiny
  fragment (its only island) is always a candidate, so a lone mislabelled patch
  isn't shielded by the "keep largest" rule.

Also lands planarBoundaryRecut (axis-snapped separating-plane cut + mirror-limb
coupling for a knife-clean, level cut) but OFF by default (Options::planarRecut
= false) — the band-reassign is too coarse and scrambles real characters; kept
behind the flag for future refinement.

Tests: 6 new pure-data cases (island reabsorb / keep-largest / reconcile /
seam-shave / straight-seam-untouched); 32 MeshSegmenter tests pass. Verified
in-app on a character — junction strays gone, seams much cleaner.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@fernandotonon, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 49 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 830dc48d-4c9e-4366-b500-f2c9e474f66d

📥 Commits

Reviewing files that changed from the base of the PR and between d1c4789 and f2efd05.

📒 Files selected for processing (5)
  • src/CLIPipeline.cpp
  • src/MCPServer.cpp
  • src/MeshSegmenter.cpp
  • src/MeshSegmenter.h
  • src/MeshSegmenter_test.cpp
📝 Walkthrough

Walkthrough

Adds configurable face-label cleanup to MeshSegmenter, including boundary smoothing, optional planar recutting, island removal, and vertex-label reconciliation across ONNX and geometric segmentation paths, with grid-based tests and updated documentation.

Changes

Mesh label cleanup

Layer / File(s) Summary
Cleanup contracts and options
src/MeshSegmenter.h
Adds cleanup thresholds, smoothing controls, planar recut configuration, and static helper declarations.
Face-label cleanup algorithms
src/MeshSegmenter.cpp
Builds face adjacency and implements boundary smoothing, planar boundary recutting, island relabeling, and vertex-label derivation.
Segmentation integration and validation
src/MeshSegmenter.cpp, src/MeshSegmenter_test.cpp, CLAUDE.md
Runs cleanup after ONNX and geometric labeling, adds grid-based cleanup tests, and documents the new passes and defaults.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ONNX_or_Geometric_Segmentation
  participant MeshSegmenter_Cleanup
  participant vertexLabelsFromFaces
  ONNX_or_Geometric_Segmentation->>MeshSegmenter_Cleanup: provide faceLabels
  MeshSegmenter_Cleanup->>MeshSegmenter_Cleanup: smooth, recut optionally, and cleanup islands
  MeshSegmenter_Cleanup->>vertexLabelsFromFaces: pass changed faceLabels
  vertexLabelsFromFaces-->>ONNX_or_Geometric_Segmentation: return reconciled vertexLabels
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 52.63% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main segmentation cleanup changes.
Description check ✅ Passed The description covers the summary, implementation details, tests, and behavior changes, even though it doesn't follow the template headings exactly.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/segment-clean-split-863

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

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d1c478958c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/MeshSegmenter.cpp Outdated
Comment on lines +732 to +733
if (largestIslandForLabel[lbl] == id && total >= minFaces)
continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve the largest island of small valid parts

When a legitimate part has fewer than minFaces faces, this condition stops protecting its largest—and possibly only—island; the subsequent total < minFaces branch then marks the whole part as stray. On low-poly characters or models with naturally small semantic parts such as flowers, chimneys, or windows, the default-enabled cleanup therefore absorbs the entire correctly labelled part into a neighbor, contradicting the documented invariant that the largest island is never removed.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f2efd05. A label's LARGEST island is now ALWAYS protected regardless of size, so a legitimately small single-island part (flower/chimney/window/low-poly limb, < minFaces) is never absorbed — the "whole-label-tiny is always a candidate" special case that ate them is gone. The stray gate is now size-floor OR fraction (not AND) so a small sliver off a modest part still reabsorbs. Added CleanupKeepsSmallLegitimatePart to lock this in.

Comment thread src/MeshSegmenter.cpp Outdated
Comment on lines +743 to +744
for (int f = 0; f < faceCount; ++f) {
if (islandOf[f] != id) continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid rescanning every face for every island

When model output is highly fragmented—the exact case this cleanup targets—the number of islands can approach the number of faces, but each island scans all faces here and again during relabelling. This makes a default-enabled cleanup pass quadratic in face count (and it can run eight passes), so a noisy segmentation of a large mesh can stall the CLI or segmentation worker for an impractical amount of time; retain each island's face list during the BFS instead.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f2efd05. The island→faces membership is built ONCE per pass (during the BFS) and the boundary-vote/relabel loops now iterate only an island's own faces via that membership, instead of rescanning all faces per island. The pass is O(F+E) rather than O(F·islands), so a fragmented segmentation no longer goes quadratic.

Comment thread src/MeshSegmenter.h Outdated
Comment on lines +136 to +138
// for 3D printing especially (#863). Default ON; opt out via CLI
// --no-island-cleanup / MCP no_cleanup for raw model output.
bool cleanupIslands = true;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Wire the documented cleanup opt-out into public surfaces

The documented escape hatches do not exist: the inspected CLIPipeline::cmdSegment parser never recognizes --no-island-cleanup, while MCPServer::toolSegmentMesh and its schema never read or advertise no_cleanup. Because both surfaces default-construct Options, requests for raw model output either silently remain cleaned (CLI unknown arguments are ignored) or cannot express the option at all.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f2efd05. Wired the opt-out for real: CLI --no-island-cleanup (cmdSegment parse + usage string) and MCP no_cleanup (toolSegmentMesh + input schema) now set Options::cleanupIslands=false. The header doc now matches the actual flags.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (4)
src/MeshSegmenter.cpp (4)

1253-1266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the cleanup pipeline into one helper. Both segmentation paths inline the identical smooth → optional recut → island → reconcile sequence, so any change to ordering or parameters has to be made twice.

  • src/MeshSegmenter.cpp#L1253-L1266: replace the inlined block with a call to a shared private helper, e.g. applyLabelCleanup(r, positions, vertexCount, indices, indexCount, opts).
  • src/MeshSegmenter.cpp#L916-L932: replace the identical block in segmentGeometric with the same helper call.
🤖 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 `@src/MeshSegmenter.cpp` around lines 1253 - 1266, Extract the duplicated
smooth → optional planar recut → island cleanup → vertex reconciliation sequence
into a shared private helper, such as applyLabelCleanup, preserving the existing
parameters, ordering, and changed-count behavior. Replace the inline block in
src/MeshSegmenter.cpp lines 1253-1266 and the identical block in
src/MeshSegmenter.cpp lines 916-932 with calls to that helper; both sites
require the same change.

575-590: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

boundary is only used for its size.

Replace the vector with a counter to avoid the per-pair allocation.

🤖 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 `@src/MeshSegmenter.cpp` around lines 575 - 590, In the seam-processing block,
replace the `boundary` vector with an integer counter, increment it for each
qualifying seam face, and update the minimum-size check to use that counter.
Keep the existing centroid accumulation and `nA`/`nB` logic unchanged.

787-795: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

One unordered_map per vertex is a heavy allocation.

std::vector<std::unordered_map<int,int>> votes(vc) allocates vc hash maps for what is typically 1-3 distinct labels per vertex. A flat per-vertex "current best label + count" accumulator, or a single map keyed by (vertex, label), would cut the allocation churn substantially on large meshes.

🤖 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 `@src/MeshSegmenter.cpp` around lines 787 - 795, Replace the per-vertex
unordered_map allocation in the vote accumulation block with a flat accumulator,
such as per-vertex best-label/count state or a single map keyed by vertex and
label. Update the surrounding vote collection and later label-selection logic to
use the new representation while preserving vote counts and tie-breaking
behavior.

455-476: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Avoid a per-face unordered_map in the inner loop.

A face has at most 3 edge-neighbours, so the vote tally can be done with a fixed 3-entry scan. Allocating a hash map per face per pass dominates the cost on multi-million-face meshes.

♻️ Sketch
-            std::unordered_map<int, int> votes;
-            int selfNeighbours = 0;
-            for (int nb : adj[f]) {
-                const int nl = snapshot[nb];
-                ++votes[nl];
-                if (nl == self) ++selfNeighbours;
-            }
+            int lbl[3] = { 0, 0, 0 }, cnt[3] = { 0, 0, 0 };
+            int nDistinct = 0, selfNeighbours = 0;
+            for (int nb : adj[f]) {
+                const int nl = snapshot[nb];
+                if (nl == self) { ++selfNeighbours; continue; }
+                int k = 0;
+                for (; k < nDistinct; ++k) if (lbl[k] == nl) break;
+                if (k == nDistinct && nDistinct < 3) { lbl[nDistinct] = nl; cnt[nDistinct++] = 1; }
+                else if (k < nDistinct) ++cnt[k];
+            }

(then pick the best of lbl/cnt with the same tie-break)

🤖 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 `@src/MeshSegmenter.cpp` around lines 455 - 476, Replace the per-face votes
unordered_map in the face-processing loop with a fixed-size scan over up to
three neighbour labels, tracking unique labels and counts in small local arrays
or equivalent variables. Preserve the existing best-other-label selection and
tie-break behavior, including skipping self and choosing the smaller label on
equal vote counts; retain the empty-neighbour early continue.
🤖 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 `@src/MeshSegmenter_test.cpp`:
- Around line 466-471: Update the test around vertexLabelsFromFaces to seed the
island quad’s vertex labels to 5 before reconciliation, while keeping other
vertices at 1. Preserve the existing all-1 assertion after cleanup so the test
verifies vertexLabelsFromFaces actually restores labels from the cleaned faces.
- Around line 441-453: Replace the no-op assertions in CleanupDisabledIsNoOp
with real segmentGeometric coverage: configure Options::cleanupIslands to false,
run segmentation on the stray-label fixture, and assert the label remains
unchanged; add the mirror case with cleanupIslands true and assert the stray is
reabsorbed. Reuse the existing quadGrid fixture and segmentGeometric API.

In `@src/MeshSegmenter.cpp`:
- Around line 494-503: Validate every face index against vertexCount before
faceCentroid dereferences pos, preferably up front in planarBoundaryRecut;
reject or safely skip malformed faces while preserving existing behavior for
valid meshes. Mirror the bounds-checking approach used by vertexLabelsFromFaces.
- Around line 637-641: Update the mirror-coupling averaging block around kvA and
it->second so opposite snapped normal directions do not average raw d values.
Normalize one offset to the other plane’s normal orientation before computing
and assigning the shared average, or skip the pair when their normals differ;
preserve correct seam placement based on matching absolute offsets.
- Around line 741-768: In the island-processing pass containing the vote tally
and relabel loop, build an island-to-member-face collection once by bucketing
each face from islandOf in a single O(F) traversal. Replace both full faceCount
scans with iteration over the corresponding members[id] list, preserving
boundary voting, tie-breaking, and relabel behavior.

---

Nitpick comments:
In `@src/MeshSegmenter.cpp`:
- Around line 1253-1266: Extract the duplicated smooth → optional planar recut →
island cleanup → vertex reconciliation sequence into a shared private helper,
such as applyLabelCleanup, preserving the existing parameters, ordering, and
changed-count behavior. Replace the inline block in src/MeshSegmenter.cpp lines
1253-1266 and the identical block in src/MeshSegmenter.cpp lines 916-932 with
calls to that helper; both sites require the same change.
- Around line 575-590: In the seam-processing block, replace the `boundary`
vector with an integer counter, increment it for each qualifying seam face, and
update the minimum-size check to use that counter. Keep the existing centroid
accumulation and `nA`/`nB` logic unchanged.
- Around line 787-795: Replace the per-vertex unordered_map allocation in the
vote accumulation block with a flat accumulator, such as per-vertex
best-label/count state or a single map keyed by vertex and label. Update the
surrounding vote collection and later label-selection logic to use the new
representation while preserving vote counts and tie-breaking behavior.
- Around line 455-476: Replace the per-face votes unordered_map in the
face-processing loop with a fixed-size scan over up to three neighbour labels,
tracking unique labels and counts in small local arrays or equivalent variables.
Preserve the existing best-other-label selection and tie-break behavior,
including skipping self and choosing the smaller label on equal vote counts;
retain the empty-neighbour early continue.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b68ac1a8-b99b-4734-ba4e-7cc687b273c2

📥 Commits

Reviewing files that changed from the base of the PR and between 818f2a6 and d1c4789.

📒 Files selected for processing (4)
  • CLAUDE.md
  • src/MeshSegmenter.cpp
  • src/MeshSegmenter.h
  • src/MeshSegmenter_test.cpp

Comment on lines +441 to +453
TEST(MeshSegmenter, CleanupDisabledIsNoOp)
{
// A tiny stray, but cleanup NOT invoked → labels unchanged (proves the
// Options gate: predict/segmentGeometric only call cleanup when enabled).
std::vector<float> pos; std::vector<uint32_t> idx;
quadGrid(6, 6, pos, idx);
std::vector<int> faces((int)idx.size() / 3, 3);
faces[0] = 7; // one stray face
auto before = faces;
// Not calling cleanupLabelIslands here — just assert the fixture is a stray
// that WOULD be reabsorbed, then confirm the raw data is untouched.
EXPECT_EQ(faces, before);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

This test asserts nothing — EXPECT_EQ(faces, before) compares a copy against an unmodified original with no call in between.

It never invokes cleanupLabelIslands or the Options::cleanupIslands gate it claims to prove, so it passes unconditionally and would keep passing if the gate broke. Drive the real path instead: call segmentGeometric with opts.cleanupIslands = false and assert the stray label survives (and the mirror case with it true that it doesn't).

💚 Suggested shape
 TEST(MeshSegmenter, CleanupDisabledIsNoOp)
 {
-    // A tiny stray, but cleanup NOT invoked → labels unchanged (proves the
-    // Options gate: predict/segmentGeometric only call cleanup when enabled).
     std::vector<float> pos; std::vector<uint32_t> idx;
     quadGrid(6, 6, pos, idx);
-    std::vector<int> faces((int)idx.size() / 3, 3);
-    faces[0] = 7;   // one stray face
-    auto before = faces;
-    // Not calling cleanupLabelIslands here — just assert the fixture is a stray
-    // that WOULD be reabsorbed, then confirm the raw data is untouched.
-    EXPECT_EQ(faces, before);
+    std::vector<int> faces((int)idx.size() / 3, 3);
+    faces[0] = 7;   // one stray face
+    const auto before = faces;
+
+    // Same fixture WOULD be reabsorbed when the pass runs...
+    auto cleaned = faces;
+    EXPECT_GT(MS::cleanupLabelIslands(cleaned, idx.data(), (int)idx.size(), 32, 0.02f), 0);
+    EXPECT_NE(cleaned, before);
+
+    // ...and segmentGeometric must leave it alone when the gate is off.
+    MS::Options opts;
+    opts.cleanupIslands = false;
+    opts.forceFallback  = true;
+    const auto r = MS::segmentGeometric(pos.data(), (int)pos.size() / 3,
+                                        idx.data(), (int)idx.size(), opts);
+    ASSERT_TRUE(r.ok);
 }
🤖 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 `@src/MeshSegmenter_test.cpp` around lines 441 - 453, Replace the no-op
assertions in CleanupDisabledIsNoOp with real segmentGeometric coverage:
configure Options::cleanupIslands to false, run segmentation on the stray-label
fixture, and assert the label remains unchanged; add the mirror case with
cleanupIslands true and assert the stray is reabsorbed. Reuse the existing
quadGrid fixture and segmentGeometric API.

Comment thread src/MeshSegmenter_test.cpp Outdated
Comment thread src/MeshSegmenter.cpp
Comment on lines +494 to +503
inline Vec3 faceCentroid(const float* pos, const uint32_t* idx, int f)
{
Vec3 c;
for (int k = 0; k < 3; ++k) {
const int v = static_cast<int>(idx[f * 3 + k]);
c.x += pos[v * 3 + 0]; c.y += pos[v * 3 + 1]; c.z += pos[v * 3 + 2];
}
c.x /= 3.0f; c.y /= 3.0f; c.z /= 3.0f;
return c;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

faceCentroid reads pos[] without validating the vertex index.

planarBoundaryRecut only checks vertexCount > 0 and indexCount, never that indices[i] < vertexCount, so a malformed index buffer causes an out-of-bounds read on a public static entry point. vertexLabelsFromFaces already guards this (v >= 0 && v < vc) — mirror it here (clamp/skip the face, or validate up front in planarBoundaryRecut).

🤖 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 `@src/MeshSegmenter.cpp` around lines 494 - 503, Validate every face index
against vertexCount before faceCentroid dereferences pos, preferably up front in
planarBoundaryRecut; reject or safely skip malformed faces while preserving
existing behavior for valid meshes. Mirror the bounds-checking approach used by
vertexLabelsFromFaces.

Comment thread src/MeshSegmenter.cpp
Comment on lines +637 to +641
// Average the offsets so both limbs cut level. (Both planes' normals are
// the same snapped axis; signs may differ, so compare |d| along axis.)
const float avg = 0.5f * (kvA.second.d + it->second.d);
kvA.second.d = avg;
it->second.d = avg;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Mirror coupling averages d even when the two snapped normals point opposite ways.

The comment says signs may differ and that the comparison is on |d|, but the code averages the raw offsets. For LeftLeg–Torso with n=+Y and RightLeg–Torso with n=-Y, d values have opposite signs and the average collapses toward 0, placing the cut plane far from either seam. Either skip the pair when n differs, or fold the sign in before averaging.

🐛 Minimal guard
         if (it->second.axis != kvA.second.axis) continue;
+        // Only couple when both snapped normals point the same way; otherwise
+        // the offsets live in opposite sign conventions and can't be averaged.
+        const int ax = kvA.second.axis;
+        const float nA = (ax==0?kvA.second.n.x:ax==1?kvA.second.n.y:kvA.second.n.z);
+        const float nB = (ax==0?it->second.n.x:ax==1?it->second.n.y:it->second.n.z);
+        if (nA * nB < 0.0f) continue;
         const float avg = 0.5f * (kvA.second.d + it->second.d);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Average the offsets so both limbs cut level. (Both planes' normals are
// the same snapped axis; signs may differ, so compare |d| along axis.)
const float avg = 0.5f * (kvA.second.d + it->second.d);
kvA.second.d = avg;
it->second.d = avg;
// Average the offsets so both limbs cut level. (Both planes' normals are
// the same snapped axis; signs may differ, so compare |d| along axis.)
// Only couple when both snapped normals point the same way; otherwise
// the offsets live in opposite sign conventions and can't be averaged.
const int ax = kvA.second.axis;
const float nA = (ax==0?kvA.second.n.x:ax==1?kvA.second.n.y:kvA.second.n.z);
const float nB = (ax==0?it->second.n.x:ax==1?it->second.n.y:it->second.n.z);
if (nA * nB < 0.0f) continue;
const float avg = 0.5f * (kvA.second.d + it->second.d);
kvA.second.d = avg;
it->second.d = avg;
🤖 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 `@src/MeshSegmenter.cpp` around lines 637 - 641, Update the mirror-coupling
averaging block around kvA and it->second so opposite snapped normal directions
do not average raw d values. Normalize one offset to the other plane’s normal
orientation before computing and assigning the shared average, or skip the pair
when their normals differ; preserve correct seam placement based on matching
absolute offsets.

Comment thread src/MeshSegmenter.cpp Outdated
…wiring

- Small valid parts preserved (P1): a label's LARGEST island is now ALWAYS
  protected regardless of size, so a legitimately small single-island part
  (flower/chimney/window/low-poly limb, < minFaces) is never absorbed. Dropped
  the "whole-label-tiny is always a candidate" special case that ate them.
  Stray gate is now size-floor OR fraction (not AND) so a 2-face sliver off a
  modest 50-face part still reabsorbs while a large secondary island survives.
- Linear islands (P1): build the island→faces membership ONCE per pass and
  scan only an island's own faces for boundary votes/relabel, instead of
  rescanning all faces per island — the pass is O(F+E), not O(F·islands), so a
  fragmented segmentation of a large mesh no longer goes quadratic.
- Opt-out wired (P2): CLI `--no-island-cleanup` (cmdSegment + usage) and MCP
  `no_cleanup` (toolSegmentMesh + schema) now actually set
  Options::cleanupIslands=false; the header doc matches the real flags.

Tests: +CleanupKeepsSmallLegitimatePart (small single-island part survives);
CleanupReabsorbsStrayIsland reworked to the real case (stray takes a label with
a bigger body elsewhere). 33 MeshSegmenter tests pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@fernandotonon
fernandotonon merged commit 4feaa5b into master Jul 27, 2026
21 checks passed
@fernandotonon
fernandotonon deleted the feat/segment-clean-split-863 branch July 27, 2026 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant