feat(anim): canonical clip extraction + motion-library v5 builder (t2m-v2 Slice B, #839)#843
feat(anim): canonical clip extraction + motion-library v5 builder (t2m-v2 Slice B, #839)#843fernandotonon wants to merge 8 commits into
Conversation
t2m-v2 Slice B — turns the harvested motion corpus (#838) into template library clips: - AnimationMerger::extractCanonicalClips: samples every skeletal animation at 30fps onto the 22-joint canonical skeleton as WORLD-frame quats (the v3 library convention), using the SAME bone-role matcher the retarget consumes. Scraped rigs live in arbitrary file frames (Blender FBX armatures are commonly Z-up), so each rig's frame is derived from its own BIND geometry (up=hip→head, left=rhip→lhip) and every quat conjugated into the canonical Y-up/+Z-forward frame — no per-format guessing. - CLI: `qtmesh anim <file> --dump-canonical out.json [--animation N]`. - scripts/build-motion-library-v5.py (offline dev tool): walks the corpus, dumps each validated asset, labels actions from normalized animation names (keyword table + verbatim single-word fallback that widens the prompt vocabulary), selects the highest-energy window snapped to a calm start frame, drops static poses, dedupes sibling characters semantically (same asset+animation+length), and emits the EXISTING qtmesh-motion-library-v3 schema — the shipped app consumes it unchanged — plus the corpus ATTRIBUTION.md. Validated end-to-end: 66 clips / 15 actions (4.3 MB) from the current corpus vs 47/15 in v4 — with new actions (roll, crawl, pickup, shoot, swim…); retargeted walk/roll render upright on the Mixamo test rig with quality matching the CMU baseline (the roll is the Knight's combat roll, tuck→tumble→recover). Unit tests: non-humanoid rigs return empty; a minimal Mixamo-named fixture asserts frame counts, 22-slot poses, identity in unresolved roles, and the ~90° world-frame head delta. Part of #837; implements #839 with the corpus published at https://huggingface.co/datasets/fernandotonon/QtMeshEditor-motion-corpus Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds canonical skeletal animation extraction, a ChangesCanonical motion pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MotionCorpus
participant BuildMotionLibrary
participant QtMeshCLI
participant AnimationMerger
BuildMotionLibrary->>MotionCorpus: discover animation assets and metadata
BuildMotionLibrary->>QtMeshCLI: dump canonical clips to temporary JSON
QtMeshCLI->>AnimationMerger: extract canonical clips from Ogre entity
AnimationMerger-->>QtMeshCLI: return sampled world-frame quaternions
QtMeshCLI-->>BuildMotionLibrary: write canonical clip JSON
BuildMotionLibrary->>BuildMotionLibrary: filter, window, deduplicate, and cap clips
BuildMotionLibrary-->>MotionCorpus: write motion-library.json and attribution
Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 2923f607bd
ℹ️ 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".
| if c.get("resolvedRoles", 0) < args.min_roles: | ||
| continue |
There was a problem hiding this comment.
This count-only filter accepts clips where canonical parents are missing, because the default threshold is only 12 roles. The dump fills unresolved roles with identity quats, but the retarget path reconstructs local motion from each joint's canonical parent (parent^-1 * child), so a rig that has an animated child such as an arm/leg but lacks its collar/buttock parent bakes ancestor motion into the child and corrupts the generated library. Since the v3 library has no per-role mask, reject clips unless every resolved child has its parent chain or synthesize those missing parent quats before appending.
Useful? React with 👍 / 👎.
|
|
||
| CANON_COUNT = 22 | ||
| FPS = 30 | ||
| MODEL_EXTS = (".glb", ".gltf", ".fbx", ".dae") |
There was a problem hiding this comment.
scrape-motion-corpus.py discovers and validates .blend models as corpus assets, but this builder omits .blend from the extensions it walks. Any asset whose only validated animated rig is a Blender file is silently skipped here, so running the documented scrape/build pipeline can lose those clips or even produce no clips for a source without an actionable error. Keep this list in sync with the scraper or drive it from the manifest validation record.
Useful? React with 👍 / 👎.
| def manifest_lookup(manifest, dirname): | ||
| for a in manifest.get("assets", []): | ||
| slug = re.sub(r"[^A-Za-z0-9._-]+", "_", a.get("title", "")).strip("_") | ||
| if dirname in (slug[:80],) or dirname in slug: |
There was a problem hiding this comment.
For Sketchfab assets the scraper names the directory with a title plus UID suffix, while the manifest title slug does not include that suffix; this condition only matches when the directory name equals or is contained in the shorter title slug. As a result manifest_lookup returns no provenance for Sketchfab downloads, dropping their tags and clean titles, and generic animation names that depend on search tags for labeling are skipped or deduped under UID-bearing slugs. Compare both directions or persist the harvested directory name in the manifest.
Useful? React with 👍 / 👎.
The state-set path (Skeleton::setAnimationState via the entity's AnimationStateSet) applied nothing for hand-built skeletons on the CI fixture — the head delta sampled 0°. Apply each Animation directly to the skeleton instance per frame instead: deterministic for both imported and hand-built rigs, no enabled-state bookkeeping, and the bind pose is restored after sampling. Output verified bit-identical to the previous path on real corpus files. 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: 1
🧹 Nitpick comments (4)
scripts/build-motion-library-v5.py (3)
183-183: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
withstatements for file reads to prevent resource leaks.
json.load(open(...))leaks the file handle. In CPython, reference counting closes it immediately, but this is not guaranteed on other implementations and can exhaust handles on Windows with large corpora.♻️ Proposed fix
if os.path.exists(mpath): - manifest = json.load(open(mpath)) + with open(mpath) as f: + manifest = json.load(f)- dump = json.load(open(tmp)) + with open(tmp) as f: + dump = json.load(f)Also applies to: 214-214
🤖 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 `@scripts/build-motion-library-v5.py` at line 183, Replace the direct open calls used by the manifest loads in the relevant code paths with with open(...) as ... context managers, then pass the managed file object to json.load; update both occurrences near the manifest-loading logic.
102-104: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAdd
strict=Truetozip()for a cheap safety guard.If two quaternion arrays ever have mismatched lengths,
zipsilently truncates. Addingstrict=True(Python 3.10+) turns that into a clear error.♻️ Proposed fix
d = abs(sum(x * y for x, y in zip(a, b, strict=True)))🤖 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 `@scripts/build-motion-library-v5.py` around lines 102 - 104, Update quat_angle to call zip(a, b, strict=True) when computing the quaternion dot product, ensuring mismatched array lengths raise an explicit error instead of being silently truncated.
215-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog a warning when a corpus file fails instead of silently continuing.
The bare
except Exception: continuehides qtmesh crashes, JSON decode errors, and IO failures. For an offline tool processing hundreds of files, this can silently skip valid assets and make debugging very difficult.♻️ Proposed fix
except Exception as exc: + print(f" ! skipped {fn}: {exc}", file=sys.stderr) continue🤖 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 `@scripts/build-motion-library-v5.py` around lines 215 - 216, In the corpus-file processing exception handler, replace the silent `except Exception: continue` with a warning log that includes the affected file path and exception details, then continue processing subsequent files. Use the surrounding corpus-processing function and its existing logger or warning mechanism to report failures consistently.src/AnimationMerger.cpp (1)
1020-1024: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrecompute
C.Inverse()outside the per-joint loop.
Cis constant once derived, butC.Inverse()is recomputed for every joint in every frame. Precompute it once before the sampling loop for clarity.♻️ Proposed refactor
// Sample by applying each Animation DIRECTLY to the skeleton instance — // deterministic regardless of the entity's animation-state bookkeeping // (state-set application proved instance-dependent for hand-built // skeletons), and it leaves the entity's enabled states untouched. + const Ogre::Quaternion Cinv = C.Inverse(); for (unsigned short a = 0; a < skel->getNumAnimations(); ++a) { @@ -1018,7 +1019,7 @@ Ogre::Bone* b = roleBone[static_cast<size_t>(j)]; if (!b) continue; const Ogre::Quaternion w = - C * b->_getDerivedOrientation() * C.Inverse(); + C * b->_getDerivedOrientation() * Cinv;🤖 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.cpp` around lines 1020 - 1024, Precompute the constant inverse of C once before the per-joint sampling loop, then reuse that value in the orientation calculation within the loop in AnimationMerger’s pose-generation logic, replacing each repeated C.Inverse() call.
🤖 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/CLIPipeline.cpp`:
- Around line 2382-2383: Change the SentryReporter::addBreadcrumb call in the
anim dump-canonical CLI handling to use the "cli.anim" category instead of
"ai.tool_call", matching the existing anim operation pattern near the related
CLI logic.
---
Nitpick comments:
In `@scripts/build-motion-library-v5.py`:
- Line 183: Replace the direct open calls used by the manifest loads in the
relevant code paths with with open(...) as ... context managers, then pass the
managed file object to json.load; update both occurrences near the
manifest-loading logic.
- Around line 102-104: Update quat_angle to call zip(a, b, strict=True) when
computing the quaternion dot product, ensuring mismatched array lengths raise an
explicit error instead of being silently truncated.
- Around line 215-216: In the corpus-file processing exception handler, replace
the silent `except Exception: continue` with a warning log that includes the
affected file path and exception details, then continue processing subsequent
files. Use the surrounding corpus-processing function and its existing logger or
warning mechanism to report failures consistently.
In `@src/AnimationMerger.cpp`:
- Around line 1020-1024: Precompute the constant inverse of C once before the
per-joint sampling loop, then reuse that value in the orientation calculation
within the loop in AnimationMerger’s pose-generation logic, replacing each
repeated C.Inverse() 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: 302c07e7-5410-4ec6-ae7e-10401dbbe51a
📒 Files selected for processing (6)
CLAUDE.mdscripts/build-motion-library-v5.pysrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/AnimationMerger_test.cppsrc/CLIPipeline.cpp
| SentryReporter::addBreadcrumb("ai.tool_call", | ||
| QString("anim dump-canonical: %1").arg(fi.fileName())); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use "cli.anim" breadcrumb category instead of "ai.tool_call".
Per coding guidelines, ai.tool_call is for MCP tools. This is a CLI command — the existing anim operations use "cli.anim" (line 2333). Using the wrong category corrupts Sentry telemetry grouping.
As per coding guidelines, use ai.tool_call for MCP tools; this CLI command should follow the existing "cli.anim" pattern.
🔧 Proposed fix
- SentryReporter::addBreadcrumb("ai.tool_call",
+ SentryReporter::addBreadcrumb("cli.anim",
QString("anim dump-canonical: %1").arg(fi.fileName()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SentryReporter::addBreadcrumb("ai.tool_call", | |
| QString("anim dump-canonical: %1").arg(fi.fileName())); | |
| SentryReporter::addBreadcrumb("cli.anim", | |
| QString("anim dump-canonical: %1").arg(fi.fileName())); |
🤖 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/CLIPipeline.cpp` around lines 2382 - 2383, Change the
SentryReporter::addBreadcrumb call in the anim dump-canonical CLI handling to
use the "cli.anim" category instead of "ai.tool_call", matching the existing
anim operation pattern near the related CLI logic.
Source: Coding guidelines
|
Full-corpus results (post-merge follow-up data): with the Sketchfab harvest downloaded (289 validated rigs / 1,299 raw clips, 5.7 GB — synced to the motion-corpus dataset including the 114 clips / 23 actions, 10.9 MB (v4: 47 / 15) — walk:16 run:16 idle:16 jump:15 attack:11 punch:8 death:7 shoot:4 wave:3 sit:3 dance:2 swim:2 + boxing, cough, crawl, pickup, roar, roll, salute, shakehand, strafeleft, straferight, working. Retargets validated on the Mixamo test rig: walk takes (incl. the zombie lurch), the Knight's combat roll, and the new Hosting this library on the models repo (replacing v4 for all users) is ready whenever we want to pull the trigger — it's the same v3 schema, so no app change is needed. 🤖 Generated with Claude Code |
Authored animations often OPEN on a stylized pose (a dance intro, a wind-up), and composing every generated clip onto frame 0 baked that style into the rig's neutral — generations looked "based on the previous animation's first frame" instead of a relaxed stance. The standing-pose harvest now samples the reference animation's rotation energy (pure track math, nothing applied to the live skeleton) and harvests at the calmest frame — the closest thing the rig has to a relaxed standing pose. Generated clips remain excluded from the harvest as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…st root) The root was fully locked to the standing pose (CMU/scraped clips bake whole-body facing into the hip), which zeroed the hip's natural walk sway — measured 14.5° on a Mixamo reference walk vs 0° retargeted, the single largest fidelity gap in the reference-parity harness. Split the root's world-frame delta about canonical +Y (swing–twist): the twist (facing) stays locked, the swing (pitch/roll pelvic sway) applies pre-multiplied in world axes. Reference-parity numbers (self-retarget of a Mixamo walk through dump-canonical → library → generate on the same rig): mean per-joint delta error 2.25° → 1.84°, hip 7.84° → 2.62° (amplitude 0 → 10.1° vs 14.5° ref — the residual is the removed yaw, by design), torso errors also improved; legs remain ≤0.15°. Pose-shape IoU on isometric sheets (same render pipeline): 0.787 → 0.816 (self-ceiling 1.0, residual is sub-frame phase quantization). Cross-rig (reference walk onto a dance-rigged character): 2.32° with no contamination from the dance — verifying the calm-frame standing-pose harvest end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
joint-deltas (convention-safe per-joint delta-angle trajectories) and sheets (bbox/scale-normalized silhouette IoU with cyclic frame alignment) comparisons between a reference animation and a retargeted one. Documents the local-eval-only posture for Mixamo references and the recorded 2026-07-11 baselines (1.84° mean joint error, IoU 0.816). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…venance joins Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Generated animations now reference the TARGET RIG'S BIND POSE instead of a standing pose harvested from another animation — the root cause of the "first frame of the previous animation contaminates the generated one" report. How it works (clips extracted with --dump-canonical carry two new per-clip blocks): - restWorld: the SOURCE rig's bind world orientation per canonical joint (conjugated into the canonical frame like the clip quats). - restDir: the source's canonical-topology bind bone DIRECTIONS. At apply time each source bone's constant local direction axis a_s = Ws_bind^-1 * ds_bind is rotated by the clip frame, expressed in the target's raw frame via the target's own bind-geometry conjugation Ct, and the target bone is aimed at exactly that world direction: Wt(f) = arc(dt_bind -> ds(f)) * Wt_bind, propagated hierarchy-ordered. Bind-to-bind delta transplant was rejected: Assimp reset poses are not trustworthy T-poses across importers, but each rig is self-consistent. Key correctness detail: Ogre skeleton keyframes are DELTAS applied multiplicatively onto the binding pose (NodeAnimationTrack::applyToNode rotates the reset bone), so the keyframe writes bindLocal^-1 * local — absolute locals double-compose on rigs with non-identity bind locals (the cartoon fox rig that exposed all of this). Results (eval harness, scripts/compare-motion-clips.py): - Mixamo Walk self-retarget: 2.13 deg mean joint delta, IoU 0.836 (prior baseline 0.816). - Cross-rig fox walk: pose IoU vs reference 0.44 (all standing-path attempts: ~0.25); arms hang naturally, posed bbox w/h 0.57 vs 0.58 ground truth. - v4/CMU clips (no restWorld) are untouched: they still run the standing-pose path, regression-checked on the same rig. Also: canonicalParentOf/canonicalChildOf topology tables (MotionInbetween), MotionLibrary parses optional per-clip restWorld/restDir (+ unit tests), --dump-canonical emits both blocks, and all three generate surfaces (CLI/GUI/MCP) prefer the clip's restWorld over the library-level CMU one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The builder now copies the per-clip source-bind orientation + bind bone direction blocks from the --dump-canonical sidecars into the library JSON, lighting up the bind-referenced retarget for every harvested clip. Stale sidecar caches predate these fields — delete *.canonical.json to re-extract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
1 participant
Slice B of epic #837: turns the harvested corpus (#838, published as the QtMeshEditor-motion-corpus dataset) into template-library clips.
What's in it
AnimationMerger::extractCanonicalClips— samples every skeletal animation at 30 fps onto the 22-joint canonical skeleton as world-frame quats (the v3 library convention), via the same bone-role matcher the retarget consumes. The subtle part: scraped rigs live in arbitrary file frames (Blender FBX armatures are commonly Z-up — the first extraction rendered characters lying down), so each rig's frame is derived from its own bind geometry (up = hip→head, left = rhip→lhip, forward = left×up) and every quat conjugated into the canonical Y-up/+Z-forward frame. No per-format guessing.qtmesh anim <file> --dump-canonical out.json [--animation N].scripts/build-motion-library-v5.py(offline dev tool) — corpus walk → dump → action labels from normalized animation names (keyword table + verbatim single-word fallback, widening the prompt vocabulary) → highest-energy window snapped to a calm start → static-pose drop → semantic dedup of sibling characters → emits the existingqtmesh-motion-library-v3schema (the shipped app consumes it unchanged) + the corpusATTRIBUTION.md.Validation
Shipping the built v5 library to the HF models repo is a follow-up once the Sketchfab downloads fatten the corpus (the builder is one command).
Part of #837; implements #839.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
qtmesh anim --dump-canonicalto export skeletal animations as canonical 22-joint motion data.Documentation
Tests