Support animation-only FBX imports (e.g. Unreal Engine retargets)

[fernandotonon]

Summary

• Animation-only FBX files from Unreal Engine (e.g. retargeted animations with no mesh) previously failed with a silent ERROR::ASSIMP:: error. This PR makes them importable.
• Assimp importer: relaxes the AI_SCENE_FLAGS_INCOMPLETE rejection — a file with no meshes but with animations is now treated as an animation-only import (returns null MeshPtr instead of aborting).
• BoneProcessor: new processAnimationOnlyHierarchy() builds bones from animation channel node names when no mesh-bones exist, so AnimationProcessor can create proper node tracks.
• GUI: shows an informational message box listing the extracted animations and directing the user to use Merge Animations.
• CLI info: works for animation-only files — reports skeleton bone count and animation list (text + JSON).
• CLI anim --merge: animation-only files can now be used as merge sources; their skeletons are collected and passed to an extended AnimationMerger overload.
• AnimationMerger: new 4-arg overload accepts QList<SkeletonPtr> alongside entity sources for skeleton-only merge inputs.

Test plan

• qtmesh info MM_Attack_03.FBX → shows 89 bones, 1 animation ("Unreal Take", 1.667s)
• qtmesh info MM_Attack_03.FBX --json → valid JSON output
• GUI: drag MM_Attack_03.FBX into scene → informational dialog, no crash
• qtmesh anim base_mesh.fbx --merge MM_Attack_03.FBX -o out.fbx → merges animations
• Importing a normal skinned mesh (e.g. bandit.fbx) still works correctly
• All existing unit tests pass

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Import and merge animation-only skeleton files alongside entity sources; merge animations by name with optional renaming to avoid conflicts.
  • CLI/mesh info now reports scene UpAxis and lists bone names; GUI prompts let you merge imported skeleton animations into a selected mesh.

• Bug Fixes

  • More robust handling of animation-only or incomplete scenes; clearer error when no animations or skeletons are merged.

• Improvements

  • Auto Z-up correction on import; safer GUI startup with user-visible error dialogs.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds support for merging animations from standalone (animation-only) skeletons into a base entity via a new AnimationMerger overload and merge-by-bone-name; extends Assimp import and BoneProcessor to detect/create bones for animation-only scenes; threads animation-only skeletons through MeshImporterExporter, CLIPipeline, CLI, and main GUI merge flow.

Changes

Cohort / File(s)	Summary
AnimationMerger src/AnimationMerger.h, src/AnimationMerger.cpp	New overload mergeAnimations(..., const QList<Ogre::SkeletonPtr>& sourceSkeletons, ...); added mergeAnimationsByName that copies animations by bone-name remapping; revised areSkeletonsCompatible to validate bone-name and parent-name correspondence and bind-pose within tolerances; replaced _mergeSkeletonAnimations usage and added Step 1b to merge standalone skeletons with temporary dedupe-renaming.
Assimp import & bone handling src/Assimp/Importer.h, src/Assimp/Importer.cpp, src/Assimp/BoneProcessor.h, src/Assimp/BoneProcessor.cpp	Importer exposes getLoadedSkeleton() and getSceneUpAxis(), treats meshless-but-animated scenes as animation-only (early return for animation-only path), and reads FBX UpAxis metadata; BoneProcessor adds processAnimationOnlyHierarchy to create bones for nodes referenced by animation channels.
Mesh importer/exporter src/MeshImporterExporter.h, src/MeshImporterExporter.cpp	importer() signature extended with optional out-params outAnimOnlySkeletons and outUpAxis; when no mesh is produced, reports animation-only skeleton via out param and continues; applies UpAxis rotation correction for Z-up scenes.
CLI / Pipeline integration & tests src/CLIPipeline.h, src/CLIPipeline.cpp, src/CLIPipeline_test.cpp	MeshInfo gains upAxis and bones; formatters and cmd paths include upAxis and bone lists; cmdInfo/cmdAnim --merge collect and pass animation-only skeletons to AnimationMerger; test updated to expect skeleton.boneCount.
GUI integration src/mainwindow.cpp, src/main.cpp	MainWindow::importMeshs receives animOnlySkeletons and prompts to merge animation-only skeletons into a selected entity; main.cpp adds safer Windows console attach and wraps GUI startup in try/catch showing QMessageBox on exceptions.
Misc / Glue src/AnimationMerger.cpp (post-process), src/Assimp includes, src/MeshImporterExporter callers	Renamed/cleanup logic preserved for Mixamo noise; added #include <set> and small API adjustments to propagate new data across call sites.

Sequence Diagram

sequenceDiagram
    participant CLI as CLI Pipeline
    participant Importer as MeshImporterExporter
    participant Assimp as Assimp Importer
    participant BoneProc as BoneProcessor
    participant AnimMerger as AnimationMerger

    CLI->>Importer: importer(uri, flags, &outAnimOnlySkeletons, &outUpAxis)
    Importer->>Assimp: loadModel(uri)
    alt Scene has meshes
        Assimp->>Importer: return mesh (+ optional skeleton)
    else Animation-only scene
        Assimp->>BoneProc: identify animated node names
        BoneProc->>Assimp: processAnimationOnlyHierarchy(root) -> create bones
        Assimp-->>Importer: nullptr mesh, valid skeleton
    end
    Importer-->>CLI: append skeleton to outAnimOnlySkeletons (if any)
    CLI->>AnimMerger: mergeAnimations(baseEntity, sourceEntities, animOnlySkeletons)
    AnimMerger->>AnimMerger: merge entity animations (track remap by bone name)
    AnimMerger->>AnimMerger: merge skeleton animations by name (Step 1b, rename/dedupe)
    AnimMerger->>AnimMerger: post-process renames/cleanup (Step 2)
    AnimMerger-->>CLI: merged entity (or error)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Add CLI pipeline (qtmesh) for headless mesh operations #178: Overlaps changes to AnimationMerger and the animation/skeleton merge flow.
• Fix crash importing skinned meshes without animations (e.g. bandit.fbx) #237: Related Assimp importer and skeleton creation logic for animation-only scenes.
• Improve animation merge: prefix base, clean Mixamo, deduplicate #220: Shares animation renaming/deduplication and merge internals used by the new skeleton-merge steps.

Poem

🐰✨
I hop through bones and channel names,
I stitch their moves and tame the frames.
Temp names twirl, then settle clean,
One merged dance from many a scene.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main feature: support for animation-only FBX imports from Unreal Engine retargets.
Description check	✅ Passed	The description follows the template with Summary and Technical Details sections, clearly explaining the problem, solution, and test plan, though Technical Details could be more organized.

_{✏️ 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 fix/animation-only-fbx-import

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 8

🤖 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 237-245: The current merge still only calls
areSkeletonsCompatible() (which checks name mapping) and then builds
boneHandleMap via baseSkel->_buildMapBoneByName(srcSkel.get(), boneHandleMap);
extend the compatibility check to also validate each mapped bone's parent chain
and bind-pose (rest) transform before proceeding: for each entry in
boneHandleMap verify that the mapped parent names match (walk parent pointers or
compare getParent/getParentName on baseSkel and srcSkel bones) and that the
bind/rest poses are equivalent within a small tolerance (compare local bind
matrices/quaternions/translations for the bone in both skeletons); if any
mismatch is found, set errorMsg accordingly and return nullptr instead of
continuing to merge. Ensure these checks run after _buildMapBoneByName and
before any track remapping or animation merging.

In `@src/AnimationMerger.h`:
- Around line 21-28: The doc for the three-argument overload of mergeAnimations
incorrectly references optional sourceSkeletons; update the comment to state
this is a convenience wrapper that forwards an empty sourceSkeletons list to the
four-argument mergeAnimations overload (keep mention of the standalone
sourceSkeletons only in the 4-argument overload's comment), and ensure you
reference the three-arg signature static Ogre::Entity*
mergeAnimations(Ogre::Entity* baseEntity, const QList<Ogre::Entity*>&
sourceEntities, QString& errorMsg) and the four-arg overload so callers
understand the forwarding behavior.

In `@src/Assimp/BoneProcessor.cpp`:
- Around line 163-183: The current processAnimationOnlyHierarchy creates an
animated node before ensuring its ancestor chain exists, causing rooting errors;
modify BoneProcessor::processAnimationOnlyHierarchy so that when an animated
node is discovered (isAnimated && !isExistingBone) you first walk up
node->mParent until you hit an existing skeleton bone (using node->mParent and
skeleton->hasBone(node->mName.C_Str())), calling processNonSkinnedBone for each
missing ancestor in parent-to-child order to materialize the hierarchy, and only
then call processNonSkinnedBone on the animated node itself; ensure you still
handle the existing child logic in the for-loop afterwards.

In `@src/Assimp/Importer.cpp`:
- Around line 61-71: The code wrongly treats AI_SCENE_FLAGS_INCOMPLETE as
"animation-only": replace the animationOnly check that uses (scene->mFlags &
AI_SCENE_FLAGS_INCOMPLETE) in Importer.cpp with a real mesh-count check so we
only consider a scene animation-only when it actually has no meshes;
specifically, set animationOnly based on scene->mNumMeshes == 0 and keep the
subsequent HasAnimations() check unchanged (update the variable initialization
that defines animationOnly and remove reliance on AI_SCENE_FLAGS_INCOMPLETE).

In `@src/CLIPipeline.cpp`:
- Around line 886-889: The check that emits "Error: Base file has no
mesh/entity." is unreachable because cmdAnim() already returns earlier for an
empty base; move the empty-base handling from its current location into the
merge-specific path so the clearer merge-specific diagnostic is reachable:
locate the cmdAnim() flow and the block that handles merging (the branch that
performs the merge operation and references allEntities), remove the current
allEntities.isEmpty() check, and add an equivalent check inside the merge branch
to call err() << "Error: Base file has no mesh/entity." << Qt::endl and return
the same error code when merging an animation-only base file.
- Around line 468-479: The loop that builds MeshInfo from animOnlySkeletons
dereferences every Ogre::SkeletonPtr (skel) and can crash if an entry is null;
update the loop in CLIPipeline.cpp where MeshInfo is constructed (referencing
MeshInfo, animOnlySkeletons and skel) to skip null skeleton pointers (check skel
for null/isNull() or falsy) before accessing getName(), getNumBones(), or
getAnimation(), so only valid skeletons are processed and null entries are
ignored (continue to next skel) to convert a null-pointer crash into a load
error path.

In `@src/MeshImporterExporter.cpp`:
- Around line 863-882: In MeshImporterExporter::importer(), when an imported
file yields no mesh (variable mesh is null) and a skeleton (skel from
importer.getLoadedSkeleton()) is handled (either appended to
outAnimOnlySkeletons or shown via QMessageBox), the code currently does
"return;" which aborts the entire batch; change that final "return;" to
"continue;" so the loop over URIs/files proceeds to the next file instead of
exiting the importer function; ensure this replacement occurs in the loop that
iterates import URIs so variables like mesh, skel, outAnimOnlySkeletons, file,
and importer.getLoadedSkeleton() remain valid for subsequent iterations.
- Around line 867-879: The importer currently shows a blocking QMessageBox when
it finds an animation-only skeleton (code around outAnimOnlySkeletons, skel,
getNumAnimations/getAnimation), which breaks headless callers; instead remove
the QMessageBox and ensure animation-only skeletons are returned to the caller:
make outAnimOnlySkeletons required (non-null) or alter the import API to return
or throw the skeleton data (e.g., return a vector of Skeleton* or throw an
exception containing the skeletons) so the caller receives the skel(s) and can
decide how to present UI; update the branch that currently appends animList to
either append skel to the returned container or propagate the skeletons via the
new return/exception mechanism and remove all QMessageBox usage from this code
path.

🪄 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: dfc52d96-baaf-466d-ac74-9f271dabfc5a

📥 Commits

Reviewing files that changed from the base of the PR and between df4cdac and ff9dc7e.

📒 Files selected for processing (11)

• CMakeLists.txt
• src/AnimationMerger.cpp
• src/AnimationMerger.h
• src/Assimp/BoneProcessor.cpp
• src/Assimp/BoneProcessor.h
• src/Assimp/Importer.cpp
• src/Assimp/Importer.h
• src/Assimp/MeshProcessor.cpp
• src/CLIPipeline.cpp
• src/MeshImporterExporter.cpp
• src/MeshImporterExporter.h

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/CLIPipeline.cpp (1)

910-913: ⚠️ Potential issue | 🟡 Minor

Merge count may be misleading for animation-only sources.

Line 912 reports allEntities.size() as the merge count, but animation-only skeletons are not counted. If a user merges a base file with 3 animation-only FBXs, the message would say "Merged 1 files" instead of "Merged 4 files" (or "3 animation sources").

🔧 Suggested fix: include skeleton count

-        cliWrite(QString("Merged %1 files -> %2\n").arg(allEntities.size()).arg(outFi.fileName()));
+        int sourceCount = (allEntities.size() - 1) + animOnlySkeletons.size();
+        cliWrite(QString("Merged %1 source(s) -> %2\n").arg(sourceCount).arg(outFi.fileName()));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/CLIPipeline.cpp` around lines 910 - 913, The merge summary uses
allEntities.size() which omits animation-only skeleton sources; update the final
message to include those skeleton counts by adding the number of detected
animation-only skeleton files to the reported total. Locate where animation-only
FBXs are collected (e.g., an array/vector like skeletons, animSkeletons, or
similar during processing), compute totalMerged = allEntities.size() +
animationOnlyCount, and change the cliWrite call to report totalMerged (and/or
separately report "X files + Y animation-only sources") instead of only
allEntities.size(), keeping outFi.fileName() as the output target.

🧹 Nitpick comments (1)

src/MeshImporterExporter.cpp (1)

869-879: QMessageBox usage deviates from coding guidelines.

As per coding guidelines, new UI should be built in QML (Qt Quick), not Qt Widgets. The QMessageBox::information here is a new addition for the animation-only workflow.

However, I acknowledge this is specifically for GUI drag-and-drop scenarios where outAnimOnlySkeletons is not provided, and the pattern is consistent with existing error dialogs in the codebase. Consider migrating to QML in a follow-up if the GUI is being modernized.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MeshImporterExporter.cpp` around lines 869 - 879, Replace the direct use
of QMessageBox::information (the widget-based dialog) with a platform-agnostic
signal so the UI layer (QML) can present the message; specifically, in the code
path that currently calls QMessageBox::information (and references
skel->getNumAnimations(), animList, file.fileName()), build the title and body
strings the same way, remove the QMessageBox::information call, and emit a new
signal such as animationOnlyImported(const QString& title, const QString& body)
from the MeshImporterExporter class (declare the signal in the class header).
The UI should subscribe to animationOnlyImported and present the message in QML;
this removes the QWidget dependency while preserving the exact message content
and the existing logic that constructs animList and counts from
skel->getAnimation().

🤖 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/Assimp/Importer.cpp`:
- Around line 113-115: Reset the skeleton member at the start of
AssimpToOgreImporter::loadModel() to avoid returning a stale skeleton across
calls: explicitly clear or reset the class member named skeleton (or whatever
container/type holds the loaded skeleton) before any early returns (e.g., before
the animationOnly early return) so getLoadedSkeleton() won't expose a previous
import's data when the new file has no bones/animations; ensure the reset
happens unconditionally at method entry.

---

Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Around line 910-913: The merge summary uses allEntities.size() which omits
animation-only skeleton sources; update the final message to include those
skeleton counts by adding the number of detected animation-only skeleton files
to the reported total. Locate where animation-only FBXs are collected (e.g., an
array/vector like skeletons, animSkeletons, or similar during processing),
compute totalMerged = allEntities.size() + animationOnlyCount, and change the
cliWrite call to report totalMerged (and/or separately report "X files + Y
animation-only sources") instead of only allEntities.size(), keeping
outFi.fileName() as the output target.

---

Nitpick comments:
In `@src/MeshImporterExporter.cpp`:
- Around line 869-879: Replace the direct use of QMessageBox::information (the
widget-based dialog) with a platform-agnostic signal so the UI layer (QML) can
present the message; specifically, in the code path that currently calls
QMessageBox::information (and references skel->getNumAnimations(), animList,
file.fileName()), build the title and body strings the same way, remove the
QMessageBox::information call, and emit a new signal such as
animationOnlyImported(const QString& title, const QString& body) from the
MeshImporterExporter class (declare the signal in the class header). The UI
should subscribe to animationOnlyImported and present the message in QML; this
removes the QWidget dependency while preserving the exact message content and
the existing logic that constructs animList and counts from
skel->getAnimation().

🪄 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: cbf6d564-828d-49ee-87ca-0eba25e4d5b6

📥 Commits

Reviewing files that changed from the base of the PR and between 57a2b7a and 8fe97f8.

📒 Files selected for processing (9)

• src/AnimationMerger.cpp
• src/AnimationMerger.h
• src/Assimp/BoneProcessor.cpp
• src/Assimp/BoneProcessor.h
• src/Assimp/Importer.cpp
• src/Assimp/Importer.h
• src/CLIPipeline.cpp
• src/MeshImporterExporter.cpp
• src/MeshImporterExporter.h

✅ Files skipped from review due to trivial changes (1)

• src/AnimationMerger.cpp

🚧 Files skipped from review as they are similar to previous changes (4)

• src/Assimp/Importer.h
• src/Assimp/BoneProcessor.h
• src/MeshImporterExporter.h
• src/AnimationMerger.h

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/CLIPipeline.cpp (1)

932-932: ⚠️ Potential issue | 🟡 Minor

Fix the merged-file count in the success message.

Line 932 uses allEntities.size(), which only counts loaded mesh entities. When the sources are animation-only skeletons, qtmesh anim --merge can report Merged 1 files after merging several inputs.

🛠️ Suggested tweak

-        cliWrite(QString("Merged %1 files -> %2\n").arg(allEntities.size()).arg(outFi.fileName()));
+        cliWrite(QString("Merged %1 source file(s) -> %2\n")
+            .arg(mergeFiles.size())
+            .arg(outFi.fileName()));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/CLIPipeline.cpp` at line 932, The success message uses allEntities.size()
(which counts loaded mesh entities) so animation-only skeleton merges report
wrong counts; update the cliWrite call to report the number of input files
actually merged by using the container that holds the input filenames (e.g.,
inputFiles.size() or the vector/list variable that stores the sources) instead
of allEntities.size(), keeping the rest of the message (outFi.fileName())
unchanged so the printed "Merged X files -> filename" reflects the number of
inputs merged even when no mesh entities were loaded.

♻️ Duplicate comments (2)

src/Assimp/Importer.cpp (1)

38-40: ⚠️ Potential issue | 🟡 Minor

Reset skeleton at the top of loadModel().

loadModel() now returns {} for both fatal failures and successful animation-only imports. If this importer instance gets reused, getLoadedSkeleton() can still expose the previous file's skeleton and make a failed load look like an animation-only success.

🛠️ Minimal fix

 Ogre::MeshPtr AssimpToOgreImporter::loadModel(const std::string& path, bool convertToLeftHanded, unsigned int additionalFlags) {
+    skeleton.reset();
     importer.SetPropertyBool(AI_CONFIG_IMPORT_FBX_PRESERVE_PIVOTS, false);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Assimp/Importer.cpp` around lines 38 - 40, In
AssimpToOgreImporter::loadModel reset the importer instance's skeleton at the
start of the function (e.g. clear or set member variable skeleton to an
empty/null state) so that getLoadedSkeleton() cannot return a skeleton from a
previous load when loadModel() returns {} for failures or animation-only
imports; locate the skeleton member and add the reset as the first statement in
loadModel() to ensure a fresh state for each call.

src/Assimp/BoneProcessor.cpp (1)

35-38: ⚠️ Potential issue | 🟠 Major

Don't drop keyed nodes' unanimated parent chain.

The new do NOT walk up shortcut is too aggressive here. When the first keyed node sits under a keyless parent, Line 175 creates it as a root bone, so its keys are applied in the wrong space during playback/merge. Please materialize the missing parents before the keyed node while still filtering out top-level wrapper nodes like Armature.

Also applies to: 174-176

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Assimp/BoneProcessor.cpp` around lines 35 - 38, Current shortcut that
"does NOT walk up" is too aggressive: when a keyed node's parent chain contains
unanimated parents the code creates the keyed node as a root bone (making its
keys apply in the wrong space). Change the bone-creation logic so that when
encountering a keyed node you walk up its parent chain and materialize missing
parent bones (create bones for unanimated parents) until you reach either an
animated parent or a top-level scene grouping node (filter by the same check
used for wrapper nodes, e.g. name == "Armature" or the isSceneGroupingNode
predicate), instead of immediately treating the keyed node as a root; update the
branch that currently shortcuts to root-creation so it creates the intermediate
parent bones first and then attaches the keyed node under them.

🤖 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/CLIPipeline.cpp`:
- Around line 267-268: The coordinate system is being omitted for the common
Y-up case in formatMeshInfoJson(): remove the conditional that skips emitting
the coordinate system when info.upAxis == 1 so that the code always writes the
coordinate system string/JSON field based on info.upAxis (handle 1 -> "Y-up", 2
-> "Z-up (Unreal Engine)", else -> "unknown"); update both the text output (the
s << "Coordinate system: ..." expression) and the corresponding JSON branch so
text and JSON output remain consistent for all upAxis values.

---

Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Line 932: The success message uses allEntities.size() (which counts loaded
mesh entities) so animation-only skeleton merges report wrong counts; update the
cliWrite call to report the number of input files actually merged by using the
container that holds the input filenames (e.g., inputFiles.size() or the
vector/list variable that stores the sources) instead of allEntities.size(),
keeping the rest of the message (outFi.fileName()) unchanged so the printed
"Merged X files -> filename" reflects the number of inputs merged even when no
mesh entities were loaded.

---

Duplicate comments:
In `@src/Assimp/BoneProcessor.cpp`:
- Around line 35-38: Current shortcut that "does NOT walk up" is too aggressive:
when a keyed node's parent chain contains unanimated parents the code creates
the keyed node as a root bone (making its keys apply in the wrong space). Change
the bone-creation logic so that when encountering a keyed node you walk up its
parent chain and materialize missing parent bones (create bones for unanimated
parents) until you reach either an animated parent or a top-level scene grouping
node (filter by the same check used for wrapper nodes, e.g. name == "Armature"
or the isSceneGroupingNode predicate), instead of immediately treating the keyed
node as a root; update the branch that currently shortcuts to root-creation so
it creates the intermediate parent bones first and then attaches the keyed node
under them.

In `@src/Assimp/Importer.cpp`:
- Around line 38-40: In AssimpToOgreImporter::loadModel reset the importer
instance's skeleton at the start of the function (e.g. clear or set member
variable skeleton to an empty/null state) so that getLoadedSkeleton() cannot
return a skeleton from a previous load when loadModel() returns {} for failures
or animation-only imports; locate the skeleton member and add the reset as the
first statement in loadModel() to ensure a fresh state for each call.

🪄 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: 1907c07a-dcca-440b-878e-19d7793ff01a

📥 Commits

Reviewing files that changed from the base of the PR and between 8fe97f8 and 681a015.

📒 Files selected for processing (9)

• src/AnimationMerger.cpp
• src/Assimp/BoneProcessor.cpp
• src/Assimp/Importer.cpp
• src/Assimp/Importer.h
• src/CLIPipeline.cpp
• src/CLIPipeline.h
• src/MeshImporterExporter.cpp
• src/MeshImporterExporter.h
• src/main.cpp

🚧 Files skipped from review as they are similar to previous changes (4)

• src/MeshImporterExporter.h
• src/Assimp/Importer.h
• src/AnimationMerger.cpp
• src/MeshImporterExporter.cpp

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/MeshImporterExporter.cpp (1)

842-864: ⚠️ Potential issue | 🟡 Minor

outUpAxis is only set for Assimp-imported files.

The outUpAxis parameter is only written in the Assimp code path (line 864). For .mesh and .xml files (lines 842-855), callers who pass a non-null outUpAxis pointer will read uninitialized or stale data.

Consider initializing *outUpAxis = 1 (Y-up default) at the start of the function or in the .mesh/.xml branches to ensure consistent behavior across all file formats.

🛡️ Suggested fix: Initialize outUpAxis for all formats

 void MeshImporterExporter::importer(const QStringList &_uriList, unsigned int additionalFlags,
                                      QList<Ogre::SkeletonPtr>* outAnimOnlySkeletons,
                                      int* outUpAxis)
 {
+    // Default to Y-up for non-FBX formats
+    if (outUpAxis) *outUpAxis = 1;
+
     try{
         foreach(const QString &fileName,_uriList)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/MeshImporterExporter.cpp` around lines 842 - 864, The outUpAxis output is
only written in the Assimp code path (via importer.getSceneUpAxis()) causing
uninitialized/stale results for .mesh and .xml imports; ensure outUpAxis is
always initialized by writing a default value (e.g. 1 for Y-up) either at the
start of the function or explicitly in the .mesh and .xml branches (around the
blocks using Manager::getSingleton(), importOgreXmlMesh, and the Assimp branch)
so any caller passing a non-null outUpAxis receives a defined value regardless
of format.

🧹 Nitpick comments (2)

src/mainwindow.cpp (1)

682-730: GUI layer correctly handles animation-only skeleton notifications.

This implementation properly:

• Moves UI notifications out of the shared importer API (making it headless-safe)
• Offers immediate merge when a compatible entity is selected
• Provides helpful guidance when no target is available

One consideration: After a successful merge (line 719), the animation tree/inspector may need refreshing. The existing on_actionMerge_Animations_triggered() doesn't explicitly refresh either, so this is consistent with current behavior, but you may want to emit a signal or call AnimationControlController::updateAnimationTree() to ensure the UI reflects the new animations.

💡 Optional: Refresh animation UI after merge

                 QString errMsg;
                 AnimationMerger::mergeAnimations(baseEntity, {}, {skel}, errMsg);
                 if (!errMsg.isEmpty())
                     QMessageBox::warning(this, "Merge failed", errMsg);
+                else
+                    AnimationControlController::instance()->updateAnimationTree();
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/mainwindow.cpp` around lines 682 - 730, The UI notifies and merges
animation-only skeletons but does not refresh the animation UI after a
successful merge; after the call to AnimationMerger::mergeAnimations(...) (and
only when errMsg is empty), invoke the animation refresh used elsewhere (e.g.,
call AnimationControlController::updateAnimationTree() or emit the same signal
that on_actionMerge_Animations_triggered() uses) so the animation tree/inspector
reflects the newly merged animations and any selection updates.

src/AnimationMerger.cpp (1)

306-350: Standalone skeleton merge modifies the source skeleton in-place.

The renameAnimation() calls at lines 342 and 346 modify srcSkel, but the parameter is const Ogre::SkeletonPtr&. Since SkeletonPtr is a shared pointer, the const-ref only prevents reassigning the pointer itself — the underlying skeleton is still mutable. This works but is semantically misleading.

If the caller expects their skeleton to remain unchanged after the merge, this could cause unexpected side effects. Consider either:

1. Documenting that source skeletons are modified during merge
2. Working on a clone if immutability is desired

This is a minor design consideration, not a blocking issue.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/AnimationMerger.cpp` around lines 306 - 350, The loop that merges
standalone skeletons modifies the source skeletons in-place via
renameAnimation(srcSkel.get(), ...), which is misleading because sourceSkeletons
are passed as const Ogre::SkeletonPtr&; either explicitly document that
AnimationMerger::merge (or the surrounding function) mutates source skeletons,
or avoid mutating them by operating on a clone: create a copy of srcSkel (e.g.,
via srcSkel->clone(...) or other Ogre copy mechanism) and run renameAnimation
and mergeAnimationsByName against that clone (update references to
srcTempToFinal, renameList, and mergeAnimationsByName to use the cloned
skeleton), ensuring existingNames and mergedCount semantics remain unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/MeshImporterExporter.cpp`:
- Around line 842-864: The outUpAxis output is only written in the Assimp code
path (via importer.getSceneUpAxis()) causing uninitialized/stale results for
.mesh and .xml imports; ensure outUpAxis is always initialized by writing a
default value (e.g. 1 for Y-up) either at the start of the function or
explicitly in the .mesh and .xml branches (around the blocks using
Manager::getSingleton(), importOgreXmlMesh, and the Assimp branch) so any caller
passing a non-null outUpAxis receives a defined value regardless of format.

---

Nitpick comments:
In `@src/AnimationMerger.cpp`:
- Around line 306-350: The loop that merges standalone skeletons modifies the
source skeletons in-place via renameAnimation(srcSkel.get(), ...), which is
misleading because sourceSkeletons are passed as const Ogre::SkeletonPtr&;
either explicitly document that AnimationMerger::merge (or the surrounding
function) mutates source skeletons, or avoid mutating them by operating on a
clone: create a copy of srcSkel (e.g., via srcSkel->clone(...) or other Ogre
copy mechanism) and run renameAnimation and mergeAnimationsByName against that
clone (update references to srcTempToFinal, renameList, and
mergeAnimationsByName to use the cloned skeleton), ensuring existingNames and
mergedCount semantics remain unchanged.

In `@src/mainwindow.cpp`:
- Around line 682-730: The UI notifies and merges animation-only skeletons but
does not refresh the animation UI after a successful merge; after the call to
AnimationMerger::mergeAnimations(...) (and only when errMsg is empty), invoke
the animation refresh used elsewhere (e.g., call
AnimationControlController::updateAnimationTree() or emit the same signal that
on_actionMerge_Animations_triggered() uses) so the animation tree/inspector
reflects the newly merged animations and any selection updates.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e314d26c-5bb9-420e-b294-81df3f599e60

📥 Commits

Reviewing files that changed from the base of the PR and between 681a015 and 00c3515.

📒 Files selected for processing (6)

• src/AnimationMerger.cpp
• src/Assimp/Importer.cpp
• src/CLIPipeline.cpp
• src/CLIPipeline_test.cpp
• src/MeshImporterExporter.cpp
• src/mainwindow.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• src/CLIPipeline.cpp

[sonarqubecloud]

Quality Gate passed

Issues
16 New issues
0 Accepted issues

Measures
0 Security Hotspots
37.0% Coverage on New Code
7.9% Duplication on New Code

See analysis details on SonarQube Cloud