t2m v2 (epic #837): twist transport, foot pinning, flow-matching model#904
Open
fernandotonon wants to merge 10 commits into
Open
t2m v2 (epic #837): twist transport, foot pinning, flow-matching model#904fernandotonon wants to merge 10 commits into
fernandotonon wants to merge 10 commits into
Conversation
The bind-referenced direction retarget aimed each bone but inherited the target bind's roll — gesture clips read flat and the roll component between mismatched decomposition poles leaked into swing (arm amplitude +38% on Mixamo self-retarget). Now the source's swing/twist split is recomposed about the SAME pole on the target: Qbase = arc(dt_bind -> d_ref) * Wt_bind (once per bone) Wt(f) = twist(theta*gain, ds) * arc(d_ref -> ds) * Qbase theta is the source's signed roll about its reference bone direction, wrapped to (-pi,pi] and unwrapped across frames (a +-180deg pop would flip the roll once a gain != 1 scales it), capped at 150deg; collars are damped 0.5x (they share the shoulder line's roll). Direction tracking stays absolute, so cross-rig robustness is unchanged. Mixamo Walk self-parity: 2.22deg -> 0.03deg mean joint delta (target was <=1.6deg); spine amplitude restored (abdomen 2.3deg -> 9.0deg amp), legs exact; Rumba walk/dance/wave and Quaternius Knight renders upright with natural hanging arms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pler, reference triple (#840, #858) Data (#858): scripts/prep-t2m-v5.py canonicalizes every clip (corpus *.canonical.json dumps + CMU BVH) into a rig-independent representation before windowing — per joint, aim-from-canonical-T-pose composed with the swing/twist residual: Q'(f) = twist(theta, d(f)) * arc(D_c -> d(f)). Identical for every rig performing the same motion, and exactly what the bind-referenced retarget consumes given the canonical reference triple (restWorld = identity, restDir = D_c). 17,286 windows / 18 actions (corpus 517 + CMU 16,789), replacing the v4 per-rig convention mush. Model (#840): scripts/train-t2m-flow-v5.py — small DiT-style velocity transformer (7.3M params, 6D rotations, AdaLN-Zero conditioning) trained with the rectified-flow objective + classifier-free guidance; flow matching samples a transport path to a single data mode instead of decoding a conditional mean (the CVAE's structural weakness). The Euler sampler (+CFG) is UNROLLED INSIDE the exported ONNX graph, so the shipped MotionGenerator contract (tokens[1,V], seed[1,Z] -> motion [1,T,220]) is unchanged — seed is the flattened noise tensor. Wiring (#858): MotionGenerator parses the vocab's restWorld/restDir into the generated clip; CLI/MCP/GUI model paths pass it to applyMotionClip, so model clips ride the SAME bind-referenced direction retarget as v5 template clips — the synthetic-standing-pose shim remains only as the legacy-v4 fallback. applyMotionClip now treats any valid unit-norm reference quat as present (identity counts; only zero-filled entries mean absent), which the v5 identity triple requires. Render-verified on the Rumba rig: model walk/run/wave/dance all upright and temporally coherent end-to-end (the v4 CVAE folded or tumbled); templates remain the default + automatic fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Retargeted clips slide/float feet on rigs whose proportions differ from the source — the direction retarget transfers bone directions, not world foot positions. Core (FootContact.h/cpp, Ogre-free + unit-tested): contact detection (foot within a leg-length-scaled height band of the clip's ground level AND nearly stationary horizontally -> spans; ground = low percentile so jump clips don't skew it), analytic two-bone IK (solveKnee — keeps both segment lengths, preserves the pose's own bend plane, clamps unreachable targets), and the 0->1->0 edge blend weight. AnimationMerger::pinFeet: manual FK over the tracks (pure track math — the live skeleton is never apply()'d), detection in the canonical frame (Ct) so ground is horizontal regardless of rig axes, then per contact frame locks the foot to its span-start position blended over blendFrames: thigh re-aimed at the IK knee, shin at the pinned target, foot keyframe compensated to keep its ORIGINAL world orientation (no toe pop). Only thigh/shin/foot keyframes rewritten; _keyFrameDataChanged on each (the #854 cache gotcha). Effectively idempotent (re-detection re-pins to the same targets); 'qtme.footpin.<anim>' marker on bone[0], cleared when applyMotionClip regenerates the clip. Surfaces: ON by default in generation — CLI --no-foot-pin opts out, standalone 'qtmesh anim <f> --foot-pin --animation <name> -o out'; MCP foot_pin arg on generate_motion + standalone pin_feet tool; GUI 'Pin feet (contact cleanup)' checkbox in the Animations section. Also: canonicalIndexForBone learns 'upperleg' as a thigh alias — Quaternius-style rigs (UpperLeg.L/LowerLeg.L) previously mapped BOTH leg bones to the knee role, which broke pinning and double-aimed the knee in the retarget. Verified: Rumba walk 2 contact spans pinned; Knight (UpperLeg/LowerLeg naming) pins after the alias fix; renders upright, stride intact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ectation (#857) The arm-rig helper resolves only 9/22 roles, below applyMotionClip's >=11 humanoid gate — the roll test now builds a 13-role rig (+hands, +feet). And the 150-degree runaway-unwrap cap applies BEFORE the collar gain, so a 240-degree source roll lands at 150 x 0.5 = 75 degrees, not 120; the continuity assertion (the actual unwrap regression check) is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 14, 2026
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds v5 canonical motion preparation and flow-matching export, propagates canonical rest metadata through retargeting, introduces foot-contact detection and IK pinning, and exposes cleanup through QML, CLI, MCP, and generated-motion workflows. ChangesMotion generation pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant QML
participant AnimationControlController
participant MotionGenerator
participant AnimationMerger
QML->>AnimationControlController: generateMotion(prompt, footPin)
AnimationControlController->>MotionGenerator: generate canonical motion
MotionGenerator-->>AnimationControlController: clip with restWorld/restDir
AnimationControlController->>AnimationMerger: smooth bake and pin feet
AnimationMerger-->>AnimationControlController: pinned spans and keyframes
AnimationControlController-->>QML: generation result and footPinSpans
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faa7f4f4e7
ℹ️ 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
+1588
to
+1592
| if (!thighTrk || !shinTrk | ||
| || static_cast<int>(thighTrk->getNumKeyFrames()) != nk | ||
| || static_cast<int>(shinTrk->getNumKeyFrames()) != nk | ||
| || (footTrk && static_cast<int>(footTrk->getNumKeyFrames()) != nk)) | ||
| continue; // mixed keyframe grids — not a generated clip |
There was a problem hiding this comment.
When --foot-pin/pin_feet is run on an authored animation where the thigh, shin, and foot tracks have the same number of keys but not the same key times, this count-only guard lets the post-process proceed as if the grids match. The FK samples are taken at timeSrc times, but the rewrite later uses getNodeKeyFrame(k) on the shin/foot tracks, so corrections computed for one timestamp can be written to a different timestamp and corrupt the leg pose; compare the actual keyframe times or resample/create keys before editing.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
src/AnimationMerger.cpp (2)
1803-1809: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
footPinKey()helper instead of a hardcoded literal.
footPinKey(animName)is defined right above (Line 1475-1478) specifically to produce this key, and the siblingarmSpaceKey(animName)call on the previous line correctly uses its helper — but this line re-types the literal"qtme.footpin." + animName"instead of callingfootPinKey(animName). If the key format ever changes, only one of the two call sites (the setter inpinFeet, Line 1697-1698) would get updated, silently breaking the erase-on-regenerate behavior.♻️ Proposed fix
// `#856`: same for the foot-pin marker — a regenerated clip is unpinned. skel->getBone(0)->getUserObjectBindings().eraseUserAny( - "qtme.footpin." + animName); + footPinKey(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/AnimationMerger.cpp` around lines 1803 - 1809, Replace the hardcoded foot-pin key construction in the regeneration cleanup block with the existing footPinKey(animName) helper. Keep the surrounding getUserObjectBindings().eraseUserAny calls and armSpaceKey(animName) behavior unchanged.
1481-1701: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winpinFeet math checks out; consider hardening the mixed-grid guard.
The FK reconstruction, two-bone IK correction (S1/S2), and keyframe-formula composition are consistent with the
bindLocal⁻¹·Wp⁻¹·Wnewconvention used elsewhere in this file (e.g.adjustArmSpace). One spot worth tightening: the guard at Line 1588-1592 only compares keyframe counts across thigh/shin/foot tracks to detect "mixed keyframe grids," which is a proxy for "same time grid." It's correct for the primary target (generated clips, all tracks sharing anf*dtgrid), but two tracks could coincidentally share a count while differing in actual keyframe times, silently mis-aligning the rewrite. Comparing the actual time arrays (not just counts) would close that gap cheaply.🤖 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 1481 - 1701, The mixed-grid validation in pinFeet currently checks only keyframe counts; update it to compare each thigh, shin, and optional foot track’s actual keyframe times against the reference times array before processing the leg. Skip the leg when any track’s timestamps differ, while preserving the existing count checks and generated-clip behavior.src/MCPServer.cpp (2)
4272-4341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
file.exportbreadcrumb for the re-export step.
toolPinFeetadds anai.tool_callbreadcrumb for the tool invocation, but the export viaMeshImporterExporter::exporterat line 4319 has no accompanying breadcrumb. As per coding guidelines,"Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb; ... file.import/file.export for I/O."— the export write should get its ownfile.exportbreadcrumb for observability parity with other I/O-performing tools.🔧 Proposed fix
const QString outPath = args.value("output_path").toString(); if (!outPath.isEmpty()) { + SentryReporter::addBreadcrumb(QStringLiteral("file.export"), + QStringLiteral("MCP pin_feet export: %1").arg(outPath)); auto* node = entity->getParentSceneNode(); if (MeshImporterExporter::exporter( node, outPath, CLIPipeline::formatForExtension(outPath)) != 0)🤖 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 4272 - 4341, Add a SentryReporter::addBreadcrumb call immediately before MeshImporterExporter::exporter in MCPServer::toolPinFeet, using the file.export category and including the output path or equivalent export context. Keep the existing export failure handling and result behavior unchanged.Source: Coding guidelines
4144-4150: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFoot-pin failure is silently swallowed in
generate_motion.When
AnimationMerger::pinFeetfails,fp.erroris discarded andfootPinSpansjust falls back to-1— identical to the "disabled" state. The caller has no way to distinguish "opted out" from "ran and failed" for debugging, unliketoolPinFeetwhich surfacesfp.errordirectly.🛡️ Proposed fix to surface the failure reason
int footPinSpans = -1; + QString footPinError; if (args.value("foot_pin").toBool(true)) { const auto fp = AnimationMerger::pinFeet(skel.get(), animName); - footPinSpans = fp.ok ? fp.spans : -1; + footPinSpans = fp.ok ? fp.spans : -1; + if (!fp.ok) footPinError = fp.error; }And later:
if (footPinSpans >= 0) content["foot_pin_spans"] = footPinSpans; + if (!footPinError.isEmpty()) content["foot_pin_error"] = footPinError;🤖 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 4144 - 4150, Update the foot-pinning block in generate_motion around AnimationMerger::pinFeet so failed pinFeet results surface fp.error to the caller, matching toolPinFeet behavior. Preserve footPinSpans == -1 for the explicit foot_pin:false opt-out, but do not silently treat an enabled-operation failure as disabled.qml/PropertiesPanel.qml (1)
8539-8561: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the existing
InspectorCheckBoxcomponent over a raw Rectangle checkbox.
footPinChkduplicates the ad-hoc Rectangle+MouseArea checkbox pattern ofuseModelChkright above it, instead of the file's own themedInspectorCheckBoxcomponent (lines 25-64), which already provides keyboard focus,Keys.on*Pressedactivation, andAccessible.*properties. As written, this checkbox can't be toggled via keyboard and has no accessible name — a task-completion gap for keyboard/assistive-tech users.♻️ Proposed fix
- Row { - spacing: 6 - Rectangle { - id: footPinChk - property bool checked: true - width: 14; height: 14; radius: 2 - anchors.verticalCenter: parent.verticalCenter - color: checked ? PropertiesPanelController.highlightColor - : PropertiesPanelController.inputColor - border.color: PropertiesPanelController.borderColor - Text { anchors.centerIn: parent; visible: parent.checked - text: "✓"; color: "white"; font.pixelSize: 10 } - MouseArea { anchors.fill: parent - onClicked: footPinChk.checked = !footPinChk.checked } - } - Text { - text: "Pin feet (contact cleanup)" - color: PropertiesPanelController.textColor; font.pixelSize: 10 - anchors.verticalCenter: parent.verticalCenter - } - } + InspectorCheckBox { + id: footPinChk + text: "Pin feet (contact cleanup)" + checked: true + }(Update the read site at line 8496 to
footPinChk.checked, which already works withCheckBox's built-in property.)🤖 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 8539 - 8561, Replace the raw Rectangle and MouseArea implementation of footPinChk with the existing InspectorCheckBox component, preserving its default checked state and label behavior. Update the related read site to use footPinChk.checked, and configure the component’s accessible text/name for “Pin feet (contact cleanup)” so keyboard and assistive-technology interaction work through the shared component.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/prep-t2m-v5.py`:
- Around line 213-215: Guard the per-item processing in the clip iteration
around np.asarray/canonicalize and the BVH processing around
prep4.parse_bvh/bvh_rest with try/except so malformed clips are skipped without
aborting the batch. Catch processing exceptions, log the skipped item and
exception details, then continue; preserve successful yields and existing
JSON-loading behavior.
- Around line 331-338: Guard the vocabulary-building block after computing vocab
and keep: if no actions meet a.min_action_windows, exit with a clear error
message before calling np.stack. Preserve the existing stacking behavior when
keep is non-empty, using the surrounding script’s established sys.exit
validation style.
In `@scripts/train-t2m-flow-v5.py`:
- Line 189: Update the np.load call in the training data-loading flow to remove
allow_pickle=True and explicitly use the safe default of allow_pickle=False,
while preserving the existing archive loading behavior.
In `@scripts/upload-t2m-v5-model.sh`:
- Around line 31-43: Make the backup loop in the upload script idempotent by
checking whether each destination in the case mapping already exists in the
repository before uploading; skip that backup when present so reruns never
overwrite the preserved v4 assets. Also ensure the temporary directory created
by TMP is removed on script exit, including failure paths.
In `@src/AnimationControlController.cpp`:
- Around line 1948-1954: Update the footPin handling in the animation output
flow to surface FootPinResult::error when AnimationMerger::pinFeet returns a
failure. Preserve the existing footPinSpans output for successful results with
spans, and expose the error through the established output/reporting mechanism
so users can distinguish failure from finding no contacts.
In `@src/AnimationMerger_test.cpp`:
- Around line 1310-1336: Update the stale introductory comment in the dense
sampling test around anim and maxStepDeg to state that the collar should move
continuously and end near the capped 75° result about +X. Keep it consistent
with the existing EXPECT_NEAR assertion and the subsequent 150° cap followed by
0.5× gain explanation.
- Around line 1263-1291: Add an explicit non-null assertion for the entity
returned by createEntity in TwistUnwrapKeepsDampedCollarContinuous before
calling ent->getSkeleton(), matching the existing check in
TwistTransportCarriesBoneRoll.
- Around line 1181-1185: Update twistDegAbout to avoid unguarded M_PI usage by
switching its degree conversion to Ogre::Math::PI, or add the same local
_USE_MATH_DEFINES/MSVC-safe PI fallback used elsewhere in the test suite.
---
Nitpick comments:
In `@qml/PropertiesPanel.qml`:
- Around line 8539-8561: Replace the raw Rectangle and MouseArea implementation
of footPinChk with the existing InspectorCheckBox component, preserving its
default checked state and label behavior. Update the related read site to use
footPinChk.checked, and configure the component’s accessible text/name for “Pin
feet (contact cleanup)” so keyboard and assistive-technology interaction work
through the shared component.
In `@src/AnimationMerger.cpp`:
- Around line 1803-1809: Replace the hardcoded foot-pin key construction in the
regeneration cleanup block with the existing footPinKey(animName) helper. Keep
the surrounding getUserObjectBindings().eraseUserAny calls and
armSpaceKey(animName) behavior unchanged.
- Around line 1481-1701: The mixed-grid validation in pinFeet currently checks
only keyframe counts; update it to compare each thigh, shin, and optional foot
track’s actual keyframe times against the reference times array before
processing the leg. Skip the leg when any track’s timestamps differ, while
preserving the existing count checks and generated-clip behavior.
In `@src/MCPServer.cpp`:
- Around line 4272-4341: Add a SentryReporter::addBreadcrumb call immediately
before MeshImporterExporter::exporter in MCPServer::toolPinFeet, using the
file.export category and including the output path or equivalent export context.
Keep the existing export failure handling and result behavior unchanged.
- Around line 4144-4150: Update the foot-pinning block in generate_motion around
AnimationMerger::pinFeet so failed pinFeet results surface fp.error to the
caller, matching toolPinFeet behavior. Preserve footPinSpans == -1 for the
explicit foot_pin:false opt-out, but do not silently treat an enabled-operation
failure as disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5f4b819-b876-4488-886f-997ef155bfac
📒 Files selected for processing (21)
qml/PropertiesPanel.qmlscripts/prep-t2m-v5.pyscripts/train-t2m-flow-v5.pyscripts/upload-t2m-v5-model.shsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/AnimationMerger_test.cppsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/FootContact.cppsrc/FootContact.hsrc/FootContact_test.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/MotionGenerator.cppsrc/MotionGenerator.hsrc/MotionInbetween.cpptests/CMakeLists.txt
Comment on lines
+213
to
+215
| wq = np.asarray(q, np.float32) # [T,J,4] | ||
| cq, valid = canonicalize(wq, rw, rd) | ||
| yield action, cq, valid, f"{title} — {c.get('animation')}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard per-item processing so one bad clip/BVH doesn't abort the whole batch.
json.load above is already wrapped in try/except (lines 185-188), but np.asarray/canonicalize (213-215) and prep4.parse_bvh/bvh_rest (265-272) are not. A single malformed quats entry (ragged shape) or corrupt BVH will raise uncaught and kill a scan that may have been running for hours over a large corpus. Static analysis also flags the existing try/except-continue for not logging (S112) — worth fixing alongside so skipped items are visible.
🛡️ Proposed fix
- wq = np.asarray(q, np.float32) # [T,J,4]
- cq, valid = canonicalize(wq, rw, rd)
- yield action, cq, valid, f"{title} — {c.get('animation')}"
+ try:
+ wq = np.asarray(q, np.float32) # [T,J,4]
+ cq, valid = canonicalize(wq, rw, rd)
+ except Exception as e:
+ print(f"skip {dpath} clip {c.get('animation')}: {e}", file=sys.stderr)
+ continue
+ yield action, cq, valid, f"{title} — {c.get('animation')}"- r = prep4.parse_bvh(path)
- if r is None:
- continue
- _lq, wq = r
- rd = bvh_rest(path)
- if rd is None:
- continue
+ try:
+ r = prep4.parse_bvh(path)
+ rd = bvh_rest(path) if r is not None else None
+ except Exception as e:
+ print(f"skip {path}: {e}", file=sys.stderr)
+ continue
+ if r is None or rd is None:
+ continue
+ _lq, wq = rAlso applies to: 265-272
🤖 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/prep-t2m-v5.py` around lines 213 - 215, Guard the per-item processing
in the clip iteration around np.asarray/canonicalize and the BVH processing
around prep4.parse_bvh/bvh_rest with try/except so malformed clips are skipped
without aborting the batch. Catch processing exceptions, log the skipped item
and exception details, then continue; preserve successful yields and existing
JSON-loading behavior.
Source: Linters/SAST tools
Comment on lines
+331
to
+338
| # vocab = actions with enough support | ||
| from collections import Counter | ||
| cnt = Counter(acts) | ||
| vocab = sorted(w for w, n in cnt.items() if n >= a.min_action_windows) | ||
| keep = [i for i, w in enumerate(acts) if w in vocab] | ||
| mo = np.stack([mo[i] for i in keep]).astype(np.float32) | ||
| msk = np.stack([msk[i] for i in keep]).astype(np.float32) | ||
| tk = np.zeros((len(keep), len(vocab)), np.float32) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against an empty vocab crashing np.stack.
If no action reaches --min-action-windows (e.g. a small/custom corpus), keep is empty and np.stack([]) at Line 336 raises an opaque ValueError instead of a clear message, unlike the earlier if not mo: sys.exit(...) check.
🛡️ Proposed fix
vocab = sorted(w for w, n in cnt.items() if n >= a.min_action_windows)
+ if not vocab:
+ sys.exit(f"no action reached --min-action-windows={a.min_action_windows} "
+ f"(counts: {dict(cnt)}); lower the threshold or add more data")
keep = [i for i, w in enumerate(acts) if w in vocab]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # vocab = actions with enough support | |
| from collections import Counter | |
| cnt = Counter(acts) | |
| vocab = sorted(w for w, n in cnt.items() if n >= a.min_action_windows) | |
| keep = [i for i, w in enumerate(acts) if w in vocab] | |
| mo = np.stack([mo[i] for i in keep]).astype(np.float32) | |
| msk = np.stack([msk[i] for i in keep]).astype(np.float32) | |
| tk = np.zeros((len(keep), len(vocab)), np.float32) | |
| # vocab = actions with enough support | |
| from collections import Counter | |
| cnt = Counter(acts) | |
| vocab = sorted(w for w, n in cnt.items() if n >= a.min_action_windows) | |
| if not vocab: | |
| sys.exit(f"no action reached --min-action-windows={a.min_action_windows} " | |
| f"(counts: {dict(cnt)}); lower the threshold or add more data") | |
| keep = [i for i, w in enumerate(acts) if w in vocab] | |
| mo = np.stack([mo[i] for i in keep]).astype(np.float32) | |
| msk = np.stack([msk[i] for i in keep]).astype(np.float32) | |
| tk = np.zeros((len(keep), len(vocab)), np.float32) |
🤖 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/prep-t2m-v5.py` around lines 331 - 338, Guard the vocabulary-building
block after computing vocab and keep: if no actions meet a.min_action_windows,
exit with a clear error message before calling np.stack. Preserve the existing
stacking behavior when keep is non-empty, using the surrounding script’s
established sys.exit validation style.
| ap.add_argument("--device", default="mps") | ||
| a = ap.parse_args() | ||
|
|
||
| d = np.load(a.data, allow_pickle=True) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the npz produced by prep-t2m-v5.py never stores object-dtype arrays.
python3 - <<'EOF'
import numpy as np
vocab = ["walk", "run", "idle"]
a = np.array(vocab)
print("vocab dtype:", a.dtype, "is object:", a.dtype == object)
EOFRepository: fernandotonon/QtMeshEditor
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the producer and inspect the relevant code paths.
git ls-files | rg 'prep-t2m-v5\.py|train-t2m-flow-v5\.py|t2m|np\.savez|np\.load'
echo '--- outline: scripts/prep-t2m-v5.py'
ast-grep outline scripts/prep-t2m-v5.py --view expanded || true
echo '--- outline: scripts/train-t2m-flow-v5.py'
ast-grep outline scripts/train-t2m-flow-v5.py --view expanded || true
echo '--- relevant snippets from producer/consumer'
rg -n -C 3 'savez|save\(|load\(|allow_pickle|vocab|object' scripts/prep-t2m-v5.py scripts/train-t2m-flow-v5.py || trueRepository: fernandotonon/QtMeshEditor
Length of output: 9331
Drop allow_pickle=True here. prep-t2m-v5.py only writes numeric arrays plus a Unicode vocab array, so np.load(..., allow_pickle=False) is sufficient and avoids opening a pickle-based code-execution path if the archive is ever replaced.
🤖 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/train-t2m-flow-v5.py` at line 189, Update the np.load call in the
training data-loading flow to remove allow_pickle=True and explicitly use the
safe default of allow_pickle=False, while preserving the existing archive
loading behavior.
Source: Linters/SAST tools
Comment on lines
+31
to
+43
| # keep the previous (v4 CVAE) files for rollback under versioned names | ||
| TMP=$(mktemp -d) | ||
| for f in t2m.onnx t2m-vocab.json; do | ||
| if huggingface-cli download "$REPO" "motion/$f" --local-dir "$TMP" >/dev/null 2>&1; then | ||
| v4name="${f%.onnx}"; v4name="${v4name%.json}" | ||
| case "$f" in | ||
| t2m.onnx) dst="motion/t2m-v4.onnx" ;; | ||
| t2m-vocab.json) dst="motion/t2m-vocab-v4.json" ;; | ||
| esac | ||
| huggingface-cli upload "$REPO" "$TMP/motion/$f" "$dst" \ | ||
| --commit-message "t2m: preserve v4 CVAE as $dst before the v5 flow model (#858)" | ||
| fi | ||
| done |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Backup step isn't idempotent — a re-run destroys the real v4 rollback.
If this script is ever re-run (retry after a partial failure, accidental double-invocation), Line 34 downloads what is already the v5 model uploaded by the prior run, and Line 40 overwrites motion/t2m-v4.onnx/motion/t2m-vocab-v4.json with a duplicate of v5 — silently destroying the only rollback path this script exists to provide, with no warning. This is exactly the "non-idempotent write retried without safeguards" hazard against a shared, hosted asset every app install pulls from.
🛠️ Proposed fix (skip backup if it already exists; also clean up TMP)
-TMP=$(mktemp -d)
+TMP=$(mktemp -d)
+trap 'rm -rf "$TMP"' EXIT
+
for f in t2m.onnx t2m-vocab.json; do
- if huggingface-cli download "$REPO" "motion/$f" --local-dir "$TMP" >/dev/null 2>&1; then
- v4name="${f%.onnx}"; v4name="${v4name%.json}"
- case "$f" in
- t2m.onnx) dst="motion/t2m-v4.onnx" ;;
- t2m-vocab.json) dst="motion/t2m-vocab-v4.json" ;;
- esac
+ case "$f" in
+ t2m.onnx) dst="motion/t2m-v4.onnx" ;;
+ t2m-vocab.json) dst="motion/t2m-vocab-v4.json" ;;
+ esac
+ if huggingface-cli download "$REPO" "$dst" >/dev/null 2>&1; then
+ echo "backup $dst already exists, skipping re-backup of $f" >&2
+ continue
+ fi
+ if huggingface-cli download "$REPO" "motion/$f" --local-dir "$TMP" >/dev/null 2>&1; then
huggingface-cli upload "$REPO" "$TMP/motion/$f" "$dst" \
--commit-message "t2m: preserve v4 CVAE as $dst before the v5 flow model (`#858`)"
fi
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # keep the previous (v4 CVAE) files for rollback under versioned names | |
| TMP=$(mktemp -d) | |
| for f in t2m.onnx t2m-vocab.json; do | |
| if huggingface-cli download "$REPO" "motion/$f" --local-dir "$TMP" >/dev/null 2>&1; then | |
| v4name="${f%.onnx}"; v4name="${v4name%.json}" | |
| case "$f" in | |
| t2m.onnx) dst="motion/t2m-v4.onnx" ;; | |
| t2m-vocab.json) dst="motion/t2m-vocab-v4.json" ;; | |
| esac | |
| huggingface-cli upload "$REPO" "$TMP/motion/$f" "$dst" \ | |
| --commit-message "t2m: preserve v4 CVAE as $dst before the v5 flow model (#858)" | |
| fi | |
| done | |
| # keep the previous (v4 CVAE) files for rollback under versioned names | |
| TMP=$(mktemp -d) | |
| trap 'rm -rf "$TMP"' EXIT | |
| for f in t2m.onnx t2m-vocab.json; do | |
| case "$f" in | |
| t2m.onnx) dst="motion/t2m-v4.onnx" ;; | |
| t2m-vocab.json) dst="motion/t2m-vocab-v4.json" ;; | |
| esac | |
| if huggingface-cli download "$REPO" "$dst" >/dev/null 2>&1; then | |
| echo "backup $dst already exists, skipping re-backup of $f" >&2 | |
| continue | |
| fi | |
| if huggingface-cli download "$REPO" "motion/$f" --local-dir "$TMP" >/dev/null 2>&1; then | |
| huggingface-cli upload "$REPO" "$TMP/motion/$f" "$dst" \ | |
| --commit-message "t2m: preserve v4 CVAE as $dst before the v5 flow model (`#858`)" | |
| fi | |
| done |
🤖 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/upload-t2m-v5-model.sh` around lines 31 - 43, Make the backup loop in
the upload script idempotent by checking whether each destination in the case
mapping already exists in the repository before uploading; skip that backup when
present so reruns never overwrite the preserved v4 assets. Also ensure the
temporary directory created by TMP is removed on script exit, including failure
paths.
Comment on lines
+1948
to
+1954
| // #856: foot-contact cleanup — ON by default (checkbox opts out). | ||
| if (footPin) { | ||
| const auto fp = AnimationMerger::pinFeet(skel.get(), animName); | ||
| if (fp.ok && fp.spans > 0) | ||
| out["footPinSpans"] = fp.spans; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Surface fp.error when foot-pinning fails.
FootPinResult::error (e.g. "no leg tracks on this rig/animation", "too few keyframes to detect contacts") is computed but discarded here — on failure the user sees no indication the toggle didn't do anything, and there's no way to distinguish "pinning failed" from "pinning ran, found nothing to pin."
🩹 Proposed fix
if (footPin) {
const auto fp = AnimationMerger::pinFeet(skel.get(), animName);
if (fp.ok && fp.spans > 0)
out["footPinSpans"] = fp.spans;
+ else if (!fp.ok)
+ emit generateMotionStatus(
+ QStringLiteral("Foot-pin skipped: %1").arg(fp.error), false);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // #856: foot-contact cleanup — ON by default (checkbox opts out). | |
| if (footPin) { | |
| const auto fp = AnimationMerger::pinFeet(skel.get(), animName); | |
| if (fp.ok && fp.spans > 0) | |
| out["footPinSpans"] = fp.spans; | |
| } | |
| // `#856`: foot-contact cleanup — ON by default (checkbox opts out). | |
| if (footPin) { | |
| const auto fp = AnimationMerger::pinFeet(skel.get(), animName); | |
| if (fp.ok && fp.spans > 0) | |
| out["footPinSpans"] = fp.spans; | |
| else if (!fp.ok) | |
| emit generateMotionStatus( | |
| QStringLiteral("Foot-pin skipped: %1").arg(fp.error), 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 `@src/AnimationControlController.cpp` around lines 1948 - 1954, Update the
footPin handling in the animation output flow to surface FootPinResult::error
when AnimationMerger::pinFeet returns a failure. Preserve the existing
footPinSpans output for successful results with spans, and expose the error
through the established output/reporting mechanism so users can distinguish
failure from finding no contacts.
Comment on lines
+1181
to
+1185
| float twistDegAbout(const Ogre::Quaternion& q, const Ogre::Vector3& ax) | ||
| { | ||
| const float s = q.x * ax.x + q.y * ax.y + q.z * ax.z; | ||
| return 2.0f * std::atan2(s, q.w) * 180.0f / static_cast<float>(M_PI); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether M_PI is used elsewhere and whether _USE_MATH_DEFINES is set globally.
rg -n 'M_PI' --type=cpp -g '!**/build/**'
rg -n '_USE_MATH_DEFINES' CMakeLists.txt src/CMakeLists.txt 2>/dev/nullRepository: fernandotonon/QtMeshEditor
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== files ==\n'
git ls-files | sed -n '1,200p'
printf '\n== M_PI occurrences ==\n'
rg -n --hidden --glob '!**/build/**' 'M_PI|_USE_MATH_DEFINES' .
printf '\n== CMakeLists snippets ==\n'
for f in CMakeLists.txt src/CMakeLists.txt; do
if [ -f "$f" ]; then
echo "--- $f ---"
nl -ba "$f" | sed -n '1,220p'
fi
done
printf '\n== target file around snippet ==\n'
nl -ba src/AnimationMerger_test.cpp | sed -n '1160,1205p'Repository: fernandotonon/QtMeshEditor
Length of output: 12910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== search for math constants/PI helpers ==\n'
rg -n --hidden --glob '!**/build/**' 'static_cast<float>\(M_PI\)|std::numbers::pi|M_PI_F|PI' src include .github CMakeLists.txt 2>/dev/null || true
printf '\n== includes in target file ==\n'
sed -n '1,80p' src/AnimationMerger_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 38230
Add an MSVC-safe PI fallback here
src/AnimationMerger_test.cpp:1181 uses M_PI without the local _USE_MATH_DEFINES guard used elsewhere in the test suite. Add the same fallback here or switch to Ogre::Math::PI to keep the Windows/MSVC test build portable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/AnimationMerger_test.cpp` around lines 1181 - 1185, Update twistDegAbout
to avoid unguarded M_PI usage by switching its degree conversion to
Ogre::Math::PI, or add the same local _USE_MATH_DEFINES/MSVC-safe PI fallback
used elsewhere in the test suite.
Source: Coding guidelines
Comment on lines
+1263
to
+1291
| TEST_F(AnimationMergerTest, TwistUnwrapKeepsDampedCollarContinuous) | ||
| { | ||
| // Rig WITH clavicles (Mixamo "Shoulder" → collar roles 6/10). | ||
| auto skelRes = Ogre::SkeletonManager::getSingleton().create( | ||
| "twist_collar_skel", | ||
| Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME); | ||
| unsigned short h = 0; | ||
| auto bone = [&](const char* n, const Ogre::Vector3& p, Ogre::Bone* par) { | ||
| auto* b = skelRes->createBone(n, h++); | ||
| b->setPosition(p); | ||
| if (par) par->addChild(b); | ||
| return b; | ||
| }; | ||
| auto* hips = bone("Hips", {0, 1.0f, 0}, nullptr); | ||
| auto* spine = bone("Spine", {0, 0.3f, 0}, hips); | ||
| bone("Head", {0, 0.4f, 0}, spine); | ||
| bone("LeftUpLeg", {0.15f, -0.1f, 0}, hips); | ||
| bone("RightUpLeg", {-0.15f, -0.1f, 0}, hips); | ||
| auto* lcol = bone("LeftShoulder", {0.05f, 0.25f, 0}, spine); | ||
| auto* larm = bone("LeftArm", {0.15f, 0, 0}, lcol); | ||
| bone("LeftForeArm", {0.3f, 0, 0}, larm); | ||
| auto* rcol = bone("RightShoulder", {-0.05f, 0.25f, 0}, spine); | ||
| auto* rarm = bone("RightArm", {-0.15f, 0, 0}, rcol); | ||
| bone("RightForeArm", {-0.3f, 0, 0}, rarm); | ||
| skelRes->setBindingPose(); | ||
| auto mesh = createInMemoryMesh("twist_collar_mesh", skelRes); | ||
| auto* sm = Manager::getSingleton()->getSceneMgr(); | ||
| Ogre::Entity* ent = sm->createEntity("twist_collar_ent", mesh); | ||
| Ogre::SkeletonInstance* skel = ent->getSkeleton(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Missing null check after createEntity.
Unlike TwistTransportCarriesBoneRoll (line 1219: ASSERT_NE(ent, nullptr);), this test calls ent->getSkeleton() immediately after sm->createEntity(...) with no null check. If creation ever fails, this dereferences a null pointer and crashes the test binary instead of failing the test with a clear message.
🛡️ Proposed fix
Ogre::Entity* ent = sm->createEntity("twist_collar_ent", mesh);
+ ASSERT_NE(ent, nullptr);
Ogre::SkeletonInstance* skel = ent->getSkeleton();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TEST_F(AnimationMergerTest, TwistUnwrapKeepsDampedCollarContinuous) | |
| { | |
| // Rig WITH clavicles (Mixamo "Shoulder" → collar roles 6/10). | |
| auto skelRes = Ogre::SkeletonManager::getSingleton().create( | |
| "twist_collar_skel", | |
| Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME); | |
| unsigned short h = 0; | |
| auto bone = [&](const char* n, const Ogre::Vector3& p, Ogre::Bone* par) { | |
| auto* b = skelRes->createBone(n, h++); | |
| b->setPosition(p); | |
| if (par) par->addChild(b); | |
| return b; | |
| }; | |
| auto* hips = bone("Hips", {0, 1.0f, 0}, nullptr); | |
| auto* spine = bone("Spine", {0, 0.3f, 0}, hips); | |
| bone("Head", {0, 0.4f, 0}, spine); | |
| bone("LeftUpLeg", {0.15f, -0.1f, 0}, hips); | |
| bone("RightUpLeg", {-0.15f, -0.1f, 0}, hips); | |
| auto* lcol = bone("LeftShoulder", {0.05f, 0.25f, 0}, spine); | |
| auto* larm = bone("LeftArm", {0.15f, 0, 0}, lcol); | |
| bone("LeftForeArm", {0.3f, 0, 0}, larm); | |
| auto* rcol = bone("RightShoulder", {-0.05f, 0.25f, 0}, spine); | |
| auto* rarm = bone("RightArm", {-0.15f, 0, 0}, rcol); | |
| bone("RightForeArm", {-0.3f, 0, 0}, rarm); | |
| skelRes->setBindingPose(); | |
| auto mesh = createInMemoryMesh("twist_collar_mesh", skelRes); | |
| auto* sm = Manager::getSingleton()->getSceneMgr(); | |
| Ogre::Entity* ent = sm->createEntity("twist_collar_ent", mesh); | |
| Ogre::SkeletonInstance* skel = ent->getSkeleton(); | |
| TEST_F(AnimationMergerTest, TwistUnwrapKeepsDampedCollarContinuous) | |
| { | |
| // Rig WITH clavicles (Mixamo "Shoulder" → collar roles 6/10). | |
| auto skelRes = Ogre::SkeletonManager::getSingleton().create( | |
| "twist_collar_skel", | |
| Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME); | |
| unsigned short h = 0; | |
| auto bone = [&](const char* n, const Ogre::Vector3& p, Ogre::Bone* par) { | |
| auto* b = skelRes->createBone(n, h++); | |
| b->setPosition(p); | |
| if (par) par->addChild(b); | |
| return b; | |
| }; | |
| auto* hips = bone("Hips", {0, 1.0f, 0}, nullptr); | |
| auto* spine = bone("Spine", {0, 0.3f, 0}, hips); | |
| bone("Head", {0, 0.4f, 0}, spine); | |
| bone("LeftUpLeg", {0.15f, -0.1f, 0}, hips); | |
| bone("RightUpLeg", {-0.15f, -0.1f, 0}, hips); | |
| auto* lcol = bone("LeftShoulder", {0.05f, 0.25f, 0}, spine); | |
| auto* larm = bone("LeftArm", {0.15f, 0, 0}, lcol); | |
| bone("LeftForeArm", {0.3f, 0, 0}, larm); | |
| auto* rcol = bone("RightShoulder", {-0.05f, 0.25f, 0}, spine); | |
| auto* rarm = bone("RightArm", {-0.15f, 0, 0}, rcol); | |
| bone("RightForeArm", {-0.3f, 0, 0}, rarm); | |
| skelRes->setBindingPose(); | |
| auto mesh = createInMemoryMesh("twist_collar_mesh", skelRes); | |
| auto* sm = Manager::getSingleton()->getSceneMgr(); | |
| Ogre::Entity* ent = sm->createEntity("twist_collar_ent", mesh); | |
| ASSERT_NE(ent, nullptr); | |
| Ogre::SkeletonInstance* skel = ent->getSkeleton(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/AnimationMerger_test.cpp` around lines 1263 - 1291, Add an explicit
non-null assertion for the entity returned by createEntity in
TwistUnwrapKeepsDampedCollarContinuous before calling ent->getSkeleton(),
matching the existing check in TwistTransportCarriesBoneRoll.
Comment on lines
+1310
to
+1336
|
|
||
| // Sample densely: the collar's world orientation must move CONTINUOUSLY | ||
| // (no wrap snap) and end near 240° × 0.5 = 120° about +X. | ||
| Ogre::Quaternion prev = Ogre::Quaternion::IDENTITY; | ||
| float maxStepDeg = 0.0f; | ||
| Ogre::Quaternion last; | ||
| auto* anim = skel->getAnimation("collarclip"); | ||
| const float len = anim->getLength(); | ||
| for (int s = 0; s <= 60; ++s) { | ||
| skel->reset(true); | ||
| anim->apply(skel, len * static_cast<float>(s) / 60.0f); | ||
| skel->_updateTransforms(); | ||
| const Ogre::Quaternion w = | ||
| skel->getBone("LeftShoulder")->_getDerivedOrientation(); | ||
| if (s > 0) { | ||
| const Ogre::Quaternion d = w * prev.Inverse(); | ||
| const float step = 2.0f * std::acos(std::min( | ||
| 1.0f, std::abs(d.w))) * 180.0f / static_cast<float>(M_PI); | ||
| maxStepDeg = std::max(maxStepDeg, step); | ||
| } | ||
| prev = w; | ||
| last = w; | ||
| } | ||
| EXPECT_LT(maxStepDeg, 15.0f) << "collar roll snapped mid-clip (unwrap)"; | ||
| // 240° source twist hits the 150° runaway-unwrap cap FIRST, then the | ||
| // 0.5× collar gain: 150 × 0.5 = 75°. | ||
| EXPECT_NEAR(twistDegAbout(last, Ogre::Vector3::UNIT_X), 75.0f, 8.0f); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale comment states a different expected value than the assertion.
The comment at line 1312 says the collar orientation should "end near 240° × 0.5 = 120° about +X", but the assertion (EXPECT_NEAR(..., 75.0f, 8.0f)) and the comment directly above it correctly document a 150° runaway-unwrap cap applied before the 0.5× gain (150 × 0.5 = 75°). The earlier comment appears to be left over from before the cap logic was added, and could mislead a future reader trying to understand expected retarget behavior from this test.
📝 Proposed fix
- // Sample densely: the collar's world orientation must move CONTINUOUSLY
- // (no wrap snap) and end near 240° × 0.5 = 120° about +X.
+ // Sample densely: the collar's world orientation must move CONTINUOUSLY
+ // (no wrap snap) and end near the capped+gained value asserted below.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Sample densely: the collar's world orientation must move CONTINUOUSLY | |
| // (no wrap snap) and end near 240° × 0.5 = 120° about +X. | |
| Ogre::Quaternion prev = Ogre::Quaternion::IDENTITY; | |
| float maxStepDeg = 0.0f; | |
| Ogre::Quaternion last; | |
| auto* anim = skel->getAnimation("collarclip"); | |
| const float len = anim->getLength(); | |
| for (int s = 0; s <= 60; ++s) { | |
| skel->reset(true); | |
| anim->apply(skel, len * static_cast<float>(s) / 60.0f); | |
| skel->_updateTransforms(); | |
| const Ogre::Quaternion w = | |
| skel->getBone("LeftShoulder")->_getDerivedOrientation(); | |
| if (s > 0) { | |
| const Ogre::Quaternion d = w * prev.Inverse(); | |
| const float step = 2.0f * std::acos(std::min( | |
| 1.0f, std::abs(d.w))) * 180.0f / static_cast<float>(M_PI); | |
| maxStepDeg = std::max(maxStepDeg, step); | |
| } | |
| prev = w; | |
| last = w; | |
| } | |
| EXPECT_LT(maxStepDeg, 15.0f) << "collar roll snapped mid-clip (unwrap)"; | |
| // 240° source twist hits the 150° runaway-unwrap cap FIRST, then the | |
| // 0.5× collar gain: 150 × 0.5 = 75°. | |
| EXPECT_NEAR(twistDegAbout(last, Ogre::Vector3::UNIT_X), 75.0f, 8.0f); | |
| // Sample densely: the collar's world orientation must move CONTINUOUSLY | |
| // (no wrap snap) and end near the capped+gained value asserted below. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/AnimationMerger_test.cpp` around lines 1310 - 1336, Update the stale
introductory comment in the dense sampling test around anim and maxStepDeg to
state that the collar should move continuously and end near the capped 75°
result about +X. Keep it consistent with the existing EXPECT_NEAR assertion and
the subsequent 150° cap followed by 0.5× gain explanation.
…pin, twist caps+low-pass, library gates (#837) Field feedback after the epic branch: generated walks trembled, played 'too fast', barely moved the legs, and over-moved arms/head. Four root causes, four fixes: 1. FOOT PINNING OVER-DETECTED (the frozen legs): the height band was 0.18 of leg length — a walking foot only lifts ~0.05-0.15, so contact spans swallowed the swing phase and pinned the legs into a shuffle while the upper body kept full rate (which also reads as 'too fast'). Tightened to 0.06 band / 0.012 velocity / 4-frame minimum, plus a coverage guard: any single span covering >55% of the clip is a misdetection and is discarded. 2. TWIST JITTER (the trembling): theta comes from per-frame shortest-arc decompositions and jitters near degenerate aims. Added the issue's 'optional low-pass' (5-tap binomial per role) and replaced the single 150-degree cap with per-role caps (neck/head 30, spine 45, arms 90, legs 60, feet 45; hip keeps 150 for genuine facing turns). 3. SMOOTH-BAKE POST-PASS (the user's own trick, codified): AnimationMerger::smoothBakeAnimation re-grids the clip to a sparse rate then back to the clip rate — a temporal low-pass that removes residual retarget trembling. ON by default in generation, before arm-space and foot pinning so pin targets stay exact. CLI --no-smooth-bake / --smooth-fps N; MCP smooth_bake/smooth_fps; GUI default path. 4. LIBRARY TAKE GATES (the T-pose/raised arms and hidden head): several corpus takes render broken regardless of retarget quality — unresolved arm roles freeze the target's arms in its literal bind T-pose; zombie/skeleton takes hold arms horizontal; several rigs carry a per-bone axis inversion that points one arm skyward or the neck straight down (head renders thrown back). New curation gates in build-motion-library-v5.py judge each arm's SIGNED hang direction, require an upright neck chain, and drop locomotion takes with unresolved arms; CrouchWalk/CrouchRun map to a new 'crouch' action instead of polluting walk/run. Curated v6 library (87 clips, 22 actions) uploaded to HF. Render-verified: 4/4 random Rumba walks natural (arms hanging, head visible, full stride). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Quality follow-up pushed (9c54de7) after field testing found generated walks trembling, playing "too fast", with frozen legs and over-active arms/head. Four root causes:
Render-verified: 4/4 random Rumba walks natural (arms hanging, head visible, full stride). |
…llow-up) The v5 model learned a head-down hunched walk because training windows were unfiltered — folded, idle-contaminated and placeholder-armed source content trained in. In the canonical representation the checks are trivial (d(f) = Q'(f)*D_c): spine and neck/head must stay up (stricter for locomotion), and locomotion upper arms must HANG — judged on the SIGNED up-component per arm, the library-curation lesson (abs() passes a skyward arm). Dropped 841 of 17,193 windows; the retrained model (hosted as t2m v5.1) walks upright with stepping legs on both the Mixamo test rig and the chibi mouse that exposed the hunch. 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.
Closes #857, closes #856, closes #840, closes #858 — the remaining slices of epic #837, combined into one PR (supersedes the stacked #886/#887/#888).
1. Twist transport in the direction retarget (#857)
The bind-referenced retarget (PR #843) aimed bone directions but dropped roll — gesture clips read flat. The twist is now decomposed on the source and recomposed about the same pole the source decomposed about:
Recomposing from the target-bind pole (the issue's original formula) leaks roll into swing — measured +38% arm amplitude. With matched poles, per-frame relative motion is exactly the source's world delta conjugated into target axes. θ is unwrapped across frames (a ±180° pop would flip the roll once a gain ≠ 1 scales it), capped 150°; collars damped 0.5×.
Harness: Mixamo Walk self-parity 2.22° → 0.03° mean joint delta (gate ≤1.6°); spine amplitude restored (abdomen 2.3°→9.0°); Quaternius Knight cross-rig 6.68°→5.36°, renders upright. Controlled A/Bs pin a single-clip eval library — the runtime take-pick is quality²-weighted RANDOM, so uncontrolled comparisons are noisy.
2. Foot-contact detection + IK pinning (#856)
Retargeted clips skate/float feet on rigs whose proportions differ from the source. New
FootContactpure-data core (Ogre-free, 8 unit tests): leg-length-scaled contact detection (height band + horizontal-speed gate; a fast on-ground foot — skating — is correctly NOT a contact), analytic two-bonesolveKnee(keeps segment lengths + the pose's own bend plane), 0→1→0 edge blends.AnimationMerger::pinFeetruns manual FK over the tracks (the live skeleton is neverapply()'d), detects in the canonical frame, rewrites only thigh/shin/foot keyframes (foot keeps its original world orientation — no toe pop). Effectively idempotent.Surfaces: ON by default in generation — CLI
--no-foot-pin/ standalone--foot-pin --animation <n>; MCPfoot_pinarg +pin_feettool; GUI "Pin feet" checkbox.Matcher fix uncovered by this:
upperlegnow maps to the thigh role — Quaternius rigs (UpperLeg.L/LowerLeg.L) previously mapped BOTH leg bones to the knee.3. Flow-matching model on canonical v5 data (#840, #858)
prep-t2m-v5.pycanonicalizes every clip (corpus sidecar dumps + CMU BVH) into a rig-independent representation —Q'(f) = twist(θ, d(f)) · arc(D_c → d(f))against a fixed canonical T-pose — 17,286 windows / 18 actions (v4 trained on per-rig "convention mush", the measured cause of the CVAE's gentle-average output). The neutral-start gate is gone: model clips ride the direction retarget, which references the reference triple, not clip frame 0.train-t2m-flow-v5.py— 7.3M-param DiT (6D rotations, AdaLN-Zero), rectified flow + classifier-free guidance. The Euler sampler + CFG are unrolled inside the exported ONNX graph — the shippedMotionGeneratorcontract (tokens[1,V], seed[1,Z] → motion[1,T,220]) is unchanged; seed is the flattened noise tensor.MotionGenerator+ all three model call sites pass it through, so model clips ride the same bind-referenced retarget as templates (standing-pose shim retired to v4 fallback).applyMotionClip's restWorld presence check is norm-based now (identity counts as present).Hosted + verified: HF
motion/t2m.onnx+ vocab are the v5 CFG model (v4 preserved ast2m-v4.*for rollback; interface backward-compatible). E2E: fresh download →--generate run --model→ upright, coherent clip.Eval (honest): model walk/run/wave/dance all upright + temporally coherent (every v4 CVAE variant folded or tumbled); run/dance energetic and action-distinct with CFG; wave still gentler than the template take (CMU wave trials carry long idle stretches — data follow-up). Per the epic's gate, templates remain the default + automatic fallback; the model stays opt-in behind
--model.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes