feat(skinning): Skinning v2 — geodesic voxel binding default + weight post-passes (#819 Slices A+B) - #829
Merged
Merged
Conversation
…819 Slices A+B) Skinning v2: GeodesicVoxel (Dionne & de Lasa, SCA 2013 — Maya's production bind) becomes the default skin-weights algorithm on every surface. Distances travel through the mesh's interior voxels, so cross-limb bleed is impossible by construction; voxel-scale hole closing makes non-watertight/self-intersecting/multi-component meshes work. - GeodesicVoxelBind (Ogre-free): Akenine-Möller tri/box voxelization, exterior flood-fill interior classification, 3D-DDA bone seeding with world-distance-checked snap-to-solid, one multi-source Dijkstra keeping best K=8 (bone,distance) pairs per voxel. - SkinWeightsPost (Ogre-free): Laplacian weight relaxation over the vertex adjacency (merge-mode manual weights = Dirichlet constraints), prune + renormalize, geodesic bleed metric. - Whole-mesh single-pass compute in computeAndApply: all vertex-data owners voxelized together so accessories bind through the body and bones seed one shared field (per-submesh grids stranded bones). - Algorithm enum {InverseDistance, GeodesicVoxel, UniRigML}; UniRigML falls back to GeodesicVoxel until the Slice-C ONNX export is hosted; degenerate (volume-less) input falls back to InverseDistance automatically. - Surfaces: CLI `qtmesh skin --algo/--voxel-res/--smooth-iterations`, MCP compute_skin_weights algo/voxel_resolution/smooth_iterations, GUI dialog algorithm dropdown + knobs. Report gains algorithmUsed/ fallbackReason/bleedFraction/bonesWithoutSeeds. Sentry ai.assist.skin.<algo>. - Tests: parallel-limb zero-crossover (with inverse-distance contrast assert), bend-not-chord U-fixture, cracked non-watertight box ≈ closed, plane fallback, far-bone no-seed, post-pass invariants. Verified: bandit.fbx (90k verts, 119 bones, 39 submeshes) skins via geodesic-voxel in 0.64s at --voxel-res 128, bleed 0.005, valid glb round-trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThe change introduces geodesic voxel skinning as the default, with inverse-distance fallback and UniRig compatibility. It adds voxel binding, smoothing and pruning, algorithm-aware diagnostics, CLI/MCP/QML configuration, undo propagation, updated documentation, and comprehensive tests. ChangesSkinning v2
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SkinWeightsDialog
participant SkinWeightsController
participant ComputeSkinWeightsCommand
participant SkinWeights
participant GeodesicVoxelBind
User->>SkinWeightsDialog: Select algorithm and tuning values
SkinWeightsDialog->>SkinWeightsController: computeWeightsForSelected(...)
SkinWeightsController->>ComputeSkinWeightsCommand: Create command with algorithm
ComputeSkinWeightsCommand->>SkinWeights: computeAndApply(entity, options, algorithm)
SkinWeights->>GeodesicVoxelBind: Compute geodesic weights
GeodesicVoxelBind-->>SkinWeights: Return weights and diagnostics
SkinWeights-->>ComputeSkinWeightsCommand: Return report
ComputeSkinWeightsCommand-->>SkinWeightsDialog: Show algorithm and fallback status
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Fable 5 <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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SkinWeightsController.cpp (1)
111-129: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClamp
smoothIterationshere and reject unsupported algorithms.voxelResolutionis already clamped downstream, butsmoothIterationsstill reaches the Laplacian post-pass unchecked and can make this call run far longer than intended.algorithmFromString()also silently falls back togeodesic-voxelon typos, so a bad caller can get the wrong result instead of an error.🤖 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/SkinWeightsController.cpp` around lines 111 - 129, Clamp smoothIterations to the supported range before assigning it to SkinWeightsOptions, ensuring the value passed through the Laplacian post-pass cannot exceed intended limits. Validate algorithm before calling algorithmFromString in the controller method: reject unknown or unsupported strings with an error and return, rather than allowing the fallback to geodesic-voxel; only add the breadcrumb and continue once validation succeeds.
🧹 Nitpick comments (3)
qml/SkinWeightsDialog.qml (1)
284-302: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSmoothing-iterations UI cap (20) is narrower than the backend-supported range (50).
MCP validates
smooth_iterationsup to 50, but this field caps at 20, so GUI users can't reach the full range CLI/MCP callers can.♻️ Align GUI cap with backend range
InspectorNumberField { Layout.preferredWidth: 80 value: dialog.smoothIterations minValue: 0 - maxValue: 20 + maxValue: 50 isInt: true onNewValue: dialog.smoothIterations = Math.round(v) }🤖 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/SkinWeightsDialog.qml` around lines 284 - 302, Align the smoothing-iterations UI limit with the backend-supported range by changing the maxValue in the InspectorNumberField for dialog.smoothIterations from 20 to 50. Keep the existing integer handling and default behavior unchanged.qml/PropertiesPanel.qml (1)
2255-2264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale tooltip nearby still describes the old default algorithm.
The description text here was updated to describe Geodesic Voxel as the default, but the button's
ToolTip.textjust a few lines below (Line 2307) still says "via inverse-distance to bone segments," contradicting the text just updated here.📝 Suggested tooltip update
ToolTip.text: SkinWeightsController.hasSkinnedSelection - ? "Compute per-vertex bone weights via inverse-distance to bone segments. Mesh must have a skeleton." + ? "Compute per-vertex bone weights via geodesic voxel bind (volume-aware). Mesh must have a skeleton." : "Select a skinned mesh (with a skeleton) first."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qml/PropertiesPanel.qml` around lines 2255 - 2264, Update the button’s ToolTip.text near the auto-generate weights control to remove the outdated “via inverse-distance to bone segments” description and accurately describe Geodesic Voxel binding as volume-aware with no cross-limb bleed, matching the Text content in the surrounding PropertiesPanel UI.src/MCPServer.cpp (1)
7968-7973: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
enumto thealgoschema property for stricter MCP client-side validation.
decimate_mesh/generate_lodsdeclare theiralgochoices via an"enum"JSON-schema array; this newalgoproperty only documents the allowed values in free text. Aligning it with the same pattern improves client-side validation/tooling.♻️ Proposed fix
props["algo"] = QJsonObject{{"type", "string"}, + {"enum", QJsonArray{"geodesic-voxel", "inverse-distance", "unirig"}}, {"description", "Weighting algorithm: 'geodesic-voxel' (default — Maya-style volume-aware " "bind, no cross-limb bleed; falls back to inverse-distance on volume-less " "meshes), 'inverse-distance' (legacy straight-line heuristic), or 'unirig' " "(ML skinning head — currently falls back to geodesic-voxel)."}};🤖 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 7968 - 7973, Add an "enum" array to the "algo" property schema in the relevant MCP tool definition, listing "geodesic-voxel", "inverse-distance", and "unirig" to match the documented choices and the schema pattern used by decimate_mesh/generate_lods; retain the existing description.
🤖 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/GeodesicVoxelBind.cpp`:
- Around line 184-235: Update ddaSegment to clip the segment [a, b] against the
grid’s padded AABB before converting endpoints to cells or running DDA; handle
fully disjoint and degenerate segments appropriately, then run the existing
traversal using the clipped endpoints and recomputed direction/endpoint cells.
Add a regression test where one endpoint is far outside the grid and the other
is distant on the opposite/asymmetric side, verifying the intersected cells are
returned.
- Around line 253-285: Validate all vertex coordinates in vertexPositions and
bone coordinate data in bones for finiteness before AABB/grid arithmetic, using
std::isfinite and returning an appropriate res.error for invalid input. Apply
the same validation to the later bone-processing path around the referenced
logic, ensuring no non-finite values reach ceil, floor, integer conversion, or
allocation.
In `@src/SkinWeights.cpp`:
- Around line 470-473: Check and store the boolean return value from
SkinWeights::computeWeights at this call site, and return or handle failure
before setting report.applied or reporting success; follow the existing
failure-handling pattern around lines 198–199 so failed computation cannot
produce an applied report with empty weights.
In `@src/SkinWeightsPost.cpp`:
- Around line 45-55: Renormalize the retained weights in toVertexWeights after
sorting and truncating r to maxK, dividing each remaining weight by their sum
before populating vw; preserve safe handling for an empty or zero-sum row. Add a
test covering laplacianSmooth propagation to more than eight bones and verify
the resulting vertex weights sum to one.
---
Outside diff comments:
In `@src/SkinWeightsController.cpp`:
- Around line 111-129: Clamp smoothIterations to the supported range before
assigning it to SkinWeightsOptions, ensuring the value passed through the
Laplacian post-pass cannot exceed intended limits. Validate algorithm before
calling algorithmFromString in the controller method: reject unknown or
unsupported strings with an error and return, rather than allowing the fallback
to geodesic-voxel; only add the breadcrumb and continue once validation
succeeds.
---
Nitpick comments:
In `@qml/PropertiesPanel.qml`:
- Around line 2255-2264: Update the button’s ToolTip.text near the auto-generate
weights control to remove the outdated “via inverse-distance to bone segments”
description and accurately describe Geodesic Voxel binding as volume-aware with
no cross-limb bleed, matching the Text content in the surrounding
PropertiesPanel UI.
In `@qml/SkinWeightsDialog.qml`:
- Around line 284-302: Align the smoothing-iterations UI limit with the
backend-supported range by changing the maxValue in the InspectorNumberField for
dialog.smoothIterations from 20 to 50. Keep the existing integer handling and
default behavior unchanged.
In `@src/MCPServer.cpp`:
- Around line 7968-7973: Add an "enum" array to the "algo" property schema in
the relevant MCP tool definition, listing "geodesic-voxel", "inverse-distance",
and "unirig" to match the documented choices and the schema pattern used by
decimate_mesh/generate_lods; retain the existing description.
🪄 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: d79cd35d-b3b4-4171-a907-34e607d43c32
📒 Files selected for processing (19)
CLAUDE.mdqml/PropertiesPanel.qmlqml/SkinWeightsDialog.qmlsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/GeodesicVoxelBind.cppsrc/GeodesicVoxelBind.hsrc/GeodesicVoxelBind_test.cppsrc/MCPServer.cppsrc/SkinWeights.cppsrc/SkinWeights.hsrc/SkinWeightsController.cppsrc/SkinWeightsController.hsrc/SkinWeightsPost.cppsrc/SkinWeightsPost.hsrc/SkinWeightsPost_test.cppsrc/SkinWeights_test.cppsrc/commands/ComputeSkinWeightsCommand.cppsrc/commands/ComputeSkinWeightsCommand.h
- tests/CMakeLists.txt: add GeodesicVoxelBind.cpp + SkinWeightsPost.cpp to qtmesh_test_common (Linux per-suite test build failed to link) - GeodesicVoxelBind: slab-clip bone segments to the grid AABB before the DDA walk — a bone with endpoints far outside the grid could exhaust the step budget and lose its seeds despite intersecting the mesh (regression test: ±1000-unit endpoints through the box) - GeodesicVoxelBind: reject non-finite vertex AABBs (int conversion of NaN/Inf is UB) and skip non-finite bones as seedless (tested) - SkinWeights::applyToEntity: check the computeWeights return value instead of reporting success with zero assignments - SkinWeightsPost: renormalize after truncating a >8-bone smoothed row (hub-vertex fan test: 11 bones → 8, sum stays 1) - SkinWeightsController: validate the algorithm string (reject typos instead of silently defaulting) and clamp voxelResolution / smoothIterations to the CLI/MCP ranges - Nitpicks: GUI smoothing cap 20→50 to match backend, stale "inverse-distance" tooltip in PropertiesPanel, JSON-schema enum on the MCP algo param Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Addressed all review feedback in 5062d33: Actionable:
Nitpicks: GUI smoothing cap raised 20→50, stale "inverse-distance" tooltip in PropertiesPanel updated, MCP Also fixed the 🤖 Generated with Claude Code |
|
This was referenced Jul 10, 2026
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 Slices A and B of #819, with full GUI/CLI/MCP surface parity and the Slice-C enum plumbing.
What's in
Slice A — Geodesic Voxel Binding (
src/GeodesicVoxelBind.{h,cpp}, new default)The method Maya ships as "Geodesic Voxel" bind (Dionne & de Lasa, SCA 2013), implemented natively with zero new dependencies:
voxelResolution(default 64, max 256)(1/d)^falloffweighting reusing the existingSkinWeightsOptionssemantics unchangedCross-limb bleed is impossible by construction: distances travel through the volume, so a hand near a thigh can never pick up leg weights.
Slice B — Weight post-pass pipeline (
src/SkinWeightsPost.{h,cpp}, applies to every algorithm)--smooth-iterations, 0 = off). Merge-mode (--merge) manual weights act as Dirichlet constraints — they shape the blend at the boundary but are never modifiedWhole-mesh single-pass compute:
computeAndApplynow gathers every vertex-data owner (shared + per-submesh) into one combined vertex/index set and computes once — matching the paper (voxelize the whole character). This was load-bearing: per-submesh grids stranded bones outside accessory AABBs (verified on a 39-submesh character where per-owner grids reported all 119 bones seedless).Algorithm enum + fallbacks
Algorithm { InverseDistance, GeodesicVoxel (default), UniRigML }UniRigML→ falls back to GeodesicVoxel with a clear reason until the Slice-Cskin.onnxexport is hosted (same pattern as UniRig AI: RigNet auto-rigging (ONNX, ML) #408)algorithmUsed,fallbackReason,bleedFraction,bonesWithoutSeedsSurfaces
qtmesh skin --algo geodesic-voxel|inverse-distance|unirig --voxel-res N --smooth-iterations Ncompute_skin_weightsgainsalgo,voxel_resolution,smooth_iterationsai.assist.skin.<algo>;qtmesh rig --skin/ MCPauto_rig {skin:true}chain through the new defaultQTMESH_GVB_DEBUG=1dumps grid/AABB/seed diagnostics to stderrTests
Pure-data suites (no Ogre/GL):
GeodesicVoxelBind_test.cpp,SkinWeightsPost_test.cpp+ updatedSkinWeights_test.cpp:Verified end-to-end
bandit.fbx(90,573 verts, 140k tris, 119 bones, 39 submeshes): geodesic-voxel end-to-end in 0.64 s at--voxel-res 128(target was ≤2 s at 64³/100k), bleed fraction 0.005, valid glb round-trip. Thin-rod asset falls back to inverse-distance with a clear reason.Out of scope (per issue slicing)
Closes nothing yet — tracks #819 (Slices A+B).
🤖 Generated with Claude Code
Summary by CodeRabbit