feat(#863): PartOps Slice D — split parts + Solidify (thin-shell wall volume)

[fernandotonon]

Summary

PartOps epic #859, Slice D (#863). The original goal was 3D-print alignment pegs, but that direction hit the same wall the commercial tools do: real dowel/socket connectors on organic AI-segmented character joints are unreliable (no safe flat cut plane through a hip/shoulder seam — it also slices the torso body), and Meshy/Tripo cut organic seams but ship no discrete pegs either (only slicers like PrusaSlicer/Meshmixer do dowels, and only on a user-placed flat plane over a whole model).

So this PR ships the part that reliably makes split parts usable: Solidify — give thin-shell game assets real wall volume so an exploded part shows a solid cross-section instead of its hollow interior.

What's in it

Piece	Role
SubMeshOps::solidify (SplitOptions::solidifyParts)	Offsets an INNER copy of the surface inward by a thickness (auto ≈1.5% of the AABB diagonal) along area-weighted vertex normals, reverse-winds it, and stitches a wall between every open boundary edge and its inner counterpart (wall loop b→a→ai→bi cancels both the outer a→b and the reverse-wound inner dangling edges → watertight). Turns each thin part into a closed slab AND seals it.
Surfaces	GUI "Solidify thin shells" checkbox in the Split into Parts section → SplitMeshCommand solidify param; CLI qtmesh segment <file> --split-parts --solidify -o out.glb; MCP split_mesh_by_segments {solidify: true}. Default OFF (adds geometry; only meaningful for thin shells).

The plain split keeps its existing behavior (separate geometry, exact triangle count); explode / join are unchanged.

Built and removed along the way

• 3D-print alignment pegs — preparePrintPegs/buildAlignmentPegs/estimateBoundaryPlane, AddPrintPegsCommand, the Manifold (elalish/manifold) CSG FetchContent dependency, CLI --print-pegs, MCP prepare_print_split, GUI "Prepare for 3D Print". Unreliable on organic joints (see Summary).
• capOpenBoundaries cut-face capping (SplitOptions::capParts + the explode "Cap open boundaries" toggle). Capping a cut RING is geometrically watertight, but a thin single-sided game shell still LOOKS hollow at the cut (its own back-wall sits right behind the flat cap). A recessed-rim variant was tried and produced mesh artifacts, so cap was dropped in favour of Solidify.

Tests

31 tests across SubMeshOps / SplitMeshCommand / ExplodeParts / JoinParts / CLI split coverage, incl. Solidify (open flat quad → watertight slab of exact thickness; auto-thickness + no-op on empty). Verified on Rumba Dancing.fbx (rigged, skeleton preserved) and Hip Hop Dancing.obj (each part ~2× verts, 0 welded open edges after solidify) via CLI + MCP screenshots.

🤖 Generated with Claude Code

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

PartOps split and explode workflows now support watertight boundary capping. The option is exposed in the UI and propagated through commands and scene processing. CLI splitting enables capping, mesh-building supports normal recomputation, and alignment-peg APIs and tests are removed.

Changes

PartOps watertight splitting and explosion

Layer / File(s)	Summary
Boundary capping implementation src/SubMeshOps.*, src/SubMeshOps_test.cpp, CLAUDE.md	Adds capParts and capOpenBoundaries, closes boundary loops with centroid-fan triangles, and replaces alignment-peg coverage with capping tests.
Watertight split and mesh construction src/commands/SplitMeshCommand.cpp, src/CLIPipeline.cpp, src/PartOpsMesh.*, src/CLIPipeline_cmdsplitparts_coverage_test.cpp	Enables capped split output in command and CLI paths, validates split entity creation, forwards normal recomputation, and allows additional cap triangles in rigged-mesh assertions.
Capped explode option qml/PropertiesPanel.qml, src/PartOpsController.*, src/commands/ExplodePartsCommand.*, src/PartOpsScene.*	Adds the watertight checkbox and propagates its value through the controller, undo command, and scene-level explosion preprocessing.

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

Sequence Diagram(s)

sequenceDiagram
  participant PropertiesPanel
  participant PartOpsController
  participant ExplodePartsCommand
  participant PartOpsScene
  participant SubMeshOps
  PropertiesPanel->>PartOpsController: explodeSelected(distance, capBoundaries)
  PartOpsController->>ExplodePartsCommand: push explosion command
  ExplodePartsCommand->>PartOpsScene: explodeEntity with capping flag
  PartOpsScene->>SubMeshOps: capOpenBoundaries for each submesh
  SubMeshOps-->>PartOpsScene: capped submeshes

Loading

Possibly related issues

• Epic: PartOps — AI segmented parts, explode/join, and print-ready splits #859 — The PR updates the PartOps segmentation, split/explode, and watertight-capping scope described by this epic.

Possibly related PRs

• fernandotonon/QtMeshEditor#923 — Shares the SubMeshOps split pipeline extended here with capParts and capOpenBoundaries.
• fernandotonon/QtMeshEditor#929 — Shares the explode controller, command, and scene flow extended here with capBoundaries.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR no longer implements #863's peg/socket prep, export, and undo requirements, so the linked issue's acceptance criteria are unmet.	If #863 remains in scope, restore the peg/socket feature and tests; otherwise remove the linkage or retarget the issue.
Docstring Coverage	⚠️ Warning	Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The description is detailed but claims Solidify APIs and surfaces that are absent from the changeset and inconsistent with the stated objectives.	Rewrite the description to match the actual diff: boundary capping, split/explode/join behavior, and removal of peg-related functionality; remove unsupported Solidify claims.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The changes are focused on removing the peg feature and adding watertight boundary capping, which matches the stated PR objective.
Title check	✅ Passed	The title identifies the split-parts work, but it also claims Solidify, which is not present in the provided changeset.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/partops-slice-d-print-pegs-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

• X
• Mastodon
• Reddit
• LinkedIn

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 752159b991

ℹ️ 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".

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/CLIPipeline.cpp (1)

10657-10722: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Add CLI coverage for --print-pegs.

Add a CLIPipeline test covering implied splitting, pegged export, JSON counts/warnings, and the no-safe-boundary export fallback. The pure-data tests do not validate this scene/export path.

🤖 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/CLIPipeline.cpp` around lines 10657 - 10722, The CLI pipeline lacks
coverage for the --print-pegs scene/export path. Add a CLIPipeline integration
test that exercises implied splitting with pegged export, verifies JSON peg
counts and warnings, and separately verifies export without pegs when no safe
boundary exists; use existing CLI test fixtures and assertions for output and
generated files.

Source: Coding guidelines

src/MCPServer.cpp (1)

1-1: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Validate/degenerate-height peg options before building PegOptions. preparePrintPegs() guards maxPegsPerBoundary and pegRadius, but let clearance/pegDepth pass unchanged; cylinders can be built with negative or zero depths, creating degenerate or inverted geometry. Reject or clamp each value at input time before constructing options; the MCP JSON path needs this explicitly since it accepts raw numeric args without UI slider bounds.

🤖 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/MCPServer.cpp` at line 1, Update preparePrintPegs() to validate or clamp
clearance and pegDepth, alongside maxPegsPerBoundary and pegRadius, before
constructing PegOptions. Ensure the MCP JSON input path applies these bounds to
raw numeric arguments so zero or negative values cannot produce degenerate or
inverted cylinders.

🧹 Nitpick comments (1)

src/MCPServer.cpp (1)

4901-4954: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the runOgreOp helper instead of a hand-rolled try/catch.

MCPServer.h documents runOgreOp<Body> specifically to eliminate this exact "33+ tool handlers wrap their work in the same try/catch shape" duplication (Sonar-flagged tech debt). This new handler re-introduces the same duplicated shape instead of using it, and additionally catches by non-const reference and catches the broader std::exception where the helper standardizes on const std::runtime_error&.

♻️ Suggested refactor

-QJsonObject MCPServer::toolPreparePrintSplit(const QJsonObject &args)
-{
-    // PartOps print-prep (`#859/`#863): add alignment pegs to an already-split
-    // entity, via the SAME undoable AddPrintPegsCommand the GUI button uses.
-    try {
-        Manager* mgr = Manager::getSingletonPtr();
+QJsonObject MCPServer::toolPreparePrintSplit(const QJsonObject &args)
+{
+    // PartOps print-prep (`#859/`#863): add alignment pegs to an already-split
+    // entity, via the SAME undoable AddPrintPegsCommand the GUI button uses.
+    return runOgreOp([&]() -> QJsonObject {
+        Manager* mgr = Manager::getSingletonPtr();
         ...
         return makeSuccessResult(
             QString::fromUtf8(QJsonDocument(o).toJson(QJsonDocument::Indented)));
-    } catch (Ogre::Exception& e) {
-        return makeErrorResult(QString("Error: Ogre exception — %1").arg(e.getFullDescription().c_str()));
-    } catch (std::exception& e) {
-        return makeErrorResult(QString("Error: %1").arg(e.what()));
-    }
-}
+    });
+}

🤖 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/MCPServer.cpp` around lines 4901 - 4954, Refactor
MCPServer::toolPreparePrintSplit to remove its hand-written try/catch and
execute the handler body through the existing runOgreOp helper documented in
MCPServer.h. Preserve all current validation, command execution, success, and
error-result behavior inside the helper body, relying on runOgreOp for
standardized exception handling rather than local Ogre or std::exception
catches.

🤖 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/CLIPipeline.cpp`:
- Line 10374: Update the output-validation diagnostic in the argument-processing
flow to report --print-pegs when printPegs is true, instead of always naming
--split-parts. Preserve the existing --split-parts diagnostic when only
splitParts requires output.

In `@src/commands/AddPrintPegsCommand_test.cpp`:
- Around line 8-13: Add successful redo/undo coverage to AddPrintPegsCommand
using an Ogre fixture containing a stable two-part seam. Assert fixture setup
with ASSERT_TRUE rather than skipping, verify redo() succeeds and produces the
expected peg counts, then verify undo() restores the original mesh. Keep the
existing error-branch and no-op coverage unchanged.

In `@src/PartOpsMesh.cpp`:
- Around line 181-203: Update the print-pegs operation around preparePrintPegs
and buildMesh to emit SentryReporter::addBreadcrumb telemetry named
mesh.parts.print_pegs. Record failure context before each early return, and
record successful completion with the relevant counts, including pegged
boundaries, total pegs, and produced submesh/part counts; preserve the existing
return behavior.

In `@src/SubMeshOps.cpp`:
- Around line 691-693: Replace the solid-cutter appends in the socket-generation
flow of src/SubMeshOps.cpp (lines 691-693) with boolean subtraction or
equivalent cavity generation, while preserving distinct connector_socket_*
material/group identity for the female socket. Update src/SubMeshOps_test.cpp
(lines 456-476) to validate the cavity, connector identity, and mating geometry
rather than only vertex counts. Revise the socket/export claim in CLAUDE.md
(line 412) to match the actual output.
- Around line 660-685: Before calling buildAlignmentPegs() in the
boundary-processing flow, orient plane.normal from subMeshes[a] toward
subMeshes[b] using their non-seam centroids, flipping the normal when its dot
product with the centroid direction is negative. Preserve the existing boundary
sizing and stable-boundary handling, and ensure the oriented plane is passed to
buildAlignmentPegs().

---

Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Around line 10657-10722: The CLI pipeline lacks coverage for the --print-pegs
scene/export path. Add a CLIPipeline integration test that exercises implied
splitting with pegged export, verifies JSON peg counts and warnings, and
separately verifies export without pegs when no safe boundary exists; use
existing CLI test fixtures and assertions for output and generated files.

In `@src/MCPServer.cpp`:
- Line 1: Update preparePrintPegs() to validate or clamp clearance and pegDepth,
alongside maxPegsPerBoundary and pegRadius, before constructing PegOptions.
Ensure the MCP JSON input path applies these bounds to raw numeric arguments so
zero or negative values cannot produce degenerate or inverted cylinders.

---

Nitpick comments:
In `@src/MCPServer.cpp`:
- Around line 4901-4954: Refactor MCPServer::toolPreparePrintSplit to remove its
hand-written try/catch and execute the handler body through the existing
runOgreOp helper documented in MCPServer.h. Preserve all current validation,
command execution, success, and error-result behavior inside the helper body,
relying on runOgreOp for standardized exception handling rather than local Ogre
or std::exception catches.

🪄 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: 55b941b2-e2f1-4a53-ba00-ab828d5da1c7

📥 Commits

Reviewing files that changed from the base of the PR and between 2f7a646 and 752159b.

📒 Files selected for processing (17)

• CLAUDE.md
• qml/PropertiesPanel.qml
• src/CLIPipeline.cpp
• src/CMakeLists.txt
• src/MCPServer.cpp
• src/MCPServer.h
• src/PartOpsController.cpp
• src/PartOpsController.h
• src/PartOpsMesh.cpp
• src/PartOpsMesh.h
• src/SubMeshOps.cpp
• src/SubMeshOps.h
• src/SubMeshOps_test.cpp
• src/commands/AddPrintPegsCommand.cpp
• src/commands/AddPrintPegsCommand.h
• src/commands/AddPrintPegsCommand_test.cpp
• tests/CMakeLists.txt

[coderabbitai]

Actionable comments posted: 12

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/SubMeshOps_test.cpp (1)

440-453: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

The fixture parts are single triangles, so nothing here exercises the socket boolean.

Each part has 17 vertices but exactly one triangle, so toManifold gets an open, non-solid input and subtractSockets bails at the IsEmpty() check. Every "cavity" assertion downstream is therefore satisfied by unrelated code paths. Give each part a closed solid (two boxes sharing the seam plane) if the boolean is meant to be under test.

🤖 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/SubMeshOps_test.cpp` around lines 440 - 453, The twoPartsWithSeam fixture
currently creates only open single-triangle meshes, so subtractSockets never
exercises the boolean path. Replace the triangle-only geometry in
twoPartsWithSeam with closed solid box geometry for each part, using two boxes
that meet at the seam plane while preserving the intended socket
vertices/material setup.

🧹 Nitpick comments (2)

src/SubMeshOps.cpp (2)

944-946: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Swallowing every std::exception hides real boolean failures.

Silent recovery means a user gets a mesh with male pegs and no sockets and no indication why. Record the failure so PrintPrepResult can surface it as a per-boundary reason instead of reporting a successful peg placement.

🤖 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/SubMeshOps.cpp` around lines 944 - 946, Update the exception handling
around the boolean operation in SubMeshOps so failures are recorded rather than
silently swallowed. Preserve leaving the part unchanged, but propagate the
exception details through the existing preparation result data so
PrintPrepResult reports the affected boundary’s reason instead of a successful
peg placement.

---

448-454: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

next[a] = b silently drops boundary loops that share a vertex.

A vertex with two outgoing boundary edges (two holes meeting at a corner, or a pinched/non-manifold rim — both common after a label-based split) keeps only the last edge inserted, so one loop is never walked and the hole stays open. Since the result feeds a "watertight for printing" claim, a std::unordered_multimap (or consuming edges as they're walked) would be more robust.

🤖 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/SubMeshOps.cpp` around lines 448 - 454, Update the boundary-edge
collection in the loop around dirCount so multiple outgoing edges from the same
vertex are preserved instead of overwriting through next[a] = b. Use a multimap
or equivalent edge-consumption/traversal approach, and update the subsequent
boundary-loop walking logic to process every stored a→b edge and close all
loops, including shared-vertex or pinched rims.

🤖 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/CLIPipeline_cmdsplitparts_coverage_test.cpp`:
- Around line 242-244: Update the connector happy-path test around riggedFixture
and peggedBoundaries so it always uses a generated multi-submesh fixture with a
deterministic seam instead of skipping when the asset is unavailable. Assert
that at least one boundary is pegged and that connector material is generated,
ensuring the test validates the connector-generation acceptance path.
- Line 268: Update the undo/redo test around the existing mesh assertions to
retain the original Ogre::MeshPtr before redo(), then compare the mesh returned
after undo() directly against that saved pointer. Replace the submesh-count-only
validation in the affected assertions while preserving the surrounding undo/redo
checks.

In `@src/commands/ExplodePartsCommand.h`:
- Around line 42-44: Preserve the existing three-argument constructor call
contract by moving capBoundaries after the optional QUndoCommand* parent
parameter in src/commands/ExplodePartsCommand.h lines 42-44, and apply the same
parameter order in the ExplodePartsCommand definition in
src/commands/ExplodePartsCommand.cpp lines 35-43 while retaining mCapBoundaries
initialization.

In `@src/SubMeshOps_test.cpp`:
- Around line 468-481: Replace the part B vertex-count assertion in the
preparePrintPegs test with a check that specifically verifies cavity geometry,
such as vertices inside the peg radius and behind the seam plane, or compare
against a vertex count captured after capping but before subtraction. Remove the
now-unused aVerts0 variable and its `(void)aVerts0` suppression.

In `@src/SubMeshOps.cpp`:
- Around line 805-825: Make peg placement use a single source of truth: update
buildAlignmentPegs to obtain its peg positions by calling pegRingCenters rather
than duplicating the radius, up-vector selection, basis, and ring-angle
calculations. Preserve the existing 1:1 peg-to-socket alignment and reuse the
generated centers for mesh construction.
- Around line 861-886: Replace the O(V_out × V_src) nearestSource scan in the
result-vertex reconstruction loop with a position-keyed lookup using the
existing posKeyOf helper, resolving exact matches directly; for unmatched
cavity-wall vertices, use a spatial hash or uniform grid keyed by source
positions with cell size approximately equal to the socket radius, and search
only nearby cells while preserving the nearest-source selection behavior.
- Around line 1041-1046: The current inheritNearestBoneWeights/appendGeometry
flow merges male pegs into shared malePegs instead of the owning part, violating
the per-part printable-object requirement. Update the boundary-pair assembly so
each male peg and its collar are appended to the appropriate part A/B submesh,
preserving geometry placement and skinning; only update the SubMeshOps.h
documentation if shared connector submeshes remain intentional.
- Around line 470-511: Track whether the boundary walk in the loop-construction
block actually returns to start, and only proceed with centroid calculation and
fan triangulation when the chain is closed. Skip unclosed chains that terminate
because next lacks cur, while preserving the existing minimum-size check for
closed loops.
- Around line 1007-1016: Replace the braced-init-list loops in the
plane-orientation block with direct iteration over subMeshes[a].vertices and
subMeshes[b].vertices, avoiding temporary EditableSubMesh copies while
preserving centroid calculation. Also update estimateBoundaryPlane and its call
near line 995 to accept single submeshes or non-owning references/pointers, so
{subMeshes[a]} and {subMeshes[b]} are no longer materialized.
- Around line 759-782: Validate the radial segment count before the std::vector
allocations in appendSocketCollar, rejecting non-positive segments so invalid
PegOptions::radialSegments cannot reach vector construction. Preserve the
existing collar generation for positive segment counts and keep the guard at
this formation boundary.
- Around line 830-847: Update toManifold so it stores the MeshGL-constructed
manifold, checks solid.Status() immediately, and returns without proceeding when
the status is not NoError. Only pass a successfully validated manifold to
subsequent boolean operations or fromManifold.

In `@src/SubMeshOps.h`:
- Around line 163-173: Update the capOpenBoundaries documentation to define
boundary edges as directed edges a→b whose reverse b→a is absent, rather than
edges used by exactly one triangle. Clarify that the function fills only closed
boundary loops and skips non-closed chains, while preserving the remaining
behavior description.

---

Outside diff comments:
In `@src/SubMeshOps_test.cpp`:
- Around line 440-453: The twoPartsWithSeam fixture currently creates only open
single-triangle meshes, so subtractSockets never exercises the boolean path.
Replace the triangle-only geometry in twoPartsWithSeam with closed solid box
geometry for each part, using two boxes that meet at the seam plane while
preserving the intended socket vertices/material setup.

---

Nitpick comments:
In `@src/SubMeshOps.cpp`:
- Around line 944-946: Update the exception handling around the boolean
operation in SubMeshOps so failures are recorded rather than silently swallowed.
Preserve leaving the part unchanged, but propagate the exception details through
the existing preparation result data so PrintPrepResult reports the affected
boundary’s reason instead of a successful peg placement.
- Around line 448-454: Update the boundary-edge collection in the loop around
dirCount so multiple outgoing edges from the same vertex are preserved instead
of overwriting through next[a] = b. Use a multimap or equivalent
edge-consumption/traversal approach, and update the subsequent boundary-loop
walking logic to process every stored a→b edge and close all loops, including
shared-vertex or pinched rims.

🪄 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: 138e1425-292f-40f1-9d36-968b761023c5

📥 Commits

Reviewing files that changed from the base of the PR and between 752159b and 9f3a667.

📒 Files selected for processing (17)

• CLAUDE.md
• CMakeLists.txt
• qml/PropertiesPanel.qml
• src/CLIPipeline.cpp
• src/CLIPipeline_cmdsplitparts_coverage_test.cpp
• src/CMakeLists.txt
• src/PartOpsController.cpp
• src/PartOpsController.h
• src/PartOpsMesh.cpp
• src/PartOpsScene.cpp
• src/PartOpsScene.h
• src/SubMeshOps.cpp
• src/SubMeshOps.h
• src/SubMeshOps_test.cpp
• src/commands/ExplodePartsCommand.cpp
• src/commands/ExplodePartsCommand.h
• tests/CMakeLists.txt

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

• CLAUDE.md
• tests/CMakeLists.txt
• src/PartOpsController.cpp
• qml/PropertiesPanel.qml
• src/PartOpsMesh.cpp
• src/CLIPipeline.cpp

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/SubMeshOps.cpp (1)

1022-1028: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Epsilon guard lets very tiny boundaries skip peg-size clamping.

if (maxPegR > 1e-4f && boundaryOpts.pegRadius > maxPegR) means a plane with radius below ~0.0003 (still non-degenerate per estimateBoundaryPlane's own radius > 0 check) never gets clamped, so the user's peg radius (default 1.5) is applied as-is — orders of magnitude larger than the boundary. Given the PR's acceptance criteria call for "clear rejection of tiny or noisy boundaries," this should either scale down regardless of the epsilon or explicitly reject boundaries too small to host even a minimum peg, rather than silently emitting an oversized peg.

🤖 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/SubMeshOps.cpp` around lines 1022 - 1028, The maxPegR guard in the
peg-sizing logic must not allow tiny valid planes to retain an oversized peg
radius. Update this logic to clamp pegRadius, pegDepth, and clearance for every
positive maxPegR, or explicitly reject boundaries below the supported minimum
peg size; preserve the acceptance behavior for non-degenerate boundaries while
preventing oversized pegs.

♻️ Duplicate comments (1)

src/PartOpsMesh.cpp (1)

156-198: 📐 Maintainability & Code Quality | 🟠 Major

Emit print-preparation telemetry on failure paths too.

The breadcrumb is reachable only after buildMesh succeeds. Early returns for invalid entities, insufficient parts, unreadable geometry, preparation failure, and mesh-construction failure therefore produce no mesh.parts.print_pegs event. Add failure breadcrumbs before those returns or use a finalization helper that records the failure reason while preserving the current outcomes.

As per coding guidelines, significant operations in src/**/*.cpp must be tracked with SentryReporter::addBreadcrumb.

Also applies to: 205-212

🤖 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/PartOpsMesh.cpp` around lines 156 - 198, Ensure the mesh
print-preparation flow records a mesh.parts.print_pegs
SentryReporter::addBreadcrumb for every failure path, including invalid entity,
insufficient submeshes, unreadable geometry, preparation failure, and buildMesh
failure. Update the relevant early-return branches around preparePrintPegs and
buildMesh, or introduce a finalization helper, to include the failure reason
while preserving existing errors, warnings, and return outcomes.

Source: Coding guidelines

🧹 Nitpick comments (1)

src/SubMeshOps.cpp (1)

1030-1031: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

buildAlignmentPegs still generates and discards a socket cylinder that nothing uses.

male, socketUnused is passed in, but socketUnused (the outSocket cylinder generated at Lines 698-707 of buildAlignmentPegs, including the connector_socket material tag) is never read — the real socket cavity/collar now comes from subtractSockets + appendSocketCollar further down. Every stable boundary pair now pays for building and appending an unused cylinder mesh for nothing.

Either stop building the socket cylinder inside buildAlignmentPegs (add an overload/flag that only emits the male peg), or drop the now-meaningless outSocket/material-name side effects there.

🤖 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/SubMeshOps.cpp` around lines 1030 - 1031, Update buildAlignmentPegs and
its call sites so stable boundary pairs generate only the required male peg,
without constructing, appending, or tagging the unused outSocket cylinder; use
an overload or explicit option while preserving the existing socket behavior
wherever callers still consume it, and remove the socketUnused plumbing from the
shown caller.

🤖 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.

Outside diff comments:
In `@src/SubMeshOps.cpp`:
- Around line 1022-1028: The maxPegR guard in the peg-sizing logic must not
allow tiny valid planes to retain an oversized peg radius. Update this logic to
clamp pegRadius, pegDepth, and clearance for every positive maxPegR, or
explicitly reject boundaries below the supported minimum peg size; preserve the
acceptance behavior for non-degenerate boundaries while preventing oversized
pegs.

---

Duplicate comments:
In `@src/PartOpsMesh.cpp`:
- Around line 156-198: Ensure the mesh print-preparation flow records a
mesh.parts.print_pegs SentryReporter::addBreadcrumb for every failure path,
including invalid entity, insufficient submeshes, unreadable geometry,
preparation failure, and buildMesh failure. Update the relevant early-return
branches around preparePrintPegs and buildMesh, or introduce a finalization
helper, to include the failure reason while preserving existing errors,
warnings, and return outcomes.

---

Nitpick comments:
In `@src/SubMeshOps.cpp`:
- Around line 1030-1031: Update buildAlignmentPegs and its call sites so stable
boundary pairs generate only the required male peg, without constructing,
appending, or tagging the unused outSocket cylinder; use an overload or explicit
option while preserving the existing socket behavior wherever callers still
consume it, and remove the socketUnused plumbing from the shown caller.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b0cb2d26-cfc2-4669-b7c4-1fceb0f1a6df

📥 Commits

Reviewing files that changed from the base of the PR and between 9f3a667 and bc59988.

📒 Files selected for processing (5)

• CLAUDE.md
• src/CLIPipeline_cmdsplitparts_coverage_test.cpp
• src/PartOpsMesh.cpp
• src/SubMeshOps.cpp
• src/SubMeshOps_test.cpp

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

• CLAUDE.md
• src/SubMeshOps_test.cpp

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/SubMeshOps.h (1)

248-260: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the print-preparation API documentation.

The implementation subtracts socket cavities with Manifold and appends collars; it does not merge the socket cutter itself into part B. The documentation should describe the actual cavity-plus-collar behavior and avoid promising adapter-selected boolean behavior or connector material semantics that are not reflected by the returned merged submeshes.

🤖 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/SubMeshOps.h` around lines 248 - 260, Update the documentation for
SubMeshOps::preparePrintPegs to describe that partB receives a socket cavity
created by subtracting the cutter, plus the appended collar geometry. Remove
claims that the socket cutter is merged into partB, that boolean behavior is
adapter-selected, or that connector materials are guaranteed in the returned
submeshes; preserve the documented stable-boundary pairing, skipping, naming,
and deterministic behavior.

🤖 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.

Outside diff comments:
In `@src/SubMeshOps.h`:
- Around line 248-260: Update the documentation for SubMeshOps::preparePrintPegs
to describe that partB receives a socket cavity created by subtracting the
cutter, plus the appended collar geometry. Remove claims that the socket cutter
is merged into partB, that boolean behavior is adapter-selected, or that
connector materials are guaranteed in the returned submeshes; preserve the
documented stable-boundary pairing, skipping, naming, and deterministic
behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d40e0ddb-1a64-44d3-a4d4-a736347147bd

📥 Commits

Reviewing files that changed from the base of the PR and between bc59988 and 9daecf3.

📒 Files selected for processing (7)

• src/CLIPipeline.cpp
• src/PartOpsMesh.cpp
• src/PartOpsMesh.h
• src/SubMeshOps.cpp
• src/SubMeshOps.h
• src/SubMeshOps_test.cpp
• src/commands/SplitMeshCommand.cpp

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

• src/PartOpsMesh.h
• src/SubMeshOps_test.cpp
• src/CLIPipeline.cpp
• src/SubMeshOps.cpp

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/SubMeshOps.h (1)

175-176: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Use a real polygon triangulator for concave cut loops.

A centroid fan is invalid when the loop is concave or its centroid lies outside the polygon; its triangles can overlap or extend outside the part, producing a self-intersecting, non-printable “cap.” Project the loop to a plane and triangulate it robustly (for example, ear clipping), rejecting invalid loops. Add a concave-loop test.

🤖 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/SubMeshOps.h` around lines 175 - 176, Replace the centroid-fan cap
generation described in SubMeshOps with robust planar polygon triangulation,
such as ear clipping, after projecting each cut loop to a suitable plane. Reject
degenerate or invalid loops and preserve outward winding for generated
triangles; add coverage for a concave loop to verify the cap stays inside the
polygon without self-intersections.

Source: Coding guidelines

🤖 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/CLIPipeline_cmdsplitparts_coverage_test.cpp`:
- Around line 166-172: Update the triangle-count assertion in the CLIPipeline
mesh-info test to require the capped split output to contain strictly more
triangles than srcTris. Replace the non-strict comparison while preserving the
existing diagnostic message and surrounding extractMeshInfo flow.

---

Outside diff comments:
In `@src/SubMeshOps.h`:
- Around line 175-176: Replace the centroid-fan cap generation described in
SubMeshOps with robust planar polygon triangulation, such as ear clipping, after
projecting each cut loop to a suitable plane. Reject degenerate or invalid loops
and preserve outward winding for generated triangles; add coverage for a concave
loop to verify the cap stays inside the polygon without self-intersections.

🪄 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: d4597ec0-6226-42d0-95b9-84b0e4405cfa

📥 Commits

Reviewing files that changed from the base of the PR and between 9daecf3 and 16622a0.

📒 Files selected for processing (11)

• CLAUDE.md
• qml/PropertiesPanel.qml
• src/CLIPipeline.cpp
• src/CLIPipeline_cmdsplitparts_coverage_test.cpp
• src/PartOpsController.cpp
• src/PartOpsController.h
• src/PartOpsMesh.cpp
• src/PartOpsMesh.h
• src/SubMeshOps.cpp
• src/SubMeshOps.h
• src/SubMeshOps_test.cpp

💤 Files with no reviewable changes (6)

• src/PartOpsController.h
• qml/PropertiesPanel.qml
• src/PartOpsController.cpp
• src/PartOpsMesh.h
• src/SubMeshOps_test.cpp
• src/SubMeshOps.cpp

[sonarqubecloud]

Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
65.9% Coverage on New Code
0.5% Duplication on New Code

See analysis details on SonarQube Cloud