Skip to content

feat(#862): PartOps Slice C — explode parts into scene nodes + join back - #929

Merged
fernandotonon merged 7 commits into
masterfrom
feat/partops-slice-c-explode-join-862
Jul 27, 2026
Merged

feat(#862): PartOps Slice C — explode parts into scene nodes + join back#929
fernandotonon merged 7 commits into
masterfrom
feat/partops-slice-c-explode-join-862

Conversation

@fernandotonon

@fernandotonon fernandotonon commented Jul 25, 2026

Copy link
Copy Markdown
Owner

Summary

PartOps epic #859, Slice C (#862) — Blender-style explode/join for segmented parts, built on the Slices A+B split (already merged in #923).

  • Explode a multi-submesh mesh (typically one produced by "Split into Parts") into independent, selectable/movable scene nodes, pushed outward by a configurable distance.
  • Join a multi-selection of part nodes back into one fused mesh, baking each node's world transform into the vertices (same-material submeshes coalesce).
  • Both fully undoable (Ctrl+Z).

What's in it

Piece Role
PartOpsScene.{h,cpp} Scene-level Ogre adapter above PartOpsMesh. explodeEntity() → one single-submesh Ogre::Mesh per part (attributes/material/skeleton+bone-assignments + part name preserved) + outward SubMeshOps::explodeOffsets. joinEntities() → bakes each node's _getFullTransform() into positions (inverse-transpose into normals/tangents), merges via SubMeshOps::joinParts. Pure builders — no scene mutation.
ExplodePartsCommand Undoable: redo destroys the fused node, creates N sibling part nodes at srcTransform + local-frame offset, reselects them; undo restores the fused node bound to the resident original mesh.
JoinPartsCommand Undoable: redo captures each part's mesh + node TRS, destroys the parts, creates one fused node at the origin (positions already world-baked); undo recreates every part. Both clear the SelectionSet before destroying entities (SplitMeshCommand dangling-sub-entity-ref rationale).
PartOpsController explodeSelected(distance) / joinSelected() + canExplode (one multi-submesh selection) / canJoin (2+ selected) gate props.
qml/PropertiesPanel.qml Object-mode Inspector "Explode / Join Parts" section: logarithmic explode-distance slider (0–10, default 0.1) + Explode/Join buttons.

Breadcrumbs mesh.parts.explode / mesh.parts.join.

Design decisions (confirmed with the maintainer)

  • Explode consumes the source — the fused entity is replaced by N sibling nodes; undo restores the single fused mesh. Matches the split epic's swap-the-node pattern and makes Join the obvious inverse.
  • Join operates on the current multi-selection — works for any set of part nodes, not just ones from a single explode.
  • Skeletons are not reconciled on join (documented limitation) — the merged mesh is static geometry.

Tests

  • SubMeshOps_test.cpp: new JoinBakesRotationIntoPositionsAndNormals (proves positions and normals rotate under a 90° transform — the "bakes transforms correctly" AC) + single-part passthrough.
  • ExplodePartsCommand_test.cpp / JoinPartsCommand_test.cpp: no-Ogre error-branch coverage (ctor/text, initial state, unresolvable-entity redo, undo-before-redo no-op), mirroring SplitMeshCommand_test.cpp. The full GL round-trip stays with the Ogre-gated CLI tests (arriving with Slice E).
  • All 24 pass locally; verified end-to-end in the GUI (explode → move → join → undo).

Scope

Explode/join CLI + MCP parity is Slice E (#864); the print-peg dialog is Slice D (#863) — both remain open per the epic.

Closes #862.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added Object-mode “Explode / Join Parts” tools with an adjustable explode distance, selection-based enablement, and completion feedback.
    • Introduced undo/redo for explode and join, preserving transforms and reselecting created parts.
  • Bug Fixes
    • Improved joined-mesh results for mirrored transforms by fixing triangle winding and tangent handedness.
    • Enhanced submesh material handling and submesh reading accuracy.
  • Tests
    • Added unit tests for explode/join commands plus additional transform/material edge cases.
  • Documentation
    • Expanded PartOps guidance with scene-level explode/join semantics and command behavior.

fernandotonon and others added 2 commits July 24, 2026 03:43
Bridges AI segmentation to Blender-style explode/join authoring, on top of
the Slices A+B split. New scene-level adapter + two undo commands + GUI.

- PartOpsScene: SCENE-level Ogre adapter (above PartOpsMesh). explodeEntity()
  splits every submesh of a fused mesh into its own single-submesh Ogre::Mesh
  (attributes/material/skeleton+bone-assignments + part name preserved) and
  computes an outward per-part offset via SubMeshOps::explodeOffsets.
  joinEntities() bakes each part node's world transform into vertex positions
  (inverse-transpose into normals/tangents) and merges same-material submeshes.
  Pure builders — no scene mutation (the commands own node create/destroy).
- ExplodePartsCommand: redo destroys the fused node, creates N sibling part
  nodes at srcTransform + local-frame offset, reselects them; undo restores
  the fused node bound to the resident original mesh.
- JoinPartsCommand: redo captures each part's mesh+TRS, destroys the parts,
  creates one fused node at the origin; undo recreates every part. Both clear
  the SelectionSet before destroying entities (SplitMeshCommand rationale).
- PartOpsController: explodeSelected(distance) / joinSelected() + canExplode
  (one multi-submesh selection) / canJoin (2+ selected) props.
- GUI: Object-mode Inspector "Explode / Join Parts" section with an explode
  distance slider and Explode/Join buttons.
- Breadcrumbs mesh.parts.explode / mesh.parts.join.
- Tests: join rotation-bakes-normals + single-part passthrough in
  SubMeshOps_test.cpp; no-Ogre error-branch coverage for both commands.

Explode/join CLI+MCP parity is Slice E (#864).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The linear 0–2 slider gave too little precision at the small distances used
for inspection. Drive the slider on a normalised log position t∈[0,1] with
distance = A·(e^(k·t) − 1) (k=4, A pins t=1→10): fine near 0, coarse toward
10. Default distance lowered 0.5 → 0.1. onMoved (not onValueChanged) writes
back so the value←distance binding doesn't fight the handle.

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

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ead68a3d-4347-43a9-a919-627a73e4d82d

📥 Commits

Reviewing files that changed from the base of the PR and between bce1f96 and 2143ea1.

📒 Files selected for processing (1)
  • src/PartOpsMesh_material_coverage_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/PartOpsMesh_material_coverage_test.cpp

📝 Walkthrough

Walkthrough

PartOps now supports exploding multi-submesh entities into separate nodes and joining selected mesh entities into a fused mesh. The workflow includes geometry builders, undoable commands, controller and QML integration, transform baking, mirrored-winding handling, build wiring, documentation, and tests.

Changes

PartOps Slice C

Layer / File(s) Summary
Scene geometry builders
src/PartOpsScene.*, src/PartOpsMesh.cpp, src/SubMeshOps.cpp, src/SubMeshOps_test.cpp, src/PartOpsMesh_material_coverage_test.cpp
Builds exploded and joined meshes with offsets, effective-material propagation, transform baking, and mirrored-transform winding corrections.
Undoable scene commands
src/commands/ExplodePartsCommand.*, src/commands/JoinPartsCommand.*
Adds redo/undo commands that replace fused or separate scene nodes while caching meshes, transforms, operation results, and selection state.
Controller and QML integration
src/PartOpsController.*, qml/PropertiesPanel.qml
Exposes capability properties, invokable operations, completion signals, gated controls, distance adjustment, and feedback.
Validation, documentation, and build coverage
src/commands/*_test.cpp, src/CMakeLists.txt, tests/CMakeLists.txt, CLAUDE.md
Adds command error-path tests, wires implementations into application and test builds, and documents the completed PartOps pipeline.

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

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant PropertiesPanel
  participant PartOpsController
  participant UndoStack
  participant SceneCommand
  participant PartOpsScene
  participant SceneManager

  User->>PropertiesPanel: Select Explode Parts or Join Parts
  PropertiesPanel->>PartOpsController: explodeSelected(distance) or joinSelected()
  PartOpsController->>UndoStack: Push undoable command
  UndoStack->>SceneCommand: redo()
  SceneCommand->>PartOpsScene: Build exploded or joined geometry
  SceneCommand->>SceneManager: Replace scene nodes and entities
  SceneCommand-->>PropertiesPanel: Completion status signal
Loading

Possibly related issues

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% 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 clearly summarizes the main change: PartOps Slice C adds explode/join behavior for parts.
Description check ✅ Passed The description is mostly complete, covering summary, technical areas, tests, scope, and key design decisions.
Linked Issues check ✅ Passed The changes satisfy #862: they add explode/join scene-node workflows, preserve transforms/materials, add undo/redo, and include the required tests.
Out of Scope Changes check ✅ Passed No clear out-of-scope code changes are present; the docs, UI, commands, scene adapter, and tests all support the explode/join scope.
✨ Finishing Touches
📝 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-c-explode-join-862

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: ad817bc72d

ℹ️ 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/commands/ExplodePartsCommand.cpp Outdated
if (src) {
Ogre::SceneNode* node = src->getParentSceneNode();
if (node)
mgr->destroySceneNode(node, /*destroyChildrenFirst=*/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.

P1 Badge Preserve nested nodes when exploding

When the selected mesh is inside a group or has child nodes, destroying its scene node recursively deletes the entire child subtree, but the command captures only the mesh and local TRS. The replacement parts and undo result are then created at the scene root, so children are permanently lost and a grouped mesh also jumps because its local transform is treated as world-space. Preserve the original parent and children, or reject hierarchical nodes before performing the operation.

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 d8d5e76. Explode now captures the source node's parent and reparents the part nodes back under it (and the restored fused node on undo), via Manager::reparentNode + an explicit local-TRS restore. Nodes that have child nodes are rejected up front with a clear error, since the command doesn't serialise arbitrary subtrees.

Comment thread src/commands/JoinPartsCommand.cpp Outdated
// Destroy the source part nodes (frees their names for undo to recreate).
for (const auto& sp : mSources) {
if (Ogre::SceneNode* node = mgr->getSceneNode(QString::fromStdString(sp.name)))
mgr->destroySceneNode(node, /*destroyChildrenFirst=*/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.

P1 Badge Restore the source hierarchy when undoing a join

For selected parts that are grouped, reparented, or have children, this recursive destruction removes their subtrees, while SourcePart records only each node's local TRS and mesh. Undo recreates root-level leaf nodes, so it cannot restore either the original hierarchy or the correct world transform under a transformed parent. Capture and restore parent/child relationships, or avoid destroying hierarchical nodes.

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 d8d5e76. Join now records each part's parent group + local TRS and reparents the restored parts back under it on undo; parts with child nodes are rejected up front. Grouped parts round-trip through join/undo.

Comment thread src/PartOpsScene.cpp
return out;
}
SubMeshOps::JoinPart jp;
if (!PartOpsMesh::readSubMeshes(e, jp.subMeshes)) {

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 effective material assignments during part operations

When a material has been assigned through Material Mode, ApplyMaterialCommand updates the Ogre::SubEntity, but PartOpsMesh::readSubMeshes() ultimately reads Ogre::SubMesh::getMaterialName() in EditableMesh::loadFromMesh(). Joining therefore sees stale base materials, can coalesce visibly distinct parts, and builds the result with the old assignments; explodeEntity() has the same issue. Copy each entity's effective subentity material names into the editable submeshes before rebuilding.

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 d8d5e76. PartOpsMesh::readSubMeshes now overrides each editable submesh's material with the entity's effective per-SubEntity material (SubEntity::getMaterialName), so a Material-Mode assignment survives split/explode/join and join no longer coalesces visibly-distinct parts.

Comment thread src/PartOpsScene.cpp
Comment on lines +126 to +127
jp.transform = node ? Ogre::Matrix4(node->_getFullTransform())
: Ogre::Matrix4::IDENTITY;

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 Correct winding when baking mirrored transforms

When any selected node has a world transform with a negative determinant, such as a negative X scale entered in the Transform panel, passing that transform through unchanged mirrors the vertex positions while joinParts() copies the triangle indices and tangent parity unchanged. The joined part consequently has reversed winding and can be back-face culled or render with incorrect normal mapping. Detect orientation-reversing transforms and reverse triangle winding while adjusting tangent handedness during the bake.

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 d8d5e76. SubMeshOps::joinParts detects a negative-determinant (orientation-reversing) transform and reverses triangle winding + flips tangent handedness so a mirrored/negative-scaled part joins front-facing. Covered by JoinReversesWindingUnderMirrorTransform / JoinKeepsWindingUnderNonMirrorTransform.

fernandotonon and others added 2 commits July 24, 2026 21:54
unit-tests-linux linked MaterialEditorQML_test against qtmesh_test_common,
which pulls PartOpsController.cpp but was missing the new command TUs →
undefined reference to ExplodePartsCommand/JoinPartsCommand ctors. Add
ExplodePartsCommand.cpp, JoinPartsCommand.cpp, and PartOpsScene.cpp to the
tests source list (mirrors src/CMakeLists.txt). qtmesh_test_common links
clean locally with the fix.

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

- Preserve grouping: explode/join now capture the source node's PARENT and
  reparent the part / restored-fused nodes back under it (Manager::reparentNode
  + explicit local-TRS restore, since reparentNode preserves WORLD transform).
  Nodes WITH child nodes are rejected up front with a clear error — the command
  doesn't serialise arbitrary subtrees, so undo could never restore them
  (Codex P1 ×2: exploded/joined grouped meshes were lost / jumped).
- Effective material: PartOpsMesh::readSubMeshes now prefers the entity's
  per-SubEntity material (SubEntity::getMaterialName) over the base SubMesh
  name, so a Material-Mode override survives split/explode/join and join no
  longer coalesces visibly-distinct parts (Codex P1).
- Mirror winding: SubMeshOps::joinParts reverses triangle winding + flips
  tangent handedness when a part's transform has a negative determinant
  (e.g. negative scale), so a mirrored part doesn't render back-facing /
  with inverted normal mapping (Codex P2).
- Tests: JoinReversesWindingUnderMirrorTransform +
  JoinKeepsWindingUnderNonMirrorTransform (pure-data). 26 SubMeshOps/command
  tests pass.

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

@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: 7

🧹 Nitpick comments (3)
src/PartOpsScene.cpp (1)

74-93: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Move instead of copy the per-part submesh slice.

one{ subs[i] } copies the submesh (vertices/triangles) even though subs[i] isn't reused afterward. Moving avoids an unnecessary full-geometry copy per part.

⚡ Proposed fix
-        std::vector<EditableSubMesh> one{ subs[i] };
+        std::vector<EditableSubMesh> one{ std::move(subs[i]) };
🤖 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/PartOpsScene.cpp` around lines 74 - 93, Update the per-part construction
in the loop to move subs[i] into the single-element one vector instead of
copying it, while preserving the existing buildMesh call and ensuring subs[i] is
not accessed afterward.
src/commands/ExplodePartsCommand.cpp (2)

21-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Identical entity-by-name lookup duplicated across both commands. ExplodePartsCommand::resolveSourceEntity() and the file-local findEntity() in JoinPartsCommand.cpp implement the exact same loop/filter/match logic.

  • src/commands/ExplodePartsCommand.cpp#L21-L31: extract this into a shared helper (e.g. on Manager or a small PartOps utility).
  • src/commands/JoinPartsCommand.cpp#L21-L32: replace findEntity with the same shared helper.
🤖 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/commands/ExplodePartsCommand.cpp` around lines 21 - 31, Extract the
duplicated entity-name lookup from ExplodePartsCommand::resolveSourceEntity()
into a shared helper, such as a Manager method or PartOps utility, preserving
the null-manager, movable-type, and name matching behavior. Update
src/commands/ExplodePartsCommand.cpp lines 21-31 and
src/commands/JoinPartsCommand.cpp lines 21-32 to use the helper, removing the
file-local findEntity implementation in JoinPartsCommand.cpp.

122-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Sentry breadcrumbs are only recorded on success; failure paths and all of undo() are silent. Both commands only call SentryReporter::addBreadcrumb inside the success branch of redo(); buildOnce()/redo() failures and every undo() call leave no trace for diagnosing user-reported issues.

  • src/commands/ExplodePartsCommand.cpp#L122-L128: add a breadcrumb on the failure branch (with mError) and add one in undo().
  • src/commands/JoinPartsCommand.cpp#L117-L121: add a breadcrumb on the failure branch (with mError) and add one in undo().

As per coding guidelines: "Track all user-facing actions and significant operations with Sentry breadcrumbs."

🤖 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/commands/ExplodePartsCommand.cpp` around lines 122 - 128, The redo()
failure branch in ExplodePartsCommand.cpp (lines 122-128) must record a Sentry
breadcrumb containing mError, and undo() must record a breadcrumb for every
invocation; apply the same failure-branch and undo() breadcrumb additions in
JoinPartsCommand.cpp (lines 117-121), preserving the existing success
breadcrumbs and using clear action-specific context.

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/commands/ExplodePartsCommand.cpp`:
- Around line 130-157: Update ExplodePartsCommand::undo() to create and validate
the fused scene node before destroying the part nodes, so addSceneNode failure
leaves the original scene intact. Only after successful node creation and entity
binding should it clear selection, destroy mPartNodeNames, and select the
restored node; preserve the saved transform restoration.
- Around line 88-121: The explode operation must not destroy the fused source
until all replacement part nodes and entities are successfully created. Update
the flow around source destruction, the mParts creation loop, and createEntity
checks to detect any failure, preserve or restore the source on failure, clean
up partial replacements, and ensure mOk reports failure; only destroy the source
and finalize selection after complete success.
- Around line 88-157: Make both ExplodePartsCommand redo()/undo() and
JoinPartsCommand redo()/undo() transactional: create and validate every
replacement scene node and entity before destroying the existing nodes, and roll
back any partially created replacements on failure. In ExplodePartsCommand,
ensure all part nodes are successfully created before removing the source and do
not mark mOk on partial failure; in undo(), validate the recreated fused node
before removing parts. Apply the equivalent validation and rollback in
JoinPartsCommand’s redo() and undo() paths, preserving the original scene when
replacement creation fails. The affected sites are
src/commands/ExplodePartsCommand.cpp lines 88-157 and
src/commands/JoinPartsCommand.cpp lines 97-162.

In `@src/commands/ExplodePartsCommand.h`:
- Around line 7-9: Add the direct OgreQuaternion header include to
ExplodePartsCommand.h alongside the existing Ogre includes so the
Ogre::Quaternion mSrcOrient declaration is self-contained and does not rely on
transitive includes.

In `@src/commands/JoinPartsCommand.cpp`:
- Around line 97-113: The source nodes are destroyed before successful creation
of the fused node is guaranteed. Update the flow around mSources and
mFusedNameBase so addSceneNode succeeds before removing any source nodes; only
then destroy the source nodes and continue creating the fused entity. On
fused-node creation failure, preserve the original source nodes while setting
mOk and mError as currently handled.
- Around line 123-162: The undo flow in JoinPartsCommand::undo must avoid
destroying the fused node before confirming all original part nodes can be
recreated. Preflight or otherwise ensure every mSources name is available,
aborting without destructive changes if restoration cannot fully succeed; then
destroy the fused node and recreate all parts, treating any addSceneNode failure
as an undo failure rather than silently continuing.

In `@src/PartOpsController.cpp`:
- Around line 60-64: Update PartOpsController::canJoin() and the early guard in
joinSelected() to count only non-null resolved entities, matching
joinSelected()'s name-building loop. Require at least two valid entities before
enabling Join or constructing JoinPartsCommand, while preserving the existing
clear rejection message for fewer than two parts.

---

Nitpick comments:
In `@src/commands/ExplodePartsCommand.cpp`:
- Around line 21-31: Extract the duplicated entity-name lookup from
ExplodePartsCommand::resolveSourceEntity() into a shared helper, such as a
Manager method or PartOps utility, preserving the null-manager, movable-type,
and name matching behavior. Update src/commands/ExplodePartsCommand.cpp lines
21-31 and src/commands/JoinPartsCommand.cpp lines 21-32 to use the helper,
removing the file-local findEntity implementation in JoinPartsCommand.cpp.
- Around line 122-128: The redo() failure branch in ExplodePartsCommand.cpp
(lines 122-128) must record a Sentry breadcrumb containing mError, and undo()
must record a breadcrumb for every invocation; apply the same failure-branch and
undo() breadcrumb additions in JoinPartsCommand.cpp (lines 117-121), preserving
the existing success breadcrumbs and using clear action-specific context.

In `@src/PartOpsScene.cpp`:
- Around line 74-93: Update the per-part construction in the loop to move
subs[i] into the single-element one vector instead of copying it, while
preserving the existing buildMesh call and ensuring subs[i] is not accessed
afterward.
🪄 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: b7578bdb-3a5b-4e0f-bb3b-d585949ac97c

📥 Commits

Reviewing files that changed from the base of the PR and between 742ef6a and a7492e8.

📒 Files selected for processing (15)
  • CLAUDE.md
  • qml/PropertiesPanel.qml
  • src/CMakeLists.txt
  • src/PartOpsController.cpp
  • src/PartOpsController.h
  • src/PartOpsScene.cpp
  • src/PartOpsScene.h
  • src/SubMeshOps_test.cpp
  • src/commands/ExplodePartsCommand.cpp
  • src/commands/ExplodePartsCommand.h
  • src/commands/ExplodePartsCommand_test.cpp
  • src/commands/JoinPartsCommand.cpp
  • src/commands/JoinPartsCommand.h
  • src/commands/JoinPartsCommand_test.cpp
  • tests/CMakeLists.txt

Comment thread src/commands/ExplodePartsCommand.cpp Outdated
Comment thread src/commands/ExplodePartsCommand.cpp Outdated
Comment thread src/commands/ExplodePartsCommand.cpp
Comment thread src/commands/ExplodePartsCommand.h
Comment thread src/commands/JoinPartsCommand.cpp Outdated
Comment thread src/commands/JoinPartsCommand.cpp
Comment thread src/PartOpsController.cpp
…sing include

- Create-then-destroy in ALL four redo()/undo() paths: the replacement node(s)
  are built + entity-created (and validated) BEFORE the old node is destroyed —
  their names never collide with the node being replaced, so they coexist
  momentarily. A creation failure rolls back the new nodes and leaves the
  original intact, so redo/undo never orphan the scene (neither old nor new).
  (CodeRabbit Major ×4: destroy-before-create hazard.)
- canJoin()/joinSelected() now count only real mesh entities (getResolvedEntities
  can contain nulls for non-mesh nodes), so a 1-mesh+1-null selection no longer
  enables Join / builds a 1-part command. (CodeRabbit Major.)
- ExplodePartsCommand.h: add #include <OgreQuaternion.h> (was transitive; the
  sibling JoinPartsCommand.h already includes it). (CodeRabbit Minor.)

26 SubMeshOps/command tests pass; main + tests build clean.

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

@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: 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 `@src/PartOpsMesh.cpp`:
- Around line 22-41: Add Google Test coverage for readSubMeshes() that creates
an entity with multiple subentities, assigns distinct runtime materials to each
SubEntity, and verifies the returned submeshes preserve those effective material
names in matching positional order. Ensure the test exercises the runtime
override path rather than only base SubMesh materials.
🪄 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: 93832525-309d-4a2e-958c-add4982675f2

📥 Commits

Reviewing files that changed from the base of the PR and between a7492e8 and 864d2cd.

📒 Files selected for processing (9)
  • CLAUDE.md
  • src/PartOpsController.cpp
  • src/PartOpsMesh.cpp
  • src/SubMeshOps.cpp
  • src/SubMeshOps_test.cpp
  • src/commands/ExplodePartsCommand.cpp
  • src/commands/ExplodePartsCommand.h
  • src/commands/JoinPartsCommand.cpp
  • src/commands/JoinPartsCommand.h
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/commands/JoinPartsCommand.h
  • src/commands/ExplodePartsCommand.h
  • src/commands/JoinPartsCommand.cpp
  • CLAUDE.md
  • src/commands/ExplodePartsCommand.cpp
  • src/PartOpsController.cpp

Comment thread src/PartOpsMesh.cpp
…terial test

- JoinPartsCommand::undo() is now all-or-nothing: every restored part node +
  entity is validated, a partial failure rolls back the parts already created,
  and the fused node is destroyed only after ALL parts are back — so a failed
  undo can't leave the scene with only a subset of the parts (CodeRabbit Major).
- Add PartOpsMesh_material_coverage_test.cpp (GL-gated): a 2-submesh entity with
  a runtime SubEntity material override on submesh 0 — asserts readSubMeshes
  reports the effective override for sub0 and the base material for the
  un-overridden sub1, with correct positional mapping (CodeRabbit — coverage for
  the effective-material path).

27 PartOps tests pass; main + tests build clean.

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

@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: 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 `@src/PartOpsMesh_material_coverage_test.cpp`:
- Around line 24-32: Update clearSceneNodes() and the fixture setup/teardown to
stop deleting global Manager state: generate unique names for the test’s scene
node, mesh, and material resources, retain the created handles, and destroy only
those resources during cleanup. Remove the fixed-name resource deletion and
avoid iterating over every Manager scene node, while preserving cleanup of all
objects created by this fixture.
🪄 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: 66bc1a91-27d9-4bbc-acf8-24888c660728

📥 Commits

Reviewing files that changed from the base of the PR and between 864d2cd and bce1f96.

📒 Files selected for processing (3)
  • CLAUDE.md
  • src/PartOpsMesh_material_coverage_test.cpp
  • src/commands/JoinPartsCommand.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/commands/JoinPartsCommand.cpp
  • CLAUDE.md

Comment thread src/PartOpsMesh_material_coverage_test.cpp Outdated
CodeRabbit (Minor): the fixture wiped every Manager scene node and used
fixed resource names, which could disrupt another fixture in the shared test
process. Use unique per-test names for the node/mesh/materials, and in
TearDown destroy ONLY those (the node + attached objects, the mesh, the 3
materials) — no global scene sweep, no fixed-name removal.

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

Copy link
Copy Markdown

@fernandotonon
fernandotonon merged commit 818f2a6 into master Jul 27, 2026
21 checks passed
@fernandotonon
fernandotonon deleted the feat/partops-slice-c-explode-join-862 branch July 27, 2026 00:32
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.

PartOps Slice C — Explode segmented parts into separate scene nodes and join back

1 participant