feat(morph): sub-slice A1 — import detection + manager + CLI list#569
Merged
Conversation
First sub-slice of #518 / #517 Slice A. The repo had no morph- target handling at all (no `Ogre::Pose` / `MorphAnimationTrack` usage anywhere in `src/`); this lays the foundation the rest of the slice — Inspector sliders, dope-sheet wiring, authoring, export — will build on top of. ### What ships **Importer (MeshProcessor):** - `aiMesh::mAnimMeshes` (Assimp's blend-shape array) is now collected per submesh into a new `MorphTargetData` struct, with the same Z-up axis bake the base vertex pass uses. - At mesh-build time, each target becomes an `Ogre::Pose` storing per-vertex deltas relative to the base mesh, attached to the owning submesh. Zero-delta vertices are skipped to keep the pose sparse. - One `Ogre::Animation` per pose (single VAT_POSE track, one keyframe at t=0 referencing that pose at influence 1.0). Animation name = pose name = the upstream morph-target name. This gives every pose its own `Ogre::AnimationState` weight, which the manager drives. **`MorphAnimationManager` (QML_SINGLETON):** - `morphTargetsFor(entity)` — enumerate names in mesh-pose-list order. - `weight(entity, name)` / `setWeight(entity, name, w)` — read/write a single morph weight via the matching AnimationState (clamps to [0..1], enables the state, pins time to 0). Emits `morphWeightChanged` and `morphTargetsChanged`. - Selection-driven QML helpers (`morphTargetsForSelection`, `weightForSelection`, `setWeightForSelection`) for the future Inspector subgroup. - Sentry breadcrumbs `scene.anim.morph`. **CLI subcommand:** - `qtmesh morph <file> --list [--json]` — load the file headlessly, print the morph-target names (or zero count when there are none). Other modes (`--set`, `--add`, `--delete`) land in follow-up sub-slices once the authoring code is in place. ### Tests (10) Standalone (no Ogre): singleton, null-entity handling, empty-name rejection. Scene-fixture: builds a 3-vertex mesh with two named poses ("JawOpen", "Smile") + matching VAT_POSE animations — same shape MeshProcessor produces. Verifies `morphTargetsFor` ordering, default-zero weights, `setWeight` clamping + value retrieval, unknown-name rejection, `morphWeightChanged` signal, selection-driven QML helpers. ### What's NOT in this PR (deferred to A2–A6) - Inspector "Morph Targets" subgroup with sliders - Authoring (save edit-mode delta as new target, rename, delete) - Export round-trip (FBX/glTF preserves morph targets) - Dope sheet + curve editor recognise morph-weight tracks - MCP tools Each becomes its own focused PR built on this foundation. 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 (4)
📝 WalkthroughWalkthroughPR introduces morph target/blend shape support: imports Assimp ChangesMorph Targets / Blend Shapes Import and Runtime Control
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 |
![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: 0f75869d9e
ℹ️ 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
+306
to
+307
| if (ogreMesh->hasAnimation(animName)) continue; // duplicate pose name; skip | ||
| Ogre::Animation* anim = ogreMesh->createAnimation(animName, /*length=*/0.0f); |
There was a problem hiding this comment.
Do not skip duplicate pose names here: when two submeshes both contain a morph target like Smile, only the first pose gets an animation track and later poses are left unreferenced. MorphAnimationManager::setWeight drives morphs via AnimationState name lookup, so in multi-submesh meshes this makes the same-named target deform only one submesh while the others never move.
Useful? React with 👍 / 👎.
Comment on lines
+4998
to
+4999
| entity = static_cast<Ogre::Entity*>(obj); | ||
| break; |
There was a problem hiding this comment.
This loop exits after the first imported entity, so qtmesh morph <file> --list can report incomplete results (or No morph targets) for files that import multiple entities when later entities hold the blend shapes. The command claims to inspect the file, but currently only inspects whichever entity is encountered first.
Useful? React with 👍 / 👎.
Two findings from Codex on PR #569: **P1 (MeshProcessor)** — duplicate pose names across submeshes were dropped. Multi-submesh meshes where, e.g., body + head both carry a "Smile" target would only get the first pose registered; subsequent same-named poses were skipped via `hasAnimation`, leaving the second submesh's pose unreferenced. Since `MorphAnimationManager::setWeight` drives morphs via `AnimationState`-name lookup, the same-named target would deform only one submesh. Group same-named poses into one Animation with one VAT_POSE track per affected submesh. Each track has its own pose reference at full influence. The Animation's single AnimationState weight then drives all submeshes simultaneously, which is exactly what users author when they paint a "Smile" target across multiple meshes in a DCC. Also defends the rare same-submesh duplicate case (same name on the same submesh — content error) by reusing the existing keyframe rather than failing the second `createVertexTrack`. **P2 (CLI)** — `qtmesh morph <file> --list` exited after the first imported entity, so files that import multiple entities with blend shapes on the later entities reported false zeros. Walk every entity and union the target names (de-duped, first- sighting order preserved). 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: 4
🧹 Nitpick comments (1)
src/MorphAnimationManager_test.cpp (1)
192-203: ⚡ Quick winTighten this test to verify “real change” semantics explicitly.
Right now it only proves at least one emission. Add an idempotent write assertion to ensure unchanged values do not emit.
Minimal assertion upgrade
TEST_F(MorphAnimationManagerSceneTest, SetWeightEmitsSignalOnRealChange) { @@ auto* m = MorphAnimationManager::instance(); QSignalSpy spy(m, &MorphAnimationManager::morphWeightChanged); - m->setWeight(entity, QStringLiteral("JawOpen"), 0.5f); - EXPECT_GE(spy.count(), 1); + EXPECT_TRUE(m->setWeight(entity, QStringLiteral("JawOpen"), 0.5f)); + const int afterFirst = spy.count(); + EXPECT_GE(afterFirst, 1); + EXPECT_TRUE(m->setWeight(entity, QStringLiteral("JawOpen"), 0.5f)); + EXPECT_EQ(spy.count(), afterFirst); + EXPECT_TRUE(m->setWeight(entity, QStringLiteral("JawOpen"), 0.7f)); + EXPECT_GT(spy.count(), afterFirst); }🤖 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_test.cpp` around lines 192 - 203, The test currently only checks that setting a weight emits at least one signal; tighten it by asserting idempotent writes do not emit: after calling MorphAnimationManager::instance()->setWeight(entity, QStringLiteral("JawOpen"), 0.5f) and verifying spy.count() >= 1, store the current spy.count(), call setWeight again with the identical value (0.5f) and then assert spy.count() remains equal to the stored value (no additional signal emitted). Use the existing QSignalSpy (spy) and the same entity/"JawOpen" identifier to implement this idempotent-write check.
🤖 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/Assimp/MeshProcessor.cpp`:
- Around line 283-293: The code currently calls
ogreMesh->createPose(targetSubmesh, mt.name) before checking for non-zero vertex
deltas, which creates empty/no-op poses; change the logic in MeshProcessor.cpp
so you first scan mt.positions vs smd->vertices to detect at least one delta
with squaredLength() > 1e-12f (the same check used now), and only then call
ogreMesh->createPose(targetSubmesh, mt.name) and addVertex(...) for each
non-zero delta; alternatively accumulate the non-zero deltas/indices first and
create the Ogre::Pose only when the accumulator is non-empty, skipping creation
entirely for identical-to-base targets.
In `@src/CLIPipeline.cpp`:
- Line 1071: The new subcommand is wired via the cmd == "morph" branch calling
cmdMorph but is not listed in the CLI help; update the printUsage() function to
include the "morph" entry and a short description so it appears in --help output
(add "morph" to the commands list and any relevant usage/example text). Ensure
the help text references the cmdMorph behavior/arguments consistent with other
entries so users can discover and invoke the new subcommand.
In `@src/MorphAnimationManager_test.cpp`:
- Around line 108-111: The test currently passes because passing nullptr to
MorphAnimationManager::setWeight triggers the null-entity branch rather than
exercising empty-name validation; modify the test to use a real, scene-backed
valid entity (create or obtain an Entity/EntityItem from the test scene) and
then call MorphAnimationManager::setWeight(entityPtr, QString(), 0.5f) so that
the empty-name path in setWeight is exercised and rejected; reference
MorphAnimationManager::instance() and the setWeight(Entity*, QString, float)
overload when making this change.
In `@src/MorphAnimationManager.cpp`:
- Around line 25-41: Ensure MorphAnimationManager singleton lifecycle methods
enforce main-thread access: in MorphAnimationManager::instance(),
MorphAnimationManager::qmlInstance(), and MorphAnimationManager::kill() add a
check/assert that the current thread is the application's main thread (e.g.
compare QThread::currentThread() to qApp->thread() or
QCoreApplication::instance()->thread()) before touching s_instance; if the check
fails assert or log and return/early-exit as appropriate. Reference the methods
instance(), qmlInstance(), kill(), the static s_instance and the class
MorphAnimationManager when making the change.
---
Nitpick comments:
In `@src/MorphAnimationManager_test.cpp`:
- Around line 192-203: The test currently only checks that setting a weight
emits at least one signal; tighten it by asserting idempotent writes do not
emit: after calling MorphAnimationManager::instance()->setWeight(entity,
QStringLiteral("JawOpen"), 0.5f) and verifying spy.count() >= 1, store the
current spy.count(), call setWeight again with the identical value (0.5f) and
then assert spy.count() remains equal to the stored value (no additional signal
emitted). Use the existing QSignalSpy (spy) and the same entity/"JawOpen"
identifier to implement this idempotent-write check.
🪄 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: 2036f987-8afb-49a6-a7c2-60ce27489d23
📒 Files selected for processing (11)
src/Assimp/MeshProcessor.cppsrc/Assimp/MeshProcessor.hsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/MorphAnimationManager.cppsrc/MorphAnimationManager.hsrc/MorphAnimationManager_test.cppsrc/main.cppsrc/mainwindow.cpptests/CMakeLists.txt
Five findings from CodeRabbit on PR #569 + one CI failure: **CI** — `WeightDefaultIsZeroBeforeSet` failed: Ogre's `AnimationState` defaults to weight=1.0 (disabled), but the user-facing mental model is "untouched morph = 0". Change `MorphAnimationManager::weight()` to return 0 when the state is disabled — disabled ⇔ "not yet driving the pose". Tests + QML sliders see the expected 0→1 range. **Minor (MeshProcessor)** — `createPose` was called before confirming any non-zero delta, leaving empty poses on the mesh when an FBX exports an identical-to-base target. Lazy-create the pose on first non-zero delta. **Minor (CLI help)** — `printUsage()` listed neither the `vat` (slice 3) nor the new `morph` subcommand. Add both with concise descriptions. **Minor (test)** — `EmptyNameRejected` only tested with a null entity, so a regression where empty-name handling becomes entity-dependent could slip through. Add a scene-fixture variant that exercises the rejection with a valid live entity. **Major (singleton thread safety)** — Per CLAUDE.md "All run on the main thread", `instance()` / `qmlInstance()` / `kill()` should assert main-thread access. Adds an `assertMainThread()` helper using QCoreApplication's thread, called at each lifecycle entry point. **Nit (test)** — `SetWeightEmitsSignalOnRealChange` only proved ≥1 emission. Tighten to assert idempotent writes don't re-emit, and different writes do. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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.
First sub-slice of #518 (parent epic: #517 / Slice A). The repo had no morph-target handling at all — no
Ogre::Pose/MorphAnimationTrackusage anywhere in `src/`. This sub-slice lays the foundation the rest of #518 — Inspector sliders, dope-sheet wiring, authoring, export — will build on top of.What ships
Importer (MeshProcessor)
aiMesh::mAnimMeshes(Assimp's blend-shape array) is now collected per submesh into a newMorphTargetDatastruct, with the same Z-up axis bake the base vertex pass uses.Ogre::Posestoring per-vertex deltas relative to the base mesh, attached to the owning submesh. Zero-delta vertices are skipped to keep the pose sparse.Ogre::Animationper pose (single VAT_POSE track, one keyframe at t=0 referencing that pose at influence 1.0). Animation name = pose name = the upstream morph-target name. This gives every pose its ownOgre::AnimationStateweight.MorphAnimationManager(QML_SINGLETON)morphTargetsFor(entity)— enumerate names in mesh-pose-list order.weight(entity, name)/setWeight(entity, name, w)— read/write a single morph weight via the matchingAnimationState. Clamps to[0..1], enables the state, pins time to 0. EmitsmorphWeightChangedandmorphTargetsChanged.morphTargetsForSelection,weightForSelection,setWeightForSelection) ready for the future Inspector subgroup.scene.anim.morph.CLI
`qtmesh morph --list [--json]` — load headlessly, print morph-target names. Other modes land in follow-up sub-slices once the authoring code is in place.
10 new tests
Standalone (no Ogre): singleton identity, null-entity handling, empty-name rejection.
Scene-fixture: builds a 3-vertex mesh with two named poses ("JawOpen", "Smile") + matching VAT_POSE animations — same shape MeshProcessor produces. Verifies pose-list ordering, default-zero weights,
setWeightclamping, unknown-name rejection,morphWeightChangedsignal, selection-driven QML helpers.NOT in this PR — deferred to A2–A6
Each becomes its own focused PR built on this foundation.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests