feat(morph): sub-slice A3 — authoring (add from edit, rename, delete)#578
Merged
Conversation
Last remaining sub-slice of #518. Closes the morph epic's authoring loop: users can save the current edit-mode geometry as a new morph target, rename existing targets, and delete unwanted ones. All three operations are undoable. ## What ships ### `commands/MorphCommands.{h,cpp}` — three QUndoCommand subclasses - `AddMorphTargetCommand` — create N same-named Ogre::Pose entries (one per submesh) plus a matching VAT_POSE Animation. Slices come in as sparse `{submeshHandle, {vertexIndex: Vector3f}}` maps so the command doesn't depend on EditableMesh — anything that produces deltas can drive it (current edit, MCP, future "fix mirror axis" rules…). - `DeleteMorphTargetCommand` — snapshot the same-named pose offsets at construction, drop the pose(s) + Animation + AnimationState on redo, rebuild from the snapshot on undo. - `RenameMorphTargetCommand` — same snapshot-and-rebuild pattern, just with a different name on the rebuild side. Ogre 14.5 doesn't expose Pose::setName, so destroy-and-recreate is the only path. ### `MorphAnimationManager` — three new Q_INVOKABLE methods - `addMorphTargetFromCurrentEdit(name)` — reads `EditModeController`'s live EditableMesh, diffs every submesh against the mesh's bind positions on the GPU buffer, builds the slice list, pushes an AddMorphTargetCommand. Falls back to no-op if not in edit mode, no vertices moved, or name collides. - `renameMorphTarget(old, new)` — name-trim, collision check, push. - `deleteMorphTarget(name)` — existence check, push. All three resolve the entity from SelectionSet's first entity (same pattern as the A1/A2 setters), push onto the shared `UndoManager` stack so Ctrl+Z reverses, and emit `morphTargetsChanged` so the Inspector re-fetches. ## Tests 8 new GTest cases on `MorphAnimationManager_test.cpp`: - Add command creates + undoes pose and Animation cleanly. - Delete command round-trips (poses restored on undo). - Rename command round-trips (old name restored on undo). - Rename rejects target-name collisions, no-op self-renames, blank names. - Delete rejects unknown / empty names; succeeds on a real target. - Add-from-edit rejects empty / whitespace names before edit-mode check. - Add-from-edit returns false when no EditableMesh is available (i.e. user isn't in edit mode). - All three methods reject when nothing is selected. The command path is what's actually under test — the manager wrappers are thin selection + name-validation glue. We keep direct command tests because that's where the undo / redo contract lives and it exercises the pose snapshot / rebuild path without needing an Inspector. ## What's deferred - **Inspector UI** (Add / Rename / Delete buttons in the Morph Targets subgroup) — small but lives in QML, and the last QML change in this epic (A5's dope-sheet integration) deterministically crashed unrelated tests for reasons I never fully isolated. Doing the UI in a separate PR keeps blast radius contained. - **MCP tools** `add_morph_target` / `rename_morph_target` / `delete_morph_target` — straightforward but adds surface; can land alongside the Inspector UI. - **CLI** `qtmesh morph --rename / --delete` — needs export-after-edit plumbing through the FBX/glTF morph exporters from A4a/A4b. #518 status after this slice: | Sub-slice | Status | |-|-| | A1 — Importer + manager + CLI list | shipped (#569) | | A2 — Inspector subgroup | shipped (#570) | | A3 — Authoring | **this PR** | | A4a — glTF export | shipped (#573) | | A4b — FBX export | shipped (#575) | | A5 — Controller API | shipped (#576) | | A5b — Dope-sheet UI | follow-up | | A6 — MCP tools | shipped (#571) | | A3b — Inspector / MCP / CLI authoring surface | follow-up | Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR implements morph target (blend shape) authoring with full undo/redo support. It introduces three new QUndoCommand subclasses ( ChangesMorph Target Authoring with Undo Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/MorphAnimationManager.cpp (1)
257-286: 💤 Low valueMissing Sentry breadcrumb for user-facing authoring operation.
Per coding guidelines, significant operations should be tracked. While the command's
redo()may add a breadcrumb, capturing the operation at the API entry point provides better traceability for QML-initiated actions.🔧 Suggested breadcrumb addition
undo->push(new AddMorphTargetCommand(entity, name, slices)); + SentryReporter::addBreadcrumb("ui.action", + QStringLiteral("addMorphTargetFromCurrentEdit: '%1'").arg(name)); + emit morphTargetsChanged(); return true; }🤖 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/MorphAnimationManager.cpp` around lines 257 - 286, The addMorphTargetFromCurrentEdit function lacks a Sentry breadcrumb at the API entry point; add a breadcrumb (e.g., via Sentry::addBreadcrumb or the project's breadcrumb helper) at the top of MorphAnimationManager::addMorphTargetFromCurrentEdit (before early returns) describing the attempted user action (name and entity id/type), so QML-initiated calls are tracked in addition to any breadcrumb added in AddMorphTargetCommand::redo; ensure you include enough context (operation "add_morph_target", parameter "name", and entity identifier) and do not log sensitive data.
🤖 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/MorphAnimationManager_test.cpp`:
- Around line 348-350: Before calling sel->append(entity) ensure
SelectionSet::getSingleton() returned a valid pointer by adding an explicit null
assertion (e.g., ASSERT_NE(sel, nullptr) or equivalent test-framework check)
immediately after auto* sel = SelectionSet::getSingleton(); at each call site
where sel->append(entity) is used; update the three occurrences that call
sel->append(entity) so the test fails loudly if the singleton is not initialized
rather than crashing.
---
Nitpick comments:
In `@src/MorphAnimationManager.cpp`:
- Around line 257-286: The addMorphTargetFromCurrentEdit function lacks a Sentry
breadcrumb at the API entry point; add a breadcrumb (e.g., via
Sentry::addBreadcrumb or the project's breadcrumb helper) at the top of
MorphAnimationManager::addMorphTargetFromCurrentEdit (before early returns)
describing the attempted user action (name and entity id/type), so QML-initiated
calls are tracked in addition to any breadcrumb added in
AddMorphTargetCommand::redo; ensure you include enough context (operation
"add_morph_target", parameter "name", and entity identifier) and do not log
sensitive data.
🪄 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
Run ID: 2dcfad2d-ee2c-46de-b883-496e25861be9
📒 Files selected for processing (6)
src/CMakeLists.txtsrc/MorphAnimationManager.cppsrc/MorphAnimationManager.hsrc/MorphAnimationManager_test.cppsrc/commands/MorphCommands.cppsrc/commands/MorphCommands.h
![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: b02f21ab11
ℹ️ 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 on lines
+230
to
+231
| const Ogre::Vector3 delta = subs[s].vertices[vi].position - bindPositions[vi]; | ||
| if (delta.squaredLength() <= 1e-12f) continue; |
There was a problem hiding this comment.
addMorphTargetFromCurrentEdit diffs EditableMesh against positions read from the entity’s current vertex buffer, but edit-mode transforms continuously write those edited positions back to the same mesh (EditModeController calls commitToEntity during translate/rotate/scale). In that common flow, both sides of the subtraction are already identical, so slice.offsets stays empty and creating a morph target incorrectly returns false even after visible edits. The baseline must come from pre-edit/bind data captured at edit-session start, not the live buffer being edited.
Useful? React with 👍 / 👎.
Comment on lines
+180
to
+181
| if (!sub || !sub->vertexData) return false; | ||
| Ogre::VertexData* vd = sub->vertexData; |
There was a problem hiding this comment.
The bind-position reader rejects any submesh without sub->vertexData, which excludes submeshes using sharedVertexData (useSharedVertices=true). Those meshes are common in this codebase, so morph authoring silently skips their geometry and often returns no slices. This breaks add-from-edit for shared-vertex assets; the read path needs to use mesh->sharedVertexData for shared submeshes (and corresponding pose target handle semantics).
Useful? React with 👍 / 👎.
Linker errors on the secondary test binaries (MaterialEditorQML_test, MaterialEditorQML_qml_test, MaterialEditorQML_perf_test) because they compile MorphAnimationManager.cpp (which now references the three MorphCommand constructors) but didn't pick up MorphCommands.cpp. src/CMakeLists.txt has it; tests/CMakeLists.txt was missing the line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses Codex P1 findings on PR #578: ## P1 #1 — diff against captured bind positions, not the live buffer `addMorphTargetFromCurrentEdit` was diffing `EditableMesh` against positions read from the entity's current GPU vertex buffer. But edit-mode ops (translate / rotate / scale) call `commitToEntity` during the gesture — both sides of the subtraction end up identical in the common flow, so `slice.offsets` stays empty and the command incorrectly returns no-op even after visible edits. Fix: snapshot the bind positions once at `EditableMesh::loadFromOgreMesh` time (`EditableSubMesh::originalPositions`). The snapshot is never mutated by edit-mode ops, so `vertices[i].position - originalPositions[i]` recovers the right delta regardless of how many commits have run since enter-edit-mode. ## P1 #2 — shared-vertex submeshes The old `readBindPositions` rejected any submesh with `useSharedVertices=true` (vertexData null in that case). Most Mixamo-style assets use shared vertices, so morph authoring silently skipped them. Fix is implicit in #1: `loadFromOgreMesh` already copies the shared vertex pool into each affected submesh's `vertices` array; the new `originalPositions` snapshot is built from that same source, so shared-vertex submeshes carry their own baseline. The GPU-buffer read path goes away entirely. ## CodeRabbit minor — null-assert SelectionSet in tests Added `ASSERT_NE(sel, nullptr)` before `sel->append(entity)` in the three new authoring tests, matching the slice-A1 pattern. If the singleton ever fails to initialize, we get a clear assertion failure instead of a null-deref crash. ## Tests - `LoadFromEntityTriangleMesh` extended to assert (a) the snapshot matches `vertices[].position` after load and (b) mutating `vertices[].position` does NOT touch `originalPositions`. Exercises the shared-vertex case (the triangle fixture uses shared verts). - New `AddMorphTargetUndoableViaUndoManager` exercises the full add → undo → redo loop through the shared `UndoManager`, with a hand-built slice that mirrors what `addMorphTargetFromCurrentEdit` will produce once edit-mode capture is wired up end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
2 tasks
fernandotonon
added a commit
that referenced
this pull request
May 17, 2026
…579) * feat(morph): A3b — Inspector add / rename / delete for morph targets Follow-up to #578. Wires the C++ authoring API from A3 into the existing Morph Targets subgroup in qml/PropertiesPanel.qml: - **"+ Add…" button** next to "Reset all" — opens a built-in Qt Quick Controls Popup with a name field, calls `addMorphTargetFromCurrentEdit`. The Popup deliberately avoids custom dialog components: A5's QML changes hit cross-engine initialization issues we never fully isolated, so this slice sticks to primitives with no singleton dependencies beyond the already-imported MorphAnimationManager. - **Inline rename** — double-click a target name to edit in place, matching the per-animation rename UX a few sections above. Empty / whitespace-only / unchanged names are filtered client-side before the call. Server-side collision rejection still applies. - **Per-row "×" delete** — pushes DeleteMorphTargetCommand through UndoManager so Ctrl+Z restores the pose. The existing `Connections` block on the manager already refreshes the target list and weight readouts on `morphTargetsChanged` / `morphWeightChanged`, so add / rename / delete all flow back into the UI for free. After this slice the morph epic (#518) is end-to-end: import → list → set weight → save current edit as new target → rename → delete → export — all with undo. Closes the authoring acceptance criterion in the issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * review(morph): A3b — Inspector UX fixes from PR #579 reviews Addresses three findings from Codex + CodeRabbit on PR #579: ## Codex P2 — keep Add popup open on failure `MorphAnimationManager.addMorphTargetFromCurrentEdit` returns false for several legitimate failure cases (duplicate name, not in edit mode, no vertex moved vs bind baseline). The popup was closing unconditionally, so users lost their typed name with no feedback about why nothing was created. Fix: branch on the return value. On true → close. On false → leave the popup open and surface an inline red error message ("Couldn't save: name already in use, or no vertex was edited."). Empty-name and edit-mode-required failures get their own explicit messages. ## Codex P2 — Escape must not commit rename Pressing Escape on the inline morph-rename input previously fired `Keys.onEscapePressed: visible = false`, but the hide path causes focus loss which then re-fires `onEditingFinished` — committing the rename the user tried to cancel. Fix: add a `cancelled` boolean property set by Escape and checked at the top of `onEditingFinished`; the cancel path resets the flag and returns without calling renameMorphTarget. ## CodeRabbit minor — visibly disable Add outside edit mode The "+ Add…" button stayed enabled even when not in edit mode, where `addMorphTargetFromCurrentEdit` always returns false. Fix: bind a `canAddFromEdit: EditModeController.editModeActive` property on the button; control opacity, hover highlight, MouseArea enabled state, cursor shape (PointingHand vs Forbidden), and add a tooltip that says "Enter Edit Mode (Tab) to add morph targets from current edit." The Save handler in the popup also re-checks `editModeActive` defensively — protects against the edge case where the user opens the popup mid-edit, exits edit mode, then clicks Save. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
fernandotonon
added a commit
that referenced
this pull request
May 17, 2026
Final sub-slice of #518. The C++ data API (`allMorphRows`) shipped in #576 as a Q_INVOKABLE on AnimationControlController; this PR wires it into the dope-sheet view. ## What ships - New `morphRows` property on the dope-sheet root, bound to `AnimationControlController.allMorphRows()`. Refreshed via the existing `onSelectionChanged` handler so a clip change rebuilds both bone and morph rows in lockstep. - `rowsView` (bone ListView) bottom anchor switches to `morphBand.top` when the morph band is visible. Bone-only entities → band collapses to `height=0` → bone list draws full height as before. - New `morphBand` Rectangle at the bottom: header row showing "Morph Targets (N)", then one row per pose with the target name on the left and `#88ccff` diamond markers at each keyframe time on the right. Diamonds share the bone-row timeline math (`pxPerSec`, `viewStart`) so they line up vertically. ## Why this is small and safe The original A5 (PR #576's first commit) deterministically crashed unrelated MainWindow / MCPServer visibility-toggle suites in CI. Root cause was never fully isolated but the suspicion was an `import PropertiesPanel 1.0` triggering a different QML singleton chain during MainWindow construction. This PR keeps the discipline that worked for A3b: - **No new QML imports.** `AnimationControl 1.0` was already imported. `allMorphRows()` was deliberately put on AnimationControlController (not MorphAnimationManager) for exactly this reason. - **No new `Connections` blocks.** The existing one already fires on the signals we care about. - **No MouseAreas, no selection, no drag** on morph diamonds — this is strictly read-only display. Interactive morph keyframes (selection, multi-select, drag, copy/paste) need their own controller surface and land in a separate PR. ## #518 status After this slice the issue is feature-complete and ready to close: | Sub-slice | Status | |-|-| | A1 — Importer + manager + CLI list | shipped (#569) | | A2 — Inspector subgroup | shipped (#570) | | A3 — Authoring data layer | shipped (#578) | | A3b — Inspector authoring UI | shipped (#579) | | A4a — glTF export | shipped (#573) | | A4b — FBX export | shipped (#575) | | A5 — Controller API | shipped (#576) | | A5b — Dope-sheet UI | **this PR** | | A6 — MCP tools | shipped (#571) | Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 tasks
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.
Per #518: "User can author a new morph target from an edit-mode mesh delta." Slice A3 ships the authoring loop: add from current edit, rename, delete — all undoable.
What ships
commands/MorphCommands.{h,cpp}— threeQUndoCommandsubclassesAddMorphTargetCommand— creates N same-namedOgre::Poseentries (one per affected submesh) + a matchingVAT_POSEAnimation. Input is sparse{submeshHandle, {vertexIndex: Vector3f}}slices so the command stays decoupled fromEditableMesh— anything that produces deltas can drive it (current edit, MCP, future "fix mirror axis" rules…).DeleteMorphTargetCommand— snapshots same-named pose offsets at construction, drops pose(s) + animation + AnimationState on redo, rebuilds from the snapshot on undo.RenameMorphTargetCommand— same snapshot-and-rebuild pattern. Ogre 14.5 has noPose::setName, so destroy-and-recreate is the only path.MorphAnimationManager— three newQ_INVOKABLEmethodsaddMorphTargetFromCurrentEdit(name)— readsEditModeController's liveEditableMesh, diffs every submesh against the mesh's bind positions on the GPU buffer, builds the slice list, pushesAddMorphTargetCommand. Falls back to no-op if not in edit mode, no vertices moved, or name collides.renameMorphTarget(old, new)— trim, collision check, push.deleteMorphTarget(name)— existence check, push.All three resolve entity from
SelectionSet's first entity, push onto the sharedUndoManagerstack so Ctrl+Z reverses, emitmorphTargetsChangedso the Inspector re-fetches.8 new tests
EditableMeshis available.What's deferred
add_morph_target/rename_morph_target/delete_morph_target.qtmesh morph --rename / --delete— needs export-after-edit plumbing through the FBX/glTF morph exporters from A4a/A4b.#518 status after this slice
Test plan
MorphAnimationManagertests pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores