feat(t2m): curate library + Animation picker (#838)

[fernandotonon]

Summary

Stacked on #919 (locomotion cull gate). Two things, both driven by hands-on review of every library variant:

1. Library curation (build-motion-library-v5.py)

Dropped clips by license, bad retarget, and user review:

• Mixamo-derived clips (MIXAMO_MARKERS) — a Sketchfab CC-BY upload doesn't clear Adobe's ToS on the underlying animation; not redistributable.
• Review drop-list (REVIEW_DROP, matched by asset+anim so it survives rebuilds):
  • GIGI & KAI Fox rigs — tip/hunch/invert on every action.
  • Shar Pei dog — wrong body plan.
  • 4 Quaternius packs (Animated Men/Women Feb-2019, Alien Apr-2019, Knight Jul-2018) — mis-map the right upper-arm bone: it stays raised/out (mean up-Y +0.3…+0.7) the whole clip vs −0.7…−0.9 for a correct hanging arm. Matched by full pack name so the good "Man/Woman Animated" Oct/Dec-2017 packs are preserved.
• fix_first_frame_flip — Mini Chibi Kid (and similar) export frame 0 with a rotated hip while the rest is upright (loop-seam artifact); detect + replace frame 0 with frame 1. Verified walk/run/jump open upright; the genuinely-horizontal death clip is left untouched.

Library 109 → 74 clips / 18 actions, zero Mixamo/Fox/dog. (swim/roll dropped with the arm-broken packs — re-scrape clean ones later.)

2. Animation picker

Replace the quality-weighted random pick with explicit browse-and-select:

• AnimationPickerDialog.qml — searchable list of every clip with a human-readable label ("Walk (Man)", "Run (Zombie)", "Punch (Alien)") and a per-row Apply that retargets that exact clip onto the selected rig.
• AnimationControlController::listMotionClips() → the list model; generateMotion(variantIndex) forces a specific clip (no model, no matchAmong).
• Panel: "Browse animation library…" is now the primary button; the free-text field is relabelled as the experimental AI-model path only.
• CLI: qtmesh anim <file> --generate <action> --variant N (the curation harness that produced the review sheets).

Verified live: picker lists 74 named clips and Apply retargets correctly.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added an animation library picker dialog with search, scrollable results, progress/outcome status, and one-click “Apply” to generate an animation.
  • Added a lower-body “descent” option that improves crouch/pickup/sit/crawl-type motions, with deterministic selection support via a library variant index.
  • Updated generation and export to carry per-clip vertical offset data (when available).
• Bug Fixes
  • Improved motion-library curation and corrected first-frame rotation issues.
  • Enhanced retargeting accuracy to preserve natural upright posture and vertical movement during application.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds a searchable QML animation-library picker and extends motion generation with explicit clip selection and optional vertical descent. Motion extraction, library parsing, CLI/MCP flows, retargeting, and tests now carry per-frame root-Y descent data.

Changes

Animation library and vertical descent

Layer / File(s)	Summary
Clip metadata and extraction scripts/build-motion-library-v5.py, src/AnimationMerger.*, src/MotionLibrary.*, src/*test.cpp	Clip filtering and first-frame correction are added; canonical extraction and JSON parsing now produce validated per-frame rootY data and classify vertical-descent actions.
Root descent retargeting src/AnimationMerger.*, src/AnimationMerger_test.cpp	Bind-referenced and pure-local retargeting optionally lower root translations from negative rootY values scaled by target leg length; tests cover enabled, disabled, descending, and rising frames.
Generation API and integrations src/AnimationControlController.*, src/CLIPipeline.*, src/MCPServer.cpp	Generation supports explicit clip indices, root-Y retiming, descent controls, CLI flags, MCP schema input, and canonical dump output.
Animation picker UI qml/AnimationPickerDialog.qml, qml/PropertiesPanel.qml, src/qml_resources.qrc	A searchable picker lists clips and applies exact selections, while the properties panel exposes the dialog, propagates results, and adds a lower-body toggle.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PropertiesPanel
  participant AnimationPickerDialog
  participant AnimationControlController
  participant MotionLibrary
  participant AnimationMerger
  PropertiesPanel->>AnimationPickerDialog: open()
  AnimationPickerDialog->>AnimationControlController: listMotionClips()
  AnimationControlController->>MotionLibrary: load clips
  MotionLibrary-->>AnimationPickerDialog: clip metadata
  AnimationPickerDialog->>AnimationControlController: generateMotion(variantIndex, verticalDescent)
  AnimationControlController->>MotionLibrary: resolve selected clip and rootY
  AnimationControlController->>AnimationMerger: applyMotionClip(clipRootY, verticalDescent)
  AnimationMerger-->>AnimationControlController: applied animation result
  AnimationControlController-->>AnimationPickerDialog: success or error
  AnimationPickerDialog-->>PropertiesPanel: applied(animation, entity)

Loading

Possibly related PRs

• fernandotonon/QtMeshEditor#783: Shares the generateMotion integration point and prompt-to-motion pipeline.
• fernandotonon/QtMeshEditor#792: Shares the AnimationMerger::applyMotionClip retargeting entry point.
• fernandotonon/QtMeshEditor#904: Shares the motion-generation path and generateMotion argument wiring.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise and accurately summarizes the main changes: motion-library curation plus an animation picker.
Description check	✅ Passed	The description is detailed and on-topic, but it omits the template’s explicit Technical Details, Features, and Bugfixes sections.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/t2m-curate-library

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b9dd171363

ℹ️ 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".

[chatgpt-codex-connector]

Move picker callback into animation component scope

When using the new Browse animation library flow and clicking Apply, this applied handler runs in the top-level PropertiesPanel/Loader scope, but lastGeneratedAnim, setArmSpaceTarget, and refreshAnimData are declared inside the animationComponent instance below, not on root. A successful apply therefore hits a QML ReferenceError in this callback instead of wiring the generated clip into the panel/arm-space state; keep the connection in the animation component instance or expose a root-level method that can reach it.

Useful? React with 👍 / 👎.

[fernandotonon]

Fixed — good catch. The picker's applied handler was in the top-level Loader scope where lastGeneratedAnim/setArmSpaceTarget/refreshAnimData (declared in the animationComponent) don't resolve, so Apply threw a ReferenceError. The Loader now re-emits a root-level animationPicked(animation, entity) signal, and a Connections { target: root } inside the animation component does the wiring in-scope. Verified the app loads with no QML errors.

[coderabbitai]

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/MCPServer.cpp (1)

4183-4185: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Expose exact clip selection through MCP.

The MCP path still requires prompt and always uses matchPrompt(), so callers cannot select a curated variant deterministically.

• src/MCPServer.cpp#L4183-L4185: accept variant_index, allow it in place of prompt, force the template path, and bounds-check it before reading the clip.
• src/MCPServer.cpp#L8859-L8869: publish variant_index in generate_motion and make prompt optional when an index is supplied.

🤖 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 4183 - 4185, Update src/MCPServer.cpp lines
4183-4185 in the generate_motion handling to accept variant_index as an
alternative to prompt, force the template-based selection path when it is
provided, and bounds-check the index before accessing the clip; retain prompt
validation when neither input is supplied. Update src/MCPServer.cpp lines
8859-8869 to expose variant_index in the generate_motion MCP schema and make
prompt optional when variant_index is present.

🧹 Nitpick comments (2)

qml/AnimationPickerDialog.qml (1)

210-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Busy indicator never actually renders — generateMotion runs synchronously between the set/reset of busyIndex.

dialog.busyIndex = idx and dialog.busyIndex = -1 bracket a single synchronous call with no yield to the event loop in between, so the "…" busy state on the Apply button (bound to busyIndex) never gets a chance to paint before the (potentially slow) retarget completes — the UI just appears to freeze with no feedback.

🛠️ Proposed fix — defer the call so the busy state can paint first

     function applyClip(idx, name) {
         if (dialog.busyIndex >= 0) return
         dialog.busyIndex = idx
         pickStatus.isError = false
         pickStatus.text = "Applying " + name + "…"
-        // variantIndex forces this exact clip (template path, no random pick).
-        // Pass the picker's own descent checkbox (`#838`) as the last arg.
-        var r = AnimationControlController.generateMotion("", 0.0, false, 0.0, true, idx,
-                                                          pickDescentChk.checked)
-        dialog.busyIndex = -1
-        if (r && r.ok) {
-            pickStatus.text = "Applied: " + name
-            dialog.applied(r.animation || "", r.entity || "")
-        } else {
-            pickStatus.isError = true
-            pickStatus.text = (r && r.error) ? r.error : "Failed to apply."
-        }
+        Qt.callLater(function() {
+            // variantIndex forces this exact clip (template path, no random pick).
+            // Pass the picker's own descent checkbox (`#838`) as the last arg.
+            var r = AnimationControlController.generateMotion("", 0.0, false, 0.0, true, idx,
+                                                              pickDescentChk.checked)
+            dialog.busyIndex = -1
+            if (r && r.ok) {
+                pickStatus.text = "Applied: " + name
+                dialog.applied(r.animation || "", r.entity || "")
+            } else {
+                pickStatus.isError = true
+                pickStatus.text = (r && r.error) ? r.error : "Failed to apply."
+            }
+        })
     }

🤖 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 210 - 227, Update applyClip so
the generateMotion call executes asynchronously after busyIndex is set, yielding
to the event loop so the busy state can render before processing begins. Keep
the existing success/error handling and reset busyIndex after generateMotion
completes, using the dialog’s established deferred-callback mechanism if
available.

scripts/build-motion-library-v5.py (1)

694-711: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Stale/contradictory comment on rootY windowing.

The block comment at Lines 694-697 says the sliced rootY is carry the per-frame hip Y offset, sliced to the SAME active window as the quats and re-based so frame 0 of the window reads ~0 (the retarget deltas the descent against its start frame), but the code that follows explicitly does the opposite: rootY is a crouch DEPTH vs the rig's BIND-pose standing height (absolute, ≤ 0 leg-lengths), so an always-low crawl/sit keeps its real depth — do NOT re-anchor to the window. Only the window-slice + 0.6 gain are applied — there is no re-basing to zero. A future maintainer reading only the top comment could "fix" this into re-anchoring the descent depth, silently breaking always-low clips (crawl/sit).

Update the top-level comment to match the actual (correct) behavior described below it.

📝 Proposed comment fix

-                        # `#838` vertical descent: carry the per-frame hip Y
-                        # offset, sliced to the SAME active window as the quats
-                        # and re-based so frame 0 of the window reads ~0 (the
-                        # retarget deltas the descent against its start frame).
+                        # `#838` vertical descent: carry the per-frame hip Y
+                        # offset, sliced to the SAME active window as the
+                        # quats. NOT re-based to the window start — rootY is
+                        # an absolute depth vs bind-pose standing height (see
+                        # below), so re-anchoring would zero out clips that
+                        # open already crouched.
                         ry = c.get("rootY")

🤖 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 694 - 711, Update the
top-level comment above the rootY handling in the active window processing to
remove the incorrect claim that rootY is re-based to zero. Describe that rootY
is sliced to the same window as the quaternions, preserves absolute crouch depth
relative to the bind-pose standing height, and receives the existing 0.6 display
gain.

🤖 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/AnimationControlController.cpp`:
- Around line 1840-1846: Update AnimationControlController::listMotionClips() to
emit a ui.action breadcrumb immediately before calling
MotionLibrary::ensureLibraryBlocking(). Record the picker-library operation,
then preserve the existing blocking library acquisition and early-return
behavior.

In `@src/AnimationMerger_test.cpp`:
- Around line 1346-1415: Update the skeleton setup in
VerticalDescentLowersRootDescentOnly so the right and left leg chains are
vertically aligned beneath Hips, removing their horizontal ±0.15 offsets while
preserving the existing arm offsets. Keep the foot positions and test
expectations unchanged so the bind-pose Euclidean leg length is exactly 1.0.

In `@src/AnimationMerger.cpp`:
- Around line 2667-2690: Update the vertical-descent measurement around
derivedYForCanon and the related target-position lookup to use target bind-pose
positions from readTargetBindFrame() and tb.bindPos. Ensure the helper resets
and updates the skeleton to bind pose before reading positions, then restores
the previously applied pose. Compute targetLegLen from bind-pose hip and foot
positions so crouched runtime poses cannot affect scaling.

In `@src/CLIPipeline.cpp`:
- Around line 2405-2410: Update the --variant parsing in the argument-processing
block to validate conversion with QString::toInt’s ok flag and reject negative
indices. For malformed or negative values, emit the existing usage/error
response and terminate with exit code 2; only assign generateVariant and
continue when the supplied value is a valid non-negative integer.

In `@src/MotionLibrary.cpp`:
- Around line 345-356: Add the canonical "crouch" action label to the kDescent
set in MotionLibrary::isVerticalDescentAction, preserving the existing
normalization and classification behavior for all other actions.

In `@src/MotionLibrary.h`:
- Around line 47-52: Update the rootY documentation in the optional
vertical-descent field comment to state that values preserve bind-pose-relative
hip depth rather than being re-based to the selected window’s first frame. Keep
the existing normalization, target-leg-length scaling, root-bone Y application,
and empty-value behavior descriptions unchanged.

---

Outside diff comments:
In `@src/MCPServer.cpp`:
- Around line 4183-4185: Update src/MCPServer.cpp lines 4183-4185 in the
generate_motion handling to accept variant_index as an alternative to prompt,
force the template-based selection path when it is provided, and bounds-check
the index before accessing the clip; retain prompt validation when neither input
is supplied. Update src/MCPServer.cpp lines 8859-8869 to expose variant_index in
the generate_motion MCP schema and make prompt optional when variant_index is
present.

---

Nitpick comments:
In `@qml/AnimationPickerDialog.qml`:
- Around line 210-227: Update applyClip so the generateMotion call executes
asynchronously after busyIndex is set, yielding to the event loop so the busy
state can render before processing begins. Keep the existing success/error
handling and reset busyIndex after generateMotion completes, using the dialog’s
established deferred-callback mechanism if available.

In `@scripts/build-motion-library-v5.py`:
- Around line 694-711: Update the top-level comment above the rootY handling in
the active window processing to remove the incorrect claim that rootY is
re-based to zero. Describe that rootY is sliced to the same window as the
quaternions, preserves absolute crouch depth relative to the bind-pose standing
height, and receives the existing 0.6 display gain.

🪄 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 Plus

Run ID: 8c18f21f-d1b9-40fa-aab1-3ff27e7bdd65

📥 Commits

Reviewing files that changed from the base of the PR and between ebd8531 and c0807c5.

📒 Files selected for processing (15)

• qml/AnimationPickerDialog.qml
• qml/PropertiesPanel.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/CLIPipeline.h
• src/MCPServer.cpp
• src/MotionLibrary.cpp
• src/MotionLibrary.h
• src/MotionLibrary_test.cpp
• src/qml_resources.qrc

[coderabbitai]

🧹 Nitpick comments (1)

src/MCPServer.cpp (1)

4184-4193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the required MCP breadcrumb category.

This MCP invocation is logged as ai.assist.text_to_motion; update it to ai.tool_call and include the selected variant/action without logging the prompt. As per coding guidelines, MCP invocations must use ai.tool_call breadcrumbs.

🤖 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 4184 - 4193, Update the breadcrumb emitted by
the MCP invocation around the variant/prompt validation in the relevant
text-to-motion handler: use the required category ai.tool_call instead of
ai.assist.text_to_motion, and record the selected variant/action while excluding
the prompt from logged data.

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.

Nitpick comments:
In `@src/MCPServer.cpp`:
- Around line 4184-4193: Update the breadcrumb emitted by the MCP invocation
around the variant/prompt validation in the relevant text-to-motion handler: use
the required category ai.tool_call instead of ai.assist.text_to_motion, and
record the selected variant/action while excluding the prompt from logged data.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1ccf577c-4b57-47fe-815d-c08356835053

📥 Commits

Reviewing files that changed from the base of the PR and between c0807c5 and d6811e9.

📒 Files selected for processing (7)

• src/AnimationControlController.cpp
• src/AnimationMerger.cpp
• src/AnimationMerger_test.cpp
• src/CLIPipeline.cpp
• src/MCPServer.cpp
• src/MotionLibrary.cpp
• src/MotionLibrary.h

🚧 Files skipped from review as they are similar to previous changes (6)

• src/MotionLibrary.cpp
• src/MotionLibrary.h
• src/AnimationMerger_test.cpp
• src/CLIPipeline.cpp
• src/AnimationControlController.cpp
• src/AnimationMerger.cpp

[sonarqubecloud]

Quality Gate passed

Issues
16 New issues
0 Accepted issues

Measures
0 Security Hotspots
26.6% Coverage on New Code
5.5% Duplication on New Code

See analysis details on SonarQube Cloud