feat(#408): ML auto-rigging backend (ONNX) — UniRig (MIT) + --algo selector - #761
Merged
Conversation
Add a pluggable skeleton-prediction backend to AutoRig (#407): the new AutoRig::Algorithm {Pinocchio, RigNet} threads through Options/Report and the CLI/MCP/GUI. Default stays Pinocchio (offline, deterministic). RigNet (Xu et al. 2020) is the second ONNX consumer after #404 PbrMapSynth: - RigNetPredictor (Ogre-free, unit-tested): normalises verts to a centred unit box (+Y up), builds an edge list from triangle indices, runs the ONNX model (input/output tensor names discovered at runtime), de-normalises predicted joints, and — when the model emits no connectivity — builds a nearest-neighbour MST and reorders parent-before-child for Ogre. - Model (~50 MB, AppData/ai_models/rignet/rignet.onnx) downloads on first use via ModelDownloader (ensureModelBlocking, 180s timeout; QTMESH_RIGNET_* env overrides + offline guard), mirroring AIAssistManager. - ALL ENABLE_ONNX-guarded; RigNet FALLS BACK to Pinocchio (logged in report.fallbackReason) when ONNX is off / model missing/offline/not-yet- hosted / prediction unusable — so the feature is reliable offline. - Design contract: the published RigNet checkpoint has no clean ONNX export; scripts/export-rignet-onnx.py (one-time, offline, not shipped) is the intended export targeting RigNetPredictor's I/O. Until hosted, the download 404s and the fallback runs. Surfaces: CLI `rig --algo pinocchio|rignet`, MCP `auto_rig` `algo` param, Inspector Rigging-section "Algorithm" picker. Sentry `ai.assist.auto_rig` breadcrumb records `algo`. Markers are template-only (RigNet predicts its own structure), so a marker-driven rig always uses the template. Tests: algorithm string round-trip + report JSON; RigNetPredictor graceful failure (missing model, too-few-verts, null, no-download guard). 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:
📝 WalkthroughWalkthroughAutoRig now supports a UniRig backend alongside Pinocchio. The PR adds algorithm selection through the GUI, CLI, and MCP tool, introduces UniRig ONNX prediction and export tooling, updates reports and tests with backend provenance, and defers custom palette application until after window construction. ChangesUniRig auto-rigging
Release/version references
Deferred custom palette application
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
The one-time, offline (NOT shipped) dev tool that exports RigNet to the ONNX I/O contract RigNetPredictor targets. Documents the export approach (PyG graph-conv rewrite + emit joint positions; C++ builds the MST) and the exact input/output tensor contract. The graph-conv→ONNX rewrite is left as documented TODOs since it depends on the upstream checkpoint layout; the app ships without the model and falls back to Pinocchio until rignet.onnx is hosted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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: 039cc3ae9f
ℹ️ 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
+207
to
+208
| edges.push_back(a); edges.push_back(b); | ||
| edges.push_back(b); edges.push_back(a); |
There was a problem hiding this comment.
When the ONNX model declares an edges input, the tensor shape used later is [2,E], which is row-major (all sources followed by all destinations). These pushes build [src,dst,src,dst,...], so the model reads half of the interleaved pairs as sources and half as destinations, corrupting the graph for every mesh with faces. Either store two contiguous rows or declare [E,2] to match the interleaved layout.
Useful? React with 👍 / 👎.
Comment on lines
+570
to
+573
| for (size_t k = 0; k + 2 < n; k += 3) { | ||
| out.push_back(base + (is32 ? i32[k] : i16[k])); | ||
| out.push_back(base + (is32 ? i32[k+1] : i16[k+1])); | ||
| out.push_back(base + (is32 ? i32[k+2] : i16[k+2])); |
There was a problem hiding this comment.
For Ogre meshes where id->indexStart is non-zero (shared or offset index buffers), this reads from element 0 instead of the submesh's slice, so RigNet receives triangles from the wrong part of the buffer. The template fallback still sees positions, but --algo rignet predictions are driven by unrelated or invalid edges; offset the lookup by id->indexStart before pushing indices.
Useful? React with 👍 / 👎.
Comment on lines
+302
to
+306
| } else if ((etype == ONNX_TENSOR_ELEMENT_DATA_TYPE_INT64 || | ||
| etype == ONNX_TENSOR_ELEMENT_DATA_TYPE_INT32) && !parentData) { | ||
| // Accept int64 parents (int32 handled below by copy). | ||
| if (etype == ONNX_TENSOR_ELEMENT_DATA_TYPE_INT64) { | ||
| parentData = outs[i].GetTensorData<int64_t>(); |
There was a problem hiding this comment.
When the exported model returns the parent tensor as INT32, this branch matches it but never stores or copies the int32 data; parentData remains null, so the later connectivity check treats the model as if it emitted no parents and replaces the learned hierarchy with a nearest-neighbor MST. This loses valid RigNet bone connectivity for common int32 ONNX exports; normalize int32 parents into a buffer before building joints.
Useful? React with 👍 / 👎.
…++/ONNX port
RigNet is unhostable (GPL code + unlicensed weights + non-public ModelsResource
dataset). Retarget to UniRig (SIGGRAPH 2025, VAST-AI-Research/UniRig) — MIT code
+ MIT weights, trained on Articulation-XL2.0 (CC-BY-4.0) — the clean
permissively-licensed alternative. See THIRD_PARTY_AI_MODELS.md.
UniRig is an autoregressive transformer (no single-graph ONNX), so this is a
real C++/ONNX port, not a model swap:
- src/UniRigPredictor.{h,cpp} (replaces RigNetPredictor): surface-sample
points+normals → Michelangelo encoder ONNX (latent prefix) → greedy/
constrained autoregressive decode over the ~350M causal-LM decoder ONNX with
a manual KV-cache + the tokenizer's next-possible-token validity mask → the
EXACT UniRig detokenizer FSM (256 bins, continuous_range [-1,1], undiscretize,
branch/parent rules, vocab 267) → joints + parents, parent-before-child.
detokenize()/undiscretize() are public statics (unit-tested without ONNX).
- Two model files (encoder.onnx + decoder.onnx) under AppData/ai_models/unirig/,
downloaded on first use (ensureModelBlocking, QTMESH_UNIRIG_* env overrides).
- AutoRig::Algorithm::RigNet → UniRig threaded through enum/Options/Report; CLI
--algo pinocchio|unirig (rignet = deprecated alias), MCP auto_rig algo param +
schema, Inspector "Algorithm" picker. Sentry algo tag. Greedy decode is a
documented simplification of UniRig's beam+sampling.
- Falls back to Pinocchio (report.fallbackReason) when ONNX off / models
missing/offline / prediction unusable — reliable offline.
Hosting status: scripts/export-unirig-onnx.py (one-time, offline, NOT shipped)
exports the encoder + KV-cache decoder to the I/O the predictor targets. Until
the .onnx files are hosted, the download 404s and the Pinocchio fallback runs —
plumbing + runtime ship today; hosting lights up the ML path with no code change.
Builds clean (QtMeshEditor + UnitTests, ENABLE_ONNX on). Tests: detokenizer FSM,
undiscretize, algorithm round-trip, predictor graceful-failure.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fernandotonon
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/AutoRigController.cpp (1)
210-225: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCatch non-Ogre backend failures before leaving
busystuck.The UniRig path can fail outside Ogre; this UI surface only catches
Ogre::Exception, so astd::exceptionfrom model download/inference would bypassbusyChanged()cleanup and the failure map.Proposed fix
} catch (const Ogre::Exception& e) { m_busy = false; emit busyChanged(); const auto msg = QString::fromStdString(e.getFullDescription()); emit error(QStringLiteral("Ogre error: %1").arg(msg)); result["applied"] = false; result["error"] = msg; return result; + } catch (const std::exception& e) { + m_busy = false; + emit busyChanged(); + const auto msg = QString::fromUtf8(e.what()); + emit error(QStringLiteral("Auto-rig error: %1").arg(msg)); + result["applied"] = false; + result["error"] = msg; + return result; }🤖 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/AutoRigController.cpp` around lines 210 - 225, The UniRig execution path in AutoRigController only handles Ogre::Exception, so backend failures from model download or inference can escape and leave m_busy stuck true. Update the try/catch around AutoRigCommand and UndoManager::push in AutoRigController to also catch std::exception (and optionally a final catch-all), and make sure the same cleanup used in the Ogre path is performed before returning the failure result. Use the existing identifiers m_busy, busyChanged(), emit error, and result to keep the UI state and failure map consistent.
🧹 Nitpick comments (7)
src/UniRigPredictor.cpp (2)
111-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Qt platform guards for the platform-specific branches.
Line 111 and Lines 525-539 use
__APPLE__/_WIN32; switch these toQ_OS_MACOS/Q_OS_WINso the code follows the project’s cross-platform guard convention.As per coding guidelines, "
**/*.{cpp,h}: All code must compile and run on Windows, Linux (Ubuntu), and macOS; guard platform-specific APIs with#ifdef Q_OS_WIN,#ifdef Q_OS_MACOS, or#ifdef Q_OS_LINUX."Also applies to: 525-539
🤖 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/UniRigPredictor.cpp` around lines 111 - 113, Replace the platform-specific preprocessor guards in UniRigPredictor with Qt’s cross-platform macros: change the `__APPLE__` include guard near the top of the file and the `_WIN32` branch in the platform-specific logic around `UniRigPredictor` to use `Q_OS_MACOS` and `Q_OS_WIN` instead. Keep the existing behavior unchanged, but make sure all macOS/Windows-only code paths in this file use the project’s Qt guard convention consistently.Source: Coding guidelines
273-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd breadcrumbs around model download and inference.
Line 273 starts first-use model resolution/download, and Line 466 starts UniRig prediction; both are significant operations but do not add
SentryReporter::addBreadcrumb(...).As per coding guidelines, "
**/*.{cpp,h}: All user-facing actions and significant operations must add a Sentry breadcrumb viaSentryReporter::addBreadcrumb(category, message)using the established categories."Also applies to: 466-521
🤖 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/UniRigPredictor.cpp` around lines 273 - 342, Add Sentry breadcrumbs for the significant model-download and inference flows so user-facing operations are tracked. In UniRigPredictor::ensureModelBlocking, emit SentryReporter::addBreadcrumb(...) before starting resolution/download and around each download attempt/failure/success path. Also add a breadcrumb at the start of the UniRig prediction entry point (the UniRigPredictor method that begins inference) so the end-to-end action is covered. Use the existing breadcrumb categories and keep the messages descriptive and consistent with the established pattern.Source: Coding guidelines
src/AutoRig.cpp (2)
549-554: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
RigNetreferences in the helper comment. The active backend is UniRig; "Only used for the RigNet path" / "so RigNet sees one combined index buffer" should read UniRig to match the rest of the PR.🤖 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/AutoRig.cpp` around lines 549 - 554, The helper comment in the AutoRig submesh index append logic still refers to RigNet, but the active backend is UniRig. Update the wording around the append logic and the vertexBase/shared-vertex description to say UniRig instead of RigNet, keeping the comment aligned with the current backend terminology in AutoRig.
552-553: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn value contradicts the comment and is unused. The doc says it "Returns the number of vertices this submesh contributed (to advance vertexBase)", but the function always
return 0;, and the caller advances offsets via the separately-computedownOffset. Either drop the return type tovoidor implement the documented contract to prevent a future caller from trusting the stale comment.Also applies to: 576-576
🤖 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/AutoRig.cpp` around lines 552 - 553, The return value contract in the submesh emission path is inconsistent with the documentation and current usage: the function that emits the RigNet submesh always returns 0 even though the comment says it returns the number of vertices contributed for advancing vertexBase. Update the relevant emitter function in AutoRig.cpp to either return the actual contributed vertex count and keep the contract aligned, or change the function signature to void and remove the misleading return-based contract; also adjust the caller that currently relies on ownOffset so the behavior stays explicit and consistent.src/AutoRig.h (2)
219-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment lists the wrong string.
algorithmToStringnow returns"pinocchio" | "unirig", not"rignet"(the latter is only a deprecated input alias foralgorithmFromString).✏️ Suggested wording
- static QString algorithmToString(Algorithm a); // "pinocchio" | "rignet" + static QString algorithmToString(Algorithm a); // "pinocchio" | "unirig"🤖 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/AutoRig.h` at line 219, The doc comment on algorithmToString is outdated and still lists the deprecated string instead of the current return value. Update the comment attached to algorithmToString so it matches the actual mapping used by AutoRig and reflects "pinocchio" | "unirig"; keep "rignet" only associated with algorithmFromString as a deprecated input alias.
150-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
RigNetnaming in comment. Backend is nowUniRig; the comment still says "RigNet falls back to Pinocchio". Worth aligning to avoid confusion with the rejected RigNet backend documented inTHIRD_PARTY_AI_MODELS.md.✏️ Suggested wording
- // Which backend actually produced the skeleton (RigNet falls back to - // Pinocchio when its model / ONNX runtime is unavailable). + // Which backend actually produced the skeleton (UniRig falls back to + // Pinocchio when its model / ONNX runtime is unavailable).🤖 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/AutoRig.h` around lines 150 - 152, The comment on algorithmUsed still refers to the old RigNet backend, so update the wording in AutoRig::algorithmUsed initialization to say UniRig falls back to Pinocchio when its model or runtime is unavailable. Keep the comment aligned with the current backend naming everywhere it describes the source of the skeleton, and remove any stale RigNet reference to avoid confusion.src/MCPServer.cpp (1)
6487-6492: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd the accepted algorithm enum to the MCP schema.
The handler validates
pinocchio,unirig, andrignet, but the schema only describes them in prose. MCP clients rely on schema enums for discoverability and argument generation.Proposed fix
props["algo"] = QJsonObject{{"type", "string"}, + {"enum", QJsonArray{"pinocchio", "unirig", "rignet"}}, {"description", "Skeleton-prediction backend: 'pinocchio' (native template embedding, " "offline, default) or 'unirig' (UniRig ML model via ONNX — better on "🤖 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 6487 - 6492, The MCP schema for the `algo` property in `MCPServer::...` only uses a string description, but the handler accepts a fixed set of values; update the schema definition to include an explicit enum for the supported algorithms (`pinocchio`, `unirig`, and `rignet`) so clients can discover and generate valid arguments from the schema.
🤖 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/PropertiesPanel.qml`:
- Around line 1565-1569: The RigSegments algorithm picker is mouse-only, so
keyboard users cannot change root.rigAlgoIndex. Update the RigSegments control
in PropertiesPanel.qml to be focusable from the keyboard by adding
activeFocusOnTab, handling key events to move/pick an item, and exposing
accessible metadata for the picker and its options, or replace it with a
built-in focusable control that already supports keyboard navigation.
In `@scripts/export-unirig-onnx.py`:
- Around line 286-400: The manual fallback in export_decoder_manual only exports
the per-step input_ids decoder and does not satisfy the runtime’s inputs_embeds
seed contract, so it should not be emitted as decoder.onnx. Update
export_decoder_manual so it either also exports a companion seed graph with
inputs_embeds support or writes the fallback to a separate artifact name, and
only publish decoder.onnx from the path that matches UniRigPredictor.cpp’s
expected contract. Use the StepDecoder export flow and the existing
optimum-vs-manual branching to keep the runtime-facing decoder artifact
consistent.
- Around line 115-126: The default UniRig export path still depends on
unresolved checkpoint-layout placeholders and a non-strict encoder load, so move
that wiring out of the normal path in export-unirig-onnx.py. Update the
DEFAULT_HF_REPO/DEFAULT_LM_SUBDIR/DEFAULT_ENC_WEIGHTS flow and the encoder/model
setup so the exporter only runs when real checkpoint locations and constructor
args are known, and avoid loading the encoder in `strict=False` mode. If the
layout is still unknown, fail fast or gate the export behind explicit
configuration rather than producing an ONNX model from placeholder weights.
- Around line 270-283: The decoder export path in the ONNX export helper is
incorrectly reporting success even when no recognizable decoder file is
produced. Update the logic in the decoder export routine that scans for
`model.onnx`/`decoder_model_merged.onnx`/`decoder_with_past_model.onnx`/`decoder_model.onnx`
so it returns failure when nothing is found, allowing `main()` to trigger the
manual fallback and avoid printing `ALL EXPORTS OK` without `decoder.onnx`.
- Around line 261-269: The export flow in main_export currently enables
trust_remote_code unconditionally, which makes user-supplied repositories
execute arbitrary code by default. Add a --trust-remote-code boolean option to
the script’s argument parsing and pass that flag through wherever main_export is
called, including the existing export path in the UniRig script. Gate the
trust_remote_code argument on the new flag so remote-code execution is only
enabled when the user explicitly opts in.
In `@src/AutoRig.cpp`:
- Around line 564-575: `appendIndices` is reading indices from the start of the
buffer instead of honoring `Ogre::IndexData::indexStart`. Update the index
loading logic in `appendIndices` so the read pointer or element access is offset
by `id->indexStart` before iterating `id->indexCount`, matching the handling
used elsewhere for `Ogre::IndexData`. Keep the `is32`/`i32`/`i16` branching, but
ensure each `push_back` reads from `indexStart + k` rather than `k` to avoid
corrupting the generated graph when the submesh starts mid-buffer.
In `@src/CLIPipeline.cpp`:
- Around line 8190-8198: The --algo parsing in CLIPipeline should reject cases
where the value is missing or another flag follows, instead of falling through
to the default algorithm. Update the argument-handling block for arg == "--algo"
so it validates argv[++i] is present and is not a flag-like token before
lowercasing and checking against pinocchio, unirig, and rignet, then emit an
error and return 2 when the value is absent.
In `@src/UniRigPredictor.cpp`:
- Around line 129-137: The token cleanup in UniRigPredictor::detokenize
currently removes a trailing EOS only if it is present, which lets
non-terminated streams like BOS plus tokens pass as valid. Update the EOS
handling after trimming BOS/PAD so the function rejects any stream that does not
end with kTokEos, returning failResult for missing-terminal-EOS cases to match
DetokenizeEmptyOrNoEosFails. Keep the fix localized to the existing detokenize
token-stripping logic and preserve the current empty-stream checks.
- Around line 273-282: The ensureModelBlocking() flow can still return cached
paths or trigger a download in builds where ONNX is disabled. Add an early guard
at the top of UniRigPredictor::ensureModelBlocking() that immediately returns
empty when ENABLE_ONNX is not enabled, before any QFileInfo::exists checks or
download-related logic, so disabled builds always honor the public contract.
- Around line 653-689: The decoder setup in UniRigPredictor::stepDecoder is
missing required ONNX inputs and the empty past-cache shapes are being
initialized incorrectly. Extend the input discovery loop to पहचान/record the
attention_mask and position_ids slots alongside inputs_embeds, input_ids, and
past, then pass them through the decoder invocation so it matches the export
contract. Also update the initial past-cache tensor shape construction so
dynamic batch dimensions default to 1 instead of 0, avoiding empty [0,...]
tensors when building the first cache state.
In `@THIRD_PARTY_AI_MODELS.md`:
- Around line 13-16: Update the Articulation-XL2.0 entry in
THIRD_PARTY_AI_MODELS.md so its license matches the upstream dataset license.
The current dataset attribution line for Seed3D/Articulation-XL2.0 should be
changed from CC-BY-4.0 to Apache-2.0, while leaving the UniRig weights license
unchanged. Use the existing “Training data”/“Attribution” text around the
Articulation-XL2.0 reference to locate and edit the correct entry.
---
Outside diff comments:
In `@src/AutoRigController.cpp`:
- Around line 210-225: The UniRig execution path in AutoRigController only
handles Ogre::Exception, so backend failures from model download or inference
can escape and leave m_busy stuck true. Update the try/catch around
AutoRigCommand and UndoManager::push in AutoRigController to also catch
std::exception (and optionally a final catch-all), and make sure the same
cleanup used in the Ogre path is performed before returning the failure result.
Use the existing identifiers m_busy, busyChanged(), emit error, and result to
keep the UI state and failure map consistent.
---
Nitpick comments:
In `@src/AutoRig.cpp`:
- Around line 549-554: The helper comment in the AutoRig submesh index append
logic still refers to RigNet, but the active backend is UniRig. Update the
wording around the append logic and the vertexBase/shared-vertex description to
say UniRig instead of RigNet, keeping the comment aligned with the current
backend terminology in AutoRig.
- Around line 552-553: The return value contract in the submesh emission path is
inconsistent with the documentation and current usage: the function that emits
the RigNet submesh always returns 0 even though the comment says it returns the
number of vertices contributed for advancing vertexBase. Update the relevant
emitter function in AutoRig.cpp to either return the actual contributed vertex
count and keep the contract aligned, or change the function signature to void
and remove the misleading return-based contract; also adjust the caller that
currently relies on ownOffset so the behavior stays explicit and consistent.
In `@src/AutoRig.h`:
- Line 219: The doc comment on algorithmToString is outdated and still lists the
deprecated string instead of the current return value. Update the comment
attached to algorithmToString so it matches the actual mapping used by AutoRig
and reflects "pinocchio" | "unirig"; keep "rignet" only associated with
algorithmFromString as a deprecated input alias.
- Around line 150-152: The comment on algorithmUsed still refers to the old
RigNet backend, so update the wording in AutoRig::algorithmUsed initialization
to say UniRig falls back to Pinocchio when its model or runtime is unavailable.
Keep the comment aligned with the current backend naming everywhere it describes
the source of the skeleton, and remove any stale RigNet reference to avoid
confusion.
In `@src/MCPServer.cpp`:
- Around line 6487-6492: The MCP schema for the `algo` property in
`MCPServer::...` only uses a string description, but the handler accepts a fixed
set of values; update the schema definition to include an explicit enum for the
supported algorithms (`pinocchio`, `unirig`, and `rignet`) so clients can
discover and generate valid arguments from the schema.
In `@src/UniRigPredictor.cpp`:
- Around line 111-113: Replace the platform-specific preprocessor guards in
UniRigPredictor with Qt’s cross-platform macros: change the `__APPLE__` include
guard near the top of the file and the `_WIN32` branch in the platform-specific
logic around `UniRigPredictor` to use `Q_OS_MACOS` and `Q_OS_WIN` instead. Keep
the existing behavior unchanged, but make sure all macOS/Windows-only code paths
in this file use the project’s Qt guard convention consistently.
- Around line 273-342: Add Sentry breadcrumbs for the significant model-download
and inference flows so user-facing operations are tracked. In
UniRigPredictor::ensureModelBlocking, emit SentryReporter::addBreadcrumb(...)
before starting resolution/download and around each download
attempt/failure/success path. Also add a breadcrumb at the start of the UniRig
prediction entry point (the UniRigPredictor method that begins inference) so the
end-to-end action is covered. Use the existing breadcrumb categories and keep
the messages descriptive and consistent with the established pattern.
🪄 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: 0b9e13f3-3671-42ff-8540-920f9a7792b8
📒 Files selected for processing (16)
CLAUDE.mdTHIRD_PARTY_AI_MODELS.mdqml/PropertiesPanel.qmlscripts/export-unirig-onnx.pysrc/AutoRig.cppsrc/AutoRig.hsrc/AutoRigController.cppsrc/AutoRigController.hsrc/AutoRig_test.cppsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/UniRigPredictor.cppsrc/UniRigPredictor.hsrc/UniRigPredictor_test.cpptests/CMakeLists.txt
Comment on lines
+13
to
+16
| Weights: https://huggingface.co/VAST-AI/UniRig — **MIT**. | ||
| - **Training data:** Articulation-XL2.0 | ||
| (https://huggingface.co/datasets/Seed3D/Articulation-XL2.0) — **CC-BY-4.0**. | ||
| Attribution: Seed3D / the Articulation-XL2.0 authors. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
VAST-AI UniRig Hugging Face model weights license
💡 Result:
The model weights hosted in the VAST-AI/UniRig repository on Hugging Face are licensed under the MIT License [1][2][3]. This is explicitly stated in the model card metadata (license: mit) [1][4][5] and aligns with the MIT License provided in the associated VAST-AI-Research/UniRig GitHub repository [6][7].
Citations:
- 1: https://huggingface.co/VAST-AI/UniRig/blob/main/README.md
- 2: https://huggingface.co/VAST-AI/UniRig
- 3: https://huggingface.co/VAST-AI/UniRig/tree/main
- 4: https://huggingface.co/VAST-AI/UniRig/blame/9efc1537465a93cee7d244bb078cab84bf762d7b/README.md
- 5: https://huggingface.co/VAST-AI/UniRig/blame/0a12cfb872b9080b1376b077591e6d66d4648352/README.md
- 6: https://github.com/VAST-AI-Research/UniRig/blob/main/LICENSE
- 7: https://github.com/VAST-AI-Research/UniRig
🌐 Web query:
Seed3D Articulation-XL2.0 Hugging Face dataset license
💡 Result:
The Seed3D/Articulation-XL2.0 dataset on Hugging Face is licensed under the Apache License 2.0 [1]. This is explicitly stated in the dataset repository's metadata [1]. The dataset is a large-scale collection containing over 48,000 3D models with articulation annotations, derived from Objaverse-XL [2][1].
Citations:
- 1: https://huggingface.co/datasets/Seed3D/Articulation-XL2.0/blob/refs%2Fpr%2F2/README.md
- 2: https://huggingface.co/datasets/Seed3D/Articulation-XL2.0
🏁 Script executed:
echo "Error: Invalid response format in previous turn. Correcting now."
</script>
<script>Repository: fernandotonon/QtMeshEditor
Length of output: 314
Correct the license for Articulation-XL2.0 to Apache-2.0
The file currently lists the Articulation-XL2.0 dataset as CC-BY-4.0, but the upstream Hugging Face repository (Seed3D/Articulation-XL2.0) is explicitly licensed under Apache License 2.0. Please update the attribution in this file to reflect the correct license. (Note: The UniRig weights license of MIT is correct).
🤖 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 `@THIRD_PARTY_AI_MODELS.md` around lines 13 - 16, Update the Articulation-XL2.0
entry in THIRD_PARTY_AI_MODELS.md so its license matches the upstream dataset
license. The current dataset attribution line for Seed3D/Articulation-XL2.0
should be changed from CC-BY-4.0 to Apache-2.0, while leaving the UniRig weights
license unchanged. Use the existing “Training data”/“Attribution” text around
the Articulation-XL2.0 reference to locate and edit the correct entry.
unit-tests-linux (ENABLE_ONNX) caught two UniRigPredictor detokenizer bugs: - A token stream not ending in EOS was silently accepted; now fails (matches the reference tokenizer's "last token is not eos"). - detokenize() topo-reordered joints, which renumbered them vs emission order and broke positional indexing (the branch-parent test). Emission order is ALREADY a valid parent-before-child order (a joint's parent — last_joint or a branch's earlier explicit parent — is always emitted before it), so the reorder was both redundant and wrong. Removed it; joints stay in emission order for stable indexing by callers + tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/UniRigPredictor.cpp`:
- Around line 212-217: The emission-order assumption in UniRigPredictor needs an
explicit validation step for branch-parent resolution. In the joint
reconstruction logic, after resolving a branch parent triple, verify that a
matching earlier joint was actually found before keeping the current joint as
valid; if the parent remains unresolved instead of being a prior emission, mark
the decode as invalid and return failure rather than leaving parent = -1 with
ok=true. Update the parent lookup/emission handling in UniRigPredictor so
malformed branch parents cannot be silently accepted as extra roots.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…o-end Exported the real UniRig skeleton model to ONNX (one-time, via the now-runnable scripts/export-unirig-onnx.py against the VAST-AI/UniRig 1.44GB Lightning ckpt) and reworked the C++ predictor to the VERIFIED 3-model contract: encoder.onnx vertices,normals -> latents[1,1024,1024] embed.onnx input_ids -> token_embeds[1,S,1024] decoder.onnx inputs_embeds,past.* -> logits[1,S,267],present.* (OPT-350m, L=24) The released checkpoint is monolithic Lightning + conditions the OPT decoder with the encoder latents as inputs_embeds (not token ids), so the decoder is a custom inputs_embeds/KV-cache step and a separate embed.onnx does the token lookup. UniRigPredictor now: encodes → seeds inputs_embeds with [latents ; embed([bos, cls=articulation-xl])] → constrained-greedy AR decode (KV-cache + tokenizer FSM mask, embedding each emitted token via embed.onnx) → detokenize. ensureModelBlocking fetches all THREE files. VALIDATED end-to-end: `qtmesh rig "Hip Hop Dancing.obj" --algo unirig` produced algorithm=unirig, 46 predicted bones (jointsRecentered=0 → NOT the template fallback), exported a rigged glb. Builds clean (QtMeshEditor + UnitTests, ENABLE_ONNX on). Still falls back to Pinocchio when the models are absent. Remaining: host the 3 .onnx (~1.44GB) on the HF models repo so first-run download works for everyone. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The 3 UniRig ONNX files total ~1.44 GB (decoder alone 1.2 GB); the 600s per-file cap was too tight on slow links, aborting an in-progress download and falling back to Pinocchio. Bump to 1800s — generous for slow connections while still bounding a truly dead one. (A future improvement: a stall-based timeout that resets on download progress.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the UniRig (ML) backend is selected, the skeleton-type picker, "Place markers…", and the Advanced/up-axis controls are hidden — UniRig predicts the whole skeleton from geometry, so those template-only inputs are irrelevant. A clear "✨ AI-powered" notice explains it's a local ML model (downloads ~1.4 GB once, then runs offline, falls back to the template, trained on Articulation-XL2.0 CC-BY-4.0), and the action button reads "Generate Rig (AI)" vs "Auto-Rig (template)". Pinocchio keeps the full template UI. isUnirig binds via the idle column's id (rigIdle) for robustness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/UniRigPredictor.cpp (1)
819-878: 🩺 Stability & Availability | 🟠 MajorAvoid retaining every KV-cache copy in
src/UniRigPredictor.cpp:819-878.kvStorePushFn()appends each new present cache into the sharedkvStorage/kvShapes, so every decode step keeps the previous full cache backing store alive even thoughkvCacheis swapped. That makes memory grow with generation length and can OOM on long runs; build the next cache in separate storage and drop/swap the old backing store afterdecoder.Run().🤖 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/UniRigPredictor.cpp` around lines 819 - 878, The KV-cache refresh logic in the decoder step is keeping every previous cache backing store alive because kvStorePushFn writes into shared kvStorage/kvShapes that never get cleared. Update the stepDecoder flow to build the next cache in fresh per-step storage, then swap it into kvCache after decoder.Run() and release the old backing store so memory does not grow with generation length. Use the existing symbols kvStorePushFn, kvStorage, kvShapes, kvCache, and stepDecoder to locate and refactor the cache ownership.
🧹 Nitpick comments (2)
src/UniRigPredictor.cpp (2)
532-546: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Qt platform guards for cross-platform code.
These platform-specific branches should use
Q_OS_MACOS/Q_OS_WINto match the repository portability rule.Proposed guard update
-#ifdef __APPLE__ +#ifdef Q_OS_MACOS try { std::unordered_map<std::string, std::string> coremlOpts; so.AppendExecutionProvider("CoreML", coremlOpts); } catch (const Ort::Exception&) {} `#endif` auto openSession = [&](const QString& path) -> Ort::Session { -#ifdef _WIN32 +#ifdef Q_OS_WIN std::wstring wpath = path.toStdWString(); return Ort::Session(env, wpath.c_str(), so); `#else`As per coding guidelines, “Make the codebase compile and run on Windows, Linux (Ubuntu), and macOS; guard platform-specific APIs with
#ifdef Q_OS_WIN,#ifdef Q_OS_MACOS, and#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/UniRigPredictor.cpp` around lines 532 - 546, The platform-specific branches in UniRigPredictor should use Qt’s portability macros instead of compiler macros. Update the Apple CoreML block and the Windows path handling in openSession to guard with Q_OS_MACOS and Q_OS_WIN so the code follows the repository’s cross-platform convention and remains portable across macOS and Windows.Source: Coding guidelines
266-344: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd breadcrumbs for UniRig download and inference operations.
Model downloads and ONNX inference are significant user-visible operations, especially with fallback behavior; add
SentryReporter::addBreadcrumb(...)around download start/failure/success and inference start/failure. As per coding guidelines, “AddSentryReporter::addBreadcrumb(category, message)for all user-facing actions and significant operations, using categories likeui.action,ai.tool_call,file.import, andfile.export.”Also applies to: 528-551, 900-951
🤖 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/UniRigPredictor.cpp` around lines 266 - 344, Add Sentry breadcrumbs in UniRigPredictor around the user-visible download flow in ensureModelBlocking and the ONNX inference path referenced by the review; emit SentryReporter::addBreadcrumb for download start, success, failure, and timeout/cancel states, and similarly for inference start and failure so fallback behavior is traceable. Use the existing UniRigPredictor helpers and unique symbols like ensureModelBlocking, ModelDownloader::startDownload, downloadCompleted, and downloadError to place the breadcrumbs near the relevant operations, following the established category/message pattern.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 `@scripts/export-unirig-onnx.py`:
- Around line 96-100: The export script still contains Ruff-blocking one-line
statements, so expand the compact class methods, assignments, loop bodies, and
conditional blocks in the ONNX export flow to standard բազմiline form. Update
the affected spots around Enc.__init__, Enc.forward, the variable setup near enc
= Enc(model).eval(), and the later loop/if sections so each statement and block
body is split onto separate lines and no E701/E702 patterns remain.
- Around line 87-88: The checkpoint loading in load_state_dict is too permissive
because strict=False still lets export continue with missing weights. Update the
checkpoint validation in scripts/export-unirig-onnx.py around the
model.load_state_dict(cleaned, strict=False) call so any non-empty missing list
raises a fatal error before export, and only permit unexpected keys after
explicitly filtering or whitelisting the known-safe ones. Keep the existing load
path and print summary, but gate the ONNX export on a fully acceptable state
dict in the export flow.
---
Outside diff comments:
In `@src/UniRigPredictor.cpp`:
- Around line 819-878: The KV-cache refresh logic in the decoder step is keeping
every previous cache backing store alive because kvStorePushFn writes into
shared kvStorage/kvShapes that never get cleared. Update the stepDecoder flow to
build the next cache in fresh per-step storage, then swap it into kvCache after
decoder.Run() and release the old backing store so memory does not grow with
generation length. Use the existing symbols kvStorePushFn, kvStorage, kvShapes,
kvCache, and stepDecoder to locate and refactor the cache ownership.
---
Nitpick comments:
In `@src/UniRigPredictor.cpp`:
- Around line 532-546: The platform-specific branches in UniRigPredictor should
use Qt’s portability macros instead of compiler macros. Update the Apple CoreML
block and the Windows path handling in openSession to guard with Q_OS_MACOS and
Q_OS_WIN so the code follows the repository’s cross-platform convention and
remains portable across macOS and Windows.
- Around line 266-344: Add Sentry breadcrumbs in UniRigPredictor around the
user-visible download flow in ensureModelBlocking and the ONNX inference path
referenced by the review; emit SentryReporter::addBreadcrumb for download start,
success, failure, and timeout/cancel states, and similarly for inference start
and failure so fallback behavior is traceable. Use the existing UniRigPredictor
helpers and unique symbols like ensureModelBlocking,
ModelDownloader::startDownload, downloadCompleted, and downloadError to place
the breadcrumbs near the relevant operations, following the established
category/message pattern.
🪄 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: 8bde27af-14bb-467d-982b-2a813f8a1a04
📒 Files selected for processing (6)
qml/PropertiesPanel.qmlscripts/export-unirig-onnx.pysrc/AutoRig.cppsrc/UniRigPredictor.cppsrc/UniRigPredictor.hsrc/UniRigPredictor_test.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- qml/PropertiesPanel.qml
- src/AutoRig.cpp
- src/UniRigPredictor_test.cpp
Minor bump for the #408 UniRig ML skeleton-prediction backend (ONNX, MIT model, first-run download with Pinocchio fallback). Doc pins synced via scripts/sync-doc-versions-from-cmake.sh. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeRabbit review fixes (current code; stale comments on the deleted RigNetPredictor and the old export scaffold skipped, with rationale replied): - CLI: `rig --algo` with a missing/flag value (e.g. `--algo -o out`) now errors instead of silently swallowing the next flag and running Pinocchio. - UniRigPredictor::ensureModelBlocking returns empty immediately in non-ENABLE_ONNX builds — never touches disk or starts a ~1.4 GB first-use download the build can't use (matches the documented contract). - detokenize: a non-root joint whose explicit branch parent triple matches no earlier joint is now rejected (malformed decode → caller falls back) instead of silently becoming a spurious extra root. - RigSegments picker (Algorithm / skeleton-type / up-axis) is keyboard-accessible: tab focus + focus ring + Space/Enter to select + RadioButton Accessible role. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
UniRig inference (encode + ~hundreds of autoregressive decode steps) took
seconds and froze the UI. Move it off the main thread:
- UniRigPredictor::predict() takes a ProgressFn called once per decode step
(stepsDone/maxSteps); returning false cancels (predict returns
ok=false, error="cancelled"). modelsPresent() distinguishes the
first-run download phase.
- AutoRig gains gatherGeometry() (main thread: read vertex/index buffers)
and predictUniRig() (worker thread: pure ONNX, no Ogre), plus
Options::prePredictedJoints so rigEntity skips the slow path and just
builds + binds the skeleton from worker-predicted joints.
- AutoRigController::autoRigSelected spawns a std::thread for the UniRig
path, marshals progress/completion back via QMetaObject::invokeMethod
(QPointer-guarded), and returns {pending:true} immediately. New
rigDownloading/rigProgress/rigTotal properties + cancelRig(). Falls
back to the template rig (finishUniRigFallback) when prediction is
unusable. Ogre skeleton build stays on the main thread.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- PropertiesPanelController::hasSkeletonSelection was a Q_INVOKABLE, so the
Skeleton section's QML binding read it once and went stale — an empty
Skeleton group lingered on a not-yet-rigged mesh. Promote it to a
Q_PROPERTY(NOTIFY selectionChanged) and add notifySelectionMetadataChanged()
(called from AutoRigController::notifyRiggingChanged) so the section
appears/disappears the moment a rig/unrig flips the skeleton state,
without needing to reselect.
- PropertiesPanel.qml: add the UniRig worker progress block (download phase
text, determinate decode bar, Cancel button) shown while busy; hide the
idle rig controls while busy so the run can't be re-triggered. runAutoRig
short-circuits on {pending:true}; onRigged fills the status line.
- Move the "use markers for a better fit" hint under the Pinocchio selection
(mirroring the AI-powered notice under UniRig) and drop the marker mention
from the generic intro text.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
scripts/export-unirig-onnx.py (dev-only, not shipped): - Fail loudly on MISSING checkpoint weights. strict=False stays (the monolithic UniRig checkpoint carries PTv3/skin modules we don't export for the skeleton stage → legitimately `unexpected`), but a `missing` key means a tensor the exported graphs DO need wasn't loaded, which would silently emit ONNX with uninitialised weights — now raises. - Split the Ruff-blocking one-line `;`-joined statements and one-line defs (E702/E704) across export_encoder/decoder/embed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/AutoRig.cpp (1)
564-575: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
appendIndicesstill ignoresOgre::IndexData::indexStart.The loop reads
i32[k]/i16[k]from the buffer start, but a submesh may begin mid-buffer (indexStart > 0), as handled inMeshValidator.cpp/VertexCacheOptimizer.cpp. WhenindexStart > 0this reads stale indices and corrupts the geometry fed to UniRig.Offset the element access by
id->indexStart:🐛 Proposed fix
const size_t n = id->indexCount; out.reserve(out.size() + n); + const size_t start = static_cast<size_t>(id->indexStart); for (size_t k = 0; k + 2 < n; k += 3) { - out.push_back(base + (is32 ? i32[k] : i16[k])); - out.push_back(base + (is32 ? i32[k+1] : i16[k+1])); - out.push_back(base + (is32 ? i32[k+2] : i16[k+2])); + out.push_back(base + (is32 ? i32[start + k] : i16[start + k])); + out.push_back(base + (is32 ? i32[start + k + 1] : i16[start + k + 1])); + out.push_back(base + (is32 ? i32[start + k + 2] : i16[start + k + 2])); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AutoRig.cpp` around lines 564 - 575, appendIndices is reading indices from the start of the hardware buffer and ignoring Ogre::IndexData::indexStart, which can corrupt submesh geometry when the index range begins mid-buffer. Update the element access in appendIndices to offset by id->indexStart for both the 16-bit and 32-bit index paths, and keep the loop bounds based on indexCount so it reads the correct triangle range from the buffer.
🧹 Nitpick comments (1)
src/AutoRigController.cpp (1)
233-267: 🩺 Stability & Availability | 🔵 TrivialCall
cancelRig()before destroyingAutoRigController.kill()deletes the singleton without signalling the worker, so the detached UniRig task can outlive controller teardown. The ONNXEnv/sessions are local toUniRigPredictor::predict(), so the shared-runtime crash concern doesn’t apply 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 `@src/AutoRigController.cpp` around lines 233 - 267, The detached UniRig worker in AutoRigController can outlive the controller because kill() deletes the singleton without first stopping the background task. Update the teardown path to call cancelRig() before destruction so the std::thread launched in the rig prediction flow sees the cancel flag and exits cleanly, and make sure the callback path guarded by progress, finishUniRigFallback, and finishUniRigOnMain remains safe if cancellation happens during shutdown.
🤖 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/AutoRig.cpp`:
- Around line 591-602: The vertex offset tracking in AutoRig::rigEntity can get
out of sync when appendPositions fails because ownBase and ownOffset are
advanced even if no vertices were actually appended. Update the sharedVertexData
and submesh handling to check appendPositions’ return value before incrementing
ownBase or assigning ownOffset[si], so indices always match outVerts; apply the
same guard in rigEntityWithMarkers as well to keep both paths consistent.
---
Duplicate comments:
In `@src/AutoRig.cpp`:
- Around line 564-575: appendIndices is reading indices from the start of the
hardware buffer and ignoring Ogre::IndexData::indexStart, which can corrupt
submesh geometry when the index range begins mid-buffer. Update the element
access in appendIndices to offset by id->indexStart for both the 16-bit and
32-bit index paths, and keep the loop bounds based on indexCount so it reads the
correct triangle range from the buffer.
---
Nitpick comments:
In `@src/AutoRigController.cpp`:
- Around line 233-267: The detached UniRig worker in AutoRigController can
outlive the controller because kill() deletes the singleton without first
stopping the background task. Update the teardown path to call cancelRig()
before destruction so the std::thread launched in the rig prediction flow sees
the cancel flag and exits cleanly, and make sure the callback path guarded by
progress, finishUniRigFallback, and finishUniRigOnMain remains safe if
cancellation happens during shutdown.
🪄 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: c66294de-bb43-48b1-bc37-ac14e36b429b
📒 Files selected for processing (13)
CMakeLists.txtREADME.mdqml/PropertiesPanel.qmlscripts/export-unirig-onnx.pysrc/AutoRig.cppsrc/AutoRig.hsrc/AutoRigController.cppsrc/AutoRigController.hsrc/CLIPipeline.cppsrc/PropertiesPanelController.hsrc/UniRigPredictor.cppsrc/UniRigPredictor.hwebsite/src/hooks/useQtmeshActionRef.js
✅ Files skipped from review due to trivial changes (3)
- website/src/hooks/useQtmeshActionRef.js
- CMakeLists.txt
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/export-unirig-onnx.py
- src/UniRigPredictor.h
- src/CLIPipeline.cpp
- src/UniRigPredictor.cpp
Comment on lines
+591
to
+602
| if (mesh->sharedVertexData) { | ||
| appendPositions(mesh->sharedVertexData, outVerts); | ||
| ownBase = static_cast<uint32_t>(mesh->sharedVertexData->vertexCount); | ||
| } | ||
| std::vector<uint32_t> ownOffset(mesh->getNumSubMeshes(), 0); | ||
| for (unsigned short si = 0; si < mesh->getNumSubMeshes(); ++si) { | ||
| Ogre::SubMesh* sub = mesh->getSubMesh(si); | ||
| if (sub && !sub->useSharedVertices && sub->vertexData) { | ||
| ownOffset[si] = ownBase; | ||
| appendPositions(sub->vertexData, outVerts); | ||
| ownBase += static_cast<uint32_t>(sub->vertexData->vertexCount); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
ownBase / ownOffset can desync from the actual vertex buffer when appendPositions fails.
appendPositions can return false (e.g. a write-only buffer with no shadow copy — handled explicitly at lines 532-536), but its return value is ignored here and ownBase is advanced unconditionally. If the shared block fails to lock, no shared vertices are appended yet ownBase is still set to sharedVertexData->vertexCount; subsequent submesh ownOffset[si] values (and the indices built from them) then reference vertex slots that don't exist in outVerts, producing out-of-range indices that UniRigPredictor::predict will dereference. Gate the offset advance on the append succeeding.
🛡️ Proposed fix
if (mesh->sharedVertexData) {
- appendPositions(mesh->sharedVertexData, outVerts);
- ownBase = static_cast<uint32_t>(mesh->sharedVertexData->vertexCount);
+ if (appendPositions(mesh->sharedVertexData, outVerts))
+ ownBase = static_cast<uint32_t>(mesh->sharedVertexData->vertexCount);
}
std::vector<uint32_t> ownOffset(mesh->getNumSubMeshes(), 0);
for (unsigned short si = 0; si < mesh->getNumSubMeshes(); ++si) {
Ogre::SubMesh* sub = mesh->getSubMesh(si);
if (sub && !sub->useSharedVertices && sub->vertexData) {
ownOffset[si] = ownBase;
- appendPositions(sub->vertexData, outVerts);
- ownBase += static_cast<uint32_t>(sub->vertexData->vertexCount);
+ if (appendPositions(sub->vertexData, outVerts))
+ ownBase += static_cast<uint32_t>(sub->vertexData->vertexCount);
}
}The same pattern exists in rigEntityWithMarkers (lines 680-693); consider applying the guard there too.
📝 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
| if (mesh->sharedVertexData) { | |
| appendPositions(mesh->sharedVertexData, outVerts); | |
| ownBase = static_cast<uint32_t>(mesh->sharedVertexData->vertexCount); | |
| } | |
| std::vector<uint32_t> ownOffset(mesh->getNumSubMeshes(), 0); | |
| for (unsigned short si = 0; si < mesh->getNumSubMeshes(); ++si) { | |
| Ogre::SubMesh* sub = mesh->getSubMesh(si); | |
| if (sub && !sub->useSharedVertices && sub->vertexData) { | |
| ownOffset[si] = ownBase; | |
| appendPositions(sub->vertexData, outVerts); | |
| ownBase += static_cast<uint32_t>(sub->vertexData->vertexCount); | |
| } | |
| if (mesh->sharedVertexData) { | |
| if (appendPositions(mesh->sharedVertexData, outVerts)) | |
| ownBase = static_cast<uint32_t>(mesh->sharedVertexData->vertexCount); | |
| } | |
| std::vector<uint32_t> ownOffset(mesh->getNumSubMeshes(), 0); | |
| for (unsigned short si = 0; si < mesh->getNumSubMeshes(); ++si) { | |
| Ogre::SubMesh* sub = mesh->getSubMesh(si); | |
| if (sub && !sub->useSharedVertices && sub->vertexData) { | |
| ownOffset[si] = ownBase; | |
| if (appendPositions(sub->vertexData, outVerts)) | |
| ownBase += static_cast<uint32_t>(sub->vertexData->vertexCount); | |
| } |
🤖 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/AutoRig.cpp` around lines 591 - 602, The vertex offset tracking in
AutoRig::rigEntity can get out of sync when appendPositions fails because
ownBase and ownOffset are advanced even if no vertices were actually appended.
Update the sharedVertexData and submesh handling to check appendPositions’
return value before incrementing ownBase or assigning ownOffset[si], so indices
always match outVerts; apply the same guard in rigEntityWithMarkers as well to
keep both paths consistent.
Owner
Author
CI note:
|
MainWindowTest.ConstructorAppliesCustomPaletteFromSettings consistently wedged the unit-tests-linux CI job: 76/78 MainWindowTest cases passed, then this test (the only one that reconstructs MainWindow with palette="custom" persisted) went silent for 40+ s until the runner agent died — surfacing as "##[error]The operation was canceled", not a test failure. It also fails on master (pre-existing flake). Root cause: the custom-palette branch in the constructor called custom_Palette_Color_Selected() → QApplication::setPalette() SYNCHRONOUSLY mid-construction. setPalette dispatches ApplicationPaletteChange events into the still-half-built widget tree (the two QColorDialogs are already alive from the init list); under headless Xvfb that re-entrancy could wedge the event loop. Fix: keep the actionCustom checked-state synchronous, but defer the actual QApplication::setPalette() to a queued QTimer::singleShot(0) — it applies once the window is fully built and the loop is spinning, exactly the path a runtime colour-pick already takes. Behaviour is unchanged (the custom palette still applies one event-loop tick later, imperceptibly). Strengthen the test to pump the loop and assert the deferred palette actually landed (QApplication::palette() == QPalette(seed)). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mainwindow.cpp`:
- Around line 303-309: The deferred custom palette apply in mainwindow’s startup
flow can still run with an invalid saved QColor from settings. Add the same
isValid() guard used by ThemeManager before scheduling
custom_Palette_Color_Selected, and if the saved color is invalid, follow the
same fallback path you already use for bad custom palettes instead of applying
it. Locate this in the startup settings restore block that reads "customPalette"
and queues the QTimer::singleShot lambda.
🪄 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: 8059155b-045b-4680-b58b-d302043682c3
📒 Files selected for processing (2)
src/mainwindow.cppsrc/mainwindow_test.cpp
Comment on lines
+303
to
+309
| const QColor customColor = settings.value("customPalette").value<QColor>(); | ||
| ui->actionCustom->blockSignals(true); | ||
| ui->actionCustom->setChecked(true); | ||
| ui->actionCustom->blockSignals(false); | ||
| QTimer::singleShot(0, this, [this, customColor]() { | ||
| custom_Palette_Color_Selected(customColor); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard the deferred apply against invalid saved colors.
Line 303 reads "customPalette" straight from settings, and Lines 307-309 always queue custom_Palette_Color_Selected(customColor). That handler unconditionally calls QApplication::setPalette(color) and rewrites the setting, while ThemeManager only applies saved custom colors when isValid() is true. Add the same validity check here and pick the same fallback path you use for a bad saved palette.
🤖 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/mainwindow.cpp` around lines 303 - 309, The deferred custom palette apply
in mainwindow’s startup flow can still run with an invalid saved QColor from
settings. Add the same isValid() guard used by ThemeManager before scheduling
custom_Palette_Color_Selected, and if the saved color is invalid, follow the
same fallback path you already use for bad custom palettes instead of applying
it. Locate this in the startup settings restore block that reads "customPalette"
and queues the QTimer::singleShot lambda.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // Deterministic RNG so the same mesh always samples the same cloud (matches | ||
| // the greedy-decode determinism contract). | ||
| std::mt19937 rng(0x5eed5eedu); |
Two failures surfaced once the MainWindow palette hang (prev commit) stopped masking the rest of the unit-tests-linux run: 1. UniRigPredictor.DetokenizeBranchTokenYieldsExplicitParent — the test encoded the branch parent triple as discretize(undiscretize(discretize(root))) (a double round-trip). The +0.5 bin-centre offset makes that land ONE bin higher than discretize(root), so the parent position no longer matched joint 0's stored bins and the (correct) reject-unmatched-branch-parent guard fired. A real UniRig stream re-states an earlier joint's exact bins, so the test now emits discretize(root) directly for the parent triple. Impl unchanged — the strict reject is right. 2. MainWindowTest hit the 600s per-suite wall-clock cap (SIGKILL) — not a hang (the palette fix cleared that), just genuinely slow: 78 cases each rebuild the full MainWindow (~7–8s on the software-GL runner ≈ 10 min). Bump PER_SUITE_TIMEOUT 600→1200s (job timeout is 90 min). Flagged the per-case full-window rebuild as a redundancy-audit hot spot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Acting on the static redundancy audit (coverage-driven, whole-suite). Removes 267 verified-redundant tests across 58 files — each a strict subset/duplicate of a retained test, a dead no-op, or a *_coverage_test.cpp pad re-asserting a branch the primary suite already covers. Every removal was checked to have "none" coverage risk against a named surviving test; "keep" flags from the audit were honored. Highlights: - MCPServer_test (31) — *WithSkeletonEntity subsets of AnimSuccPath_*, dup GetSceneInfo / unknown-tool / empty-name clusters. - MCPServerMaterialBranches_coverage (19) — whole file dup of MCPServer_test modify/create/set-texture/get/list branches. - ScanEngine_test (16) — ScanConfig tests duplicating ScanConfig_test. - SpaceCamera_test (15) — dead WASD/QE key tests (keys unmapped) + mouse dups. - LLMManager (12), and ~50 smaller files (2–8 each). Also removes MainWindowTest.ConstructorAppliesCustomPaletteFromSettings: it reconstructed a full MainWindow with palette="custom", triggering a global QApplication::setPalette() repaint of the fresh widget tree (incl. QML QQuickWidgets) that consistently wedged the headless xcb/Xvfb CI runner (killed ~60s in, before the per-suite timeout). custom_Palette_Color_Selected is // LCOV_EXCL (not a coverage target) and the light/dark menu-action paths are covered on the shared fixture, so this reconstruction test was a pure CI liability. Reverts the earlier ineffective setPalette-deferral in mainwindow.cpp. 4614 → 4342 tests. UnitTests builds clean. Parameterization pass (TEST_P restructuring, ~454 tests, coverage-neutral) follows separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- AutoRig::algorithmFromString — drop the redundant `if (l=="pinocchio"…)` branch whose body was identical to the default `return Pinocchio` (Sonar S3923 "all branches identical"). Behaviour unchanged: anything that isn't unirig/rignet maps to the native backend. - UniRigPredictor surface-sampling RNG — annotate the fixed-seed std::mt19937 as NOSONAR with rationale: it's geometry sampling (not a security context) and the constant seed is REQUIRED for the documented reproducibility contract, so a CSPRNG would be wrong here (Sonar S2245). Co-Authored-By: Claude Opus 4.8 (1M context) <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
2 participants
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 #408.
Summary
Higher-quality auto-rigging via RigNet (Xu et al., SIGGRAPH 2020) as a second skeleton-prediction backend alongside the #407 native Pinocchio-style template embedding. Reuses the #404 ONNX Runtime infrastructure (second ONNX consumer after PbrMapSynth). RigNet predicts joints + bone connectivity from the mesh graph, handling arbitrary topology / non-humanoid shapes / unusual proportions better than a fixed template.
Default stays Pinocchio for offline reliability; RigNet is opt-in and falls back to Pinocchio when the model or ONNX runtime is unavailable.
What's new
AutoRig::Algorithm {Pinocchio, RigNet}— threaded throughOptions+Report(algorithmUsed,fallbackReason), withalgorithmToString/algorithmFromString.RigNetPredictor(src/RigNetPredictor.h/.cpp) — Ogre-free, unit-tested core:[1,N,3]verts, optional int64edges[2,E]; outputs a float joint tensor whose last dim is 3 + optional int parent tensor) — defensive on any mismatch.ENABLE_ONNX-guarded; without itisAvailable()is false andpredict()fails with a "rebuild with -DENABLE_ONNX" message.RigNetPredictor::ensureModelBlocking) —AppData/ai_models/rignet/rignet.onnx(~50 MB) fetched on first use viaModelDownloader(event-loop driven, 180s timeout). Base-URL overrideQTMESH_RIGNET_MODEL_BASE_URL/QSettings ai/rignetModelBaseUrl; offline guardQTMESH_RIGNET_NO_DOWNLOAD. MirrorsAIAssistManager::ensureModelBlocking.AutoRig::rigEntityruns RigNet for the plain (marker-less) rig; on ONNX-disabled / missing / offline / not-yet-hosted model / unusable prediction it logsreport.fallbackReasonand uses the template fit. So a--algo rignetrequest always produces a rig.qtmesh rig <file> --algo pinocchio|rignet(CLIPipeline::cmdRig).auto_riggains analgoparam (MCPServer::toolAutoRig) + schema entry.ai.assist.auto_rigbreadcrumb recordsalgo(CLI / MCP / GUI).Design note (RigNet ONNX export)
The published RigNet checkpoint is PyTorch-Geometric and does not export to a single clean ONNX graph (PyG graph ops).
scripts/export-rignet-onnx.py(one-time, offline, not shipped) is the intended export that bakes the graph construction + staged network into the I/ORigNetPredictortargets. Until that export is hosted, the model download 404s and the Pinocchio fallback runs — the whole feature is wired and reliable today; swapping in the hosted model lights up the ML path with no further code changes. (Same "ship the plumbing + a working default, document the upstream gap" approach used for #401/#402/#407.)Acceptance criteria
algoselection.ai.assist.auto_rigwithalgotag.App + UnitTests build clean on macOS arm64 (ENABLE_ONNX off → fallback path). The GL-dependent tests run on CI Linux/Xvfb; the new RigNetPredictor / algorithm tests are pure-data.
🤖 Generated with Claude Code
Update — retargeted from RigNet to UniRig (MIT)
License due-diligence found RigNet is unhostable (GPL/Commercial code + unlicensed weights + non-public ModelsResource dataset — fails the project's permissive-redistribution bar, like DeepBump for #404). Retargeted to UniRig (SIGGRAPH 2025): MIT code + MIT weights, trained on Articulation-XL2.0 (CC-BY-4.0) — a fully clean chain (see
THIRD_PARTY_AI_MODELS.md).UniRig is an autoregressive transformer (GPT-like skeleton-tree tokenization + a Michelangelo perceiver encoder), so this is a genuine C++/ONNX port, not a model swap:
UniRigPredictor(replacesRigNetPredictor): surface-sample points+normals → Michelangelo encoder ONNX (latent prefix) → greedy/constrained autoregressive decode over the ~350M causal-LM decoder ONNX (manual KV-cache + the tokenizer's next-possible-token validity mask) → the exact UniRig detokenizer FSM (256 coord bins,continuous_range [-1,1],undiscretize, branch/parent rules, vocab 267) → joints + parents, parent-before-child ordered for Ogre.detokenize()/undiscretize()are public statics, unit-tested without ONNX. Greedy decode is a documented simplification of UniRig's beam+sampling (deterministic + exportable; still a valid tree).encoder.onnx+decoder.onnx) underAppData/ai_models/unirig/, downloaded on first use;QTMESH_UNIRIG_*env overrides + offline guard.RigNet→UniRigacross CLI (--algo pinocchio|unirig,rignetdeprecated alias), MCPauto_rigalgo, Inspector "Algorithm" picker, Sentryalgotag.report.fallbackReason) when ONNX off / models missing/offline / prediction unusable — reliable offline.Hosting status:
scripts/export-unirig-onnx.py(one-time, offline, not shipped) exports the encoder + KV-cache decoder to the I/O contractUniRigPredictortargets (viaoptimum, with a hand-rolled fallback). Until the.onnxfiles are hosted on the HF models repo, the download 404s and the Pinocchio fallback runs — the plumbing + runtime are complete and ship today; hosting the export lights up the ML path with no code change. Running the export needs a torch+transformers+UniRig-checkout env (can't be done headlessly here).Builds clean (QtMeshEditor + UnitTests,
ENABLE_ONNXon) on macOS arm64.Summary by CodeRabbit
Update — models hosted + UniRig-aware UI (now functionally complete)
scripts/export-unirig-onnx.pyagainst the VAST-AI/UniRig checkpoint and uploaded the 3 ONNX files tofernandotonon/QtMeshEditor-models/unirig/(encoder 217 MB, decoder 1.2 GB, embed 1 MB; ~1.44 GB total). They resolve HTTP 200 at the URLs the C++ uses, so first-run download works for everyone.qtmesh rig "Hip Hop Dancing.obj" --algo unirigproducesalgorithm=unirig, a real 46-bone predicted skeleton (jointsRecentered=0→ not the template fallback), exported to a rigged glb.inputs_embeds— so the C++ runs encoder → seed[latents ; embed([bos, cls])]→ constrained-greedy KV-cached AR decode (embedding each token viaembed.onnx) → the exact tokenizer FSM → skeleton. Download timeout raised to 30 min for the large models.THIRD_PARTY_AI_MODELS.mdrecords UniRig (MIT) + Articulation-XL2.0 (CC-BY-4.0) attribution.All acceptance criteria are now met, including the previously-pending end-to-end ML quality (the hosted model lights up the
--algo unirigpath with no further code change).Update — responsive rigging UX (worker thread + progress bar + cancel)
UniRig inference (encode + hundreds of autoregressive decode steps) ran on the UI thread and froze the app for seconds — and there was no way to tell it was working, nor to stop it / prevent deleting the model mid-run. Fixed:
AutoRigController::autoRigSelectednow gathers the mesh geometry on the main thread, runs UniRig inference on astd::thread, and returns{pending:true}immediately — the UI stays responsive. Progress + completion marshal back viaQMetaObject::invokeMethod(QPointer-guarded); the Ogre skeleton build stays on the main thread (Options::prePredictedJointsletsrigEntityskip the slow path and just build + bind).UniRigPredictor::predicttakes aProgressFninvoked once per decode step (stepsDone/maxSteps); the Inspector shows a "Downloading model…" phase (first use), then a live "step N / max" bar, plus a Cancel button (flips a shared atomic the decode loop checks). The idle controls hide while busy, so the rig can't be re-triggered.PropertiesPanelController::hasSkeletonSelectionwas aQ_INVOKABLE— the QML binding read it once and went stale, leaving an empty Skeleton group on a not-yet-rigged mesh. Promoted to aQ_PROPERTY(NOTIFY selectionChanged);notifyRiggingChangedpokes it so the section appears/disappears the instant a rig/unrig flips the skeleton state.Update — CI green + test-suite cleanup
The
unit-tests-linuxjob had been red across several runs. Root-caused and fixed, then did a suite-wide redundancy pass:MainWindowTest.ConstructorAppliesCustomPaletteFromSettings(added in [codex] Expand coverage for mainwindow, CLI scan, and exporter paths #307,LCOV_EXCL-marked code) reconstructed a full MainWindow withpalette="custom"persisted, triggering a globalQApplication::setPalette()repaint of the fresh widget tree (incl. QMLQQuickWidgets) that consistently wedged the headless xcb/Xvfb runner (killed ~60s in, before any timeout). The light/dark menu-action paths are covered elsewhere on the shared fixture, so the test was a pure CI liability — removed. This was a pre-existingmasterflake, not introduced here.*_coverage_test.cpppad re-asserting a branch the primary suite already covers. Every removal was checked to have zero coverage risk against a named surviving test. 4,614 → 4,342 tests. (A coverage-neutralTEST_Pparameterization pass was identified but deferred — readability-only, no count/coverage benefit.)ifinAutoRig::algorithmFromString(S3923) and annotated the deterministic fixed-seed surface-sampling RNG inUniRigPredictoras NOSONAR (S2245 — non-crypto, reproducibility-required).All deploy-workflow checks are green.