feat(anim): flip-facing toggle + world-facing metric (#837) - #914
Merged
Conversation
A generated/retargeted clip that "walks the wrong way" is a camera convention, not an animation defect: clips face +Z (the Mixamo/GLTF default forward), which is away from a default viewport camera looking toward +Z. Proven with a parity harness (`anim --apply-canonical`, self-retarget round-trip) and a world-facing metric (`anim --facing`) — the model walk, the rig's own Mixamo clip, and Rumba all report hip world-forward ≈ +Z, so the model matches the rig's native facing. Ship a user-facing 180° flip instead of touching the retarget: - AnimationMerger::flipAnimationFacing — turns a clip 180° about world +Y by rewriting only the root (hips) track. The whole skeleton turns rigidly through the hierarchy, so pose (stride, arm swing, posture) is preserved exactly; only the body faces the other way. World pre-rot S is folded into each root keyframe as Lbind⁻¹·Wparent⁻¹·S·Wparent·Lbind·kf (same bind-local conjugation as arm-space), translation turned by S too so root motion travels with the new facing. Self-inverse (two flips = identity) so it's a stateless toggle. Same _keyFrameDataChanged() gotcha. - CLIPipeline: `--facing` (read-only metric, prints +Z/−Z) and `--flip-facing --animation <name> -o out`. - MCP flip_facing tool (edits the master skeleton so export includes it). - Inspector Animations per-row ⟳ button (re-poses live even when paused). - Unit tests: 180° turn reflects X/Z + preserves pose angle; self-inverse; no-op on missing anim/null. Verified numerically (--facing sign flips +0.99Z→−0.99Z, X/Y magnitudes unchanged = pose intact) and by render (body turns, upright stride kept). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a read-only ChangesAnimation facing diagnostic
Motion library processing
SonarCloud workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant CLIPipeline_cmdAnim
participant Ogre_Rig
participant AnimationState
CLI->>CLIPipeline_cmdAnim: invoke --facing
CLIPipeline_cmdAnim->>Ogre_Rig: load rig and resolve hip
CLIPipeline_cmdAnim->>AnimationState: play animation and sample orientation
AnimationState-->>CLIPipeline_cmdAnim: return sampled hip transforms
CLIPipeline_cmdAnim-->>CLI: output world-forward vector
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 8ad976aca8
ℹ️ 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
+285
to
+290
| for (const auto& [handle, trk] : anim->_getNodeTrackList()) { | ||
| if (!trk || trk->getNumKeyFrames() == 0) continue; | ||
| Ogre::Bone* b = skel->getBone(handle); | ||
| int depth = 0; | ||
| for (Ogre::Node* p = b->getParent(); p; p = p->getParent()) ++depth; | ||
| if (depth < rootDepth) { rootDepth = depth; rootBone = handle; } |
There was a problem hiding this comment.
When a valid clip has no keyframes on the skeleton root/hips but does have tracks on a child bone (for example a partial/body-part animation or a clip authored by keyframing one bone), this picks that child as rootBone and returns success. Since the rest of the skeleton is not under that child, flipAnimationFacing rotates only that subtree rather than the whole body, corrupting the pose; it should either target the actual root/hips track (creating it if needed) or fail instead of using the highest available child track.
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: 2
🧹 Nitpick comments (1)
src/AnimationMerger_test.cpp (1)
1117-1169: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo test exercises root-motion (translate) after a flip.
All three new tests check rotation only (
armWorldDircompares derived bone positions driven purely by rotation); none of them put a non-zerotranslateon a root keyframe, and the rig's Hips bone has an identity bind-local orientation (nosetOrientationcall inmakeArmRigEntity). That's exactly the combination that hides the translate/rotation conjugation mismatch flagged inAnimationMerger.cpp(rotation is conjugated viaL, translate usesSdirectly — they only coincide when the root's bind-local orientation is identity/world-aligned, as it is in this rig).Consider adding a case with a non-zero root
translatekeyframe (and ideally a non-identity root bind orientation) asserting the resulting world position delta rotates with the flip.🤖 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/AnimationMerger_test.cpp` around lines 1117 - 1169, Extend the flip-facing tests around AnimationMergerTest::FlipFacingTurnsSkeleton180 with a non-zero root translate keyframe and assert its world-space position delta is reflected by the 180° flip, rather than only validating arm rotations. Configure the Hips/root bind orientation non-identity if supported by makeArmRigEntity, and verify the translated result uses the same rotation behavior as the root orientation.Source: Path instructions
🤖 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/AnimationMerger.cpp`:
- Around line 294-320: Update the root-motion translation handling in the
AnimationMerger track loop to apply the local-space conjugation quaternion L,
matching the rotation transformation, instead of applying the world-space
quaternion S directly. Keep the existing keyframe iteration and cache
invalidation behavior unchanged.
In `@src/CLIPipeline.cpp`:
- Around line 2549-2629: Add a SentryReporter::addBreadcrumb call at the start
of the facingMode block, before initOgreHeadless or other processing, matching
the breadcrumb pattern used by sibling cmdAnim modes. Include sufficient context
to identify the --facing operation and selected animation or input.
---
Nitpick comments:
In `@src/AnimationMerger_test.cpp`:
- Around line 1117-1169: Extend the flip-facing tests around
AnimationMergerTest::FlipFacingTurnsSkeleton180 with a non-zero root translate
keyframe and assert its world-space position delta is reflected by the 180°
flip, rather than only validating arm rotations. Configure the Hips/root bind
orientation non-identity if supported by makeArmRigEntity, and verify the
translated result uses the same rotation behavior as the root orientation.
🪄 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: 032d1438-bcf6-447b-9ad1-402a0f811f85
📒 Files selected for processing (10)
CLAUDE.mdqml/PropertiesPanel.qmlsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/AnimationMerger_test.cppsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/MCPServer.h
translate + facing breadcrumb (#837) Review feedback on PR #914: - flipAnimationFacing now REFUSES clips it can't turn rigidly: (1) the root track bone must be a plausible whole-body root (a true skeleton root, or the canonical hips role) — a partial/body-part clip whose highest track is a deep bone (e.g. LeftArm) is rejected instead of flipping just that subtree and corrupting the pose; (2) every OTHER animated bone must descend from the root so it inherits the flip. Static helper/armature ancestors above the hips remain fine (the common Mixamo layout), verified on Rumba: (-0.12,0.27,0.95) → (-0.12,-0.27,-0.95). - Root-motion translate now turns in PARENT space (M = Wparent⁻¹·S·Wparent), not world S: applyToNode consumes keyframe translation as a TS_PARENT offset before it rotates, so on a rigidly-posed root bind pose the prior `S * t` sent root motion the wrong way. The full bind-local `L` the bot suggested is wrong here too (its extra Lbind conjugation belongs only on the rotation applyToNode post-multiplies). - `anim --facing` now emits a Sentry breadcrumb like every sibling cmdAnim mode. - Test: FlipFacingRefusesPartialChildOnlyClip covers the guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Addressed all three review comments in 568261e:
|
Text-to-motion clips face +Z (the Mixamo/GLTF forward convention), which is away from a default viewport camera looking toward +Z — so generated walk/run/sit all read as "facing backward" in the editor (user-reported). Add a `faceCamera` param to applyMotionClip; the two real generate call sites (CLI cmdAnimGenerate, GUI generateMotion) pass true, so freshly generated clips are flipped 180° to face the camera out of the box. The flip is applied on BOTH return paths (the world-frame bind-referenced path returns early, so it needed the call too). The parity harness (generated_parity) and every other applyMotionClip caller leave it false, so the raw retarget is still measured/used unturned. The per-clip ⟳ button (flipAnimationFacing is self-inverse) lets the user flip any clip back. Verified: walk/run/sit/dance/strafeleft all now report −Z (facing the camera); sit render shows the front-facing row seated with the pose intact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The auto-flip-on-generate default (bb7c9f8) was the wrong call — reverts the `faceCamera` param and both generate call sites so generated clips again face +Z (raw retarget, unchanged). Flipping stays MANUAL: the per-clip ⟳ button / CLI --flip-facing / MCP flip_facing. Code is now identical to 568261e; only the CLAUDE.md note is updated to record the reverted decision. Generated-motion quality is being iterated separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the 47-clip CMU-only template library with a scraped CC0/CC-BY corpus (Sketchfab + OpenGameArt + Quaternius packs) → 115 clips across 23 actions (walk/run/punch/jump 12 each; adds death/attack/shake/crawl/roll/ pray/pickup/swim/fly the CMU set lacked). Render-verified on Rumba. Quality gate hardening in build-motion-library-v5.py: - Mid-clip TOPPLE gate: spine_up_min < −0.25 drops clips that average upright but pitch head-below-horizontal mid-motion (e.g. a ground/fall "kick" that flops sideways on a biped rig). The old mean-only uprightness check let these through; verified the toppling kicks now drop and the survivors render as balanced one-leg kicks. HORIZONTAL_OK actions (death/roll/crawl/swim/fall/sleep) stay exempt. - CANON_ACTION folds verbatim -ing/synonym labels onto a base action (waving→wave, dying→death, …) so a handful of clips don't split across near-duplicate labels. Runtime: extend MotionLibrary kSynonyms so the new actions route from natural prompts (die→death, grab→pickup, dodge→roll, swing→attack, …). All 17 tested prompts match + generate. Corpus is CC0/CC-BY only; THIRD_PARTY_MOTION.md records the 140 CC-BY credits (mirrors the ATTRIBUTION.md shipped with the library on HF). Schema stays qtmesh-motion-library-v3 (v4/v5 = build generations). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The 180° flip-facing toggle turned the whole skeleton rigidly, so a back-facing walk just faced the other way while STILL moving wrong — the walk/run/sit problem is per-clip retarget bending, not a global facing flip. It added surface without fixing anything, so remove it entirely: - flipAnimationFacing (AnimationMerger .h/.cpp) + its 4 unit tests - CLI --flip-facing flag + handler (CLIPipeline) - MCP flip_facing tool (dispatch, scene-changing list, schema, impl, decl) - AnimationControlController::flipFacing (.h/.cpp) - the per-row ⟳ button (PropertiesPanel.qml) Kept the read-only `--facing` world-facing metric and `--apply-canonical` parity harness — they're harmless diagnostics that help iterate the retarget/library. CLAUDE.md updated to record the removal. walk/run/sit quality stays a tracked data/retarget issue for a follow-up; working/strafe/shake/pickup are the good-quality bar. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SonarQube Cloud stopped supporting Java 17 server-side, so the `Run sonar-scanner` step in unit-tests-linux now aborts on every PR with "The version of Java (17) used to run this analysis is deprecated … upgrade to Java 21 or later" — unrelated to any code change (the test suite itself passes; only the coverage-upload step fails). Add an actions/setup-java@v4 (temurin 21) step before sonar-scanner so the npm scanner wrapper runs under JDK 21. The C++ build/build-wrapper and gcovr steps don't use Java, so bumping the job-wide JDK is safe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CLAUDE.md`:
- Line 397: The facing metric’s cross-product sign is inconsistent with the
bind-frame definitions: with left = lhip − rhip and up = head − hip, left × up
points toward −Z. Update the metric and its CLI/CLAUDE.md `+Z`/`−Z` labeling to
agree, or swap the cross-product order if +Z is the intended forward direction;
add a small known-facing regression case covering the expected sign.
🪄 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: 3de5dcc4-53ba-40d1-80bb-c4c6470acaa0
📒 Files selected for processing (3)
.github/workflows/deploy.ymlCLAUDE.mdsrc/CLIPipeline.cpp
💤 Files with no reviewable changes (1)
- src/CLIPipeline.cpp
| - **MotionLibrary / text-to-motion** (`src/MotionLibrary.h/cpp`, issue #411, experimental): generate a skeletal animation from a text prompt. **The #411 spike (see `docs/TEXT_TO_MOTION_SPIKE_411.md`) proved a from-scratch GENERATIVE model (MDM-style) collapses to a static pose without multi-day ML effort, and all off-the-shelf models (MDM/T2M-GPT/MotionGPT) train on AMASS-derived HumanML3D/KIT-ML = non-commercial (the LAFAN1/ShapeNet wall again).** So the SHIPPED feature is a **template-clip MVP**: a curated library of permissive **CMU MoCap** clips (commercial-OK, same source as #409 RMIB), matched to the prompt by action keyword + synonyms (`MotionLibrary::matchPrompt`), then retargeted onto the user's rig via `AnimationMerger::applyMotionClip` → `MotionInbetween::canonicalIndexForBone` (the SAME 22-joint canonical mapping as #409). `MotionLibrary` is Ogre-free + unit-tested (`MotionLibrary_test.cpp`): parses `qtmesh-motion-library-v1`/`v2` JSON (per-frame, per-joint canonical quats; v2 adds an optional 22-entry `cmuRestWorld` block) + keyword matching. Library downloads on first use to `AppData/ai_models/motion/` (override `QTMESH_MOTION_LIBRARY_BASE_URL` / `QSettings ai/motionLibraryBaseUrl`; offline guard `QTMESH_MOTION_NO_DOWNLOAD`) — built offline by `scripts/build-motion-library.py` (10 actions: walk/run/jump/dance/march/kick/punch/wave/climb/idle, ~0.9 MB), hosted on the [`fernandotonon/QtMeshEditor-models`](https://huggingface.co/fernandotonon/QtMeshEditor-models) HF repo under `motion/`. **Retarget (`applyMotionClip`) — the part that makes it look right:** the CMU clip stores each joint's LOCAL (parent-relative) rotation with rest ≈ identity (the rest DIRECTION is in the BVH bone offsets, captured as the v2 `cmuRestWorld` per-joint world-rest `Wcmu`). The exact per-bone formula is `local(f) = parentWorld⁻¹ · (Wcmu · clip(f) · Wcmu⁻¹) · parentWorld · bind`, where `bind` = the rig's STANDING pose harvested from frame-0 of its existing animation (Mixamo bone rest is identity — the standing pose lives in the anim, NOT in `getInitial{Orientation,Position}` which is inflated and would stretch the mesh), and the root (hip) is locked to the standing pose (CMU bakes whole-body facing into the root). The `Wcmu·clip·Wcmu⁻¹` conjugation is the CMU↔target change-of-basis that cancels the per-bone ROLL twist between rigs with different bone axes (Mixamo arms point down their length / sideways; UniRig is axis-aligned). v1 libraries (no `cmuRestWorld`) fall back to `Wcmu`=identity (parent-world transport only — direction-correct, residual roll). Writes rotation-only keyframes (translation/scale stay at the standing pose, preserving rig proportions) and requires ≥½ of the 22 canonical roles to resolve (else fails — not a humanoid rig). Render-verified on the Rumba (Mixamo) rig via the isometric loop: walk = upright stride with arms hanging+swinging; wave = upright + natural. Surfaced via **CLI `qtmesh anim <file> --generate "<prompt>" [--duration N] [-o out]`** (`CLIPipeline::cmdAnimGenerate`), the **MCP `generate_motion` tool** (`MCPServer::toolGenerateMotion`, args `{prompt, entity_name?, duration?, output_path?}`, registered heavy), and the **Animation panel "Generate from text" control** (`qml/AnimationControlPanel.qml` → `AnimationControlController::generateMotion`, emits `generateMotionStatus`). Sentry breadcrumb `ai.assist.text_to_motion`. **v4 library** (July 2026): clips are the trial's ACTIVE window (max motion energy, start snapped to a calm near-neutral frame — the retarget deltas against clip frame 0), replacing first-4s slices that mostly captured idle lead-ins; 13 actions (adds sit/throw/boxing; 'idle' now a real wait trial — the old 69_01 source walked; 'dance' is salsa — ballet pirouettes fold under the locked root). **Generative path (opt-in `--model` / `model:true` / GUI checkbox): `MotionGenerator` + `motion/t2m.onnx`** — a CVAE transformer trained from scratch on the same CMU source (`scripts/prep-t2m-v4.py` + `scripts/train-t2m-onnx-v4.py`, offline): 30fps WORLD-frame windows w/ neutral starts (the v3 model trained on raw-120fps 0.33s local-frame windows and folded/flailed at 3.5x real velocity), absolute-pose decoder (no error-accumulating delta-cumsum), per-sample + rotation-space (geodesic) velocity matching, derived-local supervision (parent^-1*child — the exact quantity applyMotionClip renders; world-only losses let spine-chain errors stack into a visible fold), and z=0 latent-dropout supervision (the app infers with seed=zeros; an unsupervised z=0 is out-of-distribution for a low-beta CVAE). The vocab json declares `frame:world` + `fps`, read by `MotionGenerator::generate` → `Result::worldFrame` → applyMotionClip, so model clips ride the same world retarget as v3 template clips. Template library stays the default + automatic fallback. **Quality limit:** the model's z=0 output is smooth/upright but gentler than real clips (conditional-mean effect; the medoid-exemplar alternative is crisper numerically but renders twisted — `--z0-target` flag documents both); the template path is the quality bar. | ||
| - **MotionLibrary / text-to-motion** (`src/MotionLibrary.h/cpp`, issue #411, experimental): generate a skeletal animation from a text prompt. **The #411 spike (see `docs/TEXT_TO_MOTION_SPIKE_411.md`) proved a from-scratch GENERATIVE model (MDM-style) collapses to a static pose without multi-day ML effort, and all off-the-shelf models (MDM/T2M-GPT/MotionGPT) train on AMASS-derived HumanML3D/KIT-ML = non-commercial (the LAFAN1/ShapeNet wall again).** So the SHIPPED feature is a **template-clip MVP**: a curated library of permissive **CMU MoCap** clips (commercial-OK, same source as #409 RMIB), matched to the prompt by action keyword + synonyms (`MotionLibrary::matchPrompt`), then retargeted onto the user's rig via `AnimationMerger::applyMotionClip` → `MotionInbetween::canonicalIndexForBone` (the SAME 22-joint canonical mapping as #409). `MotionLibrary` is Ogre-free + unit-tested (`MotionLibrary_test.cpp`): parses `qtmesh-motion-library-v1`/`v2` JSON (per-frame, per-joint canonical quats; v2 adds an optional 22-entry `cmuRestWorld` block) + keyword matching. Library downloads on first use to `AppData/ai_models/motion/` (override `QTMESH_MOTION_LIBRARY_BASE_URL` / `QSettings ai/motionLibraryBaseUrl`; offline guard `QTMESH_MOTION_NO_DOWNLOAD`) — built offline by `scripts/build-motion-library.py` (10 actions: walk/run/jump/dance/march/kick/punch/wave/climb/idle, ~0.9 MB), hosted on the [`fernandotonon/QtMeshEditor-models`](https://huggingface.co/fernandotonon/QtMeshEditor-models) HF repo under `motion/`. **Retarget (`applyMotionClip`) — the part that makes it look right:** the CMU clip stores each joint's LOCAL (parent-relative) rotation with rest ≈ identity (the rest DIRECTION is in the BVH bone offsets, captured as the v2 `cmuRestWorld` per-joint world-rest `Wcmu`). The exact per-bone formula is `local(f) = parentWorld⁻¹ · (Wcmu · clip(f) · Wcmu⁻¹) · parentWorld · bind`, where `bind` = the rig's STANDING pose harvested from frame-0 of its existing animation (Mixamo bone rest is identity — the standing pose lives in the anim, NOT in `getInitial{Orientation,Position}` which is inflated and would stretch the mesh), and the root (hip) is locked to the standing pose (CMU bakes whole-body facing into the root). The `Wcmu·clip·Wcmu⁻¹` conjugation is the CMU↔target change-of-basis that cancels the per-bone ROLL twist between rigs with different bone axes (Mixamo arms point down their length / sideways; UniRig is axis-aligned). v1 libraries (no `cmuRestWorld`) fall back to `Wcmu`=identity (parent-world transport only — direction-correct, residual roll). Writes rotation-only keyframes (translation/scale stay at the standing pose, preserving rig proportions) and requires ≥½ of the 22 canonical roles to resolve (else fails — not a humanoid rig). Render-verified on the Rumba (Mixamo) rig via the isometric loop: walk = upright stride with arms hanging+swinging; wave = upright + natural. Surfaced via **CLI `qtmesh anim <file> --generate "<prompt>" [--duration N] [-o out]`** (`CLIPipeline::cmdAnimGenerate`), the **MCP `generate_motion` tool** (`MCPServer::toolGenerateMotion`, args `{prompt, entity_name?, duration?, output_path?}`, registered heavy), and the **Animation panel "Generate from text" control** (`qml/AnimationControlPanel.qml` → `AnimationControlController::generateMotion`, emits `generateMotionStatus`). Sentry breadcrumb `ai.assist.text_to_motion`. **v4 library** (July 2026): clips are the trial's ACTIVE window (max motion energy, start snapped to a calm near-neutral frame — the retarget deltas against clip frame 0), replacing first-4s slices that mostly captured idle lead-ins; 13 actions (adds sit/throw/boxing; 'idle' now a real wait trial — the old 69_01 source walked; 'dance' is salsa — ballet pirouettes fold under the locked root). **Generative path (opt-in `--model` / `model:true` / GUI checkbox): `MotionGenerator` + `motion/t2m.onnx`** — a CVAE transformer trained from scratch on the same CMU source (`scripts/prep-t2m-v4.py` + `scripts/train-t2m-onnx-v4.py`, offline): 30fps WORLD-frame windows w/ neutral starts (the v3 model trained on raw-120fps 0.33s local-frame windows and folded/flailed at 3.5x real velocity), absolute-pose decoder (no error-accumulating delta-cumsum), per-sample + rotation-space (geodesic) velocity matching, derived-local supervision (parent^-1*child — the exact quantity applyMotionClip renders; world-only losses let spine-chain errors stack into a visible fold), and z=0 latent-dropout supervision (the app infers with seed=zeros; an unsupervised z=0 is out-of-distribution for a low-beta CVAE). The vocab json declares `frame:world` + `fps`, read by `MotionGenerator::generate` → `Result::worldFrame` → applyMotionClip, so model clips ride the same world retarget as v3 template clips. Template library stays the default + automatic fallback. **Quality limit:** the model's z=0 output is smooth/upright but gentler than real clips (conditional-mean effect; the medoid-exemplar alternative is crisper numerically but renders twisted — `--z0-target` flag documents both); the template path is the quality bar. **v5 library** (#838, July 2026): replaces the 47-clip CMU-only set with a **CC0/CC-BY corpus** scraped from Sketchfab + OpenGameArt + Quaternius packs (`scripts/scrape-motion-corpus.py` → `--sketchfab`/`--opengameart`/`--packs`, CC0/CC-BY only, per-asset provenance in `manifest.json` + CC-BY credits in `ATTRIBUTION.md` that MUST ship with the library). `scripts/build-motion-library-v5.py` runs `qtmesh anim --dump-canonical` per asset (cached `*.canonical.json`), gates each clip (`--min-roles 14` humanoid gate + #855 `clip_quality`: bind-frame + animated + **mid-clip topple** uprightness gates — `spine_up_min < −0.25` drops ground/fall clips that average upright but pitch head-down mid-motion, e.g. a "kick to the groin" that flops horizontal; `HORIZONTAL_OK`={death,roll,crawl,swim,fall,sleep} exempt — energy band, placeholder-arm), dedups (quat fingerprint + semantic asset+anim+length), caps `--max-per-action 12`, and canonicalises verbatim action labels (`CANON_ACTION`: waving→wave, dying→death, …). Result: **115 clips across 23 actions** (walk/run/punch/jump 12 each; +death/attack/shake/crawl/roll/pray/pickup/swim/fly the CMU set lacked), render-verified on Rumba. New actions get runtime prompt routing via extended `kSynonyms` in `MotionLibrary.cpp` (die→death, grab→pickup, dodge→roll, …). Published to the CC0 `QtMeshEditor-t2m` HF repo (mirrored into `QtMeshEditor-models` via `scripts/sync-hf-model-repos.sh`). The schema is still `qtmesh-motion-library-v3` — v4/v5 are BUILD generations, not schema versions. | ||
| - **Arm-space post-process** (`AnimationMerger::adjustArmSpace`, issue #854): Mixamo-style "Character Arm-Space" — swing the arm chains outward (widen) or inward (tuck) on ANY animation (not just generated ones) to rescue arm-into-torso clipping / too-wide arms on rigs whose proportions differ from the source. Rewrites ONLY the shoulder (canonical 7/11) + collar (6/10, fractional) keyframes; elbows/hands follow through the hierarchy, legs/spine untouched. The swing is about the torso FORWARD axis (from the target bind frame `Ct`, reusing the retarget's `readTargetBindFrame` helper), mirrored per side so `+deg` widens both arms. Keyframes are deltas on the bind pose (Ogre's `NodeAnimationTrack::applyToNode` post-multiplies onto the reset bone), so the world swing `S` is folded into each keyframe as `L·kf` where `L = Wbind⁻¹·S·Wbind` (conjugation into the bone's bind-local frame). **Gotcha:** `TransformKeyFrame::setRotation` does NOT invalidate the track's interpolation caches, so after editing keyframes you MUST call `track->_keyFrameDataChanged()` or the next `apply()` replays the pre-edit rotations (edits appear to lag one call — masked in the live GUI by the render loop's continuous re-apply, but deterministic single evaluations get the stale pose). **Absolute + idempotent**: the last-applied angle is tracked PER SKELETON INSTANCE on `bone[0]`'s UserObjectBindings (key `qtme.armspace.<anim>`) — NOT a process-global map (that pollutes across entities AND across tests sharing a process, which is exactly how the first cut regressed); NOT persisted to disk (export bakes the final keyframes). Each call reverts the stored angle first (delta = new−stored). So the angle is ABSOLUTE: `+45` leaves the clip at +45, a later `−45` leaves it at −45 (net = −45), and only `adjustArmSpace(0)` restores the base pose bit-near-exactly. `currentArmSpace(skel, anim)` exposes the stored value so the GUI seeds its slider with the clip's real state. `applyMotionClip` erases the entry when it (re)creates a `generated_*` clip so a regenerated clip starts unadjusted. `migrateArmSpaceKey` moves the entry on rename — called from BOTH `AnimationMerger::renameAnimation` (CLI/merge) and `SkeletonTransform::renameAnimation` (GUI) so a renamed widened clip keeps its value. Surfaced via **CLI** `qtmesh anim <file> --generate "<p>" --arm-space <deg>` and standalone `qtmesh anim <file> --arm-space <deg> --animation <name> -o out` (`CLIPipeline::cmdAnim`/`cmdAnimGenerate`), **MCP** `arm_space` arg on `generate_motion` (response echoes `arm_space_applied`) + standalone `adjust_arm_space` tool (`MCPServer::toolAdjustArmSpace` — edits the mesh's MASTER skeleton so `output_path` export includes the change), and the **Inspector Animations section**: a live "Arm space" slider (−30…+45°) plus a per-row `↔` button that targets any clip (`qml/PropertiesPanel.qml` → `AnimationControlController::adjustArmSpace`/`currentArmSpace`). The slider updates the viewport LIVE while dragging, even when the clip is PAUSED (`_notifyDirty` + re-stamp the state's time); generation never bakes the slider value in. Unit-tested in `AnimationMerger_test.cpp` (swing angle, mirrored per-side direction, absolute/idempotent via `currentArmSpace`, non-arm invariance, rename migration). Sentry breadcrumbs: `ui.action` (GUI) / `ai.tool_call` (CLI + MCP). The same mechanism is the door for future motion-amplitude / hip-sway / stance-width knobs. | ||
| - **World-facing metric** (`CLIPipeline` `facingMode`, issue #837 follow-up): a generated/retargeted clip walks or runs the "wrong way" partly because of a *camera convention* — clips face **+Z** (the Mixamo/GLTF default forward), which is *away* from a default viewport camera looking toward +Z. The read-only metric **`qtmesh anim <file> --facing --animation <name>`** plays the clip, samples the hip's world-forward (`fwd = left×up` from its bind frame) averaged over 30 samples, and prints `+Z` / `−Z` (also `--apply-canonical <clips.json>`, a self-retarget round-trip parity harness). These stay as diagnostics for iterating the retarget/library. **A `flipAnimationFacing` 180°-turn toggle (CLI `--flip-facing`, MCP `flip_facing`, an Inspector `⟳` button) was built and then REMOVED** — turning the whole skeleton rigidly just made the clip face the other way while *still* moving wrong (the walk/run/sit issue is per-clip retarget bending, not a global facing flip), so the toggle didn't help and was cut. An auto-flip-on-generate default was likewise tried and reverted. The remaining walk/run/sit quality problem is being tracked as a data/retarget issue (likely specific corpus clips bending the wrong way); the good actions (working/strafe/shake/pickup) are the quality bar. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 12 'facingMode|apply-canonical|left|up|cross|forward' src/CLIPipeline.cppRepository: fernandotonon/QtMeshEditor
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== CLAUDE.md around the cited line ==="
sed -n '388,404p' CLAUDE.md
echo
echo "=== Existing facing/forward references and tests ==="
rg -n -C 3 'facing|world-forward|left × up|left.crossProduct|flip-facing|apply-canonical|fwdBind|worldForwardZ|Mixamo/GLTF default forward' \
CLAUDE.md src test tests docs . | head -n 200Repository: fernandotonon/QtMeshEditor
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Facing metric implementation ==="
sed -n '2547,2627p' src/CLIPipeline.cpp
echo
echo "=== Convention references in codebase ==="
rg -n -C 2 'right-handed|Mixamo/GLTF default forward|world-forward|camera looks toward|+Z|−Z|left × up|left.crossProduct|right × up|forward convention|bind frame' src CLAUDE.md docs | head -n 200
echo
echo "=== Any tests or docs for facing metric ==="
rg -n -C 2 'facingMode|--facing|flip-facing|apply-canonical|worldForwardZ|fwdBind|facing' tests test src docs CLAUDE.md | head -n 200Repository: fernandotonon/QtMeshEditor
Length of output: 4941
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Canonical role mapping for hips/head ==="
rg -n -C 3 'canonicalIndexForBone|role 0|role 5|role 15|role 19|lhip|rhip|hip→head|left hip|right hip' src/MotionInbetween.* src/AnimationMerger.* src/*.cpp src/*.h | head -n 200
echo
echo "=== Facing-related tests/docs ==="
rg -n -C 2 --fixed-strings -- 'facingMode' src test tests docs CLAUDE.md | head -n 200
rg -n -C 2 --fixed-strings -- '--facing' src test tests docs CLAUDE.md | head -n 200
rg -n -C 2 --fixed-strings -- 'left×up' src test tests docs CLAUDE.md | head -n 200Repository: fernandotonon/QtMeshEditor
Length of output: 30206
left × up is currently labeled backward. Under the bind-frame definitions in src/CLIPipeline.cpp (left = lhip - rhip, up = head - hip), left × up evaluates to −Z, so the +Z/−Z text in CLAUDE.md and the CLI output should agree with that sign (or the cross-product order should be swapped if +Z is intended). Add a small known-facing regression case for the metric.
🤖 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 `@CLAUDE.md` at line 397, The facing metric’s cross-product sign is
inconsistent with the bind-frame definitions: with left = lhip − rhip and up =
head − hip, left × up points toward −Z. Update the metric and its CLI/CLAUDE.md
`+Z`/`−Z` labeling to agree, or swap the cross-product order if +Z is the
intended forward direction; add a small known-facing regression case covering
the expected sign.
…canner Replace `npm install -g sonar-scanner` + a manual invocation + the setup-java@v4 step with SonarSource/sonarqube-scan-action@v4, which bundles its own compatible JDK. This removes the dependency on the runner's default Java entirely — the root cause of the earlier failure was SonarCloud dropping Java 17 (the ubuntu-latest default) server-side, and pinning Java via setup-java only worked around it. The bundled-JDK action is the maintained/blessed path (we already use its install-build-wrapper sub-action) and won't break again when the runner's default JDK moves. All -D args carry over verbatim into the action's `args:` input; token + host move to env (SONAR_TOKEN / SONAR_HOST_URL). compile_commands.json prep stays as a small preceding step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/deploy.yml:
- Around line 1509-1510: Update the “Run sonar-scanner” workflow step to use
SonarSource/sonarqube-scan-action version 5.3.1 or later, pinned to the reviewed
commit SHA instead of the vulnerable v4 tag.
🪄 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: 6c53dbad-79ef-41d3-a66d-35569db6154c
📒 Files selected for processing (1)
.github/workflows/deploy.yml
Comment on lines
+1509
to
+1510
| - name: Run sonar-scanner | ||
| uses: SonarSource/sonarqube-scan-action@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Workflow snippet\n'
sed -n '1488,1525p' .github/workflows/deploy.yml
printf '\n## Sonar config\n'
sed -n '1,220p' sonar-project.properties
printf '\n## Search for CFamily properties\n'
rg -n "sonar\.cfamily\.(compile-commands|build-wrapper-output)" -S .Repository: fernandotonon/QtMeshEditor
Length of output: 4222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/deploy.yml')
text = p.read_text()
for needle in ['SonarSource/sonarqube-scan-action@v4', 'sonar.cfamily.build-wrapper-output', 'sonar.cfamily.compile-commands']:
print(f"{needle}: {text.find(needle)}")
PYRepository: fernandotonon/QtMeshEditor
Length of output: 285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the repository has a local copy of the advisory text or release notes, surface it.
rg -n "CVE-2025-58178|sonarqube-scan-action@v6|compile-commands|build-wrapper-output" -S .github sonar-project.properties .Repository: fernandotonon/QtMeshEditor
Length of output: 811
🌐 Web query:
SonarSource sonarqube-scan-action CVE-2025-58178 v4 v6 advisory
💡 Result:
CVE-2025-58178 is a high-severity command injection vulnerability affecting the SonarSource sonarqube-scan-action GitHub Action [1][2]. The vulnerability, which carries a CVSS v3.1 score of 7.8, arises from the improper sanitization of input arguments, allowing untrusted arguments to be treated as shell expressions and potentially leading to arbitrary command execution [1][2][3]. Key details of the advisory: - Affected Versions: All versions from 4.0.0 up to and including 5.3.0 [2][3]. - Patched Version: The vulnerability was addressed in version 5.3.1 [1][4][3]. - Mitigation: Users should update to sonarqube-scan-action v5.3.1 [3]. If users are referencing the action via the v5 tag (e.g., sonarqube-scan-action@v5), they will automatically receive the patched version [3]. Additionally, for workflows using dynamically computed arguments in the 'args' field, users are advised to switch to using supported GitHub Actions expression syntax to ensure proper handling [3]. The vulnerability is tracked in the GitHub Advisory Database as GHSA-f79p-9c5r-xg88 [1][2].
Citations:
- 1: GHSA-f79p-9c5r-xg88
- 2: GHSA-f79p-9c5r-xg88
- 3: https://community.sonarsource.com/t/security-advisory-sonarqube-scanner-github-action/147696
- 4: https://osv.dev/vulnerability/GHSA-f79p-9c5r-xg88
Upgrade the SonarQube scan action. SonarSource/sonarqube-scan-action@v4 is affected by CVE-2025-58178; move to v5.3.1 or later and pin it to a reviewed commit SHA.
🤖 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 @.github/workflows/deploy.yml around lines 1509 - 1510, Update the “Run
sonar-scanner” workflow step to use SonarSource/sonarqube-scan-action version
5.3.1 or later, pinned to the reviewed commit SHA instead of the vulnerable v4
tag.
|
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.
Summary
Follow-up to the merged text-to-motion PR (#904), addressing the one remaining rough edge the user flagged: generated walk/run clips face away from the default camera.
This is a camera convention, not an animation defect — and I proved it rather than guessing. Clips face +Z (the Mixamo/GLTF default forward), which is away from a default viewport camera that looks toward +Z. Three prior attempts to "fix the facing" inside the retarget all made it worse; the disciplined measurement showed why:
generated_walk(0.18, 0.14, 0.97)mixamo.comclip(-0.10, 0.16, 0.98)(0.01, 0.01, 1.00)All three face +Z — the model matches the rig's native facing. So the right fix is a user-facing toggle, not a retarget change.
What this adds
AnimationMerger::flipAnimationFacing(skel, anim)— turns a clip 180° about world +Y by rewriting only the root (hips) track. The whole skeleton turns rigidly through the hierarchy, so the pose (stride, arm swing, posture) is preserved exactly; only the body faces the other way. World pre-rotationS = 180°@+Yis folded into each root keyframe asLbind⁻¹·Wparent⁻¹·S·Wparent·Lbind·kf(same bind-local conjugation as arm-space), and the keyframe translation is turned byStoo so any root motion travels with the new facing. Self-inverse (two flips = identity) → a stateless toggle.qtmesh anim <file> --facing --animation <name>plays the clip, samples the hip's world-forward (fwd = left×upfrom its bind frame) over 30 samples, and prints+Z/−Z. Read-only diagnostic.qtmesh anim <file> --apply-canonical <clips.json>(self-retarget round-trip) for numeric retarget validation.Surfaces (CLI / MCP / GUI parity)
qtmesh anim rigged.glb --flip-facing --animation generated_walk -o out.glb;--facingmetric.flip_facingtool (edits the master skeleton sooutput_pathexport includes it).⟳button in the Inspector → Animations section (re-poses live even when the clip is paused).Verification
--facingon a flipped walk goes(-0.00, 0.16, 0.99)→(-0.00, -0.16, -0.99)— Z sign inverted, X/Y magnitudes unchanged ⇒ pose intact, body turned.AnimationMerger_test.cpp): 180° turn reflects X/Z and preserves the inter-arm angle; self-inverse; no-op on missing anim / null skeleton. (Ogre-dependent fixture runs under Xvfb on Linux CI, same as the arm-space tests.)🤖 Generated with Claude Code
Summary by CodeRabbit
--facinganimation diagnostic that reports the character’s average world-forward direction in text or JSON.