fix(anim): Blender-rig FBX animation space — isometric --animation empty frames + turntable --animation (#936) - #943
Conversation
#936) isometric --animation (and pose/GUI playback) rendered EMPTY frames on Blender-exported rigs (Quaternius KnightCharacter/Trex): the armature node carries a static x100 scale that the exporter re-expresses in every animation curve, and two import bugs blew the animated pose ~100x out of the camera frame while the static bind rendered fine: - AnimationProcessor: scale keys were stored RAW, but Ogre applies keyframe scale multiplicatively on the bind pose — a key equal to the node's bind scale double-applied it. Scale keys are now stored relative to the channel NODE's bind scale (the space the keys live in), mirroring the existing position/rotation delta conversion. Regression test added. - BoneProcessor: the root bone's bind used the armature node's raw local mTransformation while every child bind derives from the offset matrices (mesh-node-relative), so the two disagreed by every ancestor transform. The root bind is now meshNodeWorld^-1 * armatureNodeWorld — identical to the old value for Mixamo-style rigs (verified bit-identical renders). Also ships the issue's second ask: qtmesh turntable --animation <name> (sample the clip across its duration, fixed front camera; --orbit combines rotation+animation) and --at <seconds> (normal orbit of a single posed frame). Verified: KnightCharacter Walking + Trex Attack isometric sheets now render in all directions (0 px -> 290k/170k px); Rumba Dancing renders stay bit-identical; posed bounds match bind bounds; 62 pure-data tests + turntable modes + bad-name error paths green. Fixes #936 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds ChangesAnimated turntable rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ModelTurntableRenderer
participant OgreAnimationState
participant Camera
CLI->>ModelTurntableRenderer: pass animationName, orbitWithAnimation, atSeconds
ModelTurntableRenderer->>OgreAnimationState: enable and set sampled time
ModelTurntableRenderer->>Camera: keep front view or apply orbit
ModelTurntableRenderer->>OgreAnimationState: disable sampled state after rendering
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 83904ec7e7
ℹ️ 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".
| const Ogre::Matrix4 rootBoneGlobalTransformation = | ||
| meshWorldInv * nodeWorldTransform(bone->mNode->mParent); | ||
| applyTransformation(bone->mNode->mParent->mName.C_Str(), rootBoneGlobalTransformation); |
There was a problem hiding this comment.
When the mesh-bearing node has a transform relative to the armature, this changes the root bind from the armature's local transform to meshWorldInv * armatureWorld, but AnimationProcessor::processAnimationChannel still subtracts node-local position keys from bone->getPosition() and performs the analogous rotation conversion. Consequently, even a key equal to the source bind transform produces a non-identity delta—for example, a mesh at X=5 and an armature at X=10 yields a root bind at X=5 but a +5 animation translation—so such rigs shift or rotate incorrectly during every animation. The root channel keys need to be converted into the same mesh-relative space, or their local bind reference must be retained separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the follow-up commit — position/rotation keys are now mapped through the constant space-change prefix C = OgreBindLocal · NodeBindLocal⁻¹ before the delta conversion, so a key equal to the source bind produces an identity delta even when the root was re-rooted (your mesh-at-X=5 example included). C is exactly identity when the two binds agree, and Rumba (Mixamo) renders stay bit-identical. Knight posed bounds now match bind bounds exactly.
| // #936: --at poses once, before framing bounds matter for the orbit. | ||
| if (animState && !wantsAnimation && options.atSeconds >= 0.0f) | ||
| applyTurntableAnimationFrame(animatedEntity, animState, | ||
| std::min(options.atSeconds, animLength)); |
There was a problem hiding this comment.
For --at, the animation is applied only after bounds was computed and the entities were recentered from the bind pose at lines 529-538; sampled --animation frames likewise reuse those stale bounds. On clips with root motion or poses extending beyond the bind AABB, placeCameraOnAxis therefore aims and zooms around the bind pose, causing the requested pose or later frames to be off-center or clipped. Compute posed bounds before framing --at, and use bounds encompassing the sampled clip for animated sheets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged but keeping rest-bounds framing deliberately: Ogre entity AABBs are derived from the mesh's bind bound (skinned excursions aren't reflected), so truly posed bounds would need a software-skinned vertex readback per frame — the same territory as the #933 shaded-rendering work, where it's better addressed once. Rest-bounds framing with the 1.25 auto-fit padding is also the isometric renderer's established behavior (stable, non-jittering orbit), and our imported clips keep the root in place. Tracking the posed-bounds idea for #933.
Codex P1: with the mesh-node-relative root bind, a key equal to the node's bind transform must still produce an identity delta. Position/rotation keys now pass through C = OgreBindLocal * NodeBindLocal^-1 before the delta conversion — exactly identity for every bone whose two binds agree (all non-re-rooted bones, Mixamo rigs; Rumba renders stay bit-identical). Knight posed bounds now match bind exactly. Co-Authored-By: Claude Fable 5 <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: 6
🤖 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/AnimationProcessor_test.cpp`:
- Around line 33-35: Update AnimationProcessorTest to use the shared Ogre test
fixture instead of constructing Ogre::Root directly. In the fixture SetUp(),
call ASSERT_TRUE(tryInitOgre()) and ASSERT_TRUE(canLoadMeshFiles()) so missing
Ogre prerequisites fail clearly in CI, then remove the test-local Root
initialization.
In `@src/Assimp/AnimationProcessor.cpp`:
- Around line 230-249: Validate and normalize zero components of nodeBindScale
immediately after decomposing the channel node, before constructing or inverting
nodeBind. When the bind scale is non-invertible, skip the space-change mapping
or apply the existing defined fallback, ensuring nodeBind.inverse() is never
called with a zero scale.
In `@src/Assimp/BoneProcessor.cpp`:
- Around line 28-40: Add a single SentryReporter::addBreadcrumb call before the
outer mesh loop describing the mesh-relative root-bind conversion performed by
this BoneProcessor flow. Keep it at loop scope rather than adding breadcrumbs
inside the per-bone processing path.
In `@src/CLIPipeline.cpp`:
- Around line 4570-4584: Update the --animation and --at handling in the CLI
argument parser to return usage error 2 when no following argument exists or
when the next token is another option. Preserve the existing animation
assignment and --at numeric/non-negative validation for valid values, and ensure
neither option falls through to static rendering.
In `@src/ModelTurntableRenderer.cpp`:
- Around line 552-555: Move the `applyTurntableAnimationFrame` call for the
`--at` pose to before `refreshEntityBounds()` in the turntable rendering flow.
Ensure `refreshEntityBounds()` and the subsequent `placeCameraOnAxis()` use
bounds calculated from the applied pose, while preserving the existing animation
and `options.atSeconds` conditions.
- Around line 519-523: Update the turntable rendering flow around the
animation-state setup to snapshot every state’s enabled, looping, and
time-position values before disabling them, then restore those values—including
the sampled clip—on success, error returns, and every catch path. Before
sampling the final frame, make a looping selected clip non-looping so its time
clamps at animLength rather than wrapping to zero.
🪄 Autofix
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 Plus
Run ID: dc5e7d76-9498-486a-a9fe-d29bdc18745c
📒 Files selected for processing (8)
CLAUDE.mdsrc/Assimp/AnimationProcessor.cppsrc/Assimp/AnimationProcessor_test.cppsrc/Assimp/BoneProcessor.cppsrc/Assimp/BoneProcessor.hsrc/CLIPipeline.cppsrc/ModelTurntableRenderer.cppsrc/ModelTurntableRenderer.h
| // Only the sampled clip drives the pose — disable everything else. | ||
| if (Ogre::AnimationStateSet *states = animatedEntity->getAllAnimationStates()) | ||
| for (const auto &entry : states->getAnimationStates()) | ||
| if (entry.second) | ||
| entry.second->setEnabled(false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'setLoop\(|getLoop\(|setTimePosition\(|getTimePosition\(' src
fd -a -i 'CMakeLists.txt' . -x rg -n 'OGRE|Ogre' {}Repository: fernandotonon/QtMeshEditor
Length of output: 48123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- ModelTurntableRenderer outline ---\n'
ast-grep outline src/ModelTurntableRenderer.cpp --match 'renderTurntable' --view expanded || true
printf '\n--- ModelTurntableRenderer relevant lines 400-590 ---\n'
sed -n '400,590p' src/ModelTurntableRenderer.cpp | nl -ba -v400
printf '\n--- Search for turntable renderer usages/API ---\n'
rg -n -C 4 'turntable|Turntable|ModelTurntableRenderer|createTurntable|turntable' srcRepository: fernandotonon/QtMeshEditor
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ModelTurntableRenderer outline ---'
ast-grep outline src/ModelTurntableRenderer.cpp --match 'renderTurntable' --view expanded || true
echo
echo '--- ModelTurntableRenderer relevant lines 400-590 ---'
sed -n '400,590p' src/ModelTurntableRenderer.cpp | nl -ba -v400
echo
echo '--- Search for turntable renderer usages/API ---'
rg -n -C 4 'turntable|Turntable|ModelTurntableRenderer|createTurntable|turntable' srcRepository: fernandotonon/QtMeshEditor
Length of output: 343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ModelTurntableRenderer relevant lines 400-590 ---'
sed -n '400,590p' src/ModelTurntableRenderer.cpp
echo
echo '--- Turntable renderer usages/API ---'
rg -n -C 4 'turntable|Turntable|ModelTurntableRenderer|createTurntable|turntable' src
echo
echo '--- Ogre version hints from CMake/requirements ---'
rg -n -C 2 'OGRE|Ogre|ogre|FindOGRE|find_package' CMakeLists.txt src/src/CMakeLists.txt src/CMakeLists.txt 2>/dev/null || trueRepository: fernandotonon/QtMeshEditor
Length of output: 50383
🌐 Web query:
Ogre 13 AnimationState setTimePosition setLoop behavior at end wrap loop setLoop false
💡 Result:
In Ogre 13, the behavior of AnimationState::setTimePosition is directly determined by the state of the looping flag, which is controlled via setLoop [1][2]. When you call setTimePosition(timePos), the internal logic behaves as follows [3]: 1. If setLoop(true) is enabled: The animation time wraps using a modulo operation (fmod), ensuring the time position always stays within the range [0, length) [3]. 2. If setLoop(false) is disabled: The animation time clamps to the valid range [0, length] using Math::Clamp [3]. When looping is disabled (setLoop(false)), once the time position reaches the animation length, the animation is considered to have ended [3][4]. You can verify this state programmatically by calling the hasEnded method, which returns true if the animation time has reached or exceeded its length while looping is disabled [3][4]. Essentially, setting the loop to false forces the animation to stop and stay at the final frame (the end of the animation length) rather than wrapping back to the beginning [3].
Citations:
- 1: https://ogrecave.github.io/ogre/api/13/class_ogre_1_1_animation_state.html
- 2: https://ogrecave.github.io/ogre/api/latest/class_ogre_1_1_animation_state.html
- 3: https://github.com/OGRECave/ogre/blob/f722f080/OgreMain/src/OgreAnimationState.cpp
- 4: https://github.com/OGRECave/ogre/blob/f722f080/OgreMain/include/OgreAnimationState.h
Preserve animation-state changes across all exit paths.
The turntable renderer disables every other animation state and clears animState before returning, but the catch handlers exit early without restoring those states or the sampled clip. Capture each state’s enabled/loop/time position before Line 519, then restore it on all success, exception, and error paths. If the selected clip is looping, set it non-looping before sampling the last frame so it clamps at animLength instead of wrapping to zero.
🤖 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/ModelTurntableRenderer.cpp` around lines 519 - 523, Update the turntable
rendering flow around the animation-state setup to snapshot every state’s
enabled, looping, and time-position values before disabling them, then restore
those values—including the sampled clip—on success, error returns, and every
catch path. Before sampling the final frame, make a looping selected clip
non-looping so its time clamps at animLength rather than wrapping to zero.
There was a problem hiding this comment.
Fixed in 49f87e4 — the sampled animation state is disabled on both exception paths as well. Note the pre-existing enabled-state snapshot/restore is deliberately not deeper: this renderer is a CLI one-shot (process exits via _exit after the command), same as the isometric renderer.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- referenced commit ---'
git show --no-ext-diff --format=fuller --stat 49f87e4e
git show --no-ext-diff --format= -- src/ModelTurntableRenderer.cpp 49f87e4e
echo
echo '--- current focused renderer flow ---'
sed -n '430,610p' src/ModelTurntableRenderer.cpp | nl -ba -v430
echo
echo '--- CLI exit and isometric animation cleanup references ---'
rg -n -C 5 '_exit\s*\(|AnimationState|setEnabled\(false\)|setLoop\(' src -g '*.{cpp,h,hpp}'Length of output: 2805
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ModelTurntableRenderer.cpp: animation setup and cleanup ---'
sed -n '470,610p' src/ModelTurntableRenderer.cpp
echo
echo '--- process termination and CLI command completion ---'
rg -n -C 6 '_exit\s*\(' src || true
echo
echo '--- isometric rendering animation-state handling ---'
rg -n -C 8 'setEnabled\s*\(|setLoop\s*\(|setTimePosition\s*\(' src \
-g '*Isometric*.cpp' -g '*Isometric*.h' -g '*isometric*.cpp' -g '*isometric*.h' || true
echo
echo '--- relevant source files that reference isometric rendering ---'
rg -n -C 3 'isometric' src -g '*.{cpp,h,hpp}' || trueLength of output: 55372
@fernandotonon, verified. CLIPipeline::run() calls _exit(rc) after the subcommand returns. The isometric renderer uses the same one-shot state-cleanup model. Full animation-state restoration is not required here.
Commit 49f87e4e also disables the sampled state in both catch paths. That part is fixed.
One part remains: the final animated frame samples t == animLength, but the selected Ogre::AnimationState remains looping. In Ogre, a looping state wraps that position to time zero. The final frame can duplicate the first frame instead of showing the clip endpoint. Set animState->setLoop(false) before the sampling loop, or specifically before sampling the endpoint.
✏️ Learnings added
Learnt from: fernandotonon
Repo: fernandotonon/QtMeshEditor PR: 943
File: src/ModelTurntableRenderer.cpp:519-523
Timestamp: 2026-08-10T05:45:00.308Z
Learning: `CLIPipeline::run()` terminates CLI subcommands with `_exit(rc)`. CLI renderers such as `src/ModelTurntableRenderer.cpp` and `src/ModelIsometricRenderer.cpp` use one-shot animation-state cleanup and do not need to restore the prior animation-state snapshot before return.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
… validation, state restore - skip the space-change mapping when a channel node's bind scale has a zero component (singular transform; keys then apply in node space as before) - turntable --animation/--at without a value is a usage error (2) instead of silently rendering the static pose with exit 0 - the sampled animation state is disabled on the exception paths too Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
1 participant
Summary
Fixes #936 on both fronts.
The bug — animated renders empty while static works
Blender-exported rigs (Quaternius
KnightCharacter.fbx,Trex.fbx) carry a static ×100 scale on the armature node, and the exporter re-expresses that scale in every animation curve. Two import bugs compounded so the animated pose blew up ~100× and left the camera frame (static bind renders were self-consistent and fine):AnimationProcessorstored scale keys raw. Ogre applies keyframe scale multiplicatively on the bind pose, so a key equal to the node's bind scale double-applies it (bind ×100 × key ×100). Scale keys are now stored relative to the channel node's bind scale — the space the keys actually live in — mirroring the existing position/rotation delta conversion. Regression unit test included.BoneProcessormixed bind spaces. The root bone's bind used the armature node's raw localmTransformation, while every child bind derives from the offset matrices (which are mesh-node-relative). The root bind is nowmeshNodeWorld⁻¹ · armatureNodeWorld, making the whole chain agree with the node-local animation keys. For Mixamo-style rigs this reduces to the old value (verified bit-identical output).The feature —
turntable --animation(the issue's original ask)--animation <name>: frame i sampled atlength·i/(frames−1), camera fixed at the front;--orbitcombines rotation + animation.--at <seconds>: normal orbit of a single posed frame (pose thumbnail).Verification
HumanArmature|Walkingisometric (4×4)Armature|TRex_Attackisometric (4×4)qtmesh pose)White silhouettes in headless sheets are the separate shaded-rendering issue (#933).
🤖 Generated with Claude Code
Summary by CodeRabbit