Skip to content

feat(morph): sub-slice A4b — FBX morph-target export round-trip#575

Merged
fernandotonon merged 2 commits into
masterfrom
feat/morph-slice-a4b-fbx-export
May 17, 2026
Merged

feat(morph): sub-slice A4b — FBX morph-target export round-trip#575
fernandotonon merged 2 commits into
masterfrom
feat/morph-slice-a4b-fbx-export

Conversation

@fernandotonon
Copy link
Copy Markdown
Owner

@fernandotonon fernandotonon commented May 17, 2026

Fourth and final morph sub-slice of #518's export coverage. A4a landed glTF (PR #573); this lands FBX so save-then-reload through either format preserves every blend shape.

What ships

FBX represents blend shapes as a four-record chain hanging off each Geometry node:

```
Geometry (mesh)
Deformer "BlendShape" ← one per submesh that has poses
Deformer "BlendShapeChannel" "" ← one per pose, user-facing weight target
Geometry "" Shape ← sparse vertex deltas (Indexes + Vertices)
```

Connections: `Shape →OO BlendShapeChannel →OO BlendShape →OO Geometry`.

New `writeBlendShapeDeformers()` in `FBXExporter`:

  • Walks every emitted geometry's submesh, filters `mesh->getPoseList()` by target handle (handle 0 for shared-vertex submeshes, `si + 1` otherwise — matches the import + glTF export filter).
  • For each pose:
    • `Deformer`/`BlendShapeChannel` with `DeformPercent: 0` (runtime weight) + `FullWeights: [100.0]`.
    • `Geometry`/`Shape` with sparse `Indexes` (vertex indices that move, from `Pose::getVertexOffsets()`'s key set) and flat `Vertices` (per-vertex delta XYZ, Z mirrored to match the rest of the binary FBX export).
  • `writeConnections` emits the full chain.
  • `writeDefinitions` counts the new objects so the `Definitions/ObjectType` totals line up (Blender's importer warns + may bail when those drift).

Test

`Exporter_FbxBinary_WritesBlendShapeRecords`: build a mesh with two named poses (JawOpen, Smile), export, byte-level check for the `BlendShape`, `BlendShapeChannel`, `JawOpen`, and `Smile` strings (FBX binary stores node tags as length-prefixed ASCII, so signatures appear verbatim).

Reimport-side validation (Assimp's FBX reader) is a separate concern with its own coverage path — same scope decision A4a made for the glTF reader.

What's left in #518

  • A3 — authoring (save edit-mode delta as new target, rename, delete, undo).
  • A5 — dope sheet + curve editor recognise morph-weight tracks.

Test plan

  • CI Linux: new test passes; existing FBX exporter test still passes.
  • CI macOS / Windows: build clean.
  • Manual: load an FBX with blend shapes → save as .fbx → reload in Blender / Maya — morph targets present with original names.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • FBX export now supports morph targets and blend shapes. Users can export mesh deformations with proper FBX BlendShape object structure, including vertex delta calculations and Z-axis mirroring convention. Supports both shared and per-submesh vertex handling.
  • Tests

    • Added integration test to validate blend shape export functionality, ensuring morph target data is correctly written to FBX binary files with proper structure and naming.

Review Change Stack

Fourth and final morph sub-slice of #518's export coverage. A4a
landed glTF; this lands FBX so save-then-reload through either
format preserves every blend shape.

### What ships in FBXExporter

FBX represents blend shapes as a four-record chain hanging off
each Geometry node:

```
Geometry (mesh)
  Deformer "BlendShape"             ← one per submesh that has poses
    Deformer "BlendShapeChannel" "<name>"  ← one per pose, user-facing weight target
      Geometry "<name>" Shape       ← sparse vertex deltas (Indexes + Vertices)
```

with connections:
`Shape →OO BlendShapeChannel →OO BlendShape →OO Geometry`.

New `writeBlendShapeDeformers()`:
- Walks every emitted geometry's submesh, filters `mesh->getPoseList()`
  by the submesh's target handle (handle 0 for shared-vertex
  submeshes, `si + 1` otherwise — matches the import + glTF
  export filter).
- For each pose, writes:
  - A `Deformer`/`BlendShapeChannel` with `DeformPercent: 0` (the
    runtime weight) and `FullWeights: [100.0]` (one entry per
    Shape — single-influence channels in this minimal version).
  - A `Geometry`/`Shape` carrying:
    - `Indexes` — vertex indices that move (sparse, from
      `Pose::getVertexOffsets()`'s key set).
    - `Vertices` — flat per-vertex delta XYZ with Z mirrored to
      match the rest of the binary FBX export's coordinate
      convention.

### Connections + Definitions

- `writeConnections` emits `Shape →OO Channel →OO BlendShape →OO Geometry`
  on every blend-shape chain.
- `writeDefinitions` counts the new objects so the
  `Definitions/ObjectType` totals line up with reality (Blender's
  importer warns and may bail when those drift).

### Test

`Exporter_FbxBinary_WritesBlendShapeRecords`: build a mesh with
two named poses (JawOpen, Smile), export, then byte-level check
the resulting file for the `BlendShape`, `BlendShapeChannel`,
`JawOpen`, and `Smile` strings. FBX binary stores node tags as
length-prefixed ASCII so these signatures appear verbatim.

Reimport-side validation (Assimp's FBX reader) is a separate
concern with its own coverage path — same scope decision A4a
made for the glTF reader.

### What's left in #518

- **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>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

Warning

Rate limit exceeded

@fernandotonon has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 40 minutes and 47 seconds before requesting another review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 50d7fd3b-f81a-4469-9b16-5c62b4d8db4b

📥 Commits

Reviewing files that changed from the base of the PR and between f816975 and e1f92cf.

📒 Files selected for processing (2)
  • src/FBX/FBXExporter.cpp
  • src/MeshImporterExporter_test.cpp
📝 Walkthrough

Walkthrough

This PR extends the FBX exporter to serialize Ogre morph targets as FBX BlendShape deformer objects. It adds definition counting, per-geometry BlendShape/Channel/Shape emission with sparse vertex delta data, FBX object graph wiring, and an integration test validating the exported blend shape records.

Changes

BlendShape Export for Ogre Morph Targets

Layer / File(s) Summary
BlendShape state and object accounting
src/FBX/FBXExporter.cpp
Internal connection-tracking structures (BlendShapeConn, ChannelConn) are added as member state. In writeDefinitions(), the exporter now counts additional Deformer objects (one BlendShape plus one BlendShapeChannel per targeted pose per submesh) and Geometry objects (one Shape per pose) by scanning pose lists and respecting the useSharedVertices rule.
BlendShape serialization and integration
src/FBX/FBXExporter.cpp
New writeBlendShapeDeformers() method writes, for each geometry, a top-level BlendShape deformer, then per-pose a BlendShapeChannel deformer and a Shape geometry containing sparse vertex indexes and delta offsets from Ogre::Pose::getVertexOffsets(). Vertex deltas apply the same Z-mirroring convention as the rest of the exporter. The method is invoked from writeObjects() alongside material/texture outputs.
BlendShape connection graph wiring
src/FBX/FBXExporter.cpp
In writeConnections(), the FBX object graph is completed by connecting Shape geometries to their BlendShapeChannel deformers, channels to BlendShapes, and BlendShapes back to target geometries, using the stored IDs collected during serialization.
BlendShape export integration test
src/MeshImporterExporter_test.cpp
Test Exporter_FbxBinary_WritesBlendShapeRecords creates a scene with two morph targets (JawOpen, Smile) on the shared-vertex submesh, exports to FBX Binary, and validates the output file body contains the expected BlendShape, BlendShapeChannel, and target name strings.

Possibly related issues

  • #518: The BlendShape export implementation in FBXExporter.cpp (definition counting, serialization, and connection wiring) directly fulfills the exporter requirement for morph/blend-shape export functionality.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Whiskers twitch with delight so bright,
Morphs take shape in FBX light,
Poses dance, deformers glow,
BlendShape chains now steal the show!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 77.78% 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 accurately summarizes the main change: implementing FBX morph-target export as the fourth slice of a larger morph-target feature effort.
Description check ✅ Passed The description is comprehensive and well-structured, covering the what (FBX blend shape chain), how (new writeBlendShapeDeformers method with pose filtering and vertex delta handling), test coverage, and remaining work.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/morph-slice-a4b-fbx-export

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 and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

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

ℹ️ 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/FBX/FBXExporter.cpp Outdated
Comment on lines +2304 to +2308
for (const auto& bs : m_blendShapeConns)
writeConnection("OO", bs.blendShapeId, bs.geomId);
for (const auto& ch : m_channelConns) {
writeConnection("OO", ch.channelId, ch.blendShapeId);
writeConnection("OO", ch.shapeGeomId, ch.channelId);
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 Move morph connections outside skeleton guard

writeBlendShapeDeformers() is called for every mesh export, but the new Shape→Channel→BlendShape connections are only emitted inside if (m_hasSkeleton). For meshes that have poses but no skeleton, this leaves the emitted BlendShape objects orphaned (no links to the Geometry), so FBX importers will typically ignore them and morph targets will not round-trip. This regression is specific to non-skinned morph meshes and should be fixed by writing these morph connections whenever !m_skeletonOnly, not only when a skeleton exists.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/FBX/FBXExporter.cpp`:
- Around line 2302-2309: The BlendShape "OO" connection writes in the
skeleton-only branch, causing orphaned BlendShape/Channel/Shape objects for
morph-only meshes; move the connection emission out of the m_hasSkeleton check
so targets are linked regardless of skeleton presence. Specifically, in
writeBlendShapeDeformers() ensure the loops that iterate m_blendShapeConns and
m_channelConns call writeConnection("OO", ...) unconditionally (or at least when
blendshape data exists) instead of being guarded by m_hasSkeleton, so
blendShapeId ↔ geomId and channelId ↔ blendShapeId/shapeGeomId ↔ channelId links
are always written.

In `@src/MeshImporterExporter_test.cpp`:
- Around line 2247-2254: The test currently asserts presence of "BlendShape",
"BlendShapeChannel", and the target names but misses asserting the actual shape
record; add an explicit assertion similar to the others (e.g.,
EXPECT_TRUE(body.contains("Shape")) with a clear failure message like "FBX
should carry Shape records") so that regressions where channels exist but the
shape geometry payload is missing are caught; locate the assertions block
containing EXPECT_TRUE(body.contains("BlendShape")) /
EXPECT_TRUE(body.contains("BlendShapeChannel")) and insert the new EXPECT_TRUE
check for "Shape" alongside them.
🪄 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: ab02dbe2-503a-4b6d-8a0b-e7924ccc6984

📥 Commits

Reviewing files that changed from the base of the PR and between 5c938c5 and f816975.

📒 Files selected for processing (2)
  • src/FBX/FBXExporter.cpp
  • src/MeshImporterExporter_test.cpp

Comment thread src/FBX/FBXExporter.cpp Outdated
Comment thread src/MeshImporterExporter_test.cpp
Two reviewers on PR #575 (Codex P1 + CodeRabbit Major), both
catching the same regression:

`writeBlendShapeDeformers()` ran for every mesh export, but the
Shape → Channel → BlendShape → Geometry `OO` connections were
emitted only inside `if (m_hasSkeleton)`. A pure-morph mesh (no
skin) therefore got the BlendShape/Channel/Shape *records* but
no links between them — Blender/Maya/Unreal importers ignore
orphaned blend-shape records, so the targets silently didn't
round-trip.

Move the chain emit out of the skeleton branch into a non-
skeleton-gated section right after the texture connections.
Gated only on `!m_skeletonOnly` (skeleton-only exports skip
geometry, so there are no morph records to link).

Plus a minor: test now also asserts on the `Shape` token to
catch regressions where channel records exist but the per-pose
Shape geometry payload is missing.

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

@fernandotonon fernandotonon merged commit 185c2dd into master May 17, 2026
20 checks passed
@fernandotonon fernandotonon deleted the feat/morph-slice-a4b-fbx-export branch May 17, 2026 15:35
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>
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>
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>
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.

1 participant