feat(#864/#865): PartOps Slices E+F — explode/join CLI+MCP parity + docs - #938
Merged
Conversation
Slice E (#864) — CLI/MCP parity for segmented part operations: - CLI `qtmesh segment <file> --explode-parts [--explode-distance <d>] [--solidify] -o scene.glb`: splits into per-part submeshes, then explodes each into its own scene node offset outward, and exports the multi-node scene via sceneExporter. Usage error (exit 2) without -o or with a bad distance. - MCP `explode_mesh_parts` ({entity_name?, distance?}) and `join_mesh_parts` ({entity_names?} — omit to join all mesh entities), both undoable via the same ExplodePartsCommand / JoinPartsCommand the GUI uses; registered heavy + mapped to the `segmentation` gamification cluster. - `segment_mesh` already returns `face_labels` unconditionally (the epic's `return_face_labels`). - Print-split/pegs descoped (removed earlier this epic — no reliable flat cut plane on organic joints; Meshy/Tripo ship no discrete pegs either). Slice F (#865) — docs/tests/polish: - New docs/PART_OPS.md: full workflow (split/explode/join/solidify), all three surfaces, and the documented limitations (body-centric labels, model-unit dimensions, static join, thin-shell solidify, no pegs/boolean). - README feature bullet + CLAUDE.md CLI/MCP/PartOps updates. - Breadcrumbs already present: mesh.parts.{segment_preview,split_segments, explode,join}. Tests: 3 new CLI explode coverage tests (usage errors + a multi-node-scene glb that parses the exported .glb JSON chunk and asserts >1 node with outward offsets — reimport-free so it doesn't hit glTF same-material merging or scene lights). Full PartOps suite 35 green. App builds clean; CLI explode verified end-to-end on Rumba (12 parts, offset nodes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds CLI support for exploding segmented mesh parts and MCP tools for exploding and joining mesh entities. It adds validation, undoable command execution, structured results, multi-node export coverage, PartOps documentation, and a pinned stb revision update. ChangesPartOps interface parity
Build dependency update
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MCPServer
participant PartOpsCommands
participant Scene
MCPClient->>MCPServer: Call explode_mesh_parts or join_mesh_parts
MCPServer->>PartOpsCommands: Execute undoable mesh-part command
PartOpsCommands->>Scene: Create offset nodes or join mesh entities
Scene-->>MCPServer: Return operation counts
MCPServer-->>MCPClient: Return structured JSON result
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c4ac5cf12
ℹ️ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/MCPServer.cpp (1)
4906-4951: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNegative
distanceis silently clamped instead of rejected, unlike the CLI.
args.value("distance").toDouble(0.15)followed byif (distance < 0.0) distance = 0.0;silently coerces a negative value to0.0. The CLI counterpart (CLIPipeline.cpp,--explode-distanceparsing) rejects negative values outright withError: --explode-distance must be a non-negative number.and exit code 2. For a PR whose objective is CLI/MCP parity, the same bad input should produce the same outcome on both surfaces: either both reject it, or both document the clamp.♻️ Proposed fix (reject to match the CLI)
double distance = args.value("distance").toDouble(0.15); - if (distance < 0.0) distance = 0.0; + if (distance < 0.0) + return makeErrorResult("Error: distance must be a non-negative number");🤖 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 4906 - 4951, Update MCPServer::toolExplodeMeshParts to reject negative distance values instead of clamping them to zero. Preserve the existing default and valid non-negative distance behavior, and return an error matching the CLI’s non-negative distance validation outcome before creating or pushing ExplodePartsCommand.
🤖 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`:
- Around line 10706-10711: Move the outputPath validation for explodeParts next
to the existing split-parts validation, before initOgreHeadless() and any
segmentation work, so --explode-parts without -o exits immediately with the same
error and status. Remove the redundant outputPath check from the later
explodeParts block while preserving its remaining processing.
- Around line 10706-10784: After splitEntity succeeds, remove the original scene
node owning entity before calling explodeEntity or sceneExporter, while
retaining the temporary srcNode/splitEnt needed for explosion. Ensure the
original fused entity is detached and destroyed so only the exploded part nodes
remain in the exported scene.
In `@src/MCPServer.cpp`:
- Around line 4961-4988: Update MCPServer::toolJoinMeshParts to validate every
requested entity_names entry: return an error immediately for unknown names and
reject duplicate names before adding them to names. Preserve the existing entity
filtering and automatic all-entity behavior when entity_names is absent or
empty, ensuring JoinPartsCommand receives only unique, valid entities.
---
Nitpick comments:
In `@src/MCPServer.cpp`:
- Around line 4906-4951: Update MCPServer::toolExplodeMeshParts to reject
negative distance values instead of clamping them to zero. Preserve the existing
default and valid non-negative distance behavior, and return an error matching
the CLI’s non-negative distance validation outcome before creating or pushing
ExplodePartsCommand.
🪄 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: 127e5dd3-296d-400e-a7c5-9bfa964a429c
📒 Files selected for processing (7)
CLAUDE.mdREADME.mddocs/PART_OPS.mdsrc/CLIPipeline.cppsrc/CLIPipeline_cmdsplitparts_coverage_test.cppsrc/MCPServer.cppsrc/MCPServer.h
CI was failing on EVERY platform at the CMake configure step: `fatal: unable to read tree (31c1ad3745…)` while cloning nothings/stb — the pinned commit was force-pushed out of upstream, so the shallow clone couldn't resolve it. This breaks master too, not just this PR. Re-pin to the current upstream master head 2c980bb5…. stb_image.h's .hdr decode API is unchanged; HDR/HdrEquirectLoader.cpp (the only consumer) compiles and the app builds clean against the new pin. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- CLI --explode-parts: destroy the ORIGINAL imported source node (not just the temp split node) before sceneExporter, which walks every entity-bearing node — otherwise the exported scene overlaid an intact un-exploded mesh on top of the parts, doubling geometry (P1). Verified: exploded Rumba glb now has 12 meshes (was 13). - CLI --explode-parts: move the -o requirement check up-front, before segmentation (which may download an ONNX model), matching --split-parts — so a missing -o fails fast (Major). - MCP join_mesh_parts: reject an entity_names list with an UNRESOLVED name (previously silently joined the valid subset) or a DUPLICATE name (previously duplicated geometry + broke undo). Validate all resolve + are unique (Major). - MCP explode_mesh_parts / join_mesh_parts: map to the catalogued `ai_assist` gamification cluster (like the sibling segment tools) instead of the non-existent `segmentation` key, so usage events aren't dropped (P2). 34 PartOps tests green; app builds clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 `@CMakeLists.txt`:
- Around line 434-437: Update the comment immediately above GIT_TAG in the stb
dependency block to remove the inaccurate force-push/rebase and “dropped from
upstream” explanation, while retaining the immutable
2c980bb59875b0d32144a71867fbdebb2f77cd20 pin and documenting only the verified
CI fetch failure reason or the new pin.
🪄 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: 5ffbd988-8677-4f32-b779-4507846f2489
📒 Files selected for processing (3)
CMakeLists.txtsrc/CLIPipeline.cppsrc/MCPServer.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/MCPServer.cpp
- src/CLIPipeline.cpp
The previous comment asserted a force-push/rebase cause that isn't verifiable; state only what was observed (the shallow-clone "unable to read tree" failure in CI) and that the new pin clones cleanly (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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
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.
Summary
PartOps epic #859, final slices — E (#864) CLI/MCP parity for segmented
part operations, and F (#865) docs / tests / polish. Completes the epic.
Slice E — CLI/MCP parity
CLI (
qtmesh segment):Splits into per-part submeshes, explodes each into its own scene node offset
outward, and exports the multi-node scene. Usage error (exit 2) without
-oorwith a negative/non-numeric distance.
MCP:
explode_mesh_partsentity_name?,distance?ExplodePartsCommandjoin_mesh_partsentity_names?(omit → all mesh entities)JoinPartsCommandBoth run through the same undoable commands as the GUI buttons, are registered
as heavy tools, and map to the
segmentationgamification cluster.segment_meshalready returnsface_labelsunconditionally (the epic'sreturn_face_labels).Descoped:
--print-split/prepare_print_split(the 3D-print pegs wereremoved earlier this epic — no reliable flat cut plane on organic joints, and
Meshy/Tripo ship no discrete pegs either).
Slice F — docs, tests, polish
join / solidify), all three surfaces (GUI / CLI / MCP), export behaviour, and
the documented limitations: body-centric labels, model-unit dimensions, static
join (skeletons not reconciled), thin-shell solidify, no pegs/boolean.
mesh.parts.{segment_preview, split_segments, explode, join}.Tests
3 new CLI explode coverage tests (usage errors + a multi-node-scene check that
parses the exported
.glbJSON chunk and asserts >1 node with outwardtranslations — reimport-free, so it avoids glTF same-material merging and scene
lights). Full PartOps suite 35 green. App builds clean; CLI explode verified
end-to-end on Rumba (12 parts at offsets).
Closes #864, #865.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests