feat(anim): V2 motion library — fingers in the data model, curated shipping set (#838) - #941
Merged
Conversation
…x first-frame flip (#838) User review of every template variant (rendered via the --variant curation harness) identified clips to remove and a frame glitch to repair: - License: drop ALL Mixamo-derived clips (MIXAMO_MARKERS). A Sketchfab uploader's CC-BY covers the upload, not Adobe's underlying animation — not redistributable in a standalone library. (Removes climb, which was Mixamo-only.) - Review drop-list (REVIEW_DROP, matched by asset+anim so it survives rebuilds): GIGI & KAI Fox rigs (tip/hunch/invert on every reviewed action), Shar Pei dog (wrong body plan), Square Head "Loose" shake, Samurai dance. - fix_first_frame_flip: Mini Chibi Kid (and similar) export frame 0 with a rotated hip while the rest is upright — a loop-seam artifact. Detect (hip up-Y flipped past horizontal on frame 0 but upright on 1&2) and replace frame 0 with frame 1. Verified: walk/run/jump now open upright; the genuinely-horizontal death clip is correctly left untouched. Library 109 → 96 clips / 20 actions, zero residual Mixamo/Fox/dog sources. Core actions still well-stocked (walk/run/jump 12, punch/death 11, idle 8). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the random quality-weighted pick with an explicit, Mixamo-style
"browse and select" flow — the reliable path — while keeping the free-text
prompt for the experimental AI model only.
- AnimationPickerDialog.qml: a searchable list of every library clip with a
human-readable label ("Walk (Zombie)" vs "Walk (Monkey D. Luffy)") and a
per-row Apply that retargets that EXACT clip onto the selected rig.
- AnimationControlController::listMotionClips() → the list model
{index, action, name, source, quality, frames}; name derives the most
descriptive source segment (Quaternius pack name, else the asset).
- generateMotion() gains variantIndex: ≥0 forces the template path + that
clip (no model, no matchAmong). Wired into the picker's Apply.
- PropertiesPanel Animations section: "Browse animation library…" is now the
primary button; the text field is relabelled as the experimental AI path.
- CLI: `qtmesh anim <file> --generate <action> --variant N` (the curation
harness that rendered the review sheets) — cmdAnimGenerate variantIndex.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
User + measurement (mean upper-arm world up-Y) found four Quaternius packs whose RIGHT upper-arm bone mis-maps onto the canonical skeleton — the arm stays raised/out (mean up-Y positive ~+0.3..+0.7) the whole clip on every action, while a correct hanging arm reads ~−0.7..−0.9. Add them to REVIEW_DROP: "Animated Men Characters" + "Animated Women Characters" (Feb 2019), "Alien Animated" (Apr 2019), "Knight Character Animated" (Jul 2018). NB: matched by FULL pack name — the good "Man/Woman Animated" (Oct/Dec 2017) packs are DIFFERENT releases and are preserved (verified: 6 + 8 clips kept). Side effect: swim (Alien-only) and roll (Knight-only) drop out — their only sources were these packs; re-scrape clean versions later. Library 90 → 74 clips / 18 actions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…prompt (#838) Two bugs in the picker's first cut: - AnimationPickerDialog imported only PropertiesPanel, so AnimationControlController (registered under the AnimationControl module) resolved to undefined and listMotionClips() returned nothing ("0 animations"). Add `import AnimationControl 1.0`. - generateMotion's empty-prompt guard fired before the variant-index check, so the picker's Apply (which passes "" prompt + an index) failed with "Enter a motion prompt". Skip the guard when variantIndex >= 0. Both verified live: picker lists 74 named clips and Apply retargets the selected clip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#838) Review (chatgpt-codex P2): the animation picker's `applied` handler ran in the top-level Loader scope, but lastGeneratedAnim / setArmSpaceTarget / refreshAnimData are declared inside the animationComponent instance — so a successful Apply threw a QML ReferenceError instead of wiring the clip into the panel/arm-space state. Fix: the Loader now just re-emits a root-level `animationPicked(animation, entity)` signal; a Connections{ target: root } INSIDE the animation component handles it, where those members resolve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Non-locomotion template clips (working/crawl/death/…) rendered "floating" — the retarget locked the hip at the standing pose and discarded the source's hip translation, so a crouch played in place at full height. Extract a per-frame normalized hip-Y track (`rootY`, in source leg-lengths, max hip→foot as the reference length) during canonical extraction, carry it through --dump-canonical → the v5 library builder (windowed, re-based to frame 0, descent clamped to one leg length), and apply it to the root bone's keyframe Y, scaled by the TARGET rig's leg length. The descent is applied in BOTH retarget branches: the bind-referenced path (the one template clips with restDir actually take — root keyframe is a parent-space delta, so the drop is Ct⁻¹·(0,−y,0) along canonical-up mapped to the rig frame) and the legacy standing-pose transport (model / no-restDir clips). Target leg length comes from the bind-pose derived hip→foot distance. Descent-only: only the negative (lowering) component is applied, so it can pull a floating crouch down but never lift a grounded pose — source rigs vary in whether/which-sign they bake hip translation (many author the squat purely in knee/hip joint rotations we already retarget). Scoped to MotionLibrary::isVerticalDescentAction() (pickup/working/sit/crawl/death/ pray); locomotion keeps a flat root. Wired through GUI/CLI/MCP callers + their retime paths. Render-verified on Rumba (Mixamo): `working` hip Y drops 0.96→0.12 (0.85 leg-length crouch); `walk` stays flat (0.06 pelvic sway only). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Curation: add "Rigged and Animated Humanoid" (OpenGameArt) to the
REVIEW_DROP list — it retargets badly on BOTH Mixamo and UniRig skeletons
(user review). Library drops from 63 → 51 clips (18 actions).
- Vertical descent is now user-controllable, since it helps most crouch/
pickup/sit/crawl/death clips but a few author the squat purely in the
joints (no hip translation) and over-sink with it on. Exposed as:
* GUI "Lower body (crouch/pickup)" checkbox in the Generate-from-text
section (default ON), passed as generateMotion's verticalDescent arg
* CLI `--no-descent` on `qtmesh anim --generate`
* MCP `vertical_descent` boolean on generate_motion (default true)
The flag ANDs with isVerticalDescentAction(), so it only ever gates the
non-locomotion actions; locomotion stays flat regardless.
Verified: working with descent ON drops hip Y 0.85; --no-descent keeps it
flat (0.06). "Rigged and Animated Humanoid" no longer appears in the library.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Raw hip-Y translation was unreliable per rig — some Quaternius clips read
the hip RISING during pickup/sit (rig-dependent axis), so descent-only
zeroed them ("pickup not going down"), while working over-sank at the −1.0
clamp ("too far down").
Replace it with a sign-safe CROUCH-DEPTH measure: the hip's height ABOVE the
foot along canonical +Y (always ≥ 0). Standing = the max across the clip;
each frame's rootY is its drop below that (≤ 0) in leg-lengths. A genuine
crouch always lowers the hip toward the planted foot regardless of rig axes,
so pickup/sit now correctly descend. Builder re-anchors the window to its
shallowest frame and applies a 0.6 gain (full-kneel hip-to-foot compression
is nearly a whole leg — 0.6 lands a believable depth).
Verified on Rumba: pickup hip-Y-range 0.06→0.38 (descends), death 0.39,
and the descent toggle (checkbox / --no-descent / vertical_descent) flips
pickup between 0.38 and 0.06 as expected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…838) Descent-supposed clips that never STAND (crawl, ground-sit) read ~0 crouch depth because the reference "standing" height was the clip's own max hip height — an always-low clip has no tall frame to measure the drop from, so it floated upright in the center. Anchor the crouch depth to the rig's BIND-pose (T-pose) hip-above-foot height instead — an absolute upright reference. Now a crawl that opens and stays low reads its true full depth (0.07 → 0.50 hip drop on Rumba); sit and pickup likewise descend. Fall back to the clip max only if the clip ever stands taller than the bind pose (non-upright bind). Builder no longer re-anchors rootY to the window (that re-zeroed always-low clips); keeps the 0.6 display gain. Curation: drop FNaf_DLC_moon_sun (jumpscare rig) and Low_Poly_Zombie_Game_Animation (weak) per user review. Library now 46 clips. Verified on Rumba: crawl/working/death hip-Y-range ~0.50, pickup 0.38, sit 0.31 — all descend; render confirms crawl stays low instead of floating. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "Lower body (crouch/pickup)" toggle lived only in the Generate-from-text section, but applying a clip from the Browse-library PICKER hard-coded verticalDescent=true — so toggling the panel checkbox had no effect on picker-applied clips (the likely "checkbox is being ignored" report). Give the picker its own "Lower body (crouch/pickup)" checkbox (default ON) and pass it as generateMotion's verticalDescent arg, so descent is controllable wherever the user applies from. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Samurai-dance clip snapped the model in half — its rig stacks the Spine2 joint BELOW its parent in bind pose, so that bone's canonical bind direction points DOWN the hip→head axis. The aim-based retarget faithfully reproduced a chest folded UNDER the hip. Detect a spine-chain bone (abdomen/chest/neck/neck1, roles 1..4) whose bind direction has a clearly-downward component along the hip→head axis (dot < −0.2) and ZERO its restDir. The retarget's existing `squaredLength() <= 1e-8` guard then skips that bone, so the target holds its upright bind pose while the rest of the clip plays — the spine stays intact. Verified on Rumba: the Samurai dance now reads as a coherent crouching dance with an upright torso instead of a chest-below-hip fold. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- MotionLibrary::isVerticalDescentAction: include "crouch" (was missing, so crouch-labelled clips never descended). - MotionLibrary.h: correct the rootY doc — it preserves BIND-pose-relative depth (not re-based to the window's first frame), so always-low clips stay down. - applyMotionClip legacy path: compute targetLegLen from the BIND pose (readTargetBindFrame + tb.bindPos) instead of live derived positions, so a crouched runtime pose can't skew the descent scale (matches the bind-referenced path). - AnimationMerger_test: make the descent-test legs purely vertical so the bind hip→foot distance is exactly 1.0 (a lateral offset made it ~1.011 and skewed the expected hip delta). - CLIPipeline: reject non-numeric / negative --variant with exit code 2 instead of silently selecting clip 0. - AnimationControlController::listMotionClips: emit a ui.action breadcrumb before the blocking library load (picker instrumentation). - MCP generate_motion: accept variant_index for deterministic clip selection (parity with CLI --variant + the GUI picker); prompt is optional when it's given; bounds-checked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
VerticalDescentLowersRootDescentOnly failed on Linux CI with "skeleton resolved only 9/22 canonical joints — not a humanoid rig": the minimal 9-bone test skeleton didn't clear applyMotionClip's ~half-of-22 role- resolution gate, so the retarget rejected it before any descent ran. Build a full Mixamo-named humanoid (spine chain + collar/shoulder/elbow/hand + upleg/knee/foot per side, 19 bones) so >11 canonical roles resolve. Legs stay purely vertical → bind hip→foot distance is exactly 1.0, keeping the rootY=-0.5 → -0.5 hip-delta assertion exact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The canonical bone matcher recognized Mixamo/CMU/generic naming but not the
3ds Max Biped convention ("Bip001 L UpperArm", "Bip001 R Thigh", …), where the
side is a standalone space-delimited L/R token and an armature "Bip001" prefix
leads every bone. After separator-stripping, "Bip001 L UpperArm" collapsed to
"bip001lupperarm" — sideOf() couldn't find the side, so every limb bone failed
to resolve and a full Biped humanoid mapped only 5/22 roles (spine + head only,
which need no side), below the retarget threshold.
bipedSideToken() reads the L/R token from the raw space-split name before
normalisation and folds it into a "left"/"right" word; normaliseBoneName()
also drops the "bipNNN" prefix. A 3ds Max Biped rig now resolves 19/22 roles.
Verified on real assets: Gregorio (16 work/gather clips — cut/build/farm/
fruit/dance/…) and a cyclops both jumped 5/22 → 19/22 and retarget cleanly
onto the Mixamo test rig; Gregorio's Cut renders as a coherent chopping swing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ck (#838) Humanoid rigs legitimately LEAN, crouch, recline, throw the head back, or go to the ground — the animated uprightness gates (spine-up, mid-clip topple, thigh-down, neck/head-up) wrongly rejected those clips and skewed the quality score toward artificially-upright takes. Remove them so non-upright HUMAN motion is curated normally (e.g. the cyclops hunched attacks now pass). KEEP the bind-frame non-biped gate: it checks the REST skeleton's torso is vertical, filtering true non-humanoid body plans (quadruped / dino / spider) that cannot retarget onto the 22-joint humanoid canonical and would render as horizontal garbage. That's a body-plan guardrail, not a motion gate — proper multi-body-plan support is tracked separately (task #24). Verified: library 61→66 clips (cyclops attack1/attack2 recovered); Gregorio's give-item clip retargets upright+coherent onto a standard humanoid (the tilt seen earlier was the cyclops mesh's own hunched bind posture, not a pipeline bug). Reinos Supremos sources renamed to the character names (Gregório / Shitclops) via the corpus manifest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Gregório's build/farm/cut/gather clips floated: the source keeps the feet PLANTED (root at bind height), but the folded-leg crouch retargeted onto a differently-proportioned rig with the hip locked at standing height left the feet dangling. Source-derived rootY reads ~0 (feet don't move in the source), so it couldn't ground them. Add AnimationMerger::groundRootToFeet: a retarget-time post-pass that FKs the TARGET skeleton per frame (like pinFeet), finds the lowest foot's world-Y, and lowers the root by how far it floats above the bind ground plane so the foot re-plants. Target-rig-based, so it grounds ANY crouch/kneel/work pose regardless of source proportions; descent-only (never raises). Wired after smooth-bake / before foot-pin in GUI/CLI/MCP, gated on isVerticalDescentAction. Also: add the Gregório ground-work labels (build/cut/farm/fruit/*give/gather + *loop variants) to the descent classifier; skip meaningless "tempmotion"/ "temp"/"untitled" source labels in the builder. Verified: buildloop hip Y 0.88 → 0.44 and renders as a grounded kneeling worker instead of floating. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ground-work crouches (Gregório build/farm/cut) retargeted torso-VERTICAL and
"reaching into the air" — the pose should pitch the whole body FORWARD over
the hands. Cause: the bind-referenced retarget aims each bone by DIRECTION
(hip→abdomen), which for the hip captures the spine lean but drops the pelvis
PITCH. Since the hip is the FK-chain root, that lost DOF is exactly the
forward body tilt — so the crouch stayed upright ("won't rotate").
For descent-classified clips (doVerticalDescentBR), transport the hip's FULL
3-DOF source rotation (clip vs its own restWorld reference, canonical frame)
onto the target bind hip instead of the direction-aim. Scoped to descent
only: the full delta also carries whole-body FACING (yaw), which must stay
locked for locomotion so a walk doesn't spin — walk/run keep the aim-only
root path.
Verified on Rumba: buildloop now crouches with the torso pitched forward and
both hands reaching to the ground (feet planted via the grounding pass); walk
stays upright/striding, pickup unaffected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…838) Gregório's kneel/crouch work clips (BuildLoop/FarmLoop/…) retargeted RECLINED because the canonical leveling frame C was derived from the calm frame f*, which for these clips is itself a deep lean (measured torso-up ~60° off vertical). C baked that tilt in, so the extracted spine restDirs pointed down/back (abdomen [0,0.66,-0.75]) and the whole retarget reclined. When f*'s torso-up diverges >~20° from the BIND torso-up, derive C AND sample the reference triple (restWorld/restDir) from the upright BIND pose instead — consistent with the per-frame quats (C·raw·Cinv either way). Spine restDirs now read clean +Y ([0,0.998,0.06]); verified in the viewport the kneel matches the source (upright torso, knee down, hand to ground). Walk/pickup unaffected (their f* is already upright, so the bind path isn't taken). Also flush _keyFrameDataChanged() after every keyframe-writing pass (applyMotionClip both branches, bakeAnimationAtFps) so a re-generate onto the LIVE skeleton doesn't replay a stale interpolation cache — this is why the live viewport lagged the exported result during debugging. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Transfer source finger animation onto a differently-structured target rig (Biped Finger0-4 × 2seg → Mixamo Thumb/Index/… × 3seg). Copying finger rotations mangled the hand — inspecting the real Gregório .skeleton (via OgreXMLConverter) showed the Biped finger bones carry large, wildly-varying per-segment bind rotations (300-350° about arbitrary axes), so a rotation copy lands on the wrong axis. Instead use the body-retarget's DIRECTION approach: store each finger segment's canonical-frame pointing direction (segment→child) per frame, and aim the target finger bone's bind direction at it — rig-axis-independent. Pipeline: MotionInbetween::fingerRoleForBone classifies Biped + Mixamo finger names into (side,finger,segment); extraction samples 30 finger slots; --dump-canonical + the v5 builder carry a `fingers` block; MotionLibrary parses it; AnimationMerger::applyFingerCurl aims + redistributes source segments across the target's (2→3). Wired through GUI + CLI (MCP follow-up). Verified on Gregório→Rumba: the four fingers curl into a correct fist. Known follow-ups: the THUMB roll (its bend axis is ~perpendicular; direction-only aim drops its roll) and a slight body-retarget hand/wrist-aim offset. Also lands the finger + earlier work: _keyFrameDataChanged flushes so live re-generate isn't stale, and entity->_initialise(true) so the live instance picks up post-retarget track additions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…838) Adds a parallel V2 canonical skeleton (canonicalJointCountV2()=52: the 22 V1 body joints byte-identical at 0..21, plus 30 finger joints at 22..51 in fingerSlot order, 2 sides x 5 fingers x 3 segments) so finger animation can live in the trained data model instead of a side-channel. New V2 API: canonicalJointNameV2 / canonicalParentOfV2 (seg0 parents to rhand/lhand, deeper to the previous segment) / canonicalChildOfV2 / fingerJointIndexV2 / canonicalIndexForBoneV2 (body via the V1 matcher, fingers via fingerRoleForBone). V1 entry points are untouched, so 22-joint libraries and the shipped RMIB/t2m models keep working. Unit-tested (topology, Mixamo + Biped matcher, V1 equivalence at 0..21). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ad (#838) - Schema qtmesh-motion-library-v4: 52-joint clips (fingers folded in as canonical joints 22..51). The loader is joint-count-aware (jointCount() = 22 for v1..v3, 52 for v4) and still reads every older schema; per-clip quats/restWorld/restDir parse at the schema's width. Adds the optional fingerRestDir side-channel field for v1-era clips. - Curation store (the ship gate): curationPath()/loadCuration()/ saveCuration() persist the user-approved "good" clip sources in curation.json next to the library, keyed by the stable source string so stars survive library rebuilds. - Download prefers the V2 file: libraryPath() returns motion-library-v2.json when present, and ensureLibraryBlocking() now tries the v2 filename first with a fallback to the v1 name (partial/404 bodies are removed). Old app builds only request the v1 name and their loader rejects v4, so hosting both is non-breaking. The curated 36-clip v4 library is live on the QtMeshEditor-models HF repo under motion/. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s leveling (#838) Extraction (extractCanonicalClips gains v2=false): - V2 mode emits 52-joint clips: finger world orientations sampled per frame (canonicalised like the body), restWorld/restDir grown to 52; fingerless rigs still pad to 52 so the v4 loader's width guard holds. - Finger replication: rigs with one non-thumb finger bone driving all four (Quaternius Woman) copy the populated finger onto empty middle/ring/pinky. - Soft-weighted up-leveling of the canonical frame (torso-up aggregated over all frames, weight max(0,u.y)^3): fixes rigs whose bind and animation live in different frames (woman on the X axis) without breaking one-sided clips (death) or crouches; reference-pose selection now detects the bind-vs-animation frame mismatch and references an animated frame. - Finger REST directions captured per slot; nub/tip fallback for the last segment's pointing direction. Retarget (applyMotionClip): - V2 dispatch from clip width: 52-joint clips use the V2 matcher/topology (readTargetBindFrame gains v2), the humanoid gate stays against the 22 body joints, twist caps/gains are bounds-safe past joint 21. - Fingers ride the STANDARD hierarchical bind-referenced transport (the #411 change-of-basis): Drel = Dp^-1 Df applied onto the target's bind, carried by the target's animated hand. Exact by construction (0.2 deg measured parity vs the source), carries twist so nails stay put, and holds bind for source-empty segments (a transported identity against a curling parent used to counter-rotate the tip and straighten fingers). - V1 applyFingerCurl stays for <=v3 libraries with the relative-bend + knuckle-line flexion + thumb-side fixes; v4 clips skip it (fingers are joints now, callers gate the side-channel on lib.jointCount()==22). Also documents the measured limits: the aim's minimal-arc Qbase drops the source's bind->reference roll (50-170 deg arm-chain loss on self-retarget, cascading), and orientation-level fixes are unsound while the extraction frame C and the retarget frame Ct disagree (up to ~57 deg/clip) — the schema-v5 bindC frame link is the prescribed fix. QTMESH_EXTRACT_DEBUG prints per-clip reference choices. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ness (#838) - GUI + CLI generate paths feed the finger side-channel only for V1 (22- joint) libraries; v4 clips retarget fingers through the joint path (no double application). clipFingerRest plumbed for the V1 path. - setClipApproved(source, bool): the picker's star writes the curation store; listMotionClips() reports each clip's approved state. - CLI: --dump-canonical --v2 emits qtmesh-canonical-clips-v2 (52 joints, jointCount field, no redundant fingers side-channel). - Parity harness upgrades on --apply-canonical: a GROUND-TRUTH self-parity report (plays the original and generated_parity on the same skeleton and compares raw per-bone world orientations — immune to the extraction's reference/leveling choices, which contaminate dump-based comparisons), and an IN-PROCESS re-extraction dump (--dump-canonical alongside) that skips the glTF export->import round-trip (measured ~25 deg arm-chain loss on its own). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) Per-row star toggles a clip's "good" mark (persisted via setClipApproved -> curation.json, stable across library rebuilds), an "Only good" checkbox filters the list to the approved set, and the header shows the approved count. The approved set is what ships: build-motion-library-v6.py --approved-only consumes the same file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…y ship gate (#838) v6 builds the 52-joint (fingers-as-joints) library: dumps each corpus asset with --dump-canonical --v2 into a .canonical.v2.json sidecar cache, accepts rest arrays at the dump's own joint width, drops the legacy fingers side- channel, and emits schema qtmesh-motion-library-v4. --curation/--approved- only gate the output to the user-starred clips from the app's curation.json (auto-discovered when the flag is omitted) so the curated set ships while the rest of the corpus is iterated on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 56 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds curated motion-library browsing, schema v4 52-joint clips with finger data, vertical-descent processing, updated offline builders, expanded generation APIs, and keyboard-accessible QML controls. ChangesMotion library V2 workflow
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related issues
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
QtMeshEditor/src/MotionLibrary.cpp
Line 354 in 061ec5b
Fetch V2 before accepting cached V1
When an existing install already has motion-library.json but not motion-library-v2.json, libraryPath() falls back to the V1 path and this early return exits before tryDownload(kLibraryFileV2) runs. Those users never receive the curated schema-v4/finger library unless they manually delete the old cache, so the new shipping set is effectively disabled for upgrades; check/download V2 before returning the V1 cache.
QtMeshEditor/src/AnimationMerger.cpp
Line 2885 in 061ec5b
Swap V2 finger roles with the hands
When handedness compensation triggers (lx < rx), only the body L/R roles 6..21 are swapped here. For a V2 52-joint clip, finger roles 22..51 stay on their original side even though their hand parents (9/13) are swapped, so mirrored target rigs drive a hand with the opposite-side arm motion but same-side finger deltas/rest parents; one-handed poses or asymmetric hand animation will come out on the wrong/mismatched hand. Add the finger role pairs to the swap table when clipIsV2.
ℹ️ 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
2171
to
+2174
| std::vector<std::vector<std::array<float, 4>>> retimed(want); | ||
| std::vector<float> retimedY; | ||
| const bool hadY = static_cast<int>(clipRootY.size()) == clip.frames; | ||
| if (hadY) retimedY.resize(want); |
There was a problem hiding this comment.
In the CLI --duration path with a 22-joint library that carries the new fingers side channel, only body quats and rootY are resampled; clipFingers remains at the original frame count. applyFingerCurl() later creates finger keyframes from clipFingers.size() at the original fps, so finger tracks keep the source duration while the body animation/export uses the retimed length. Resample clipFingers in this block like the GUI path does.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in e803ee0 — the CLI --duration retime now resamples the V1 clipFingers side-channel alongside the body quats/rootY, matching the GUI path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CLIPipeline.cpp (1)
2558-2566: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAllow
--variantwithout a prompt, for parity with the GUI and MCP surfaces.
cmdAnimGenerateignorespromptwhenvariantIndex >= 0(lines 2134-2141). This guard still rejects an empty prompt, so--variant Nalone cannot run, and--generate "<anything>" --variant Nsilently discards the text.AnimationControlController::generateMotionandMCPServer::toolGenerateMotionboth accept a variant without a prompt. Align the CLI.♻️ Proposed change
- if (generateMode) { - if (generatePrompt.trimmed().isEmpty()) { + if (generateMode || generateVariant >= 0) { + if (generateVariant < 0 && generatePrompt.trimmed().isEmpty()) { err() << "Error: --generate requires a prompt, e.g. --generate \"walking\"." << Qt::endl; return 2; }🤖 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 2558 - 2566, Update the generate-mode validation around cmdAnimGenerate so an empty generatePrompt is allowed when generateVariant specifies a variant, while still requiring a non-empty prompt for prompt-based generation. Preserve the existing error and return code for requests lacking both a prompt and a valid variant, and continue passing both arguments to cmdAnimGenerate.
🧹 Nitpick comments (13)
src/MotionInbetween.cpp (2)
199-225: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBind
kFandkFingerCountV2to the publickFingerCount.
MotionInbetween::kFingerCount(header, Line 165) already declares 5 fingers.kFingerCountV2re-declares the same value, andkF[5]infingerJointNameV2hardcodes it a third time. If any one changes,fingerJointNameV2indexes out of range without a compile error.Derive
kFingerCountV2from the public constant and add astatic_assertfor the name table.♻️ Proposed refactor
-constexpr int kFingerCountV2 = 5; // thumb..pinky +constexpr int kFingerCountV2 = MotionInbetween::kFingerCount; // thumb..pinky @@ static const char* kF[5] = {"thumb", "index", "middle", "ring", "pinky"}; + static_assert(kFingerCountV2 == 5, "kF must cover every finger");🤖 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/MotionInbetween.cpp` around lines 199 - 225, Update kFingerCountV2 to derive its value from MotionInbetween::kFingerCount instead of redeclaring 5, and change the kF table in fingerJointNameV2 to use that shared count. Add a static_assert verifying the name table size matches kFingerCountV2, preserving the existing indexing and naming behavior.
57-67: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the separator regex to a static.
bipedSideTokencompilesQRegularExpression("[ _\\-.:]")on every call.normaliseBoneNamealso compiles"^bip\\d+"on every call. Both run once per bone for every rig bone during retarget mapping. Make them function-local statics so Qt compiles each pattern once.♻️ Proposed refactor
char bipedSideToken(const QString& raw) { - const QStringList toks = raw.split(QRegularExpression("[ _\\-.:]"), - Qt::SkipEmptyParts); + static const QRegularExpression kSep(QStringLiteral("[ _\\-.:]")); + const QStringList toks = raw.split(kSep, Qt::SkipEmptyParts);🤖 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/MotionInbetween.cpp` around lines 57 - 67, Update bipedSideToken and normaliseBoneName to store their respective QRegularExpression patterns as function-local static constants, reusing the compiled separator and bip-prefix regexes across calls instead of constructing them per invocation.scripts/build-motion-library-v6.py (3)
517-523: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark the SHA-1 fingerprint as non-security.
Ruff S324 flags
hashlib.sha1at Line 518. The hash is a dedup fingerprint, not a security primitive, so the algorithm is fine. Passusedforsecurity=Falseto record that intent and to silence the rule.♻️ Proposed refactor
- h = hashlib.sha1(action.encode()) + h = hashlib.sha1(action.encode(), usedforsecurity=False)🤖 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-v6.py` around lines 517 - 523, Update the hashlib.sha1 invocation in fingerprint to pass usedforsecurity=False, preserving the existing deduplication input processing and digest behavior while explicitly marking the hash as non-security.Source: Linters/SAST tools
726-756: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a Windows candidate path, and close the curation file handle.
Two points:
- Lines 732-739 list only macOS and Linux candidates for the default curation path. On Windows,
QStandardPaths::AppDataLocationresolves under%APPDATA%/QtMeshEditor/QtMeshEditor. A Windows developer who runs--approved-onlywithout--curationalways hits thesys.exitat Lines 743-745.- Line 746 calls
open(cur)without a context manager, so the handle stays open until garbage collection.♻️ Proposed refactor
+ appdata = os.environ.get("APPDATA", "") for cand in ( os.path.join(home, "Library/Application Support/QtMeshEditor/" "QtMeshEditor/ai_models/motion/curation.json"), os.path.join(home, "Library/Application Support/QtMeshEditor/" "ai_models/motion/curation.json"), os.path.join(home, ".local/share/QtMeshEditor/QtMeshEditor/" "ai_models/motion/curation.json"), + os.path.join(appdata, "QtMeshEditor", "QtMeshEditor", + "ai_models", "motion", "curation.json") + if appdata else "", ): - if os.path.exists(cand): + if cand and os.path.exists(cand): cur = cand break @@ - approved = set(json.load(open(cur)).get("approved", [])) + with open(cur) as cf: + approved = set(json.load(cf).get("approved", []))🤖 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-v6.py` around lines 726 - 756, Add the Windows `%APPDATA%/QtMeshEditor/QtMeshEditor/ai_models/motion/curation.json` candidate to the default path search in the approved_only curation block, while preserving the existing macOS and Linux candidates. Update the curation file loading near json.load to use a context manager so the file handle is closed deterministically.
36-548: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared builder logic into one module.
Lines 36-548 duplicate
scripts/build-motion-library-v5.pyalmost exactly. The shared surface includesKEYWORDS,STOPWORDS,BAD_ACTIONS,norm_anim_name,CANON_ACTION,action_for,quat_angle,frame_energy,select_window,qmul,qrot,vnorm,HORIZONTAL_OK,MIXAMO_MARKERS,REVIEW_DROP,_excluded,fix_first_frame_flip, the role constants,FORWARD_FACING, the three hip metrics,clip_quality,fingerprint,find_qtmesh, andmanifest_lookup.Only
main()and the output schema differ. Every quality gate and every drop-list entry must now be edited in two places, and the two copies will diverge. Three findings in this review already apply to both files identically.Move the shared code to
scripts/motion_library_common.py, and import it from both builders.🤖 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-v6.py` around lines 36 - 548, Extract the shared symbols listed in the review from both build-motion-library-v5.py and build-motion-library-v6.py into scripts/motion_library_common.py. Update both builders to import and reuse that module, leaving only each builder’s distinct main() flow and output schema local. Remove duplicated definitions and preserve all existing quality gates, drop-list entries, and behavior through the shared implementation.src/MotionInbetween_test.cpp (1)
303-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative coverage for finger-word substrings and for the
fingerNub/pinkiepaths.
some_propcontains no finger word, so it does not exercise the substring risk infingerRoleForBone. Add cases that pin the intended behavior:
- A spring/jiggle bone that contains
ring, for exampleSpringBone_L_01. This currently maps to the left ring finger. See the related comment onsrc/MotionInbetween.cppLines 297-312.Bip001 L FingerNub, whichfingerRoleForBonemust reject.- The
pinkiespelling, for examplemixamorig:LeftHandPinkie2.- A segment beyond the V2 cap, for example
mixamorig:LeftHandIndex4, which must return -1.As per coding guidelines: "Add Google Test unit tests for new functionality."
💚 Proposed additional cases
// A non-bone returns -1. EXPECT_EQ(MIB::canonicalIndexForBoneV2("some_prop"), -1); + // A spring/jiggle bone containing "ring" is NOT a finger. + EXPECT_EQ(MIB::canonicalIndexForBoneV2("SpringBone_L_01"), -1); + // Biped helper tips are ignored. + EXPECT_FALSE(MIB::fingerRoleForBone("Bip001 L FingerNub").valid()); + // "pinkie" spelling resolves like "pinky". + EXPECT_EQ(MIB::canonicalIndexForBoneV2("mixamorig:LeftHandPinkie2"), + MIB::fingerJointIndexV2(1, 4, 1)); + // Segments beyond the 3 V2 keeps are dropped. + EXPECT_EQ(MIB::canonicalIndexForBoneV2("mixamorig:LeftHandIndex4"), -1); }🤖 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/MotionInbetween_test.cpp` around lines 303 - 318, Extend the MotionInbetween CanonicalV2BoneMatcher test with negative cases for substring false positives and unsupported finger variants: assert canonicalIndexForBoneV2 returns -1 for SpringBone_L_01, Bip001 L FingerNub, mixamorig:LeftHandPinkie2, and mixamorig:LeftHandIndex4. Keep the existing positive mappings and non-bone assertion unchanged.Source: Coding guidelines
scripts/build-motion-library-v5.py (1)
231-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilence the unused unpack, and move the helper below the role constants.
Two points:
- Ruff RUF059 flags
yandwat Line 240 as unused. Use_for both.up_yreads the module globalHIP, which is defined at Line 250, after this function. Python resolves the global at call time, so the current code works. The ordering is still fragile for any future module-level call.♻️ Proposed refactor
def up_y(f): - x, y, z, w = f[HIP] + x, _, z, _ = f[HIP] return 1.0 - 2.0 * (x * x + z * z)🤖 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 231 - 248, Update fix_first_frame_flip by unpacking the HIP quaternion with underscore placeholders for the unused y and w values. Move the nested up_y helper or the entire fix_first_frame_flip definition below the module-level role constants, including HIP, so its global dependency is declared before the function can be invoked; preserve the existing frame-repair behavior.Source: Linters/SAST tools
src/MotionLibrary.cpp (1)
333-349: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake
saveCurationreport write failures, and write atomically.Two gaps:
- Line 347 ignores the return value of
QFile::write.saveCurationreturnstrueafter a short write or a disk-full error. The caller insrc/AnimationControlController.cpptreats that as success, so the user sees a saved approval that did not persist.QIODevice::Truncateempties the existing file before the new content lands. If the process stops mid-write, the whole approval set is lost. The set accumulates across a manual review session, so the loss is expensive to redo.Use
QSaveFile, which writes to a temporary file and renames oncommit().♻️ Proposed refactor
QDir().mkpath(QFileInfo(curationPath()).absolutePath()); - QFile f(curationPath()); - if (!f.open(QIODevice::WriteOnly | QIODevice::Truncate)) return false; - f.write(QJsonDocument(root).toJson(QJsonDocument::Indented)); - return true; + QSaveFile f(curationPath()); + if (!f.open(QIODevice::WriteOnly)) return false; + const QByteArray out = QJsonDocument(root).toJson(QJsonDocument::Indented); + if (f.write(out) != out.size()) { f.cancelWriting(); return false; } + return f.commit();Add the include:
`#include` <QSaveFile>🤖 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/MotionLibrary.cpp` around lines 333 - 349, Update MotionLibrary::saveCuration to use QSaveFile instead of QFile, preserving the existing path and JSON generation while writing atomically. Check the write result and return false on any short or failed write; only return true after a successful commit(), and add the required QSaveFile include.src/MotionLibrary_test.cpp (1)
135-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for schema v4,
jointCount(), and the finger arrays.
miniLib()builds a 22-joint schema-v1 document. The new parser branches insrc/MotionLibrary.cppare untested:
- Schema
qtmesh-motion-library-v4must setjointCount()to 52 and must require 52-wide poses.- A v4 document with 22-wide poses must fail with the joint-count error at Lines 131-136.
fingersandfingerRestDirparsing, including the malformed shapes that Lines 179-215 must reject. See the related comment onsrc/MotionLibrary.cppLines 179-215.Add a
miniLibV2()helper that emits 52 joints, and assert each case.As per coding guidelines: "Add Google Test unit tests for new functionality."
🤖 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/MotionLibrary_test.cpp` around lines 135 - 157, Extend MotionLibrary_test.cpp with a miniLibV2() helper producing a schema-v4 document containing 52 joints and valid 52-wide poses. Add tests covering jointCount() == 52, successful fingers and fingerRestDir parsing, rejection of malformed finger arrays, and failure of v4 documents with 22-wide poses using the joint-count error. Keep the existing schema-v1 and rootY coverage unchanged.Source: Coding guidelines
src/MotionInbetween.h (1)
165-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
kMaxFingerSeg.
MotionInbetween::kMaxFingerSegis declared only insrc/MotionInbetween.hand has no consumers. The active finger layout uses 30 slots (2 × 5 × 3), and the segment caps are expressed byAnimationMerger::kFingerSegs/kFingerSlotsandMotionInbetweenV2 role checks, so keep the public/sized constants and drop the unused declaration.🤖 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/MotionInbetween.h` around lines 165 - 166, Remove the unused MotionInbetween::kMaxFingerSeg declaration from the constants section in MotionInbetween.h, while preserving kFingerCount and the existing AnimationMerger::kFingerSegs, kFingerSlots, and MotionInbetween V2 role checks.qml/AnimationPickerDialog.qml (2)
176-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the
indexmodel role to avoid collision with the delegate'sindex.The delegate mixes two different values that share one name:
- Line 202 passes the bare
index(the delegate row) totoggleApproved().- Lines 225 and 236 pass
model.index(the library clip index) tobusyIndexandapplyClip().When the search filter or the "Only good" filter is active, the two values differ. The reader cannot tell them apart, and a later edit that drops the
model.prefix applies the wrong clip without any error. Rename the role toclipIndexinAnimationControlController::listMotionClips()and use it here.♻️ Proposed change
- property bool busy: dialog.busyIndex === model.index + property bool busy: dialog.busyIndex === model.clipIndex @@ - onClicked: dialog.applyClip(model.index, model.name) + onClicked: dialog.applyClip(model.clipIndex, model.name)In
src/AnimationControlController.cppline 1873:- m["index"] = i; + m["clipIndex"] = i;🤖 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 `@qml/AnimationPickerDialog.qml` around lines 176 - 244, Rename the library index role returned by AnimationControlController::listMotionClips() from index to clipIndex, then update this delegate’s busyIndex comparison and applyClip() call to use model.clipIndex while retaining the delegate index for row-based toggleApproved().
114-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared themed controls instead of hand-rolled
RectangleplusMouseArea.The two checkboxes (lines 118-154) and the Close button (lines 260-270) reimplement controls that already exist as
ThemedCheckBox.qmlandThemedButton.qmlin the same resource prefix (src/qml_resources.qrclines 24-25). The hand-rolled versions duplicate the theming logic and provide no keyboard focus, no focus ring, and no activation by Space or Return, so the dialog cannot be operated without a mouse. Add anEscapeshortcut for Close as well.As per coding guidelines: "Build new user interfaces in QML/Qt Quick rather than Qt Widgets; use
qml/PropertiesPanel.qmlandqml/MaterialEditorWindow.qmlas references."Also applies to: 248-271
🤖 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 `@qml/AnimationPickerDialog.qml` around lines 114 - 161, Replace the hand-rolled checkbox Rectangles and MouseAreas in the lower-body and only-good controls with the shared ThemedCheckBox component, preserving their labels, checked bindings, and rebuildModel behavior. Replace the Close control with ThemedButton, preserving its action, and add an Escape shortcut that triggers the same close behavior; rely on the shared themed controls for keyboard focus, focus rings, and Space/Return activation.Source: Coding guidelines
src/MCPServer.cpp (1)
4302-4321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompute the descent condition once.
args.value("vertical_descent").toBool(true) && MotionLibrary::isVerticalDescentAction(action)appears at lines 4304-4305 and again at lines 4319-4320. If one copy changes later, the retarget and the grounding pass disagree. Store it in a localconst bool.♻️ Proposed refactor
+ const bool doDescent = + args.value("vertical_descent").toBool(true) + && MotionLibrary::isVerticalDescentAction(action); const auto r = AnimationMerger::applyMotionClip(skel.get(), animName, quats, fps, ... clipRootY, - args.value("vertical_descent").toBool(true) - && MotionLibrary::isVerticalDescentAction(action)); + doDescent); @@ - if (args.value("vertical_descent").toBool(true) - && MotionLibrary::isVerticalDescentAction(action)) + if (doDescent) AnimationMerger::groundRootToFeet(skel.get(), animName);🤖 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/MCPServer.cpp` around lines 4302 - 4321, Compute the combined vertical-descent condition once in a local const bool near the retarget flow, then reuse that variable for both the retarget argument and the groundRootToFeet condition. Remove the duplicated args.value and MotionLibrary::isVerticalDescentAction expression while preserving the current default-enabled behavior.
🤖 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 `@qml/AnimationPickerDialog.qml`:
- Around line 275-292: Update applyClip to defer the synchronous
AnimationControlController.generateMotion call until after the current
JavaScript turn, allowing busyIndex and pickStatus to render first; preserve the
existing arguments, success/error handling, and busyIndex cleanup.
In `@qml/PropertiesPanel.qml`:
- Around line 9201-9204: Update the model-generated clip flow around
AnimationControlController.generateMotion so footPinChk/descentChk
vertical-descent handling is applied before or during model retargeting. Ensure
the generated model result does not reset useModel in a way that bypasses
descent processing, while preserving existing template-library behavior.
- Around line 9123-9133: Make the animation library browse control keyboard and
assistive-technology accessible by adding focus handling, keyboard activation,
and button accessibility metadata to the control containing browseMa, while
preserving root.openAnimationPicker() as its activation action. Apply the
corresponding focus handling, key activation, and checkbox accessibility
metadata to the descent checkbox control at qml/PropertiesPanel.qml lines
9274-9294.
In `@scripts/build-motion-library-v5.py`:
- Around line 682-699: Update the outer comment above the rootY handling in the
descent-window logic to remove the stale claim that values are re-based so frame
0 reads approximately zero. Keep the description of carrying the per-frame hip-Y
offset and slicing it to the same active window as the quaternions, consistent
with the non-re-anchoring behavior in the rootY block.
- Around line 375-385: Update the uprightness comment near the upness
calculation to acknowledge that upness still contributes to the quality score
through the later q calculation. Remove the now-unused spine_up_min
initialization and update logic, since its gating condition was removed; leave
the remaining upness signal and scoring behavior unchanged.
- Around line 197-228: Update _excluded so review-drop title matching uses word
boundaries rather than unanchored substring checks, preventing short entries
such as KAI and GIGI from matching unrelated titles while preserving full-name
matching for the other entries and animation filtering. Correct the _excluded
docstring to describe its actual return contract: a review/license reason string
when excluded, otherwise None.
In `@scripts/build-motion-library-v6.py`:
- Around line 1-34: Update the module docstring in the build script to describe
the v6 builder, including its 52-joint --v2 dump, qtmesh-motion-library-v4
output schema, and the correct v6 usage command. Add documentation for the
--curation and --approved-only options so the argparse description and help text
accurately reflect the script’s behavior; leave the implementation unchanged.
- Around line 661-675: Update clip_quality and its callers to accept the dump’s
joint width, using that width when normalizing rest-direction completeness
instead of CANON_COUNT. Also update frame_energy’s averaging range to use the
same width so V2 clips measure all joints while preserving the existing
body-joint count for V1 data.
In `@src/AnimationControlController.cpp`:
- Around line 2093-2099: After entity->_initialise(true) in the
generated-animation flow, immediately call rebindSelectedSkeleton() so
m_selectedSkeleton points to the recreated Ogre::SkeletonInstance. Preserve the
existing m_selectedTrack and m_currentKeyframe reset behavior.
In `@src/AnimationMerger.cpp`:
- Around line 2524-2528: Update the documentation for the relevant
AnimationMerger declaration in AnimationMerger.h to state that retargeting
computes and uses the clip-mean rest directions, and that the clipFingerRest
parameter is ignored. Remove the outdated claim that stored rest directions
affect the result; do not change the implementation.
- Around line 2683-2707: In AnimationMerger::applyFingerCurl(), validate that
every clipFingers row has the required kFingerSlots width before the
side/finger/segment loop indexes rows via fingerSlot(). Preserve the existing
empty-input handling and return early when any row is too narrow; no changes are
needed for the five-finger mapping.
In `@src/AnimationMerger.h`:
- Around line 296-300: Update AnimationMerger::fingerSlot() to reject side
values of 2 or greater and finger values of 5 or greater, returning -1 for those
inputs while preserving the existing checks and slot calculation for valid
ranges.
In `@src/CLIPipeline.cpp`:
- Around line 2719-2724: Update the in-process parity dump logic around QFile of
and dumpCanonicalPath to check both file opening and writing; report the failure
through the existing CLI error mechanism and return a nonzero error instead of
printing the success message. Only emit the success line and return 0 after the
dump is written successfully.
In `@src/MotionInbetween.cpp`:
- Around line 297-312: Update fingerRoleForBone’s named-finger matching loop so
a finger word is accepted only when preceded by an alphabetic boundary,
preventing matches inside names such as “spring” or “string.” Also require
confirmed hand context before returning a valid finger role, while preserving
the existing segment parsing and side assignment for legitimate hand/finger bone
names.
In `@src/MotionLibrary.cpp`:
- Around line 374-410: Remove the stale Q_UNUSED(dest) in ensureLibraryBlocking,
since dest is already used by the early-exit check. Update the V2/V1 fallback
flow and tryDownload result handling so a V2 timeout does not trigger a second
300-second download attempt; only perform the V1 fallback for fast V2 failures
such as a missing file, preserving the existing successful V2 return path.
- Around line 179-215: Enforce kFingerSlots width while parsing both finger
arrays: in the fingers parsing block, require every slotArr to contain exactly
kFingerSlots entries before accepting the row, and reject the entire fingers
payload when any row is invalid. In the fingerRestDir parsing block, require
fRest.size() to equal kFingerSlots and clear the parsed data on any invalid
entry, preserving the existing three-component validation.
In `@src/MotionLibrary.h`:
- Around line 99-102: Update the documentation for the quats member to describe
its size as frames × jointCount() rather than frames × 22, matching the variable
joint count returned by jointCount() for all supported schemas.
---
Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Around line 2558-2566: Update the generate-mode validation around
cmdAnimGenerate so an empty generatePrompt is allowed when generateVariant
specifies a variant, while still requiring a non-empty prompt for prompt-based
generation. Preserve the existing error and return code for requests lacking
both a prompt and a valid variant, and continue passing both arguments to
cmdAnimGenerate.
---
Nitpick comments:
In `@qml/AnimationPickerDialog.qml`:
- Around line 176-244: Rename the library index role returned by
AnimationControlController::listMotionClips() from index to clipIndex, then
update this delegate’s busyIndex comparison and applyClip() call to use
model.clipIndex while retaining the delegate index for row-based
toggleApproved().
- Around line 114-161: Replace the hand-rolled checkbox Rectangles and
MouseAreas in the lower-body and only-good controls with the shared
ThemedCheckBox component, preserving their labels, checked bindings, and
rebuildModel behavior. Replace the Close control with ThemedButton, preserving
its action, and add an Escape shortcut that triggers the same close behavior;
rely on the shared themed controls for keyboard focus, focus rings, and
Space/Return activation.
In `@scripts/build-motion-library-v5.py`:
- Around line 231-248: Update fix_first_frame_flip by unpacking the HIP
quaternion with underscore placeholders for the unused y and w values. Move the
nested up_y helper or the entire fix_first_frame_flip definition below the
module-level role constants, including HIP, so its global dependency is declared
before the function can be invoked; preserve the existing frame-repair behavior.
In `@scripts/build-motion-library-v6.py`:
- Around line 517-523: Update the hashlib.sha1 invocation in fingerprint to pass
usedforsecurity=False, preserving the existing deduplication input processing
and digest behavior while explicitly marking the hash as non-security.
- Around line 726-756: Add the Windows
`%APPDATA%/QtMeshEditor/QtMeshEditor/ai_models/motion/curation.json` candidate
to the default path search in the approved_only curation block, while preserving
the existing macOS and Linux candidates. Update the curation file loading near
json.load to use a context manager so the file handle is closed
deterministically.
- Around line 36-548: Extract the shared symbols listed in the review from both
build-motion-library-v5.py and build-motion-library-v6.py into
scripts/motion_library_common.py. Update both builders to import and reuse that
module, leaving only each builder’s distinct main() flow and output schema
local. Remove duplicated definitions and preserve all existing quality gates,
drop-list entries, and behavior through the shared implementation.
In `@src/MCPServer.cpp`:
- Around line 4302-4321: Compute the combined vertical-descent condition once in
a local const bool near the retarget flow, then reuse that variable for both the
retarget argument and the groundRootToFeet condition. Remove the duplicated
args.value and MotionLibrary::isVerticalDescentAction expression while
preserving the current default-enabled behavior.
In `@src/MotionInbetween_test.cpp`:
- Around line 303-318: Extend the MotionInbetween CanonicalV2BoneMatcher test
with negative cases for substring false positives and unsupported finger
variants: assert canonicalIndexForBoneV2 returns -1 for SpringBone_L_01, Bip001
L FingerNub, mixamorig:LeftHandPinkie2, and mixamorig:LeftHandIndex4. Keep the
existing positive mappings and non-bone assertion unchanged.
In `@src/MotionInbetween.cpp`:
- Around line 199-225: Update kFingerCountV2 to derive its value from
MotionInbetween::kFingerCount instead of redeclaring 5, and change the kF table
in fingerJointNameV2 to use that shared count. Add a static_assert verifying the
name table size matches kFingerCountV2, preserving the existing indexing and
naming behavior.
- Around line 57-67: Update bipedSideToken and normaliseBoneName to store their
respective QRegularExpression patterns as function-local static constants,
reusing the compiled separator and bip-prefix regexes across calls instead of
constructing them per invocation.
In `@src/MotionInbetween.h`:
- Around line 165-166: Remove the unused MotionInbetween::kMaxFingerSeg
declaration from the constants section in MotionInbetween.h, while preserving
kFingerCount and the existing AnimationMerger::kFingerSegs, kFingerSlots, and
MotionInbetween V2 role checks.
In `@src/MotionLibrary_test.cpp`:
- Around line 135-157: Extend MotionLibrary_test.cpp with a miniLibV2() helper
producing a schema-v4 document containing 52 joints and valid 52-wide poses. Add
tests covering jointCount() == 52, successful fingers and fingerRestDir parsing,
rejection of malformed finger arrays, and failure of v4 documents with 22-wide
poses using the joint-count error. Keep the existing schema-v1 and rootY
coverage unchanged.
In `@src/MotionLibrary.cpp`:
- Around line 333-349: Update MotionLibrary::saveCuration to use QSaveFile
instead of QFile, preserving the existing path and JSON generation while writing
atomically. Check the write result and return false on any short or failed
write; only return true after a successful commit(), and add the required
QSaveFile include.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 734fb456-9a6e-42e1-a4d4-067bbcf62fc0
📒 Files selected for processing (19)
qml/AnimationPickerDialog.qmlqml/PropertiesPanel.qmlscripts/build-motion-library-v5.pyscripts/build-motion-library-v6.pysrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/AnimationMerger_test.cppsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/MCPServer.cppsrc/MotionInbetween.cppsrc/MotionInbetween.hsrc/MotionInbetween_test.cppsrc/MotionLibrary.cppsrc/MotionLibrary.hsrc/MotionLibrary_test.cppsrc/qml_resources.qrc
…rary # Conflicts: # qml/AnimationPickerDialog.qml # scripts/build-motion-library-v5.py # src/AnimationControlController.cpp # src/AnimationControlController.h # src/AnimationMerger.cpp # src/AnimationMerger.h # src/AnimationMerger_test.cpp # src/CLIPipeline.cpp # src/MotionLibrary.cpp # src/MotionLibrary.h
- rebind cached SkeletonInstance after entity->_initialise(true) in generateMotion (dangling m_selectedSkeleton on next scrub/keyframe) - fingerRoleForBone: reject mid-word finger-name hits (ring in SpringBone/String/EarRing, index in IndexHelper) via separator-aware boundary + hand/side context; regression test - fingerSlot(): bound side<=1 and finger<=4 - MotionLibrary: enforce 30-wide fingers rows + fingerRestDir at parse; applyFingerCurl guards row width; no V1 fallback after a V2 download TIMEOUT (only after fast 404); drop stale Q_UNUSED - CLI --generate --duration: retime the V1 finger side-channel with the body (finger keys kept source duration); report parity-dump write failure - v6 builder: quality completeness normalizes on the 22 body joints (52-wide V2 rest arrays saturated every clip to 1.0, defeating curation weighting); v6 docstring rewritten for V2/curation; v5+v6: REVIEW_DROP word-boundary match, dead spine_up_min removed, stale comments corrected - QML: picker busy status defers the synchronous generateMotion so it paints; keyboard/Accessible support on browse button + pin-feet/descent checkboxes; doc fixes (quats size, clipFingerRest unused) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/AnimationMerger.cpp (2)
3555-3558: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInvalidate bind-referenced tracks before returning.
This path creates keyframes through Line 3628, then returns at Line 3636 without calling
NodeAnimationTrack::_keyFrameDataChanged(). Ogre can evaluate stale interpolation data after generation.Invalidate every populated entry in
tracksafter all frames are written and beforeres.ok = true.Proposed fix
+ for (auto* track : tracks) + if (track) + track->_keyFrameDataChanged(); res.ok = true;As per coding guidelines, “After modifying animation keyframes, call
track->_keyFrameDataChanged()so Ogre invalidates interpolation caches before evaluation.”🤖 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 3555 - 3558, After generation finishes and all keyframes have been written, iterate over every populated entry in tracks and call NodeAnimationTrack::_keyFrameDataChanged() before setting res.ok = true. Ensure this invalidation occurs on the returning path that assigns W[static_cast<size_t>(i)] = Wt, without changing keyframe generation behavior.Source: Coding guidelines
1857-1874: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSwap V2 finger roles with their parent hand roles.
When
cmuLibraryHandednessis true, this function swaps only body roles. It leaves V2 finger roles unchanged.The V2 retarget path then maps a physical left finger to its original left canonical role while its physical left hand maps to the right canonical role. Asymmetric hand motion can apply finger articulation to the wrong hand hierarchy.
Extend the side swap to every side-specific V2 finger role. Only perform the swap when a complete left/right detection pair exists.
🤖 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 1857 - 1874, Update the handedness swap logic around swapCanon and kLR to include every side-specific V2 finger role in the left/right mapping, not just body roles. Apply the remapping only when the corresponding complete left/right detection pair is available, while preserving existing canonical-index validation and swap behavior.qml/PropertiesPanel.qml (1)
4774-4829: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd keyboard accessibility to the new PartOps and Paint Layers buttons.
These new controls use a bare
Rectangle+MouseAreawith noactiveFocusOnTab, noAccessible.role/Accessible.name, and noKeys.onSpacePressed/Keys.onReturnPressed/Keys.onEnterPressedhandler:
- The layer toolbar buttons (
+,Dup,↑,↓,Mrg,Flat,✕) at Lines 4774-4829.- The per-layer eye/solo/lock toggles at Lines 4872-4930.
partOpsSplitBtnat Lines 6767-6805.partOpsExplodeBtnat Lines 6903-6936.partOpsJoinBtnat Lines 6938-6972.Keyboard and assistive-technology users cannot reach these controls. This file already fixed the same gap for
browseLibraryBtn,footPinChk, anddescentChkafter a prior review. Apply the same fix to these new controls.Proposed fix for partOpsSplitBtn (apply the same pattern to the others)
Rectangle { id: partOpsSplitBtn property bool clickEnabled: PartOpsController.hasSelection width: Math.min(parent ? parent.width - 16 : 200, partOpsSplitBtnLabel.implicitWidth + 20) height: 26 radius: 3 opacity: clickEnabled ? 1.0 : 0.45 color: partOpsSplitBtnMa.containsMouse && clickEnabled ? PropertiesPanelController.highlightColor : PropertiesPanelController.headerColor border.color: PropertiesPanelController.borderColor border.width: 1 + activeFocusOnTab: clickEnabled + Accessible.role: Accessible.Button + Accessible.name: "Split into Parts" + Keys.onSpacePressed: if (clickEnabled) partOpsSplitBtnMa.clicked(null) + Keys.onReturnPressed: if (clickEnabled) partOpsSplitBtnMa.clicked(null) + Keys.onEnterPressed: if (clickEnabled) partOpsSplitBtnMa.clicked(null) Text {Based on learnings, a prior review flagged the identical accessibility gap for
browseLibraryBtn/footPinChk/descentChkin this file, and the fix was: "Add focus, key activation, and button accessibility metadata."Also applies to: 4872-4930, 6767-6805, 6903-6936, 6938-6972
🤖 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 `@qml/PropertiesPanel.qml` around lines 4774 - 4829, Add keyboard and assistive-technology support to the layer toolbar Repeater buttons, per-layer eye/solo/lock toggles, and partOpsSplitBtn, partOpsExplodeBtn, and partOpsJoinBtn. Follow the established browseLibraryBtn/footPinChk/descentChk pattern: enable tab focus, provide Accessible.button metadata with meaningful names, and trigger the existing click actions from Space, Return, and Enter key handlers. Preserve the current mouse behavior and disabled-state semantics.Source: Learnings
src/CLIPipeline.cpp (1)
10848-10857: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
file.exportbreadcrumbs for PartOps output operations.These paths write user-requested output files but use operation-specific breadcrumb categories. Emit a
file.exportbreadcrumb for each output operation. Retain a separate operation breadcrumb if its extra context is needed.
src/CLIPipeline.cpp#L10848-L10857: emitfile.exportafter the label JSON is written.src/CLIPipeline.cpp#L10884-L10893: emitfile.exportafter the split mesh export succeeds.src/CLIPipeline.cpp#L10965-L10975: emitfile.exportafter the exploded scene export succeeds.As per coding guidelines, “Track all user-facing actions and significant operations with
SentryReporter::addBreadcrumb” and use “file.import/file.exportfor I/O.”🤖 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 10848 - 10857, Use the file.export breadcrumb category for all three successful PartOps output operations: after writing the label JSON in src/CLIPipeline.cpp lines 10848-10857, after the split mesh export in lines 10884-10893, and after the exploded scene export in lines 10965-10975. Retain the existing operation-specific breadcrumbs where their additional context is needed.Source: Coding guidelines
🤖 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 2660-2663: Update the nSteps and t calculation in the sampling
loop around nSteps so sampling always includes the terminal parity endpoint at
len, including clips shorter than 0.1 seconds. Preserve the existing 0.1-second
intervals, but clamp the final sample time to len so terminal-keyframe errors
are evaluated.
---
Outside diff comments:
In `@qml/PropertiesPanel.qml`:
- Around line 4774-4829: Add keyboard and assistive-technology support to the
layer toolbar Repeater buttons, per-layer eye/solo/lock toggles, and
partOpsSplitBtn, partOpsExplodeBtn, and partOpsJoinBtn. Follow the established
browseLibraryBtn/footPinChk/descentChk pattern: enable tab focus, provide
Accessible.button metadata with meaningful names, and trigger the existing click
actions from Space, Return, and Enter key handlers. Preserve the current mouse
behavior and disabled-state semantics.
In `@src/AnimationMerger.cpp`:
- Around line 3555-3558: After generation finishes and all keyframes have been
written, iterate over every populated entry in tracks and call
NodeAnimationTrack::_keyFrameDataChanged() before setting res.ok = true. Ensure
this invalidation occurs on the returning path that assigns
W[static_cast<size_t>(i)] = Wt, without changing keyframe generation behavior.
- Around line 1857-1874: Update the handedness swap logic around swapCanon and
kLR to include every side-specific V2 finger role in the left/right mapping, not
just body roles. Apply the remapping only when the corresponding complete
left/right detection pair is available, while preserving existing
canonical-index validation and swap behavior.
In `@src/CLIPipeline.cpp`:
- Around line 10848-10857: Use the file.export breadcrumb category for all three
successful PartOps output operations: after writing the label JSON in
src/CLIPipeline.cpp lines 10848-10857, after the split mesh export in lines
10884-10893, and after the exploded scene export in lines 10965-10975. Retain
the existing operation-specific breadcrumbs where their additional context is
needed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 40d8312a-ee3d-4a93-891e-b6f95672365a
📒 Files selected for processing (13)
qml/AnimationPickerDialog.qmlqml/PropertiesPanel.qmlscripts/build-motion-library-v5.pyscripts/build-motion-library-v6.pysrc/AnimationControlController.cppsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/MotionInbetween.cppsrc/MotionInbetween_test.cppsrc/MotionLibrary.cppsrc/MotionLibrary.h
🚧 Files skipped from review as they are similar to previous changes (8)
- src/MotionInbetween_test.cpp
- src/MCPServer.cpp
- qml/AnimationPickerDialog.qml
- src/MotionLibrary.h
- src/MotionInbetween.cpp
- src/MotionLibrary.cpp
- src/AnimationControlController.cpp
- src/AnimationMerger.h
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/CLIPipeline.cpp (3)
2334-2334: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
--v2valid and discoverable.The parser accepts
--v2, but the canonical usage at Line 2950 does not show it. The comment also describesqtmesh-motion-library-v4, while the command writesqtmesh-canonical-clips-v2at Line 3044. The builder emits the motion-library schema later.Update the usage and comment. Reject or explicitly handle
--v2for unrelated modes instead of silently ignoring it.Also applies to: 2424-2427
🤖 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` at line 2334, Update the --v2 handling across the argument parser and usage text near dumpCanonicalV2 so the canonical usage advertises the option and its comment matches the qtmesh-canonical-clips-v2 output and motion-library schema. Ensure --v2 is rejected or explicitly handled when used with unrelated modes rather than silently ignored, while preserving valid canonical-mode behavior.
2710-2745: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve the full canonical payload across dump and apply.
--apply-canonicalis documented to consume JSON written by--dump-canonical, but the parser at Lines 2605-2627 reads onlyquats,restWorld, andrestDir. Line 2635 passes norootYor finger side-channel data.The standard dump now writes
rootY, V1fingers, andfingerRestDir. The in-process dump writes none of these fields. A dump → apply → dump flow therefore loses root descent and V1 finger motion.Parse and pass these fields through the apply path, or define a separate reduced parity format. Add a V1 round-trip test covering
rootY,fingers, andfingerRestDir.Also applies to: 3102-3134
🤖 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 2710 - 2745, Preserve the complete canonical payload through dump and apply: update the JSON parsing and apply flow around the canonical input handling and the in-process dump block near r2["clips"] to read, pass, and emit rootY, V1 fingers, and fingerRestDir alongside quats, restWorld, and restDir. Ensure --dump-canonical output can round-trip through --apply-canonical without losing root descent or finger motion, and add a V1 round-trip test covering all three fields.
2692-2695: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace
M_PIwith a portable pi value.
M_PIis not guaranteed by standard C++. Use the project’sOgre::Math::PIor add a localconstexprvalue instead of relying on platform-specific math defines.🤖 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 2692 - 2695, In the degree conversion inside the loop over nB, replace the non-portable M_PI reference with the project’s portable Ogre::Math::PI constant, or a local constexpr pi value if that symbol is unavailable. Keep the existing conversion and threshold behavior unchanged.Source: Coding guidelines
🧹 Nitpick comments (1)
src/CLIPipeline.cpp (1)
3032-3034: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord canonical output as a file export.
This path writes a user-selected canonical JSON file, but the breadcrumb at Line 3029 uses
ai.tool_call. The guideline reservesai.tool_callfor MCP tools and requiresfile.exportfor I/O.As per coding guidelines, use
file.exportfor this canonical dump.🤖 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 3032 - 3034, Update the canonical JSON dump path in the surrounding CLIPipeline export flow to record the breadcrumb with the file.export event instead of ai.tool_call. Keep the existing canonical output generation and error handling unchanged.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Line 2334: Update the --v2 handling across the argument parser and usage text
near dumpCanonicalV2 so the canonical usage advertises the option and its
comment matches the qtmesh-canonical-clips-v2 output and motion-library schema.
Ensure --v2 is rejected or explicitly handled when used with unrelated modes
rather than silently ignored, while preserving valid canonical-mode behavior.
- Around line 2710-2745: Preserve the complete canonical payload through dump
and apply: update the JSON parsing and apply flow around the canonical input
handling and the in-process dump block near r2["clips"] to read, pass, and emit
rootY, V1 fingers, and fingerRestDir alongside quats, restWorld, and restDir.
Ensure --dump-canonical output can round-trip through --apply-canonical without
losing root descent or finger motion, and add a V1 round-trip test covering all
three fields.
- Around line 2692-2695: In the degree conversion inside the loop over nB,
replace the non-portable M_PI reference with the project’s portable
Ogre::Math::PI constant, or a local constexpr pi value if that symbol is
unavailable. Keep the existing conversion and threshold behavior unchanged.
---
Nitpick comments:
In `@src/CLIPipeline.cpp`:
- Around line 3032-3034: Update the canonical JSON dump path in the surrounding
CLIPipeline export flow to record the breadcrumb with the file.export event
instead of ai.tool_call. Keep the existing canonical output generation and error
handling unchanged.
Falls back to showing all clips when nothing is starred (fresh install / un-curated library) so the list is never silently empty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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
Ships the V2 motion pipeline: fingers become part of the canonical data model (52-joint skeleton) instead of a bolt-on side-channel, the finger retarget moves to the standard bind-referenced change-of-basis transport, and a user-curated 36-clip library is published and auto-downloaded (already live on the HF
QtMeshEditor-modelsrepo undermotion/).What's in here
2 sides × 5 fingers × 3 segments). Full parallel*V2API; V1 entry points untouched so existing libraries/models keep working.motion-library-v2.jsonfirst and falls back to the v1 file (old builds are unaffected — they only request the v1 name and reject the v4 schema).Drel = Dp⁻¹·Dfonto the target bind, riding the target's animated hand). Measured 0.2° parity against source clips; fixes the flipped-fingernail and straightened-finger artifacts. Source-empty segments hold bind (a transported identity used to counter-rotate fingertips).curation.json, keyed by stable clip source);build-motion-library-v6.py --approved-onlyships exactly the starred set.--apply-canonicalharness;QTMESH_EXTRACT_DEBUGreference-frame tracing.Known limits (documented in-code, next-iteration work)
bindC) — prescribed in comments.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit