-
Notifications
You must be signed in to change notification settings - Fork 1
Support Unreal Engine FBX imports: Z-up baking, clean animation names, preserve numeric suffixes #239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Unreal Engine FBX imports: Z-up baking, clean animation names, preserve numeric suffixes #239
Changes from all commits
206cd58
7221873
ccad9fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,15 +3,32 @@ | |
| #include <OgreSkeletonInstance.h> | ||
| #include <OgreAnimation.h> | ||
| #include <QSet> | ||
| #include <QMap> | ||
| #include <QRegularExpression> | ||
| #include <unordered_map> | ||
|
|
||
| // Remove common Mixamo noise from animation names before slugifying. | ||
| // Registry: skeleton name → up-axis (1=Y-up, 2=Z-up). | ||
| // Populated by AnimationMerger::registerSkeletonUpAxis() at import time. | ||
| static QMap<QString, int> s_skeletonUpAxis; | ||
|
Check failure on line 12 in src/AnimationMerger.cpp
|
||
|
|
||
| void AnimationMerger::registerSkeletonUpAxis(const std::string& name, int upAxis) { | ||
| s_skeletonUpAxis[QString::fromStdString(name)] = upAxis; | ||
| } | ||
|
|
||
| static int lookupUpAxis(const std::string& name) { | ||
| return s_skeletonUpAxis.value(QString::fromStdString(name), 1); // default Y-up | ||
| } | ||
|
|
||
| // Remove common noise tokens from animation names before slugifying. | ||
| // e.g. "Armature|mixamo.com|Layer0" → "Armature|Layer0" | ||
| static QString cleanMixamoNoise(const QString& name) | ||
| // e.g. "Armature|unreal_take|Layer0" → "Armature|Layer0" | ||
| // e.g. "Unreal Take" (UE FBX take name, space variant) → "" | ||
| static QString cleanAnimNoise(const QString& name) | ||
| { | ||
| QString s = name; | ||
| s.replace(QRegularExpression("\\|?mixamo\\.com\\|?"), "|"); | ||
| // Match both "unreal_take" (underscore) and "Unreal Take" (space) — UE FBX exports | ||
| s.replace(QRegularExpression("\\|?unreal[_ ]take\\|?", QRegularExpression::CaseInsensitiveOption), "|"); | ||
| s.replace(QRegularExpression("\\|\\|+"), "|"); | ||
| if (s.startsWith('|')) s.remove(0, 1); | ||
| if (s.endsWith('|')) s.chop(1); | ||
|
|
@@ -36,7 +53,7 @@ | |
| static QString buildAnimName(const QString& prefix, const QString& animName) | ||
| { | ||
| QString slugPrefix = slugify(prefix); | ||
| QString cleanAnim = cleanMixamoNoise(animName); | ||
| QString cleanAnim = cleanAnimNoise(animName); | ||
| QString slugAnim = slugify(cleanAnim); | ||
|
|
||
| // Both empty — shouldn't happen, but guard against it | ||
|
|
@@ -53,21 +70,24 @@ | |
| } | ||
|
|
||
| // Deduplicate a name against an existing set, appending _2, _3, etc. if needed. | ||
| // Strips any existing trailing _N suffix first so repeated merges produce | ||
| // test_jump, test_jump_2, test_jump_3 (not test_jump_2_2, test_jump_2_2_2). | ||
| // Intentional numeric suffixes (e.g. "mm_attack_03") are preserved when the | ||
| // name is unique. _N stripping only kicks in when the name already collides, | ||
| // so repeated merges produce jump, jump_2, jump_3 (not jump_2_2, jump_2_2_2). | ||
| static QString deduplicateName(const QString& desired, QSet<QString>& existingNames) | ||
| { | ||
| // Strip existing _N suffix to find the base name | ||
| // If the desired name is free, use it as-is (preserve intentional _N suffixes). | ||
| if (!existingNames.contains(desired)) { | ||
| existingNames.insert(desired); | ||
| return desired; | ||
| } | ||
|
|
||
| // Collision — strip any trailing _N to find the canonical base, then re-suffix. | ||
| QString baseName = desired; | ||
| QRegularExpression trailingSuffix("_(\\d+)$"); | ||
| auto match = trailingSuffix.match(baseName); | ||
| if (match.hasMatch()) | ||
| baseName = baseName.left(match.capturedStart()); | ||
|
|
||
| if (!existingNames.contains(baseName)) { | ||
| existingNames.insert(baseName); | ||
| return baseName; | ||
| } | ||
| int suffix = 2; | ||
| QString candidate; | ||
| do { | ||
|
|
@@ -81,8 +101,27 @@ | |
| // This bypasses Ogre's _mergeSkeletonAnimations hierarchy check, which rejects skeletons | ||
| // that are structurally compatible (same bone names) but differ in their root chain | ||
| // (e.g. a mesh skeleton wrapping 'root' under a mesh-name bone that the animation lacks). | ||
| static void mergeAnimationsByName(Ogre::Skeleton* baseSkel, const Ogre::Skeleton* srcSkel) | ||
| // | ||
| // srcUpAxis / baseUpAxis: coordinate-system up-axis (1=Y-up, 2=Z-up). | ||
| // When the source was exported from a Z-up tool (e.g. Unreal Engine) and the base | ||
| // skeleton is Y-up (e.g. Mixamo), needsZupToYup converts the keyframe data. | ||
| // | ||
| // Additionally, AnimationProcessor stores translations in bone-local space (pre-multiplied | ||
| // by the inverse binding-pose orientation). If the source and target skeletons have | ||
| // different binding-pose orientations for the same bone — e.g. an animation-only FBX has | ||
| // identity root while a mesh FBX has a baked R_x(+90°) root — the stored delta must be | ||
| // re-expressed in the target bone's local space. | ||
| // We apply: corrected = (q_src⁻¹ * q_dst) * stored | ||
| static void mergeAnimationsByName(Ogre::Skeleton* baseSkel, const Ogre::Skeleton* srcSkel, | ||
| int srcUpAxis = 1, int baseUpAxis = 1) | ||
| { | ||
| const bool needsZupToYup = (srcUpAxis == 2 && baseUpAxis == 1); | ||
|
|
||
| // Z-up (Assimp/FBX after ConvertToLeftHanded) → Y-up (Ogre) for translations: | ||
| // R_x(-90°): (x,y,z) → (x, z, -y) | ||
| // Rotations are stored in bone-local space and pass through unchanged — | ||
| // the baked dest bone orientation provides the correct world-space result. | ||
|
|
||
| // Build name→handle map for the base skeleton | ||
| std::unordered_map<std::string, unsigned short> baseHandleByName; | ||
| for (unsigned short i = 0; i < baseSkel->getNumBones(); ++i) | ||
|
|
@@ -110,12 +149,44 @@ | |
| dstTrack->setAssociatedNode(baseSkel->getBone(baseHandle)); | ||
| dstTrack->setUseShortestRotationPath(srcTrack->getUseShortestRotationPath()); | ||
|
|
||
| // 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) { | ||
|
Check failure on line 177 in src/AnimationMerger.cpp
|
||
| 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) { | ||
|
Check failure on line 184 in src/AnimationMerger.cpp
|
||
| t = Ogre::Vector3(t.x, t.z, -t.y); | ||
| } | ||
|
|
||
| dstKf->setTranslate(t); | ||
| dstKf->setRotation(r); | ||
|
Comment on lines
+152
to
+189
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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: 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: 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 |
||
| dstKf->setScale(kf->getScale()); | ||
| } | ||
| } | ||
|
|
@@ -268,7 +339,9 @@ | |
| for (const auto& [tempName, finalName] : srcTempToFinal) | ||
| renameAnimation(srcSkel.get(), tempName, finalName); | ||
|
|
||
| mergeAnimationsByName(baseSkel.get(), srcSkel.get()); | ||
| mergeAnimationsByName(baseSkel.get(), srcSkel.get(), | ||
| lookupUpAxis(srcSkel->getName()), | ||
| lookupUpAxis(baseSkel->getName())); | ||
| mergedCount += numAnims; | ||
| } | ||
|
|
||
|
|
@@ -314,7 +387,9 @@ | |
| for (const auto& [tempName, finalName] : srcTempToFinal) | ||
| renameAnimation(srcSkel.get(), tempName, finalName); | ||
|
|
||
| mergeAnimationsByName(baseSkel.get(), srcSkel.get()); | ||
| mergeAnimationsByName(baseSkel.get(), srcSkel.get(), | ||
| lookupUpAxis(srcSkel->getName()), | ||
| lookupUpAxis(baseSkel->getName())); | ||
| mergedCount += numAnims; | ||
| } | ||
|
|
||
|
|
@@ -349,12 +424,16 @@ | |
| // This was a base animation — prefix it (unless already prefixed) | ||
| QString origSlug = slugify(origQName); | ||
| if (origSlug.startsWith(baseSlug + "_") || origSlug == baseSlug) | ||
| desired = slugify(cleanMixamoNoise(origQName)); // already prefixed | ||
| desired = slugify(cleanAnimNoise(origQName)); // already prefixed | ||
| else | ||
| desired = buildAnimName(baseRawName, origQName); // needs prefix | ||
| } else { | ||
| // This was merged from a source — already has prefix, just clean | ||
| desired = slugify(cleanMixamoNoise(origQName)); | ||
| // This was merged from a source — already renamed in Step 1b, just clean. | ||
| desired = slugify(cleanAnimNoise(origQName)); | ||
| // If cleaning removes the entire name (e.g. "unreal_take" → ""), | ||
| // keep the already-processed name from Step 1b as-is. | ||
| if (desired.isEmpty()) | ||
| desired = slugify(origQName); | ||
| } | ||
|
|
||
| QString finalName = deduplicateName(desired, finalNames); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The translation-space remap in
mergeAnimationsByNamecomputesboneCorrectionasq_src.Inverse() * q_dst, but converting a vector from source bone-local space into destination bone-local space requiresq_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 👍 / 👎.