feat(#409): AI animation in-betweening (ONNX RMIB + spline fallback)#762
Merged
Conversation
Fills the gap between two sparse keyframes with smooth, plausible intermediate poses — Robust Motion In-betweening (Harvey et al., Ubisoft, SIGGRAPH 2020) on ONNX Runtime, the third ONNX consumer after #404/#408. Per the issue's acceptance criteria the SPLINE FALLBACK is first-class: a cubic-Hermite (Catmull-Rom tangents) + shortest-arc slerp interpolator that is always compiled (no ONNX/Ogre), visibly smoother than linear, and used automatically whenever the build lacks ONNX, the model is missing/un-downloadable, the skeleton is incompatible, or the run fails — the result reports which path ran. - src/MotionInbetween.{h,cpp}: Ogre-free core. Pose = flat float array (bones × 10 DoF: tx,ty,tz, qx,qy,qz,qw, sx,sy,sz) with a Channel layout (Scalar/QuatStart/QuatCont). predict() runs RMIB (#ifdef ENABLE_ONNX, runtime I/O-name + channel-count discovery → falls back on any mismatch) or the spline. Self-contained ensureModelBlocking() downloads rmib.onnx to AppData/ai_models/inbetween/ on first use (override QTMESH_INBETWEEN_MODEL_BASE_URL / QSettings ai/inbetweenModelBaseUrl; offline guard QTMESH_INBETWEEN_NO_DOWNLOAD; non-ONNX guard returns {}). - AnimationMerger::inbetweenAnimation: Ogre adapter — packs every bracketing node track's start/end pose into ONE predict() call (full skeleton), scatters predicted per-frame poses back as keyframes; returns InbetweenResult (keyframesInserted/tracksAffected/usedModel/fallbackReason). - CLI: qtmesh anim <file> --in-between --gap-frames N [--start-time S] [--end-time S] [--no-model] [--animation NAME] [-o out]. - MCP: motion_in_between tool (registered + heavy + schema). - GUI: dope-sheet "AI in-between … Fill gap" control (shown when the selection spans a window) → AnimationControlController::inbetweenWindow → inbetweenStatus. - 16 unit tests (pure-data slerp/hermite + spline fallback, ONNX-optional). - Sentry breadcrumb ai.assist.in_between on all surfaces. - Version 3.12.0; THIRD_PARTY_AI_MODELS.md + CLAUDE.md updated. Model hosting pending → ships running the spline fallback end-to-end today; hosting a permissive rmib.onnx lights up the ML path with no code change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds MotionInbetween-based animation in-betweening with spline fallback and optional ONNX model loading, wires it into animation insertion, CLI, GUI, and MCP entry points, adds tests and build registration, and updates docs and 3.13.0 version references. ChangesAI animation in-betweening
Version pin refresh
Asset hydration cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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: 9f9149ab4a
ℹ️ 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
+322
to
+324
| Q_INVOKABLE QVariantMap inbetweenWindow(double t0, double t1, | ||
| int gapFrames, | ||
| bool noModel = false); |
There was a problem hiding this comment.
Adding the Q_INVOKABLE here does not actually surface the GUI flow: I searched the repo for inbetweenWindow and inbetweenStatus, and they only appear in this C++ declaration/implementation, with no QML/JS caller or status binding in AnimationDopeSheet.qml/AnimationControlPanel.qml. As a result the CLI/MCP paths are usable, but the promised dope-sheet "Fill gap" control cannot be triggered from the GUI.
Useful? React with 👍 / 👎.
Comment on lines
+811
to
+813
| const MotionInbetween::Pose& pose = mr.frames[static_cast<size_t>(f)]; | ||
| const size_t base = bi * 10; | ||
| Ogre::TransformKeyFrame* kf = track->createNodeKeyFrame(t); |
There was a problem hiding this comment.
For any window that already contains authored keys at the generated interior times—especially the CLI default [0, clipLength], e.g. keys at 0, 1, 2 with gapFrames=1—this unconditionally creates another keyframe at the same timestamp instead of rejecting the non-gap window or replacing/removing the existing key. Other editor paths explicitly treat same-track timestamp collisions as invalid, so this can leave tracks with duplicate keys and ambiguous playback/export results.
Useful? React with 👍 / 👎.
Comment on lines
+791
to
+792
| const MotionInbetween::Result mr = | ||
| MotionInbetween::predict(startPose, endPose, layout, modelPath, opts); |
There was a problem hiding this comment.
When the model is unavailable or --no-model is used, the fallback is called without preStart/postEnd, so gaps that have authored keys immediately before and after the selected two endpoints cannot use the Catmull-Rom tangents implemented by MotionInbetween::interpolateSpline. In that common fallback scenario, curved motion gets synthesized from only the two endpoint poses, producing endpoint velocity/kink artifacts instead of blending into the surrounding animation.
Useful? React with 👍 / 👎.
The standalone test executables (MaterialEditorQML_test, etc.) link against the qtmesh_test_common static lib, which includes CLIPipeline/MCPServer/ AnimationMerger — all now referencing MotionInbetween — but its explicit source list didn't include MotionInbetween.cpp, so those binaries failed to link with "undefined reference to MotionInbetween::*". The main UnitTests target built fine (MotionInbetween.cpp is in src/CMakeLists.txt), which is why it slipped past the local build. Add the .cpp to the test-common source list, mirroring UniRigPredictor.cpp (#408) right above it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
src/MotionInbetween.cpp (2)
56-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd breadcrumbs for model resolution and fallback decisions.
Download attempts, missing-model fallbacks, and inference failures are the user-visible branches of this feature, but none of them emit
SentryReporter::addBreadcrumb(...). Please breadcrumb at least the download start/result and the final RMIB-vs-spline decision.As per coding guidelines, "Add
SentryReporter::addBreadcrumb(category, message)calls for user-facing actions and significant operations, using the established breadcrumb categories."Also applies to: 264-403
🤖 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 56 - 115, Add Sentry breadcrumbs in MotionInbetween::ensureModelBlocking and the downstream RMIB fallback path so user-visible model resolution steps are traceable. Emit breadcrumbs when download resolution starts, when it is skipped or fails, and when the function returns the RMIB model versus falling back to spline, using SentryReporter::addBreadcrumb(category, message) with the existing breadcrumb categories. Also ensure any inference failure path in the related MotionInbetween logic referenced by the review is breadcrumbed before choosing the fallback.Source: Coding guidelines
287-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Q_OS_*guards here.The new platform-specific branches use
__APPLE__/_WIN32, but this repo standardizes on Qt OS guards insrc/**/*.{h,cpp}.Suggested change
-#ifdef __APPLE__ +#ifdef Q_OS_MACOS try { std::unordered_map<std::string, std::string> coremlOpts; so.AppendExecutionProvider("CoreML", coremlOpts); } catch (const Ort::Exception&) { /* CPU EP fallback */ } `#endif` -#ifdef _WIN32 +#ifdef Q_OS_WIN const std::wstring wpath = modelPath.toStdWString(); Ort::Session session(env, wpath.c_str(), so); `#else`As per coding guidelines, "Maintain cross-platform compatibility for Windows, Linux, and macOS; guard platform-specific APIs with
#ifdef Q_OS_WIN,#ifdef Q_OS_MACOS, or#ifdef Q_OS_LINUX."🤖 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 287 - 298, The platform-specific branches in MotionInbetween’s session setup currently use compiler macros instead of the project’s Qt OS guards. Update the conditional compilation around the CoreML append and the Windows vs non-Windows model path/session creation to use Q_OS_MACOS and Q_OS_WIN (and the matching Qt-style guard pattern used elsewhere in src), keeping the logic in the MotionInbetween constructor or session initialization flow 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.
Inline comments:
In `@src/AnimationControlController.cpp`:
- Around line 1620-1623: The insert path in inbetweenAnimation() only refreshes
the UI via refreshSliderTicks() and signals, but it does not invalidate cached
Ogre keyframe pointers. Update this block to call onUndoRedoCommandApplied() or
the equivalent cache-reset logic after the keyframe insert so m_selectedTrack
and m_currentKeyframe are cleared/rebuilt instead of left pointing at stale
data.
In `@src/AnimationMerger.cpp`:
- Around line 739-746: The current bracketed-span detection in
AnimationMerger::mergeTracks (the coverage check around evalAt and c.bracketed)
only verifies the first/last keyframes and can still treat a span as empty even
when interior keyframes already exist. Update the span selection logic to
require a true gap before writing: inspect the keyframe sequence between t0 and
t1, and only set c.bracketed / emit new keys when there are no existing keys in
that interval or when the span is formed by adjacent key pairs that are actually
empty. Keep the insertion path near the later key-writing block aligned with
this check so repeated runs cannot add duplicate or conflicting interior keys.
- Around line 703-710: In AnimationMerger’s core validation path, reject
non-finite start/end times before the existing `t1 <= t0` check, since
`NaN`/`inf` can slip through and reach inference. Also add an upper bound for
`gapFrames` in this method before it is used for allocation and `gapFrames + 1`
sizing later in the merge flow. Keep the validation and error reporting
centralized in the same `AnimationMerger` method so CLI/GUI/MCP callers are
protected consistently.
In `@src/CLIPipeline.cpp`:
- Around line 2371-2431: The in-between CLI summary in CLIPipeline::run (the
loop over animNames and the final cliWrite output) currently collapses mixed
results into a single anyUsedModel flag, so one RMIB-backed clip makes the whole
run report “via RMIB model” even when some animations fell back to spline.
Update this logic to track backend usage per animation—either by counting RMIB
vs fallback results or recording each animation’s backend—and use those
aggregates in the final summary so mixed runs are reported accurately, while
keeping lastFallbackReason handling for fallback notes.
In `@src/MCPServer.cpp`:
- Around line 3316-3329: The success path in AnimationMerger::inbetweenAnimation
handling updates the skeleton but does not notify the live animation controller,
leaving dope-sheet/keyframe caches stale in --with-mcp mode. After the
structural edit succeeds, route the main-thread success path through the same
controller refresh/invalidation hook used by
AnimationControlController::inbetweenWindow(), so the cached track/keyframe
pointers are rebuilt before returning the result.
In `@src/MotionInbetween_test.cpp`:
- Around line 16-18: Replace the non-portable use of M_PI in
MotionInbetween_test.cpp with a locally defined constant in the anonymous
namespace near layoutTRS and update every usage site to reference that constant
instead. Keep the change scoped to the existing test helpers (such as layoutTRS
and any related angle/rotation setup) so the file no longer depends on
platform-specific math macros and remains buildable on MSVC, Linux, and macOS.
In `@src/MotionInbetween.cpp`:
- Around line 259-275: MotionInbetween::predict currently ignores
Options::upAxis on the ONNX path, so non-Y-up callers still run with the wrong
coordinate convention. Update predict (and the downstream ONNX input preparation
code it calls) to either convert poses/layout into the model’s expected Y-up
convention using opts.upAxis or, as a temporary guard, route non-Y-up cases
through the existing fallback lambda. Make sure the new check is applied before
model inputs are built so the RMIB path never proceeds with unsupported axis
settings.
- Around line 375-389: The RMIB output validation in MotionInbetween::run only
checks GetElementCount() before treating the tensor as a contiguous [gap, C]
buffer, which can accept incorrectly shaped outputs and scramble poses. Update
the shape validation in this block to inspect the tensor dimensions from outTI
(not just the total element count) and confirm they match the expected
frame/component layout before the loop that builds Result frames. If the output
shape is anything other than the expected RMIB layout, return the spline
fallback from fallback(...) instead of slicing with Pose.
---
Nitpick comments:
In `@src/MotionInbetween.cpp`:
- Around line 56-115: Add Sentry breadcrumbs in
MotionInbetween::ensureModelBlocking and the downstream RMIB fallback path so
user-visible model resolution steps are traceable. Emit breadcrumbs when
download resolution starts, when it is skipped or fails, and when the function
returns the RMIB model versus falling back to spline, using
SentryReporter::addBreadcrumb(category, message) with the existing breadcrumb
categories. Also ensure any inference failure path in the related
MotionInbetween logic referenced by the review is breadcrumbed before choosing
the fallback.
- Around line 287-298: The platform-specific branches in MotionInbetween’s
session setup currently use compiler macros instead of the project’s Qt OS
guards. Update the conditional compilation around the CoreML append and the
Windows vs non-Windows model path/session creation to use Q_OS_MACOS and
Q_OS_WIN (and the matching Qt-style guard pattern used elsewhere in src),
keeping the logic in the MotionInbetween constructor or session initialization
flow unchanged.
🪄 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: e9f6ed4a-55c0-497d-90af-30f4c29dc219
📒 Files selected for processing (17)
CLAUDE.mdCMakeLists.txtREADME.mdTHIRD_PARTY_AI_MODELS.mdsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/MotionInbetween.cppsrc/MotionInbetween.hsrc/MotionInbetween_test.cpptests/CMakeLists.txtwebsite/src/hooks/useQtmeshActionRef.js
- MotionInbetween: defer non-Y up axis to the spline (RMIB is +Y-up — feeding
it a wrong-convention pose would silently produce garbage); validate the RMIB
output SHAPE (last dim == channel count), not just element count, before
slicing frames; test M_PI portability (_USE_MATH_DEFINES + fallback define).
- AnimationMerger::inbetweenAnimation: reject non-finite t0/t1 (NaN/inf bypass
t1<=t0), cap gapFrames at 1000, and skip tracks that already have a keyframe
strictly inside (t0,t1) — only fill genuine gaps, don't stack keys on authored
ones.
- CLIPipeline: report mixed RMIB/spline runs accurately ("RMIB model + spline
fallback (mixed)") instead of collapsing to one bool.
- AnimationControlController: clear cached m_selectedTrack/m_currentKeyframe
after the structural insert (a keyframe insert can reallocate a track's
keyframe vector and dangle them); new notifyExternalAnimationEdit() for the
MCP path.
- MCPServer::toolMotionInBetween: call notifyExternalAnimationEdit() so
--with-mcp dope-sheet caches don't dangle after the live edit.
- New test: non-Y up axis falls back to spline.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eting The in-between feature shipped with a spline fallback because no permissively- licensed model existed (the field standardizes on LAFAN1 = CC-BY-NC-ND). Trained our own from scratch on the CMU MoCap database (mocap.cs.cmu.edu — permissive, commercial-OK, can't resell the data) and hosted it, so the ML path now runs. Model: - scripts/export-rmib-onnx.py (one-time, offline, NOT shipped): preprocess CMU BVH → 22 core-body joints → ~421k (start,end,8-interior) windows → small Transformer (start+end + learnable interior queries → interior poses) → ONNX [1,2,220]→[1,8,220]. Validated: rotation error < HALF of slerp (the spline fallback's method) on a held-out CMU split — a real, measured win. - rmib.onnx (~13 MB) hosted at fernandotonon/QtMeshEditor-models/inbetween/; the existing ensureModelBlocking() URL now resolves (was 404 → spline). Runtime retargeting (the model is skeleton-fixed; this makes it usable on real rigs instead of always-fallback): - MotionInbetween::canonicalIndexForBone() maps arbitrary bone names → the 22 canonical roles. Handles Mixamo (mixamorig: prefix; "Shoulder"=clavicle→collar, "Arm"=upper-arm→shoulder role; Spine2→chest), generic (L_Shoulder), and CMU names; early-rejects finger/toe/face/twist/helper bones. Exhaustively unit-tested against the full naming matrix. - AnimationMerger::inbetweenAnimation now maps tracks→canonical, and when ≥¾ of the 22 roles resolve, packs the canonical [2,220] pose, runs the model, and scatters predictions back to matched bracketed tracks; unmatched/non-bracketed tracks + low-coverage rigs + non-ONNX builds use the per-track spline. Docs: THIRD_PARTY_AI_MODELS.md + CLAUDE.md updated (CMU attribution, hosting, retargeting). End-to-end verified: the app downloads the real 13 MB model and loads it through the C++ ONNX runtime ([1,2,220]→[1,8,220]). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MotionInbetween.cpp (1)
512-518: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject non-finite RMIB outputs before marking the model run successful.
A NaN/Inf from the model is copied into
Result::framesand later written to Ogre keyframes; quaternion normalization downstream can then persist invalid transforms. Fall back before slicing when any required output value is non-finite.Suggested guard
const float* d = outputs[0].GetTensorData<float>(); + const size_t required = static_cast<size_t>(gap) * C; + for (size_t i = 0; i < required; ++i) { + if (!std::isfinite(d[i])) + return fallback(QStringLiteral( + "RMIB output contained non-finite values — used the spline fallback.")); + } Result r;🤖 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 512 - 518, The RMIB output handling in MotionInbetween::process currently copies tensor values straight into Result::frames without checking for NaN/Inf, so add a non-finite validation step before building Pose objects and marking the run successful. Inspect the tensor data from outputs[0] in the same block, reject the result if any required value is not finite, and fall back early instead of pushing invalid poses into r.frames; use the surrounding Result, Pose, and outputs[0].GetTensorData<float>() flow to place the guard where it will prevent invalid keyframes downstream.
♻️ Duplicate comments (1)
src/MotionInbetween.cpp (1)
495-510: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire the output to contain exactly
gapframe rows.The current check still accepts tensors like
[1, gap + 2, C]or[2, gap, C]because the last dimension matches andelems >= gap*C; slicing the firstgaprows can write endpoints or the wrong batch as generated keyframes. Tighten this to an exactelems == gap*Ccheck, or explicitly whitelist[gap, C]/[1, gap, C].Suggested hardening
- // Total framed rows available = elems / C; must be >= gap. - if (elems < static_cast<size_t>(gap) * C) + // Total framed rows available must be exactly the requested gap. + if (elems != static_cast<size_t>(gap) * C) return fallback(QStringLiteral( - "RMIB output too small (%1 < %2) — used the spline fallback.") + "RMIB output frame count is incompatible (%1 != %2) — used the spline fallback.") .arg(elems).arg(static_cast<size_t>(gap) * C));🤖 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 495 - 510, The output validation in MotionInbetween.cpp is too permissive: the current shape checks in the RMIB path accept any tensor whose last dimension matches C and whose total elements are at least gap*C, which can still allow extra rows or batched layouts. Tighten the logic in the output-shape handling near the fallback path so it requires exactly gap frame rows (or explicitly only approved shapes like [gap, C] / [1, gap, C]) before slicing, using the existing outTI.GetShape(), elems, gap, and C checks to reject anything else.
🤖 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/export-rmib-onnx.py`:
- Line 104: Clean up the Ruff style issues in export-rmib-onnx.py by replacing
ambiguous loop/index names like the enumerate variable in the files iteration,
removing any unused loop variables, and splitting any multiple statements on a
single line into separate statements. Review the affected blocks around the
file-processing loops and the helper logic used later in the script so the code
stays lint-clean and reproducible.
- Around line 105-109: The BVH preprocessing path in the export script currently
swallows parse failures and can fall through to np.stack with an empty list,
making errors opaque. Update the parsing/selection flow around the BVH loading
and window collection logic to log each skipped file in the Bvh parsing loop
and, before calling np.stack in the window-building code, explicitly check
whether any windows were produced and raise a clear actionable error if none
were. Use the relevant Bvh parsing block and the window aggregation code that
feeds np.stack to locate the fix.
- Around line 207-210: The ONNX export path in `Wrap(net)` is still using the
model on MPS while `dummy` is created on CPU, so update the export setup to move
the wrapped model back to CPU before calling `torch.onnx.export`. Make sure the
`dummy` input and the model in the `torch.onnx.export` call are on the same
device in `export-rmib-onnx.py`, using the `Wrap(net)` export block as the place
to apply the fix.
In `@src/AnimationMerger.cpp`:
- Around line 876-898: The mixed RMIB/spline path in
AnimationMerger::interpolate/merger result handling is leaving
res.fallbackReason empty when handledByModel is partially true, so the run is
reported as model-only. Update the per-track spline fallback block to record
that the spline fallback was also used even when res.usedModel is already set,
and adjust the final result bookkeeping so callers can distinguish mixed runs
from pure RMIB runs.
In `@src/MotionInbetween.cpp`:
- Around line 63-70: The sideOf helper in MotionInbetween.cpp is over-matching
single-letter side detection by treating any name that starts with l or ends
with r as a left/right side. Tighten the logic so only real side affixes are
recognized, and avoid classifying generic names like lowerback, collar, or
shoulder as sided; update the checks in sideOf(const QString&) to require clear
side boundaries or known prefixes/suffixes before returning 'l' or 'r'.
---
Outside diff comments:
In `@src/MotionInbetween.cpp`:
- Around line 512-518: The RMIB output handling in MotionInbetween::process
currently copies tensor values straight into Result::frames without checking for
NaN/Inf, so add a non-finite validation step before building Pose objects and
marking the run successful. Inspect the tensor data from outputs[0] in the same
block, reject the result if any required value is not finite, and fall back
early instead of pushing invalid poses into r.frames; use the surrounding
Result, Pose, and outputs[0].GetTensorData<float>() flow to place the guard
where it will prevent invalid keyframes downstream.
---
Duplicate comments:
In `@src/MotionInbetween.cpp`:
- Around line 495-510: The output validation in MotionInbetween.cpp is too
permissive: the current shape checks in the RMIB path accept any tensor whose
last dimension matches C and whose total elements are at least gap*C, which can
still allow extra rows or batched layouts. Tighten the logic in the output-shape
handling near the fallback path so it requires exactly gap frame rows (or
explicitly only approved shapes like [gap, C] / [1, gap, C]) before slicing,
using the existing outTI.GetShape(), elems, gap, and C checks to reject anything
else.
🪄 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: e0455459-f91e-4d28-b5e0-3f325ffae58d
📒 Files selected for processing (11)
CLAUDE.mdTHIRD_PARTY_AI_MODELS.mdscripts/export-rmib-onnx.pysrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationMerger.cppsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/MotionInbetween.cppsrc/MotionInbetween.hsrc/MotionInbetween_test.cpp
✅ Files skipped from review due to trivial changes (2)
- THIRD_PARTY_AI_MODELS.md
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/CLIPipeline.cpp
- src/AnimationControlController.cpp
- src/MCPServer.cpp
Comment on lines
+207
to
+210
| dummy = torch.zeros(1, 2, C) | ||
| torch.onnx.export(Wrap(net).eval(), (dummy,), a.out, | ||
| input_names=["pose_pair"], output_names=["interior"], | ||
| opset_version=17, dynamo=False) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Static check: confirm the export path moves the model to CPU before torch.onnx.export.
python - <<'PY'
from pathlib import Path
text = Path("scripts/export-rmib-onnx.py").read_text().splitlines()
for i, line in enumerate(text, 1):
if "net = Net().to(dev)" in line or "dummy =" in line or "torch.onnx.export" in line or "to(\"cpu\")" in line:
print(f"{i}: {line}")
PYRepository: fernandotonon/QtMeshEditor
Length of output: 293
Move the export model back to CPU. Wrap(net) can still be on MPS when torch.onnx.export runs, while the dummy input is created on CPU, which can trigger a device mismatch on macOS.
🤖 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/export-rmib-onnx.py` around lines 207 - 210, The ONNX export path in
`Wrap(net)` is still using the model on MPS while `dummy` is created on CPU, so
update the export setup to move the wrapped model back to CPU before calling
`torch.onnx.export`. Make sure the `dummy` input and the model in the
`torch.onnx.export` call are on the same device in `export-rmib-onnx.py`, using
the `Wrap(net)` export block as the place to apply the fix.
Source: Learnings
Owner
Author
There was a problem hiding this comment.
Addressed in d1152bd — see the preprocess() empty-result guard + the ruff cleanup above (this is the dev-only export script, not shipped/CI'd).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant lines from scripts/export-rmib-onnx.py =="
nl -ba scripts/export-rmib-onnx.py | sed -n '180,240p'
echo
echo "== Search for CPU move around ONNX export =="
rg -n 'to\("cpu"\)|to\(dev\)|dummy\s*=|torch\.onnx\.export|Wrap\(' scripts/export-rmib-onnx.pyLength of output: 699
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant lines from scripts/export-rmib-onnx.py =="
python - <<'PY'
from pathlib import Path
path = Path("scripts/export-rmib-onnx.py")
lines = path.read_text().splitlines()
for i in range(180, min(240, len(lines)) + 1):
print(f"{i:4}: {lines[i-1]}")
PY
echo
echo "== Search for CPU move around ONNX export =="
python - <<'PY'
from pathlib import Path
text = Path("scripts/export-rmib-onnx.py").read_text().splitlines()
needles = ['to("cpu")', "to(dev)", "dummy =", "torch.onnx.export", "Wrap("]
for i, line in enumerate(text, 1):
if any(n in line for n in needles):
print(f"{i:4}: {line}")
PYLength of output: 3340
@fernandotonon, I re-checked the current file and this specific issue still appears to be present.
- Line 192:
net = Net().to(dev) - Line 224:
dummy = torch.zeros(1, 2, C)(CPU) - Line 225:
torch.onnx.export(Wrap(net).eval(), (dummy,), a.out, ...)
So the export path still uses a wrapped model that can remain on MPS while the dummy input is on CPU. The preprocess() empty-result guard and Ruff cleanup look orthogonal to this particular mismatch.
Given that this is a dev-only script, the fix can stay very small, e.g. moving the export model to CPU before export (or creating dummy on the same device and exporting consistently).
Owner
Author
There was a problem hiding this comment.
Addressed in d1152bd (verified present on HEAD): ruff-clean (dropped unused sys/math imports, renamed the ambiguous I tensor, split ;-joined statements, removed the dead 4 if False else 10), and preprocess() now raises an actionable SystemExit (with skipped-file count) when zero windows result instead of an opaque np.stack([]) crash. This is the offline, NOT-shipped dev export tool.
Owner
Author
There was a problem hiding this comment.
Addressed in d1152bd (present on HEAD): ruff-clean + preprocess() raises an actionable error on zero windows instead of np.stack([]). Offline dev export tool, not shipped/CI'd.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Current export block in scripts/export-rmib-onnx.py =="
python - <<'PY'
from pathlib import Path
path = Path("scripts/export-rmib-onnx.py")
lines = path.read_text().splitlines()
for i in range(185, min(len(lines), 235) + 1):
print(f"{i:4}: {lines[i-1]}")
PY
echo
echo "== Device/export-related lines =="
python - <<'PY'
from pathlib import Path
text = Path("scripts/export-rmib-onnx.py").read_text().splitlines()
needles = ["to(dev)", 'to("cpu")', "dummy =", "torch.onnx.export", "Wrap("]
for i, line in enumerate(text, 1):
if any(n in line for n in needles):
print(f"{i:4}: {line}")
PYLength of output: 3071
@fernandotonon, thanks — I rechecked HEAD, and the cleanup/preprocess() guard are present, but this specific export-device issue is still unchanged.
Current state in scripts/export-rmib-onnx.py:
- Line 192:
net = Net().to(dev) - Line 224:
dummy = torch.zeros(1, 2, C)← CPU - Line 225:
torch.onnx.export(Wrap(net).eval(), (dummy,), a.out, ...)
So if dev == "mps", the wrapped model can still be on MPS while the ONNX export input is on CPU.
A minimal fix would be:
export_net = Wrap(net.to("cpu")).eval()
dummy = torch.zeros(1, 2, C)
torch.onnx.export(export_net, (dummy,), a.out,
input_names=["pose_pair"], output_names=["interior"],
opset_version=17, dynamo=False)I agree this is an offline dev-only script, so the impact is limited — but the mismatch itself still looks real.
(˘･_･˘)
✏️ Learnings added
Learnt from: fernandotonon
Repo: fernandotonon/QtMeshEditor PR: 762
File: scripts/export-rmib-onnx.py:224-227
Timestamp: 2026-06-27T18:19:03.747Z
Learning: `scripts/export-rmib-onnx.py` in the QtMeshEditor repository is an offline developer-only export script for generating the RMIB ONNX model; it is not shipped with the app and is intentionally not wired into CMake or CI.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+876
to
+898
| // --- Per-track spline pass (every bracketed track the model didn't do) --- | ||
| for (size_t bi = 0; bi < ctxs.size(); ++bi) { | ||
| if (!ctxs[bi].bracketed || handledByModel[bi]) continue; | ||
| // Single-bone spline: layout is the 10-DoF for one bone. | ||
| MotionInbetween::Pose s(10), e(10); | ||
| packPose(ctxs[bi].startKey, s, 0); | ||
| packPose(ctxs[bi].endKey, e, 0); | ||
| std::vector<MotionInbetween::Channel> oneLayout = { | ||
| MotionInbetween::Channel::Scalar, MotionInbetween::Channel::Scalar, | ||
| MotionInbetween::Channel::Scalar, MotionInbetween::Channel::QuatStart, | ||
| MotionInbetween::Channel::QuatCont, MotionInbetween::Channel::QuatCont, | ||
| MotionInbetween::Channel::QuatCont, MotionInbetween::Channel::Scalar, | ||
| MotionInbetween::Channel::Scalar, MotionInbetween::Channel::Scalar }; | ||
| MotionInbetween::Options o; o.gapFrames = gapFrames; o.forceFallback = true; | ||
| const auto sr = MotionInbetween::interpolateSpline(s, e, oneLayout, o); | ||
| if (!sr.ok) continue; | ||
| for (int f = 0; f < gapFrames; ++f) { | ||
| writeKey(ctxs[bi].track, interiorT(f), sr.frames[f], 0); | ||
| ++res.keyframesInserted; | ||
| } | ||
| ++res.tracksAffected; | ||
| if (!res.usedModel && res.fallbackReason.isEmpty()) | ||
| res.fallbackReason = QStringLiteral("Used the spline fallback."); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report mixed RMIB/spline runs instead of hiding the spline portion.
When RMIB handles some canonical tracks and the spline pass fills unmatched bracketed tracks, res.usedModel is already true, so Line 897 never sets fallbackReason. The caller then reports a pure model run even though some tracks used spline.
Suggested tracking
+ int splineTracksAffected = 0;
for (size_t bi = 0; bi < ctxs.size(); ++bi) {
if (!ctxs[bi].bracketed || handledByModel[bi]) continue;
@@
}
++res.tracksAffected;
- if (!res.usedModel && res.fallbackReason.isEmpty())
- res.fallbackReason = QStringLiteral("Used the spline fallback.");
+ ++splineTracksAffected;
}
+ if (splineTracksAffected > 0) {
+ res.fallbackReason = res.usedModel
+ ? QStringLiteral("RMIB model used for matched tracks; spline fallback used for %1 track(s).")
+ .arg(splineTracksAffected)
+ : QStringLiteral("Used the spline fallback.");
+ }📝 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
| // --- Per-track spline pass (every bracketed track the model didn't do) --- | |
| for (size_t bi = 0; bi < ctxs.size(); ++bi) { | |
| if (!ctxs[bi].bracketed || handledByModel[bi]) continue; | |
| // Single-bone spline: layout is the 10-DoF for one bone. | |
| MotionInbetween::Pose s(10), e(10); | |
| packPose(ctxs[bi].startKey, s, 0); | |
| packPose(ctxs[bi].endKey, e, 0); | |
| std::vector<MotionInbetween::Channel> oneLayout = { | |
| MotionInbetween::Channel::Scalar, MotionInbetween::Channel::Scalar, | |
| MotionInbetween::Channel::Scalar, MotionInbetween::Channel::QuatStart, | |
| MotionInbetween::Channel::QuatCont, MotionInbetween::Channel::QuatCont, | |
| MotionInbetween::Channel::QuatCont, MotionInbetween::Channel::Scalar, | |
| MotionInbetween::Channel::Scalar, MotionInbetween::Channel::Scalar }; | |
| MotionInbetween::Options o; o.gapFrames = gapFrames; o.forceFallback = true; | |
| const auto sr = MotionInbetween::interpolateSpline(s, e, oneLayout, o); | |
| if (!sr.ok) continue; | |
| for (int f = 0; f < gapFrames; ++f) { | |
| writeKey(ctxs[bi].track, interiorT(f), sr.frames[f], 0); | |
| ++res.keyframesInserted; | |
| } | |
| ++res.tracksAffected; | |
| if (!res.usedModel && res.fallbackReason.isEmpty()) | |
| res.fallbackReason = QStringLiteral("Used the spline fallback."); | |
| // --- Per-track spline pass (every bracketed track the model didn't do) --- | |
| int splineTracksAffected = 0; | |
| for (size_t bi = 0; bi < ctxs.size(); ++bi) { | |
| if (!ctxs[bi].bracketed || handledByModel[bi]) continue; | |
| // Single-bone spline: layout is the 10-DoF for one bone. | |
| MotionInbetween::Pose s(10), e(10); | |
| packPose(ctxs[bi].startKey, s, 0); | |
| packPose(ctxs[bi].endKey, e, 0); | |
| std::vector<MotionInbetween::Channel> oneLayout = { | |
| MotionInbetween::Channel::Scalar, MotionInbetween::Channel::Scalar, | |
| MotionInbetween::Channel::Scalar, MotionInbetween::Channel::QuatStart, | |
| MotionInbetween::Channel::QuatCont, MotionInbetween::Channel::QuatCont, | |
| MotionInbetween::Channel::QuatCont, MotionInbetween::Channel::Scalar, | |
| MotionInbetween::Channel::Scalar, MotionInbetween::Channel::Scalar }; | |
| MotionInbetween::Options o; o.gapFrames = gapFrames; o.forceFallback = true; | |
| const auto sr = MotionInbetween::interpolateSpline(s, e, oneLayout, o); | |
| if (!sr.ok) continue; | |
| for (int f = 0; f < gapFrames; ++f) { | |
| writeKey(ctxs[bi].track, interiorT(f), sr.frames[f], 0); | |
| +res.keyframesInserted; | |
| } | |
| +res.tracksAffected; | |
| +splineTracksAffected; | |
| } | |
| if (splineTracksAffected > 0) { | |
| res.fallbackReason = res.usedModel | |
| ? QStringLiteral("RMIB model used for matched tracks; spline fallback used for %1 track(s).") | |
| .arg(splineTracksAffected) | |
| : QStringLiteral("Used the spline fallback."); | |
| } |
🤖 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 876 - 898, The mixed RMIB/spline path
in AnimationMerger::interpolate/merger result handling is leaving
res.fallbackReason empty when handledByModel is partially true, so the run is
reported as model-only. Update the per-track spline fallback block to record
that the spline fallback was also used even when res.usedModel is already set,
and adjust the final result bookkeeping so callers can distinguish mixed runs
from pure RMIB runs.
Comment on lines
+63
to
+70
| char sideOf(const QString& n) | ||
| { | ||
| if (n.contains("left")) return 'l'; | ||
| if (n.contains("right")) return 'r'; | ||
| if (n.startsWith('l') && n.size() > 1) return 'l'; | ||
| if (n.startsWith('r') && n.size() > 1) return 'r'; | ||
| if (n.endsWith('l')) return 'l'; | ||
| if (n.endsWith('r')) return 'r'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Constrain single-letter side detection to real side affixes.
Line 67 treats any name starting with l as left and Line 70 treats any name ending in r as right, so lowerback fails the center-spine mapping and generic collar/shoulder can be mapped as right-sided. That can feed the RMIB retargeter with wrong canonical roles.
Suggested direction
+bool isKnownSidedStem(const QString& stem)
+{
+ return stem == "shoulder" || stem == "collar" || stem == "elbow"
+ || stem == "hand" || stem == "wrist" || stem == "buttock"
+ || stem == "hip" || stem == "knee" || stem == "foot"
+ || stem == "arm" || stem == "upperarm" || stem == "forearm"
+ || stem == "lowerarm" || stem == "leg" || stem == "upleg";
+}
+
char sideOf(const QString& n)
{
if (n.contains("left")) return 'l';
if (n.contains("right")) return 'r';
- if (n.startsWith('l') && n.size() > 1) return 'l';
- if (n.startsWith('r') && n.size() > 1) return 'r';
- if (n.endsWith('l')) return 'l';
- if (n.endsWith('r')) return 'r';
+ if (n.startsWith('l') && isKnownSidedStem(n.mid(1))) return 'l';
+ if (n.startsWith('r') && isKnownSidedStem(n.mid(1))) return 'r';
+ if (n.endsWith('l') && isKnownSidedStem(n.left(n.size() - 1))) return 'l';
+ if (n.endsWith('r') && isKnownSidedStem(n.left(n.size() - 1))) return 'r';
return 0;
}📝 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
| char sideOf(const QString& n) | |
| { | |
| if (n.contains("left")) return 'l'; | |
| if (n.contains("right")) return 'r'; | |
| if (n.startsWith('l') && n.size() > 1) return 'l'; | |
| if (n.startsWith('r') && n.size() > 1) return 'r'; | |
| if (n.endsWith('l')) return 'l'; | |
| if (n.endsWith('r')) return 'r'; | |
| bool isKnownSidedStem(const QString& stem) | |
| { | |
| return stem == "shoulder" || stem == "collar" || stem == "elbow" | |
| || stem == "hand" || stem == "wrist" || stem == "buttock" | |
| || stem == "hip" || stem == "knee" || stem == "foot" | |
| || stem == "arm" || stem == "upperarm" || stem == "forearm" | |
| || stem == "lowerarm" || stem == "leg" || stem == "upleg"; | |
| } | |
| char sideOf(const QString& n) | |
| { | |
| if (n.contains("left")) return 'l'; | |
| if (n.contains("right")) return 'r'; | |
| if (n.startsWith('l') && isKnownSidedStem(n.mid(1))) return 'l'; | |
| if (n.startsWith('r') && isKnownSidedStem(n.mid(1))) return 'r'; | |
| if (n.endsWith('l') && isKnownSidedStem(n.left(n.size() - 1))) return 'l'; | |
| if (n.endsWith('r') && isKnownSidedStem(n.left(n.size() - 1))) return 'r'; |
🤖 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 63 - 70, The sideOf helper in
MotionInbetween.cpp is over-matching single-letter side detection by treating
any name that starts with l or ends with r as a left/right side. Tighten the
logic so only real side affixes are recognized, and avoid classifying generic
names like lowerback, collar, or shoulder as sided; update the checks in
sideOf(const QString&) to require clear side boundaries or known
prefixes/suffixes before returning 'l' or 'r'.
scripts/export-rmib-onnx.py (dev-only, not shipped/CI'd): - Raise an actionable error if preprocessing yields zero windows (all BVH failed to parse or none match the canonical 22-joint skeleton) instead of an opaque np.stack([]) crash; log skipped-file count. - Ruff cleanup: drop unused imports (sys, math), rename the ambiguous bare `I` tensor to `Iv`, split `;`-joined statements, remove the dead `4 if False else 10`. `ruff check` now passes clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/export-rmib-onnx.py (1)
195-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidation split is held out but never used.
nvalreserves the firstnvalsamples (perm = torch.randperm(n - nval) + nval), excluding them from training, yet no validation loss is ever computed. The effect is simply ~10% of the data is discarded with no validation signal, early stopping, or checkpoint selection. Either evaluate on the held-out set per epoch or train on all data.🤖 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/export-rmib-onnx.py` around lines 195 - 214, The training loop in scripts/export-rmib-onnx.py reserves a validation split with nval and excludes it via the perm-based sampling, but never uses that held-out data. Update the epoch loop around net.train(), qb, and the optimizer step to either compute a validation loss on S[:nval], E[:nval], Iv[:nval] each epoch (without gradients) for monitoring/early stopping/checkpoint selection, or remove the split and train on all samples if validation is not needed.
🤖 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/export-rmib-onnx.py`:
- Around line 202-203: The remaining Ruff warnings are in the reshape helper and
epoch loop: the tuple concatenation in the return statement and the unused loop
variable in the epochs iteration. Update the reshape expression in the
surrounding helper to use sequence unpacking instead of building the shape with
tuple concatenation, and change the `for ep in range(a.epochs)` loop to avoid
introducing an unused `ep` variable (for example by iterating without binding
it). Keep the fix localized to the code near the reshape logic and the
`a.epochs` loop in `export-rmib-onnx.py`.
---
Nitpick comments:
In `@scripts/export-rmib-onnx.py`:
- Around line 195-214: The training loop in scripts/export-rmib-onnx.py reserves
a validation split with nval and excludes it via the perm-based sampling, but
never uses that held-out data. Update the epoch loop around net.train(), qb, and
the optimizer step to either compute a validation loss on S[:nval], E[:nval],
Iv[:nval] each epoch (without gradients) for monitoring/early
stopping/checkpoint selection, or remove the split and train on all samples if
validation is not needed.
🪄 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: 231b1441-1f8a-4f54-b0e0-946cc6b66e7f
📒 Files selected for processing (1)
scripts/export-rmib-onnx.py
Comment on lines
+202
to
+203
| return x.reshape(x.shape[:-1] + (J, 10))[..., 3:7] | ||
| for ep in range(a.epochs): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remaining Ruff findings after the cleanup commit.
Despite the ruff-clean follow-up, two findings persist: line 203 ep is unused (B007) and line 202 prefers unpacking over tuple concatenation (RUF005).
🧹 Proposed fix
def qb(x): # [...,C] → the per-joint quaternion block
- return x.reshape(x.shape[:-1] + (J, 10))[..., 3:7]
- for ep in range(a.epochs):
+ return x.reshape((*x.shape[:-1], J, 10))[..., 3:7]
+ for _ep in range(a.epochs):📝 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
| return x.reshape(x.shape[:-1] + (J, 10))[..., 3:7] | |
| for ep in range(a.epochs): | |
| return x.reshape((*x.shape[:-1], J, 10))[..., 3:7] | |
| for _ep in range(a.epochs): |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 202-202: Consider (*x.shape[:-1], J, 10) instead of concatenation
Replace with (*x.shape[:-1], J, 10)
(RUF005)
[warning] 203-203: Loop control variable ep not used within loop body
Rename unused ep to _ep
(B007)
🤖 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/export-rmib-onnx.py` around lines 202 - 203, The remaining Ruff
warnings are in the reshape helper and epoch loop: the tuple concatenation in
the return statement and the unused loop variable in the epochs iteration.
Update the reshape expression in the surrounding helper to use sequence
unpacking instead of building the shape with tuple concatenation, and change the
`for ep in range(a.epochs)` loop to avoid introducing an unused `ep` variable
(for example by iterating without binding it). Keep the fix localized to the
code near the reshape logic and the `a.epochs` loop in `export-rmib-onnx.py`.
Source: Linters/SAST tools
… sheet The dope-sheet "Fill gap" control invoked inbetweenWindow but never displayed the controller's inbetweenStatus signal, so a successful RMIB-model fill and a "no empty gap to fill" skip both looked like nothing happened — which read as "it fell back to spline / didn't use the model." Add a status line under the control that shows the actual outcome: - success → "Inserted N keyframes across M track(s) via RMIB model" (or "via spline fallback"), straight from the controller message; - the common dense-clip case (selected range already has interior keyframes → the no-stacking guard skips it) now gets a clear hint: "Selected range already has keyframes — in-betweening fills empty gaps between sparse keys", instead of an opaque "nothing to fill" error. No behaviour change — the model was already being used on genuine gaps (verified: "inserted 416 keyframes via RMIB model" on a 2-key clip; 20/22 canonical roles resolve from a Mixamo rig). This is purely the missing feedback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
….13.0 Importing a model became slow because UVEditorController::refresh() (a full per-submesh UV read + connected-island computation) was wired directly to SelectionSet::selectionChanged AND Manager::entityCreated — both of which fire repeatedly during import (every entity + every selection change). On a multi- submesh / multi-entity model that's an O(N) storm of the expensive rebuild, even though the UV editor dock is almost always CLOSED during import. Fix: gate the signal-driven path on the panel actually being visible. - New UVEditorController::setActive(bool), driven by the UV dock's visibilityChanged. Active only while the dock is shown. - Signal-driven refreshes go through a new gated onSourceChanged(): when inactive it just marks the cache dirty (cheap) instead of rebuilding; setActive(true) flushes the deferred rebuild lazily. The explicit, Q_INVOKABLE refresh() still rebuilds immediately (QML/tests/user action). - Constructor no longer eagerly rebuilds (starts inactive+dirty). - mainwindow wires setActive(vis) from the dock instead of refresh(). - Unit test: an inactive controller does NOT rebuild on selection; setActive(true) flushes the deferred build. Net: with the UV editor closed (the common case), import does zero UV-cache work; with it open, behaviour is unchanged. Also bumps the project version 3.12.0 → 3.13.0 (this PR's in-betweening feature grew to include the trained RMIB model + retargeting); doc refs synced. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # src/UVEditorController.cpp # src/UVEditorController.h # src/UVEditorController_test.cpp # src/mainwindow.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
qml/AnimationDopeSheet.qml (1)
376-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid keying this hint off an English error substring.
This branch depends on
r.errorcontaining"bracketing", which is just the current C++ wording. A copy change insrc/AnimationMerger.cpp:901-906silently breaks the hint. Prefer a structured reason/error-code in the controller result map and switch on that here.🤖 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/AnimationDopeSheet.qml` around lines 376 - 385, The inbetween hint logic in AnimationDopeSheet.qml is brittle because it checks r.error for the English substring "bracketing"; update the controller result to expose a structured reason or error code instead of relying on wording. Then change the inbetweenBar update branch to switch on that structured field from the controller result map, using the existing inbetweenStatus flow and the r / r.ok handling to locate the affected logic.
🤖 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/AnimationDopeSheet.qml`:
- Around line 301-305: The status text in the QML panel is only cleared in
onVisibleChanged, so ibMessage can stay stale when the target gap changes.
Update the state reset logic around ibMessage and ibError in
AnimationDopeSheet.qml so changes to inbetweenT0, inbetweenT1, and
inbetweenFrames also clear the previous result, ensuring each new fill request
starts with fresh status.
---
Nitpick comments:
In `@qml/AnimationDopeSheet.qml`:
- Around line 376-385: The inbetween hint logic in AnimationDopeSheet.qml is
brittle because it checks r.error for the English substring "bracketing"; update
the controller result to expose a structured reason or error code instead of
relying on wording. Then change the inbetweenBar update branch to switch on that
structured field from the controller result map, using the existing
inbetweenStatus flow and the r / r.ok handling to locate the affected logic.
🪄 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: 31c396d2-9d3c-460d-9dab-29a3e5f7ccc3
📒 Files selected for processing (4)
CMakeLists.txtREADME.mdqml/AnimationDopeSheet.qmlwebsite/src/hooks/useQtmeshActionRef.js
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- website/src/hooks/useQtmeshActionRef.js
FBX import froze the UI for 60s+. Root cause: hydrateEntityTexturesFromSearchPaths
unconditionally did a RECURSIVE QDirIterator over the import file's directory,
building a map of EVERY file under it — and when importing from a large folder
(e.g. a repo/home dir with tens of thousands of files) that single walk is a
minute of synchronous stat()+hash on the main thread. Worse, importMeshs ran the
whole per-entity rebind (which triggers the walk) TWICE: immediately AND again
in a deferred QTimer.
Fixes:
- hydrateEntityTexturesFromSearchPaths is now lazy + bounded:
* PASS 1 (cheap): collect only the texture names the entity references whose
image isn't ALREADY loadable by Ogre (TextureManager). In the common case
(embedded / adjacent textures) nothing is missing → the directory walk is
skipped entirely.
* PASS 2 (only if something's missing): walk the roots for just those names,
EARLY-EXIT once all are found, and cap total files scanned at 20k so a
pathological root can't freeze the UI (logs a breadcrumb if capped).
- importMeshs no longer rebinds materials immediately AND in the QTimer — the
expensive rebind (hydration + RTSS sync + scene material walk) now runs ONCE,
deferred off the import-blocking path.
- AnimationDopeSheet: clear the in-between status line when the target window
(T0/T1) or frame count changes, not just on dock visibility (CodeRabbit).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Owner
Author
|
Re: CodeRabbit findings on
(In-thread replies to these outside-diff comments wouldn't attach via the API, hence this consolidated note.) |
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.
Implements issue #409 — AI: Animation in-betweening (ONNX). Follows the established AI-assist pattern (#404 PbrMapSynth, #408 UniRig).
What it does
Fills the gap between two sparse keyframes with smooth, plausible intermediate poses via Robust Motion In-betweening (Harvey et al., Ubisoft, SIGGRAPH 2020) on ONNX Runtime — the third ONNX consumer. Surfaced on CLI, MCP, and the dope sheet.
Spline fallback is first-class (per the acceptance criteria)
MotionInbetween::interpolateSpline— cubic-Hermite (Catmull-Rom tangents) for translation/scale + shortest-arc slerp for rotation — is always compiled (Ogre-free, no ONNX) and used automatically when:ENABLE_ONNX,Result::usedModel+fallbackReasonreport which path ran, so the GUI/CLI/MCP surface a clear "fell back to spline" note. It is visibly smoother than naive linear interpolation (C1-continuous endpoints; a unit test asserts it differs from a linear midpoint on curved neighbours).Surfaces (CLI / MCP / GUI parity)
qtmesh anim <file> --in-between --gap-frames N [--start-time S] [--end-time S] [--no-model] [--animation NAME] [-o out]motion_in_betweentool —{gap_frames, entity_name?, animation_name?, start_time?, end_time?, no_model?}(registered heavy).AnimationControlController::inbetweenWindow→inbetweenStatus(message, isError).Architecture
src/MotionInbetween.{h,cpp}— Ogre-free core: flat per-frame pose arrays (bones × 10 DoF) + aChannellayout (Scalar/QuatStart/QuatCont); ONNX path does runtime I/O-name + channel-count discovery and falls back on any mismatch. Self-containedensureModelBlocking()(the AI: RigNet auto-rigging (ONNX, ML) #408 pattern) downloadsrmib.onnxtoAppData/ai_models/inbetween/on first use (overridesQTMESH_INBETWEEN_MODEL_BASE_URL/QSettings ai/inbetweenModelBaseUrl; offline guardQTMESH_INBETWEEN_NO_DOWNLOAD; non-ONNX#ifndefguard).AnimationMerger::inbetweenAnimation— packs every bracketing track's start/end pose into ONEpredict()(full skeleton), scatters predicted per-frame poses back as keyframes.ai.assist.in_betweenon all three surfaces.Acceptance criteria
ai.assist.in_between.Hosting status
The exported
rmib.onnxis not yet hosted (same situation #408 shipped in), so today the feature runs the spline fallback end-to-end — fully functional and offline. Hosting a permissively-licensed export lights up the ML path with no code change. Licensing position documented inTHIRD_PARTY_AI_MODELS.md.Version bumped to 3.12.0.
🤖 Generated with Claude Code
Summary by CodeRabbit
--in-between/--gap-frames/--start-time/--end-timeand--no-model, plus an MCP tool for in-betweening.Update — real ML model trained + hosted, retargeting, UV-import perf fix
inbetween/rmib.onnx(~13 MB) infernandotonon/QtMeshEditor-models. ~421k windows / 22 core-body joints / small transformer; rotation error < half of slerp on a held-out CMU split.scripts/export-rmib-onnx.pyis the one-time offline dev tool (not shipped/CI'd). The download URL that previously 404'd now resolves, and the app loads + runs it through the C++ ONNX runtime (verified: "inserted 416 keyframes via RMIB model").MotionInbetween::canonicalIndexForBone()maps arbitrary rig bones onto the canonical roles — Mixamo (mixamorig:prefix; Shoulder=clavicle→collar, Arm=upper-arm→shoulder; Spine2→chest), generic (L_Shoulder), CMU; rejects finger/toe/face bones. Exhaustively unit-tested.AnimationMerger::inbetweenAnimationruns the model when ≥¾ of the 22 roles resolve (a Mixamo rig resolves 20/22), else per-track spline.UVEditorControllerrebuilt its (expensive per-submesh) UV cache on everyentityCreated/selectionChanged, slowing imports of multi-submesh models even with the UV dock closed. Now gated on dock visibility (setActive) — closed = zero UV-cache work during import; open = unchanged.CodeRabbit reviews (both the C++ batch and the export-script batch) addressed; all checks were green prior to this push.
Update — FBX import performance fix + review
MeshImporterExporter::hydrateEntityTexturesFromSearchPathsdid an unconditional recursiveQDirIteratorover the import file's folder, indexing every file under it — pathological when importing from a large directory (tens of thousands of files), andimportMeshsran the whole per-entity rebind twice (immediate + a deferred QTimer). Fixes:T0/T1) or frame count changes (not just on dock visibility) — CodeRabbit.master, which also brought in the UV-editor import-defer fix (fix: defer UV rebuild on import to prevent UI freeze #763).All checks green; PR is mergeable.