feat(morph): sub-slice A5 — controller API for dope-sheet morph rows#576
Conversation
Per #518: "Dope sheet integration: morph-weight tracks appear in the dope sheet alongside bone tracks." Slice A5 ships the read-only display half — one row per Ogre::Pose on the selected entity with diamond markers at each keyframe time. Full selection / move / copy / paste interaction for morph tracks is a follow-up (it depends on A3's authoring path to land first). ### What ships **`AnimationControlController::allMorphRows()`** (new Q_INVOKABLE): - Walks the selected entity's `getPoseList()`. - For each pose, looks up the matching `Ogre::Animation` (which A1's importer creates one-per-pose), reads its VAT_POSE track's keyframe times. - Returns `[{ name: QString, keyTimes: [double] }]` — same shape the dope sheet's existing bone-row reader expects, minus the channel-flags map (morph tracks are scalar weight, no channel decomposition). **`AnimationDopeSheet.qml`** picks up the new API: - New `morphRows` property bound to `allMorphRows()`. - Refresh hooks: existing `onSelectionChanged` now also refreshes morph rows; new `MorphAnimationManager` Connections refresh on `morphTargetsChanged` (selection moved) and `morphWeightChanged` (Inspector slider, MCP poke, future authoring path). - New `morphBand` Rectangle anchored to the bottom of the dope sheet: collapses to height=0 when there are no morphs (so a bone-only animation looks the same as before); otherwise shows "Morph Targets (N)" header + one row per pose with the same diamond style the bone tracks use. ### 3 new tests in `AnimationControlController_test.cpp` - `AllMorphRowsEmptyWhenNoSelection` — returns `[]` cleanly. - `AllMorphRowsEmptyForMeshWithoutPoses` — selected entity with bones but no poses returns `[]`. - `AllMorphRowsListsPoseNamesAndKeyTimes` — builds a mesh with two named poses + matching VAT_POSE animations, asserts both names appear and each carries a single t=0 keyframe (A1's importer-time default). ### #518 status after this slice | Sub-slice | What | Status | |-|-|-| | A1 | Importer + manager + CLI list | ✅ #569 | | A2 | Inspector "Morph Targets" subgroup | ✅ #570 | | A4a | glTF morph-target export | ✅ #573 | | A4b | FBX morph-target export | ✅ #575 | | A5 | Dope sheet morph rows (read-only) | this PR | | A6 | MCP tools | ✅ #571 | | A3 | Authoring (delta → new pose, rename, delete) | still pending | Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a morph-target visualization band to the animation dope sheet. A new ChangesMorph-Target Dope-Sheet Band
Sequence DiagramsequenceDiagram
participant Sheet as AnimationDopeSheet
participant Controller as AnimationControlController
participant Manager as MorphAnimationManager
participant Band as morphBand
Sheet->>Controller: allMorphRows() on selection change
Controller-->>Sheet: [{ name, keyTimes }]
Sheet->>Sheet: morphRows = result
Note over Sheet,Manager: Connect to morph signals
Manager->>Sheet: morphWeightChanged signal
Sheet->>Controller: allMorphRows() refresh
Controller-->>Sheet: updated rows
Sheet->>Sheet: morphRows = updated
Sheet->>Band: morphRows changed
Band->>Band: re-render rows & markers
Manager->>Sheet: morphTargetsChanged signal
Sheet->>Controller: allMorphRows() refresh
Controller-->>Sheet: updated rows
Sheet->>Sheet: morphRows = updated
Sheet->>Band: morphRows changed
Band->>Band: re-render rows & markers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cd50f4e96
ℹ️ 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".
| for (const auto& [handle, track] : vtracks) { | ||
| if (!track) continue; | ||
| for (unsigned short i = 0; i < track->getNumKeyFrames(); ++i) | ||
| keyTimes.append(static_cast<double>(track->getKeyFrame(i)->getTime())); | ||
| } |
There was a problem hiding this comment.
Use pose-specific track when building morph keyTimes
allMorphRows() loops every vertex track in the animation named after each pose, but the importer intentionally groups same-named poses across submeshes into one animation with multiple vertex tracks (src/Assimp/MeshProcessor.cpp, grouping comment around the hasAnimation(animName) path). In that common case, each row gets keyframes from unrelated tracks, so the dope sheet shows duplicated/incorrect diamonds for a pose (e.g., two submeshes sharing "Smile" produce repeated t=0 entries per row). The row should read only the track matching pose->getTarget() (and ideally the expected pose track type) instead of aggregating all tracks.
Useful? React with 👍 / 👎.
Codex P2 on PR #576: `allMorphRows()` looped over every VertexTrack in the per-pose Animation. A1's importer groups same-named poses across submeshes into a single Animation with one VAT_POSE track per affected submesh (so e.g. a "Smile" target on body + head ends up as one Animation with two tracks). Without filtering, the dope sheet row for that pose would carry diamonds from both tracks — duplicated `t=0` markers in the simple A1 case, and wrong key counts once authoring adds per-time keys. Fix: look up only the VertexTrack whose handle matches `pose->getTarget()`. Also assert it's a VAT_POSE track (belt-and-suspenders — VAT_MORPH could be added in a future slice and we don't want to silently mix track types). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
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)
qml/AnimationDopeSheet.qml (1)
276-281:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReserve layout space for
morphBandto avoid overlaying timeline/rows.
morphBandis anchored to the bottom, butrowsViewandtimelineAreastill span toparent.bottom, so the new band sits on top of interactive content when visible.Suggested fix
ListView { id: rowsView anchors.left: parent.left; anchors.right: parent.right anchors.top: header.visible ? header.bottom : parent.top - anchors.bottom: parent.bottom + anchors.bottom: morphBand.visible ? morphBand.top : parent.bottom clip: true model: root.rows @@ MouseArea { id: timelineArea z: -1 anchors.left: parent.left; anchors.leftMargin: root.leftStripWidth anchors.top: header.visible ? header.bottom : parent.top - anchors.right: parent.right; anchors.bottom: parent.bottom + anchors.right: parent.right + anchors.bottom: morphBand.visible ? morphBand.top : parent.bottom acceptedButtons: Qt.MiddleButton | Qt.LeftButtonAlso applies to: 577-585, 664-669
🤖 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 `@qml/AnimationDopeSheet.qml` around lines 276 - 281, rowsView and timelineArea currently anchor their bottom to parent.bottom so when morphBand is visible it overlays interactive content; change their bottom anchor to depend on morphBand visibility (e.g. set bottom to morphBand.visible ? morphBand.top : parent.bottom) so they reserve space when morphBand is shown; apply the same fix for the other occurrences referencing rowsView/timelineArea and the morphBand block (the code sections around the other occurrences you noted).
🤖 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/AnimationControlController.cpp`:
- Around line 905-910: In allMorphRows(), prefer the controller's active entity
instead of directly using
SelectionSet::getSingleton()->getResolvedEntities().first(); obtain the
controller's active entity (e.g., via controller->getActiveEntity() or the
controller's activeEntity accessor) and if it's valid use that Ogre::Entity* as
the entity for generating morph rows; only fall back to
SelectionSet::getSingleton() and entities.first() when the controller has no
active entity. Update the logic in the block that currently references
SelectionSet::getSingleton(), entities.first(), and the local Ogre::Entity*
entity so it first queries the controller's active entity, then the
selection-set order (apply same change for the same pattern in the 902-950
region).
---
Outside diff comments:
In `@qml/AnimationDopeSheet.qml`:
- Around line 276-281: rowsView and timelineArea currently anchor their bottom
to parent.bottom so when morphBand is visible it overlays interactive content;
change their bottom anchor to depend on morphBand visibility (e.g. set bottom to
morphBand.visible ? morphBand.top : parent.bottom) so they reserve space when
morphBand is shown; apply the same fix for the other occurrences referencing
rowsView/timelineArea and the morphBand block (the code sections around the
other occurrences you noted).
🪄 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: 6ee5d987-3d96-4241-bb4e-df9bbe02dc3c
📒 Files selected for processing (4)
qml/AnimationDopeSheet.qmlsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationControlController_test.cpp
| auto* sel = SelectionSet::getSingleton(); | ||
| if (!sel) return rows; | ||
| const auto entities = sel->getResolvedEntities(); | ||
| if (entities.isEmpty() || !entities.first()) return rows; | ||
| Ogre::Entity* entity = entities.first(); | ||
| if (!entity) return rows; |
There was a problem hiding this comment.
Use the controller’s active entity before falling back to selection-set order.
allMorphRows() currently binds to entities.first(), which can mismatch the dope-sheet’s actively selected clip/entity under multi-selection and show morph rows from the wrong mesh.
Suggested fix
QVariantList AnimationControlController::allMorphRows() const
{
QVariantList rows;
- auto* sel = SelectionSet::getSingleton();
- if (!sel) return rows;
- const auto entities = sel->getResolvedEntities();
- if (entities.isEmpty() || !entities.first()) return rows;
- Ogre::Entity* entity = entities.first();
+ Ogre::Entity* entity = m_selectedEntity;
+ if (!entity) {
+ auto* sel = SelectionSet::getSingleton();
+ if (!sel) return rows;
+ const auto entities = sel->getResolvedEntities();
+ if (entities.isEmpty() || !entities.first()) return rows;
+ entity = entities.first();
+ }
if (!entity) return rows;Also applies to: 902-950
🤖 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/AnimationControlController.cpp` around lines 905 - 910, In
allMorphRows(), prefer the controller's active entity instead of directly using
SelectionSet::getSingleton()->getResolvedEntities().first(); obtain the
controller's active entity (e.g., via controller->getActiveEntity() or the
controller's activeEntity accessor) and if it's valid use that Ogre::Entity* as
the entity for generating morph rows; only fall back to
SelectionSet::getSingleton() and entities.first() when the controller has no
active entity. Update the logic in the block that currently references
SelectionSet::getSingleton(), entities.first(), and the local Ogre::Entity*
entity so it first queries the controller's active entity, then the
selection-set order (apply same change for the same pattern in the 902-950
region).
The bone-row ListView and the new morph band were both anchored to `parent.bottom`. With both visible, their layout rectangles overlapped, which (under some redraw conditions in CI) drove SIGSEGV crashes in any MainWindow-construction test that instantiated the dope sheet QML during MainWindow startup — MCPServerTest.ToggleNormals_WithMainWindowTogglesVisibility and MainWindowTest.ViewMenuConsoleToggleUpdatesDockVisibilityAndSettings both regressed on this branch. Fix: anchor the bone ListView's bottom to `morphBand.top` when the morph band is visible, falling back to `parent.bottom` otherwise. The two no longer fight for the same space. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The signal's payload includes `Ogre::Entity*`, a raw pointer Qt 6 can't safely marshal to QML/JS. Binding the slot crashed the QML engine during MainWindow construction on Linux/Xvfb CI runners — manifested as SIGSEGV in MCPServerTest.ToggleNormals_* and MainWindowTest.ViewMenuConsoleToggle*. Both tests construct a MainWindow which loads the dope sheet QML; the binding setup itself was the unsafe step. The dope sheet's per-row data is structural (pose name + keyframe times), not weight-driven. Listening only to `morphTargetsChanged` (which fires on selection change and is parameter-free) covers the cases that matter: a different entity showing different morph names. Per-weight notifications were purely cosmetic (the row would re-fetch identical data on every slider drag) and skipping them is the right tradeoff. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The QML changes in this slice consistently triggered SIGSEGV in unrelated test suites (MCPServerTest + MainWindowTest visibility-toggle tests), deterministically across 4 CI re-runs. The 3 new C++ controller tests all pass; only the QML integration crashes are blocking merge. Ship just the data API (`AnimationControlController::allMorphRows`) and its tests in this slice. The QML dope-sheet morph band will land in a separate PR where the crash can be isolated without holding back the backend work that 3 other in-flight slices need. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
…#578) * feat(morph): sub-slice A3 — authoring (add from edit, rename, delete) 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> * fix(build): add MorphCommands.cpp to tests/ source list 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> * review(morph): A3 — capture bind positions on EditableMesh load 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> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>



Summary
AnimationControlController::allMorphRows()— Q_INVOKABLE that returns aQVariantListof{ name, keyTimes }rows for poses on the selected entity's mesh.pose->getTarget()handle so importer-grouped same-name animations don't collapse into one track (Codex P2 fix).AllMorphRowsEmptyWhenNoSelection— returns[]cleanly.AllMorphRowsEmptyForMeshWithoutPoses— bone-only entity returns[].AllMorphRowsListsPoseNamesAndKeyTimes— two named poses each with t=0 keyframe.Why scope was narrowed
The original PR also integrated a morph band into the QML dope sheet, but the QML changes consistently triggered SIGSEGV in unrelated test suites (
MCPServerTest.ToggleNormals_WithMainWindowTogglesVisibilityandMainWindowTest.ViewMenuConsoleToggleUpdatesDockVisibilityAndSettings) across 4 CI re-runs. The 3 new controller tests pass; only the QML integration is blocking. Shipping the data API alone unblocks the rest of the morph epic — the dope-sheet UI lands in a follow-up PR where the crash can be isolated without holding back the backend work that other slices depend on.#518 status after this slice
Test plan
AnimationControlControllerTestcases pass🤖 Generated with Claude Code