feat(anim): canonical clip extraction + motion-library v5 builder (t2m-v2 Slice B, #839)#843
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>
📝 WalkthroughWalkthroughAdds canonical skeletal animation extraction, a ChangesCanonical motion pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MotionCorpus
participant BuildMotionLibrary
participant QtMeshCLI
participant AnimationMerger
participant MotionLibrary
participant Retargeting
BuildMotionLibrary->>MotionCorpus: discover assets and manifest metadata
BuildMotionLibrary->>QtMeshCLI: request canonical clip JSON
QtMeshCLI->>AnimationMerger: extract sampled canonical clips
AnimationMerger-->>QtMeshCLI: return quaternions and rest data
QtMeshCLI-->>BuildMotionLibrary: write canonical clip JSON
BuildMotionLibrary->>BuildMotionLibrary: filter, window, deduplicate, and cap clips
BuildMotionLibrary-->>MotionLibrary: write motion-library.json
MotionLibrary-->>Retargeting: provide rest orientations and reference directions
Retargeting->>AnimationMerger: apply canonical motion clip
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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: 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>
Two fixes surfaced by retargeting the full harvested library onto a cartoon rig: 1. Extractor: restWorld/restDir/conjugation are now all measured at the clip's calmest ANIMATED frame instead of the bind/reset pose. On many scraped rigs (Quaternius, Sketchfab glTF) the animation worlds differ from the reset pose by a constant global rotation (an armature-node +/-90 deg X that Assimp bakes into one but not the other), so a bind-measured reference tipped the retargeted body horizontal. One measurement frame makes the (restWorld, restDir, quats) triple consistent by construction — the global offset cancels exactly. Verified: samurai dance / Quaternius knight jump / swim idle now retarget upright; Mixamo self-parity holds at 2.2 deg. 2. Retarget: yaw180 is no longer applied in the direction-matching path. That path anchors facing to the TARGET's own bind (clips are canonical +Z-facing), so the flip aimed collars/arms near-anti-parallel to their bind directions and destabilised getRotationTo (self-parity 2.1 -> 6.9 deg with wild collar swings). The flag keeps its meaning for the legacy standing-pose transport. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The trained-model path ("Use trained model" in the GUI, --model on the
CLI) still contaminated generations with the rig's other animations: the
model emits no reference triple, so its clips rode the legacy transport
whose standing pose is harvested from the first authored animation — on
the Hip Hop fox every generated walk started from the dance stance.
Now the retarget SYNTHESIZES the standing pose instead: callers borrow a
template clip's canonical bone directions (new
MotionLibrary::referenceDirsForPrompt — deterministic, prefers the
prompt's action and the most complete direction set, local library only)
and applyMotionClip aims every bone from the TARGET'S BIND at those
directions — the same math as the direction retarget at its reference
frame, so the base pose is bind-referenced by construction. The
harvest only runs when no synthetic pose could be built (v1/v2 CMU
libraries, missing local library), preserving the old behavior there.
The target-side bind reader (bind worlds/locals/positions, role map, Ct
conjugation, per-role bind directions, hierarchy order) is extracted
into a shared file-local helper used by both the direction retarget and
the synthetic pose; the direction path's math is unchanged
(self-parity holds at 2.22 deg, legacy v4 regression-checked).
Verified: model walk on the Hip Hop fox and on a Mixamo rig both start
upright with arms at the sides; template + v4 paths unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
scripts/build-motion-library-v5.py (3)
203-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard a missing
raw/directory.If the corpus is present but
raw/is absent,os.listdir(raw)raises an uncaughtFileNotFoundErrortraceback instead of the intended friendlyno clips extracted — is the corpus downloaded/validated?guidance. A quickos.path.isdir(raw)check before the walk keeps the failure message actionable.🤖 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 203 - 204, Update the corpus-processing flow around the raw directory iteration to check os.path.isdir(raw) before calling os.listdir(raw). When raw is absent, return or trigger the existing friendly “no clips extracted — is the corpus downloaded/validated?” guidance instead of allowing FileNotFoundError to escape; preserve the current sorted walk for valid directories.
225-243: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent skips hide extraction failures — emit a diagnostic before
continue.Both the cache-parse fallback (Lines 227-230) and the
qtmeshinvocation (Lines 231-243) swallow every failure andcontinuewith no output. Across a large corpus, a systematically failingqtmesh(bad binary, crash, non-zero exit, timeout) produces zero clues until the finalno clips extractedmessage, forcing a blind rerun. A one-line warning tostderr(includingr.stderron non-zero exit) makes the failure mode diagnosable without changing control flow.Note: Ruff's S603/
subprocess-from-requestand theopen(cache)path-traversal hints are false positives here —qtmeshis a resolved local binary andcache/fpathderive from a locally-walked corpus tree, not external request input.🔎 Suggested diagnostics
if dump is None: try: r = subprocess.run( [qtmesh, "anim", fpath, "--dump-canonical", cache], capture_output=True, text=True, timeout=600) if r.returncode != 0 \ or not os.path.exists(cache) \ or not os.path.getsize(cache): + print(f" ! skip {fpath}: qtmesh rc={r.returncode} " + f"{r.stderr.strip()[:200]}", file=sys.stderr) continue dump = json.load(open(cache)) - except Exception: + except Exception as ex: + print(f" ! skip {fpath}: {ex}", 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 225 - 243, Add concise stderr warnings before both silent continue paths in the cache JSON fallback and qtmesh extraction handling, identifying the relevant cache/input file and exception or failure context. For non-zero qtmesh exits, include r.stderr in the diagnostic; preserve the existing control flow and skip behavior.Source: Linters/SAST tools
196-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a context manager (and explicit
encoding) for the JSON reads.
json.load(open(mpath))(also Lines 228 and 241) relies on CPython refcounting to close the descriptor and inherits the platform-default locale encoding. Usingwith open(mpath, encoding="utf-8") as f:closes deterministically and keeps parsing reproducible across platforms, matching thewith open(args.out, "w")pattern already used at Line 296.🤖 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 196 - 198, Update the JSON reads in the manifest-loading logic, including the reads around mpath and the corresponding locations at lines 228 and 241, to use a with-open context manager with explicit UTF-8 encoding before passing the file handle to json.load. Preserve the existing parsing behavior while ensuring every opened input file is deterministically closed.src/AnimationMerger.cpp (1)
960-997: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated source/target-frame derivation math.
deriveFrame(insideextractCanonicalClips) and theCtcomputation insidereadTargetBindFrameimplement the same hip→head/left-right→canonical-basis geometry almost verbatim. Extracting a shared helper (e.g.,deriveCanonicalConjugation(hipPos, headPos, lPos, rPos)) would remove the duplication and prevent the two derivations from silently drifting if the geometry heuristic is ever tuned.Also applies to: 1219-1250
🤖 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 960 - 997, The canonical frame geometry is duplicated between the deriveFrame lambda in extractCanonicalClips and the Ct calculation in readTargetBindFrame. Extract the shared hip/head/left-right basis construction and quaternion conversion into a reusable helper such as deriveCanonicalConjugation, then have both call sites supply their bone positions and reuse its fallback/degenerate handling and identity result.
🤖 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 `@scripts/compare-motion-clips.py`:
- Line 83: In the point-processing code, split the assignments to xs and ys into
separate statements on separate lines, removing the semicolon while preserving
both list comprehensions and their behavior.
- Around line 44-48: Update clip_deltas to open the JSON file with a with block
so the handle is always closed, and validate the parsed document before
accessing clips[0]. Raise a clear, descriptive error when the expected clips
field is missing, invalid, or empty, while preserving the existing
quaternion-delta calculation for valid input.
- Around line 99-104: Validate that the column counts of sheets A and B match
before the alignment loop in the comparison flow using cells(path_a) and
cells(path_b); reject mismatched sheets with the existing appropriate error
behavior, and only compute scores after validation so B[r][(c + s) % cols]
cannot raise IndexError.
In `@src/AnimationMerger.cpp`:
- Around line 1412-1463: Update the guard before the synthetic stand-pose path
to require clipRestDir.size() == static_cast<size_t>(Jc), alongside the existing
non-empty check and cmuRestWorld validation. Keep the later clipRestDir indexing
in the haveRestWorld branch unchanged, so the branch only executes when all
canonical joint entries are available.
---
Nitpick comments:
In `@scripts/build-motion-library-v5.py`:
- Around line 203-204: Update the corpus-processing flow around the raw
directory iteration to check os.path.isdir(raw) before calling os.listdir(raw).
When raw is absent, return or trigger the existing friendly “no clips extracted
— is the corpus downloaded/validated?” guidance instead of allowing
FileNotFoundError to escape; preserve the current sorted walk for valid
directories.
- Around line 225-243: Add concise stderr warnings before both silent continue
paths in the cache JSON fallback and qtmesh extraction handling, identifying the
relevant cache/input file and exception or failure context. For non-zero qtmesh
exits, include r.stderr in the diagnostic; preserve the existing control flow
and skip behavior.
- Around line 196-198: Update the JSON reads in the manifest-loading logic,
including the reads around mpath and the corresponding locations at lines 228
and 241, to use a with-open context manager with explicit UTF-8 encoding before
passing the file handle to json.load. Preserve the existing parsing behavior
while ensuring every opened input file is deterministically closed.
In `@src/AnimationMerger.cpp`:
- Around line 960-997: The canonical frame geometry is duplicated between the
deriveFrame lambda in extractCanonicalClips and the Ct calculation in
readTargetBindFrame. Extract the shared hip/head/left-right basis construction
and quaternion conversion into a reusable helper such as
deriveCanonicalConjugation, then have both call sites supply their bone
positions and reuse its fallback/degenerate handling and identity result.
🪄 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: 0b317d31-a2ce-463a-b0aa-4c59799c50d1
📒 Files selected for processing (13)
scripts/build-motion-library-v5.pyscripts/compare-motion-clips.pyscripts/scrape-motion-corpus.pysrc/AnimationControlController.cppsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/MotionInbetween.cppsrc/MotionInbetween.hsrc/MotionLibrary.cppsrc/MotionLibrary.hsrc/MotionLibrary_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/AnimationMerger.h
| def clip_deltas(path): | ||
| c = json.load(open(path))["clips"][0] | ||
| q = c["quats"] | ||
| return [[q_ang(q[f][j], q[0][j]) for f in range(len(q))] | ||
| for j in range(len(JN))] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
File handle leak and missing error handling in clip_deltas.
json.load(open(path)) never closes the file. Additionally, ["clips"][0] will raise a cryptic KeyError/IndexError if the JSON is malformed or has no clips. Use a with block and give a clear error message.
🛡️ Proposed fix
def clip_deltas(path):
- c = json.load(open(path))["clips"][0]
- q = c["quats"]
+ with open(path) as f:
+ data = json.load(f)
+ clips = data.get("clips")
+ if not clips:
+ sys.exit(f"Error: no clips found in {path}")
+ q = clips[0]["quats"]
return [[q_ang(q[f][j], q[0][j]) for f in range(len(q))]
for j in range(len(JN))]📝 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.
| def clip_deltas(path): | |
| c = json.load(open(path))["clips"][0] | |
| q = c["quats"] | |
| return [[q_ang(q[f][j], q[0][j]) for f in range(len(q))] | |
| for j in range(len(JN))] | |
| def clip_deltas(path): | |
| with open(path) as f: | |
| data = json.load(f) | |
| clips = data.get("clips") | |
| if not clips: | |
| sys.exit(f"Error: no clips found in {path}") | |
| q = clips[0]["quats"] | |
| return [[q_ang(q[f][j], q[0][j]) for f in range(len(q))] | |
| for j in range(len(JN))] |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 44-44: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(path)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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/compare-motion-clips.py` around lines 44 - 48, Update clip_deltas to
open the JSON file with a with block so the handle is always closed, and
validate the parsed document before accessing clips[0]. Raise a clear,
descriptive error when the expected clips field is missing, invalid, or empty,
while preserving the existing quaternion-delta calculation for valid input.
| if not pts: | ||
| row.append(frozenset()) | ||
| continue | ||
| xs = [p[0] for p in pts]; ys = [p[1] for p in pts] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Semicolon triggers E702 lint error.
Split the two list comprehensions onto separate lines.
♻️ Proposed fix
- xs = [p[0] for p in pts]; ys = [p[1] for p in pts]
+ xs = [p[0] for p in pts]
+ ys = [p[1] for p in pts]📝 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.
| xs = [p[0] for p in pts]; ys = [p[1] for p in pts] | |
| xs = [p[0] for p in pts] | |
| ys = [p[1] for p in pts] |
🧰 Tools
🪛 Ruff (0.15.20)
[error] 83-83: Multiple statements on one line (semicolon)
(E702)
🤖 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/compare-motion-clips.py` at line 83, In the point-processing code,
split the assignments to xs and ys into separate statements on separate lines,
removing the semicolon while preserving both list comprehensions and their
behavior.
Source: Linters/SAST tools
| A, B = cells(path_a), cells(path_b) | ||
| cols = len(A[0]) | ||
| scores = [] | ||
| for r in range(rows): | ||
| best = max(sum(iou(A[r][c], B[r][(c + s) % cols]) | ||
| for c in range(cols)) / cols for s in range(cols)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Column-count mismatch between sheets can raise IndexError.
cols is derived only from A. If sheet B has fewer columns, B[r][(c + s) % cols] indexes out of range. Validate that both sheets have the same column count before the alignment loop.
🛡️ Proposed fix
A, B = cells(path_a), cells(path_b)
cols = len(A[0])
+ if any(len(row) != cols for sheet in (A, B) for row in sheet):
+ sys.exit("Error: sheets have different column counts")
scores = []📝 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.
| A, B = cells(path_a), cells(path_b) | |
| cols = len(A[0]) | |
| scores = [] | |
| for r in range(rows): | |
| best = max(sum(iou(A[r][c], B[r][(c + s) % cols]) | |
| for c in range(cols)) / cols for s in range(cols)) | |
| A, B = cells(path_a), cells(path_b) | |
| cols = len(A[0]) | |
| if any(len(row) != cols for sheet in (A, B) for row in sheet): | |
| sys.exit("Error: sheets have different column counts") | |
| scores = [] | |
| for r in range(rows): | |
| best = max(sum(iou(A[r][c], B[r][(c + s) % cols]) | |
| for c in range(cols)) / cols for s in range(cols)) |
🤖 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/compare-motion-clips.py` around lines 99 - 104, Validate that the
column counts of sheets A and B match before the alignment loop in the
comparison flow using cells(path_a) and cells(path_b); reject mismatched sheets
with the existing appropriate error behavior, and only compute scores after
validation so B[r][(c + s) % cols] cannot raise IndexError.
| bool haveRestWorld = false; | ||
| if (worldFrame | ||
| && cmuRestWorld.size() == static_cast<size_t>( | ||
| MotionInbetween::canonicalJointCount())) { | ||
| for (const auto& q : cmuRestWorld) | ||
| if (std::abs(q[0]) > 1e-5f || std::abs(q[1]) > 1e-5f | ||
| || std::abs(q[2]) > 1e-5f || std::abs(1.f - std::abs(q[3])) > 1e-5f) { | ||
| haveRestWorld = true; | ||
| break; | ||
| } | ||
| } | ||
| const int Jc = MotionInbetween::canonicalJointCount(); | ||
|
|
||
| // The rig's STANDING pose the legacy transport composes onto — declared | ||
| // here so the SYNTHETIC bind-referenced path below can fill it before | ||
| // the (contaminating) animation harvest is even considered. | ||
| struct StandXform { Ogre::Quaternion rot; Ogre::Vector3 pos; Ogre::Vector3 scale; bool has = false; }; | ||
| std::unordered_map<unsigned short, StandXform> standPose; | ||
| bool synthStand = false; | ||
|
|
||
| if (!clipRestDir.empty()) { | ||
| const TargetBindFrame tb = readTargetBindFrame(skel, boneToCanon); | ||
| const Ogre::Quaternion CtInv = tb.Ct.Inverse(); | ||
|
|
||
| if (haveRestWorld) { | ||
| // PER-FRAME DIRECTION MATCHING. "Bind" poses are NOT trustworthy | ||
| // T-poses across importers (Assimp's reset pose for Mixamo FBX is | ||
| // whatever the file stored), so bind-to-bind delta transplant | ||
| // breaks cross-rig. Each rig IS self-consistent though: the source | ||
| // bone's constant LOCAL direction axis a_s = Ws_ref⁻¹·ds_ref | ||
| // rotates to ds(f) = Ws(f)·a_s each frame, and the target bone is | ||
| // aimed at exactly that world direction: | ||
| // Wt(f) = arc(dt_bind → ds(f)) · Wt_bind | ||
| // hierarchy-ordered, twist about the bone intentionally not | ||
| // transported (roll is the least visible DoF). The generated clip | ||
| // therefore references the TARGET BIND — no pose from any other | ||
| // animation is involved. | ||
| std::vector<Ogre::Vector3> srcLocalAxis( | ||
| static_cast<size_t>(Jc), Ogre::Vector3::ZERO); | ||
| for (int c = 0; c < Jc; ++c) { | ||
| const auto& sd = clipRestDir[static_cast<size_t>(c)]; | ||
| Ogre::Vector3 srcDir(sd[0], sd[1], sd[2]); | ||
| if (srcDir.squaredLength() > 1e-8f | ||
| && tb.tgtBindDir[static_cast<size_t>(c)].squaredLength() | ||
| > 1e-8f) { | ||
| srcDir.normalise(); | ||
| const auto& q = cmuRestWorld[static_cast<size_t>(c)]; | ||
| const Ogre::Quaternion restQ(q[3], q[0], q[1], q[2]); | ||
| srcLocalAxis[static_cast<size_t>(c)] = | ||
| restQ.Inverse() * srcDir; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard clipRestDir before indexing.
clipRestDir is only checked for non-emptiness here, but it’s indexed up to Jc - 1 later in this branch. Add a size() == static_cast<size_t>(Jc) check before entering this path, matching the validation already done for cmuRestWorld, to avoid an out-of-bounds access if the caller passes a shorter vector.
🛡️ Proposed fix
- if (!clipRestDir.empty()) {
+ if (!clipRestDir.empty()
+ && clipRestDir.size() == static_cast<size_t>(Jc)) {
const TargetBindFrame tb = readTargetBindFrame(skel, boneToCanon);📝 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.
| bool haveRestWorld = false; | |
| if (worldFrame | |
| && cmuRestWorld.size() == static_cast<size_t>( | |
| MotionInbetween::canonicalJointCount())) { | |
| for (const auto& q : cmuRestWorld) | |
| if (std::abs(q[0]) > 1e-5f || std::abs(q[1]) > 1e-5f | |
| || std::abs(q[2]) > 1e-5f || std::abs(1.f - std::abs(q[3])) > 1e-5f) { | |
| haveRestWorld = true; | |
| break; | |
| } | |
| } | |
| const int Jc = MotionInbetween::canonicalJointCount(); | |
| // The rig's STANDING pose the legacy transport composes onto — declared | |
| // here so the SYNTHETIC bind-referenced path below can fill it before | |
| // the (contaminating) animation harvest is even considered. | |
| struct StandXform { Ogre::Quaternion rot; Ogre::Vector3 pos; Ogre::Vector3 scale; bool has = false; }; | |
| std::unordered_map<unsigned short, StandXform> standPose; | |
| bool synthStand = false; | |
| if (!clipRestDir.empty()) { | |
| const TargetBindFrame tb = readTargetBindFrame(skel, boneToCanon); | |
| const Ogre::Quaternion CtInv = tb.Ct.Inverse(); | |
| if (haveRestWorld) { | |
| // PER-FRAME DIRECTION MATCHING. "Bind" poses are NOT trustworthy | |
| // T-poses across importers (Assimp's reset pose for Mixamo FBX is | |
| // whatever the file stored), so bind-to-bind delta transplant | |
| // breaks cross-rig. Each rig IS self-consistent though: the source | |
| // bone's constant LOCAL direction axis a_s = Ws_ref⁻¹·ds_ref | |
| // rotates to ds(f) = Ws(f)·a_s each frame, and the target bone is | |
| // aimed at exactly that world direction: | |
| // Wt(f) = arc(dt_bind → ds(f)) · Wt_bind | |
| // hierarchy-ordered, twist about the bone intentionally not | |
| // transported (roll is the least visible DoF). The generated clip | |
| // therefore references the TARGET BIND — no pose from any other | |
| // animation is involved. | |
| std::vector<Ogre::Vector3> srcLocalAxis( | |
| static_cast<size_t>(Jc), Ogre::Vector3::ZERO); | |
| for (int c = 0; c < Jc; ++c) { | |
| const auto& sd = clipRestDir[static_cast<size_t>(c)]; | |
| Ogre::Vector3 srcDir(sd[0], sd[1], sd[2]); | |
| if (srcDir.squaredLength() > 1e-8f | |
| && tb.tgtBindDir[static_cast<size_t>(c)].squaredLength() | |
| > 1e-8f) { | |
| srcDir.normalise(); | |
| const auto& q = cmuRestWorld[static_cast<size_t>(c)]; | |
| const Ogre::Quaternion restQ(q[3], q[0], q[1], q[2]); | |
| srcLocalAxis[static_cast<size_t>(c)] = | |
| restQ.Inverse() * srcDir; | |
| } | |
| } | |
| bool haveRestWorld = false; | |
| if (worldFrame | |
| && cmuRestWorld.size() == static_cast<size_t>( | |
| MotionInbetween::canonicalJointCount())) { | |
| for (const auto& q : cmuRestWorld) | |
| if (std::abs(q[0]) > 1e-5f || std::abs(q[1]) > 1e-5f | |
| || std::abs(q[2]) > 1e-5f || std::abs(1.f - std::abs(q[3])) > 1e-5f) { | |
| haveRestWorld = true; | |
| break; | |
| } | |
| } | |
| const int Jc = MotionInbetween::canonicalJointCount(); | |
| // The rig's STANDING pose the legacy transport composes onto — declared | |
| // here so the SYNTHETIC bind-referenced path below can fill it before | |
| // the (contaminating) animation harvest is even considered. | |
| struct StandXform { Ogre::Quaternion rot; Ogre::Vector3 pos; Ogre::Vector3 scale; bool has = false; }; | |
| std::unordered_map<unsigned short, StandXform> standPose; | |
| bool synthStand = false; | |
| if (!clipRestDir.empty() | |
| && clipRestDir.size() == static_cast<size_t>(Jc)) { | |
| const TargetBindFrame tb = readTargetBindFrame(skel, boneToCanon); | |
| const Ogre::Quaternion CtInv = tb.Ct.Inverse(); | |
| if (haveRestWorld) { | |
| // PER-FRAME DIRECTION MATCHING. "Bind" poses are NOT trustworthy | |
| // T-poses across importers (Assimp's reset pose for Mixamo FBX is | |
| // whatever the file stored), so bind-to-bind delta transplant | |
| // breaks cross-rig. Each rig IS self-consistent though: the source | |
| // bone's constant LOCAL direction axis a_s = Ws_ref⁻¹·ds_ref | |
| // rotates to ds(f) = Ws(f)·a_s each frame, and the target bone is | |
| // aimed at exactly that world direction: | |
| // Wt(f) = arc(dt_bind → ds(f)) · Wt_bind | |
| // hierarchy-ordered, twist about the bone intentionally not | |
| // transported (roll is the least visible DoF). The generated clip | |
| // therefore references the TARGET BIND — no pose from any other | |
| // animation is involved. | |
| std::vector<Ogre::Vector3> srcLocalAxis( | |
| static_cast<size_t>(Jc), Ogre::Vector3::ZERO); | |
| for (int c = 0; c < Jc; ++c) { | |
| const auto& sd = clipRestDir[static_cast<size_t>(c)]; | |
| Ogre::Vector3 srcDir(sd[0], sd[1], sd[2]); | |
| if (srcDir.squaredLength() > 1e-8f | |
| && tb.tgtBindDir[static_cast<size_t>(c)].squaredLength() | |
| > 1e-8f) { | |
| srcDir.normalise(); | |
| const auto& q = cmuRestWorld[static_cast<size_t>(c)]; | |
| const Ogre::Quaternion restQ(q[3], q[0], q[1], q[2]); | |
| srcLocalAxis[static_cast<size_t>(c)] = | |
| restQ.Inverse() * srcDir; | |
| } | |
| } |
🤖 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 1412 - 1463, Update the guard before
the synthetic stand-pose path to require clipRestDir.size() ==
static_cast<size_t>(Jc), alongside the existing non-empty check and cmuRestWorld
validation. Keep the later clipRestDir indexing in the haveRestWorld branch
unchanged, so the branch only executes when all canonical joint entries are
available.
|
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
qtmesh anim <file> --dump-canonical <out.json>to export canonical 22-joint quaternion clips (including rest bind data).anim --dump-canonicalworkflow.restWorld/restDir.