Support Unreal Engine FBX imports: Z-up baking, clean animation names, preserve numeric suffixes#239
Conversation
…meric suffixes - Bake Z-up→Y-up into mesh vertex data and root skeleton bones instead of applying a scene-node rotation, so exported meshes are upright by default - Animation-only FBX files (UE retargets with no geometry) are detected and offered for instant merge when a compatible mesh is already loaded - Model name always derived from file path rather than scene->mName, which UE sets to generic values like "unreal_take" - cleanAnimNoise now matches "Unreal Take" (space variant) in addition to "unreal_take" (underscore), so UE take names are stripped and the animation gets the source file name (e.g. "mm_attack_03") - deduplicateName preserves intentional numeric suffixes (_03, _04, …) by only stripping trailing _N when the desired name already collides - Inspector panel auto-refreshes after animation merge - Add two new unit tests: UnrealTakeCleanup and NumericSuffixPreserved - Bump version 2.18.1 → 2.19.0 - Update docs/index.html to document Unreal Engine FBX workflow Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR bumps the project version, adds Unreal Engine references in docs, and implements skeleton up‑axis handling across import, mesh/bone processing, and animation merging (registration, Z-up→Y-up baking/rotation, translation conversion, and animation-name cleanup). Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant MeshImporterExporter
participant AssimpImporter
participant BoneProcessor
participant MeshProcessor
participant AnimationMerger
User->>MainWindow: Import FBX (animation-only or mesh)
MainWindow->>MeshImporterExporter: loadModel(path)
MeshImporterExporter->>AssimpImporter: parse file
AssimpImporter->>BoneProcessor: create skeleton (isZup = sceneUpAxis==2)
Note right of BoneProcessor: if isZup → rotate root rest poses +90° X
AssimpImporter->>MeshProcessor: process meshes (isZup)
Note right of MeshProcessor: if isZup → rotate vertices/normals/tangents +90° X
AssimpImporter-->>MeshImporterExporter: skeleton/mesh created (upAxis)
MeshImporterExporter->>AnimationMerger: registerSkeletonUpAxis(name, upAxis)
AnimationMerger-->>MeshImporterExporter: stored mapping
User->>MainWindow: Merge animations into base entity
MainWindow->>AnimationMerger: mergeAnimations(base, src, srcUpAxis, baseUpAxis)
AnimationMerger->>AnimationMerger: lookup upAxis for skeletons
AnimationMerger->>AnimationMerger: apply binding-pose orientation correction
AnimationMerger->>AnimationMerger: if srcUp==Z and baseUp==Y → convert translations (x,z,-y)
AnimationMerger-->>MainWindow: merged animations (or null + err)
MainWindow->>MainWindow: re-select baseEntity to refresh inspector
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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: 206cd58f4c
ℹ️ 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".
| // Only bake Z-up into mesh skeletons. Animation-only files stay in their native | ||
| // space so AnimationMerger's needsZupToYup path can apply the single correct | ||
| // coordinate conversion when merging into the already-baked mesh skeleton. | ||
| boneProcessor.processBones(skeleton, scene, isZup && !animationOnly); |
There was a problem hiding this comment.
AssimpToOgreImporter::loadModel now bakes Z-up root rest transforms via processBones(..., isZup && !animationOnly) and then immediately imports animation channels, but AnimationProcessor::processAnimationChannel still consumes keyframe positions in the file’s original coordinate basis. For Z-up files that contain embedded animations, this creates a bind/key basis mismatch, so playback gets axis-swapped offsets (notably root translation/jump motion) even though the mesh is upright.
Useful? React with 👍 / 👎.
| // corrected = (q_dst⁻¹ * q_src) * stored | ||
| const Ogre::Quaternion q_src = srcSkel->getBone(srcHandle)->getOrientation(); | ||
| const Ogre::Quaternion q_dst = baseSkel->getBone(baseHandle)->getOrientation(); | ||
| const Ogre::Quaternion boneCorrection = q_src.Inverse() * q_dst; |
There was a problem hiding this comment.
The translation-space remap in mergeAnimationsByName computes boneCorrection as q_src.Inverse() * q_dst, but converting a vector from source bone-local space into destination bone-local space requires q_dst.Inverse() * q_src (which also matches the nearby comment). With the current order, merges where bind orientations differ rotate translation deltas in the wrong direction, producing incorrect root/track motion for orientation-mismatched skeleton pairs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/AnimationMerger.cpp (1)
10-20: Thread-safety consideration for the global registry.
s_skeletonUpAxisis a globalQMapwithout synchronization. This is safe as long as registration (viaregisterSkeletonUpAxis) and lookup (vialookupUpAxis) are only called from the main/UI thread. If there's any possibility of concurrent imports or merges from background threads in the future, consider usingQReadWriteLockorstd::shared_mutex.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationMerger.cpp` around lines 10 - 20, s_skeletonUpAxis is an unsynchronized global QMap accessed by registerSkeletonUpAxis and lookupUpAxis; protect concurrent access by adding a shared/read-write lock (e.g., QReadWriteLock or std::shared_mutex) around all reads and writes: acquire write lock in AnimationMerger::registerSkeletonUpAxis when modifying s_skeletonUpAxis and acquire read lock in lookupUpAxis when reading from s_skeletonUpAxis (preserve the default value behavior). Ensure the lock object is also a static global alongside s_skeletonUpAxis so both functions use the same lock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/AnimationMerger.cpp`:
- Around line 152-189: The bone correction quaternion is computed with the
operands reversed; update the computation of boneCorrection (currently using
q_src.Inverse() * q_dst) to use the correct order q_dst.Inverse() * q_src so
that stored source-local vectors are re-expressed into the destination
bone-local space; change the assignment where boneCorrection is created (refer
to q_src, q_dst, boneCorrection, and needsBoneCorrection) accordingly and
add/adjust a unit test that uses non-identity binding-pose orientations to catch
regressions.
In `@src/Assimp/Importer.cpp`:
- Around line 114-122: The root-bake (Z-up) is being applied too early in
BoneProcessor::processBones which mutates the skeleton before
AnimationProcessor::processAnimations reads T-pose; either move the Z-up bake to
run after AnimationProcessor::processAnimations() but before capturing the final
binding pose, or instead apply the inverse rotation to raw root keys inside
AnimationProcessor so their deltas are computed against the original FBX basis;
specifically, stop calling BoneProcessor::processBones(...) with isZup &&
!animationOnly before animations are processed and re-invoke the bake afterward
(or rotate root keyframes by R_x(+90°) prior to converting them to Ogre deltas)
so AnimationProcessor uses the correct T-pose basis.
In `@src/mainwindow.cpp`:
- Around line 720-725: The current branch only inspects errMsg but must treat
AnimationMerger::mergeAnimations()'s return value as the real success indicator:
capture the returned merged entity (result of mergeAnimations()), and if that
pointer is null treat it as a failure and show the QMessageBox::warning (using
errMsg or a fallback message); only when the returned merged entity is non-null
proceed to re-select (SelectionSet::getSingleton()->append(baseEntity)) so the
inspector refreshes correctly.
---
Nitpick comments:
In `@src/AnimationMerger.cpp`:
- Around line 10-20: s_skeletonUpAxis is an unsynchronized global QMap accessed
by registerSkeletonUpAxis and lookupUpAxis; protect concurrent access by adding
a shared/read-write lock (e.g., QReadWriteLock or std::shared_mutex) around all
reads and writes: acquire write lock in AnimationMerger::registerSkeletonUpAxis
when modifying s_skeletonUpAxis and acquire read lock in lookupUpAxis when
reading from s_skeletonUpAxis (preserve the default value behavior). Ensure the
lock object is also a static global alongside s_skeletonUpAxis so both functions
use the same lock.
🪄 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: 759410d6-bec5-4443-b826-5a6dfe6cd624
📒 Files selected for processing (13)
CMakeLists.txtdocs/index.htmlsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/AnimationMerger_test.cppsrc/Assimp/AnimationProcessor.cppsrc/Assimp/BoneProcessor.cppsrc/Assimp/BoneProcessor.hsrc/Assimp/Importer.cppsrc/Assimp/MeshProcessor.cppsrc/Assimp/MeshProcessor.hsrc/MeshImporterExporter.cppsrc/mainwindow.cpp
| // Per-bone binding-pose orientation correction. | ||
| // AnimationProcessor stores translate/rotate keyframes in the source bone's | ||
| // local space (divided by the source bone's binding-pose orientation). | ||
| // If source and target have different binding poses for this bone, re-express | ||
| // the stored values in the target bone's local space: | ||
| // corrected = (q_dst⁻¹ * q_src) * stored | ||
| const Ogre::Quaternion q_src = srcSkel->getBone(srcHandle)->getOrientation(); | ||
| const Ogre::Quaternion q_dst = baseSkel->getBone(baseHandle)->getOrientation(); | ||
| const Ogre::Quaternion boneCorrection = q_src.Inverse() * q_dst; | ||
| const bool needsBoneCorrection = !boneCorrection.equals(Ogre::Quaternion::IDENTITY, Ogre::Radian(1e-4f)); | ||
|
|
||
| for (unsigned short k = 0; k < srcTrack->getNumKeyFrames(); ++k) | ||
| { | ||
| const auto* kf = srcTrack->getNodeKeyFrame(k); | ||
| auto* dstKf = dstTrack->createNodeKeyFrame(kf->getTime()); | ||
| dstKf->setTranslate(kf->getTranslate()); | ||
| dstKf->setRotation(kf->getRotation()); | ||
|
|
||
| Ogre::Vector3 t = kf->getTranslate(); | ||
| Ogre::Quaternion r = kf->getRotation(); | ||
|
|
||
| // Step 1: correct translation for binding-pose orientation mismatch. | ||
| // Translations are stored in bone-local space (AnimationProcessor divides by | ||
| // the source bone's binding-pose orientation). If the target bone has a | ||
| // different binding-pose orientation, re-express the vector in that space. | ||
| // Rotations do NOT need this correction — the target binding pose already | ||
| // provides the equivalent compensation via the scene-node/skeleton setup. | ||
| if (needsBoneCorrection) { | ||
| t = boneCorrection * t; | ||
| } | ||
| // Step 2: convert coordinate system (Z-up source → Y-up base). | ||
| // R_x(-90°) maps (x,y,z) → (x, z, -y): Z-up axis becomes Y-up. | ||
| // Rotations pass through unchanged — bone-local storage makes them | ||
| // self-consistent once the dest binding pose is baked to Y-up. | ||
| if (needsZupToYup) { | ||
| t = Ogre::Vector3(t.x, t.z, -t.y); | ||
| } | ||
|
|
||
| dstKf->setTranslate(t); | ||
| dstKf->setRotation(r); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any existing tests or usages that might validate this formula
rg -n "boneCorrection|q_src|q_dst" --type cpp -C3Repository: fernandotonon/QtMeshEditor
Length of output: 3230
🏁 Script executed:
#!/bin/bash
# Search for test files related to AnimationMerger
fd -e cpp -e h -path "*test*" | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Search for all AnimationMerger-related files and their sizes
fd -i animationmerger | xargs wc -lRepository: fernandotonon/QtMeshEditor
Length of output: 174
🏁 Script executed:
#!/bin/bash
# Look at the full context around line 114 to understand the function-level comment
sed -n '100,130p' src/AnimationMerger.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1926
🏁 Script executed:
#!/bin/bash
# Check if there's any validation or actual usage of the boneCorrection in the merge
sed -n '140,200p' src/AnimationMerger.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3340
🏁 Script executed:
#!/bin/bash
# Examine the test file for bone correction validation
head -100 src/AnimationMerger_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3751
🏁 Script executed:
#!/bin/bash
# Search for test cases related to bone correction or quaternion validation
rg -n "boneCorrection|mergeAnimationsByName|TEST|test" src/AnimationMerger_test.cpp | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 1019
🏁 Script executed:
#!/bin/bash
# Read the MergeAnimationsBasic test and related tests to understand validation
sed -n '136,220p' src/AnimationMerger_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3640
🏁 Script executed:
#!/bin/bash
# Also check the MergeAnimationsMixamoCleanup test which mentions "Mixamo" (likely Y-up with binding pose differences)
sed -n '219,270p' src/AnimationMerger_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2431
🏁 Script executed:
#!/bin/bash
# Check if there are any comments in the test file explaining bone correction validation
rg -n "binding.pose|correction|q_src|q_dst|orientation" src/AnimationMerger_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check if createTestSkeleton ever sets non-identity bone orientations
sed -n '70,100p' src/AnimationMerger_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1278
🏁 Script executed:
#!/bin/bash
# Search for any test that might set quaternion or orientation values
rg -n "setOrientation|Quaternion|R_x" src/AnimationMerger_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check if there are any integration tests or test FBX files mentioned
rg -n "\.fbx|test.*FBX|integration" src/AnimationMerger_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Double-check by reading the full function to see if there's any other context
sed -n '107,165p' src/AnimationMerger.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3481
Fix the quaternion order in the bone correction formula.
Line 157's comment states the correct formula: corrected = (q_dst⁻¹ * q_src) * stored. Line 114's docstring also correctly states: correction = q_dst.Inverse() * q_src. However, line 160 implements the inverse: boneCorrection = q_src.Inverse() * q_dst.
The stored values are in the source bone's local space (pre-multiplied by q_src⁻¹). Converting to the destination bone's local space requires: v_dest_local = q_dst⁻¹ * (q_src * v_stored) = (q_dst⁻¹ * q_src) * v_stored.
Change line 160 to:
const Ogre::Quaternion boneCorrection = q_dst.Inverse() * q_src;The current test suite does not catch this because all test skeletons use identity binding poses. This bug manifests only when merging animations between skeletons with different binding-pose orientations (e.g., Unreal Engine FBX with R_x(-90°) root merged into a Mixamo character). Recommend adding a test case with non-identity binding poses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AnimationMerger.cpp` around lines 152 - 189, The bone correction
quaternion is computed with the operands reversed; update the computation of
boneCorrection (currently using q_src.Inverse() * q_dst) to use the correct
order q_dst.Inverse() * q_src so that stored source-local vectors are
re-expressed into the destination bone-local space; change the assignment where
boneCorrection is created (refer to q_src, q_dst, boneCorrection, and
needsBoneCorrection) accordingly and add/adjust a unit test that uses
non-identity binding-pose orientations to catch regressions.
- mainwindow.cpp: check mergeAnimations() return value (not just errMsg) as the authoritative success signal, per Codex review suggestion - AnimationMerger.cpp: update boneCorrection comment to match actual formula (q_src⁻¹ * q_dst) used in code Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
BoneProcessor::processBones no longer takes an isZup parameter. Instead, a new BoneProcessor::bakeZupToYup() static method is called from Importer.cpp *after* AnimationProcessor::processAnimations() so that animation deltas are computed against the original (pre-bake) T-pose. Baking before animation processing caused a basis mismatch for Z-up FBX files with embedded animations: AnimationProcessor would read the already-rotated bone orientations as its T-pose reference while keyframe data was still in Z-up space, leading to axis-swapped root motion on playback. Addresses PR #239 review comment (Codex P1 / CodeRabbit major). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
1 participant
Summary
cleanAnimNoisenow strips "Unreal Take" (space variant) in addition to "unreal_take" (underscore). Animation names fall back to the source file name (e.g.mm_attack_03).deduplicateNameno longer preemptively strips_Nsuffixes.mm_attack_03staysmm_attack_03, notmm_attack. Stripping only happens when the name actually collides.modelNameis always derived from the file path rather thanscene->mName, which UE sets to generic values like "unreal_take".MergeAnimationsUnrealTakeCleanupandMergeAnimationsNumericSuffixPreserved.docs/index.htmlnow documents the Unreal Engine FBX workflow alongside the Mixamo workflow.Test plan
mm_attack_03)mm_attack_01,mm_attack_02, etc.)MergeAnimationsUnrealTakeCleanup,MergeAnimationsNumericSuffixPreserved🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores