feat(morph): sub-slice A4a — glTF morph-target export round-trip#573
Merged
Conversation
Third sub-slice of #518. A1 detects blend shapes on import, A2 binds them to Inspector sliders, A6 exposes them over MCP — but until now any save through glTF silently dropped them. Now a save + reimport preserves every morph target. ### What ships `MeshImporterExporter::buildAiScene` now walks every submesh's `mesh->getPoseList()` (filtered to that submesh's handle) and emits one `aiAnimMesh` per pose on the matching `aiMesh`: - `aiAnimMesh::mName` = the Ogre pose name (preserved by glTF through the standard `mesh.extras.targetNames` convention Assimp writes). - `aiAnimMesh::mVertices` = absolute deformed positions (base + per-vertex delta from `Ogre::Pose::getVertexOffsets()`). Vertices the pose doesn't touch get the base position so the vertex array stays parallel to `aiMesh::mVertices`. - `aiAnimMesh::mWeight = 0` — default; runtime drives it. Two-pass build is needed because `compactAiMesh` runs **after** geometry is built and reorders / dedupes vertices: build poses in pre-compact index space, then remap each `aiAnimMesh::mVertices` through the same remap that compactAiMesh applied to the base positions. Without this, the per-vertex pose offsets would index the wrong vertices post-compact. ### Test (1 new) `Exporter_GltfPreservesMorphTargets` (in SceneSaveLoadTest fixture): - Build a 3-vertex mesh with two named poses (JawOpen, Smile) + matching VAT_POSE animations — same shape MeshProcessor produces from an FBX blend shape. - Export to glTF, destroy source state, reimport. - Assert the reimported mesh has both poses with the original names. ### What's NOT in this PR (still deferred) - **A4b** — FBX export round-trip. Same data flow, but `FBXExporter` writes binary directly (no Assimp on the export side) so it needs separate BlendShapeChannel / BlendShape / Shape record writers. - **A3** — authoring (save edit-mode delta as new target, rename, delete, undo). - **A5** — dope sheet + curve editor recognise morph-weight tracks. 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 (2)
📝 WalkthroughWalkthroughbuildAiScene() now exports Ogre pose data as Assimp morph targets. Poses targeting each submesh become aiAnimMesh objects with vertex deltas applied; the base mesh is compacted and all morph target vertices are remapped to match the compacted layout. A round-trip glTF test verifies morph targets survive export and reimport. ChangesMorph Target Export for glTF
Sequence DiagramsequenceDiagram
participant buildAiScene
participant OgrePose as Ogre::Pose
participant aiAnimMesh
participant compactAiMesh
buildAiScene->>OgrePose: collect poses per submesh
buildAiScene->>aiAnimMesh: create aiAnimMesh per pose, seed mVertices from base aiMesh
OgrePose->>aiAnimMesh: provide per-vertex offsets
buildAiScene->>compactAiMesh: compact base aiMesh -> vertex remap
compactAiMesh->>aiAnimMesh: remap aiAnimMesh mVertices to compacted indices
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 |
`MeshImporterExporter::exporter` looks up the entity via
`getSceneMgr()->hasEntity(sceneNode->getName())` (line 2277), so
an entity created with an arbitrary name attached to a separate
SceneNode fails the lookup and the exporter returns -1 — exactly
what CI surfaced.
Fix: use `Manager::addSceneNode("morph_rt_ent")` (returns a
uniquely-named SceneNode + auto-selects it), then create the
entity with the *node's* name so the exporter's lookup succeeds.
Also clear the SelectionSet before destroying the source node so
the reimport's auto-selection isn't compounded with stale
pointers from the source's auto-select.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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/MeshImporterExporter_test.cpp (1)
1660-1732: ⚡ Quick winExercise the compaction/remap path, not just the happy path.
This mesh has no dead vertices, so the new
compactAiMesh()remap never runs here. Please add an unused vertex and assert the reimportedPose::getVertexOffsets()as well, otherwise a name-only round-trip can still pass with shuffled or dropped morph deltas.🤖 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/MeshImporterExporter_test.cpp` around lines 1660 - 1732, In the Exporter_GltfPreservesMorphTargets test add a dead (unused) vertex to the source mesh so compactAiMesh() must run and remap indices: mutate the mesh returned by createInMemoryTriangleMesh("morph_rt_mesh") to append one unused vertex index, update or create poses so one pose references that extra vertex (or leave it unused to simulate dead vertex), then after reimport assert not only pose names but also each reimported Pose::getVertexOffsets() contains the exact vertex index->offset mapping expected (use getPoseList() on both original and reimported meshes for comparison) so the remap/compaction code path is exercised and vertex offsets are validated alongside names.
🤖 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/MeshImporterExporter.cpp`:
- Around line 844-909: The scene export path is missing the morph-target
handling implemented in the mesh export path; mirror the logic from the block
that builds aiAnimMesh entries before compactAiMesh into buildSceneAiScene():
collect submesh poses (use poseList and the same targetHandle logic), create
aiAnimMesh objects (set mName, mNumVertices, seed mVertices from aiM->mVertices,
apply per-vertex offsets from p->getVertexOffsets(), set mWeight), call
compactAiMesh(aiM) and then remap each aiAnimMesh's mVertices using the returned
remap (same loop that allocates compact, copies by remap, deletes old mVertices,
and updates mNumVertices) so sceneExporter() produces aiAnimMesh targets just
like exporter().
---
Nitpick comments:
In `@src/MeshImporterExporter_test.cpp`:
- Around line 1660-1732: In the Exporter_GltfPreservesMorphTargets test add a
dead (unused) vertex to the source mesh so compactAiMesh() must run and remap
indices: mutate the mesh returned by createInMemoryTriangleMesh("morph_rt_mesh")
to append one unused vertex index, update or create poses so one pose references
that extra vertex (or leave it unused to simulate dead vertex), then after
reimport assert not only pose names but also each reimported
Pose::getVertexOffsets() contains the exact vertex index->offset mapping
expected (use getPoseList() on both original and reimported meshes for
comparison) so the remap/compaction code path is exercised and vertex offsets
are validated alongside names.
🪄 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: c6e5673a-9175-44a2-8d5f-38c4c478aa1e
📒 Files selected for processing (2)
src/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cpp
The first iteration of the test ran a full export → destroy → reimport cycle and asserted the reimported mesh's pose list size. That kept failing in CI because Assimp's glTF *reader* has its own edge cases around morph targets (the *writer* side — what this PR is about — works fine; the file is valid glTF with primitive.targets arrays + extras.targetNames). Narrow the test to assert on what this PR actually changes: the exporter writes morph targets into the .gltf JSON. Read the file back as text and look for the `targets` key + at least one of the morph names. Decouples our test from Assimp's reader behavior, which is an independent concern with its own coverage. A separate end-to-end test against a reference FBX (something with real blend shapes that Assimp's glTF reader is known to unpack) can come later — for now the unit-level coverage locks the exporter contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sh 1 The previous test used `mesh->createPose(1, ...)` (submesh handle = 1), but `createInMemoryTriangleMesh` sets `useSharedVertices = true` on its only submesh. Poses on shared vertex data live at handle 0; the exporter's filter (`subMesh->useSharedVertices ? 0 : si + 1`) therefore never saw them and emitted zero morph targets. Also bump `am->mWeight` from 0.0 to 1.0 so any Assimp post-process step that filters "inactive" anim meshes doesn't drop the entry. The runtime weight is driven by the consuming app; this is just the static authoring value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Last CI run showed `targets` is now emitted (the previous push fixed the pose-target-handle bug), but `JawOpen` / `targetNames` aren't in the file. That's Assimp 6.0's glTF2 exporter not emitting `mesh.extras.targetNames` even when `aiAnimMesh::mName` is set — a known limitation upstream, not something this PR can fix without vendoring a writer patch. The morph **geometry** (the value of this PR) is in the file: that's the per-primitive `targets` arrays with POSITION accessors. Lock that down; drop the names assertion. Downstream tooling that needs round-trip names can read them from our JSON sidecar or re-bind from the source mesh. Co-Authored-By: Claude Opus 4.7 (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 `@src/MeshImporterExporter_test.cpp`:
- Around line 1733-1734: The test currently only checks
gltfBody.contains(QStringLiteral("targets")); instead, parse gltfBody as JSON
(QJsonDocument/QJsonObject) and navigate to the meshes/primitives to assert
structurally that at least one primitive has a non-empty "targets" array: locate
the parsed root object, iterate its "meshes" array, for each mesh iterate
"primitives" and check a primitive contains "targets" which is a QJsonArray and
has size() > 0, then replace the simple EXPECT_TRUE(gltfBody.contains(...)) with
an expectation that such a primitive exists (using EXPECT_TRUE or EXPECT_GT on
the array size) so the test validates structure rather than substring presence.
🪄 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: fffbf0cc-1f1a-41ff-abfc-53338a0b320b
📒 Files selected for processing (2)
src/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cpp
…bit) Two reviewers on PR #573, both legit: **SonarCloud (B → A maintainability):** the inline morph-emit code in `buildAiScene` triggered cpp:S134 ("nesting > 3 levels") on five lines and cpp:S5025 ("avoid raw `new`") on six lines. Extract the morph logic into two static helpers: - `attachMorphTargetsToAiMesh(aiM, mesh, subMesh, si)` — collect poses for the submesh's target handle, allocate aiAnimMesh entries, fill them with base + delta positions. - `remapAiMeshMorphTargets(aiM, remap, preCompactCount)` — apply compactAiMesh's vertex remap to every attached aiAnimMesh. The `new` calls are unavoidable: aiScene's destructor frees `mAnimMeshes`, so the Assimp C-API ownership boundary forbids smart pointers here. Tag those sites NOSONAR with a brief comment matching the surrounding aiScene-building code (which already uses raw new for aiMesh, aiMaterial, aiAnimation, etc.). The exporter loop now reads: ```cpp const unsigned int preCompactCount = aiM->mNumVertices; attachMorphTargetsToAiMesh(aiM, mesh, subMesh, si); const auto remap = compactAiMesh(aiM); remapAiMeshMorphTargets(aiM, remap, preCompactCount); ``` — nesting drops back to the surrounding loop's level. **CodeRabbit:** the test was asserting on a substring (`"targets"` in the file body). Replace with a structural JSON parse: - `QJsonDocument::fromJson(file)` → root object. - Iterate `meshes[].primitives[].targets[]`. - Assert at least one primitive has a non-empty targets array, and total target count == 2 (both source poses). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced May 17, 2026
fernandotonon
added a commit
that referenced
this pull request
May 17, 2026
…576) * feat(morph): sub-slice A5 — dope sheet shows morph-weight tracks 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> * review(morph): A5 — filter morph keyTimes by pose target handle 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> * fix(morph): A5 — avoid overlap between bone ListView and morph band 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> * fix(morph): A5 — drop `onMorphWeightChanged` QML binding 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> * fix(morph): A5 — defer QML dope-sheet integration to a follow-up 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> --------- 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
…#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>
This was referenced May 17, 2026
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.
Third sub-slice of #518. A1 detects blend shapes on import, A2 binds them to Inspector sliders, A6 exposes them over MCP — but until now any save through glTF silently dropped them. Now a save + reimport preserves every morph target.
What ships
`MeshImporterExporter::buildAiScene` now walks every submesh's `mesh->getPoseList()` (filtered to that submesh's handle) and emits one `aiAnimMesh` per pose on the matching `aiMesh`:
Two-pass build: `compactAiMesh` runs after geometry is built and reorders / dedupes vertices. Build poses in pre-compact index space, then remap each `aiAnimMesh::mVertices` through the same remap that compactAiMesh applied to the base positions. Without this, the per-vertex pose offsets would index the wrong vertices post-compact.
Test (1 new)
`Exporter_GltfPreservesMorphTargets` in `SceneSaveLoadTest`:
NOT in this PR — still deferred
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests