Skip to content

feat(skin): inverse-distance auto skin weights (closes #402)#699

Merged
fernandotonon merged 6 commits into
masterfrom
feat/ai-assist-skin-weights
Jun 1, 2026
Merged

feat(skin): inverse-distance auto skin weights (closes #402)#699
fernandotonon merged 6 commits into
masterfrom
feat/ai-assist-skin-weights

Conversation

@fernandotonon
Copy link
Copy Markdown
Owner

@fernandotonon fernandotonon commented May 30, 2026

Summary

Closes #402 (epic #397, AI-Assist slice 5). Adds automatic skin weight computation for skinned meshes.

Background — why not libigl BBW

The issue proposes wrapping libigl's bounded biharmonic weights (BBW). In practice:

  • BBW solves a biharmonic equation over the volume of the mesh — requires tetrahedralization via TetGen.
  • TetGen is GPL/copyleft. Linking it would force the entire binary to GPL and close off Homebrew / Snap / WinGet redistribution under the project's permissive-license stance.
  • TetGen meshing also fails on common asset issues (non-manifold, self-intersections, degenerate tris) — Mixamo FBX would routinely fail before BBW even runs.

This slice ships an inverse-distance heuristic with zero new dependencies. The Algorithm enum is in place to plug in libigl BBW behind a future opt-in -DENABLE_LIBIGL_BBW flag for users who accept the GPL implications.

Algorithm

For each vertex v:

  1. For every bone b in the skeleton's bind pose, compute distance from v to the bone's segment (line from head to average of children, falling back to point distance for leaves).
  2. If dist > maxInfluenceDistance × mesh-diagonal, skip bone (prevents a finger from picking up weight on a foot).
  3. Compute raw weight w_b = 1 / (dist² + eps)^(falloff/2).
  4. Keep the top-K bones (default K=4 matches the hardware skinning convention).
  5. Normalize so kept weights sum to 1.

This is the same algorithm Maya / 3dsMax use as their default "smooth bind." It's heuristic, not biharmonic, so it doesn't get BBW's volumetric smoothness — but it works out of the box on any character mesh including non-manifold FBX imports.

replaceExisting=false enables a merge mode that fills in missing weights while keeping existing ones (useful for "manually skinned the torso, now auto-skin the rest" workflows).

Surface

  • CLI: qtmesh skin <file> [--max-influences N] [--falloff F] [--max-distance D] [--skip-unweighted] [--merge] -o <out> [--json]. Multi-entity inputs rejected fail-fast (matches the decimate / retopo convention).
  • MCP: compute_skin_weights tool with max_influences / falloff / max_distance / skip_unweighted / replace_existing params.
  • GUI: Edit Mode → Tools → "Compute Skin Weights…" button + top-level dialog (qml/SkinWeightsDialog.qml) driven by the SkinWeightsController QML singleton. Button binds to hasSkinnedSelection so it disables on static meshes.
  • Library: SkinWeights::computeAndApply(entity, opts, algo) for the Ogre-backed path; SkinWeights::computeWeights(positions, bones, opts, outWeights) is a pure-data variant used by tests.

Verification (Rumba Dancing.fbx)

$ qtmesh skin 'rumba115/Rumba Dancing.fbx' -o /tmp/reskin.glb
Skin Weights
============
Mesh:               Rumba Dancing
Skeleton:           Rumba Dancing.skeleton
Bones:              69
Vertices processed: 5828
Assignments:        8461 → 20129
Wrote: reskin.glb

20,129 assignments / 5828 verts = avg 3.45 influences per vertex (capped at 4). Valid 761 KB .glb produced.

Sentry breadcrumbs

  • ai.assist.skin_weights for every action (CLI, MCP, GUI).

Tests

src/SkinWeights_test.cpp covers the pure-data path:

  • Near vertices get correct bone assignment
  • Weights sum to 1.0 per vertex
  • All weights non-negative
  • max_influences cap respected
  • max_distance cap excludes far bones
  • Higher falloff sharpens the bind toward the nearest bone
  • Empty input returns false
  • Algorithm string round-trip

Test plan

  • qtmesh skin <file> -o <out> produces valid skinned output
  • --max-influences N clamps assignments per vertex (verified: 5828 verts × 2 = 10,596 assignments with --max-influences 2)
  • Invalid numeric flags rejected with clear error
  • --json emits structured output
  • Multi-entity input rejected fail-fast
  • GUI button binds to skinned selection only
  • CI green on Linux / macOS / Windows
  • CodeRabbit / Sonar review

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Automatic skin-weight computation for skinned meshes (inverse-distance heuristic)
    • "Compute Skin Weights…" dialog in Animation Mode → Mode Tools with configurable parameters and keyboard shortcuts; button disabled when no skinned selection
    • Headless CLI and MCP tool endpoints for batch/remote skin-weight processing
    • Undo/redo support for computed weight changes; JSON/text reporting of results
  • Documentation

    • CLI and README updated with new skin-weight command options
  • Tests

    • Unit tests covering core algorithm behavior and reporting

Follow-ups (commits 2910c19, bc4949c)

  • Undo/redo: the auto-skin now runs through ComputeSkinWeightsCommand — snapshots the bone-assignment lists before, restores + recompiles on undo. Ctrl+Z reverts a bad auto-skin, Ctrl+Shift+Z reapplies it. A skeleton pre-check avoids leaving a no-op undo entry.
  • Button moved to Animation Mode: skinning governs animation deformation (a rigging step, not a mesh edit), so "Compute Skin Weights…" now lives in a "Skinning" section under Animation Mode → Mode Tools rather than Edit Mode.
  • CodeRabbit review addressed (2910c19): critical null-deref guard, controller error-string, MCP arg-type validation, CLI entity filter + import/export breadcrumbs, dialog keyboard accessibility, doc/test polish. All threads confirmed resolved by CodeRabbit.
  • Test coverage: SkinWeights_test expanded 8 → 17 tests (edge cases + report serializers).

Closes #402 (epic #397, AI-Assist slice 5). Adds automatic skin
weight computation for skinned meshes.

## Background — why not libigl BBW

The issue proposes wrapping libigl's bounded biharmonic weights
(BBW), which is the gold-standard algorithm used by Blender /
Maya / Houdini. In practice:

* BBW solves a biharmonic equation over the **volume** of the
  mesh — requires tetrahedralization via TetGen.
* TetGen is **GPL/copyleft**. Linking it would force the entire
  binary to GPL and close off Homebrew / Snap / WinGet
  redistribution under the project's permissive-license stance.
* TetGen meshing also fails on common asset issues (non-manifold,
  self-intersections, degenerate tris) — character FBX from
  Mixamo would routinely fail before BBW even runs.

This slice ships an inverse-distance heuristic with **zero new
dependencies**. The `Algorithm` enum is in place to plug in
libigl BBW behind a future opt-in `-DENABLE_LIBIGL_BBW` flag for
users who accept the GPL implications.

## Algorithm

For each vertex `v`:

1. For every bone `b` in the skeleton's bind pose, compute the
   distance from `v` to the bone's segment — line from the bone's
   head to the average position of its children (or just to head
   for leaf bones).
2. If `dist > maxInfluenceDistance × mesh-diagonal`, skip bone
   (prevents a finger bone from picking up weight on a foot).
3. Compute raw weight `w_b = 1 / (dist² + eps)^(falloff/2)`.
4. Keep the top-K bones (default K=4, matches the hardware
   skinning convention).
5. Normalize so the kept weights sum to 1.

This is the same algorithm Maya / 3dsMax use as their default
"smooth bind." It's heuristic, not biharmonic, so it doesn't
get the volumetric smoothness BBW provides — but it works out
of the box on any character mesh including non-manifold FBX
imports.

`replaceExisting=false` enables a merge mode that fills in
missing weights while keeping existing ones (useful for
"manually skinned the torso, now auto-skin the rest" workflows).

## Surface

* **CLI**: `qtmesh skin <file> [--max-influences N] [--falloff F]
  [--max-distance D] [--skip-unweighted] [--merge] -o <out>
  [--json]`. Multi-entity inputs rejected fail-fast (matches the
  `decimate` / `retopo` convention).
* **MCP**: `compute_skin_weights` tool with `max_influences /
  falloff / max_distance / skip_unweighted / replace_existing`
  params. Returns a structured `skin` object.
* **GUI**: Edit Mode → Tools → "Compute Skin Weights…" button +
  top-level dialog (`qml/SkinWeightsDialog.qml`) driven by the
  `SkinWeightsController` QML singleton. The button binds to
  `hasSkinnedSelection` so it disables on static meshes. Same
  Inspector-styled idiom as QuadRetopoDialog including the Esc/
  Enter keyboard handlers.
* **Library**: `SkinWeights::computeAndApply(entity, opts, algo)`
  for the Ogre-backed path; `SkinWeights::computeWeights(
  positions, bones, opts, outWeights)` is a pure-data variant
  used by tests and headless callers.

## Verification (Rumba Dancing.fbx)

```
$ qtmesh skin 'rumba115/Rumba Dancing.fbx' -o /tmp/reskin.glb
Skin Weights
============
Mesh:               Rumba Dancing
Skeleton:           Rumba Dancing.skeleton
Bones:              69
Vertices processed: 5828
Assignments:        8461 → 20129
Wrote: reskin.glb
```

20,129 assignments / 5828 verts = avg 3.45 influences per vertex
(capped at 4). Valid 761 KB .glb produced; the rig drives
skeletal deformation on round-trip.

## Sentry breadcrumbs

* `ai.assist.skin_weights` for every action (CLI, MCP, GUI).

## Tests

`src/SkinWeights_test.cpp` covers the pure-data path:

* Near vertices get correct bone assignment
* Weights sum to 1.0 per vertex
* All weights non-negative
* `max_influences` cap is respected
* `max_distance` cap excludes far bones
* Higher falloff sharpens the bind toward the nearest bone
* Empty input returns false
* Algorithm string round-trip

`tests/CMakeLists.txt` updated to include `SkinWeights.cpp` and
`SkinWeightsController.cpp` (same pattern as the previous slices
needed once each test binary links `mainwindow.cpp`).

## Documentation

* `CLAUDE.md` — new `SkinWeights` entry under AI-Assist, including
  the GPL rationale for deferring BBW.
* `README.md` — `qtmesh skin` CLI examples.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 30, 2026

Review Change Stack

Warning

Review limit reached

@fernandotonon, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 15 minutes and 4 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c38d521e-2d59-48c0-99c0-33d1c6264bc4

📥 Commits

Reviewing files that changed from the base of the PR and between bc4949c and 9c14ac7.

📒 Files selected for processing (3)
  • qml/PropertiesPanel.qml
  • src/SkinWeightsController.cpp
  • src/commands/ComputeSkinWeightsCommand.cpp
📝 Walkthrough

Walkthrough

Adds an inverse-distance skin-weight generator with core C++ implementation, undoable command, QML singleton/controller and modal dialog, CLI subcommand, MCP tool, unit tests, and related build/resource and docs updates.

Changes

Automatic Skin Weight Computation (SkinWeights)

Layer / File(s) Summary
Core SkinWeights API and types
src/SkinWeights.h
New SkinWeights API: computeAndApply(), computeWeights(), SkinWeightsOptions, SkinWeightsReport, and Algorithm enum (InverseDistance).
Inverse-distance Computation and Ogre Integration
src/SkinWeights.cpp
Geometry helpers (point-to-segment), top‑K weight accumulation, inverse-distance weighting and normalization, bone-segment construction from bind pose, merge/replace application to Ogre entities, and JSON/text report serialization.
Undoable compute command
src/commands/ComputeSkinWeightsCommand.h, src/commands/ComputeSkinWeightsCommand.cpp
QUndoCommand that snapshots per-owner bone assignments, runs compute on first redo, captures after-state, and restores before/after snapshots for undo/redo.
QML Controller and Selection Integration
src/SkinWeightsController.h, src/SkinWeightsController.cpp
QML singleton exposing computeWeightsForSelected(...), hasSkinnedSelection and busy properties, emits weightsApplied/error, pushes undo command, and reports structured result maps.
QML Dialog, UI Controls, and PropertiesPanel Integration
qml/SkinWeightsDialog.qml, qml/PropertiesPanel.qml, qml/qmldir, src/qml_resources.qrc
Modal dialog with compute params, keyboard handling, status lines, and a lazy-loaded "Compute Skin Weights…" button in PropertiesPanel; dialog registered in qmldir and bundled in QRC.
CLI Subcommand & main wiring
src/CLIPipeline.h, src/CLIPipeline.cpp, src/main.cpp
New qtmesh skin subcommand: arg parsing/validation, headless Ogre import, single-entity enforcement, compute+apply, export, and JSON/text reporting; main.cpp treats skin as CLI-only.
MCP Tool Integration
src/MCPServer.h, src/MCPServer.cpp
Adds compute_skin_weights MCP tool with strict JSON arg validation, defaults/range checks, Sentry breadcrumb logging, calls core computeAndApply, and returns text + JSON report or structured error.
Build System, Resources, Tests, and App Wiring
src/CMakeLists.txt, tests/CMakeLists.txt, src/mainwindow.cpp
Adds source/header entries for SkinWeights and controller to build lists and test targets, registers QML singleton in MainWindow, calls SkinWeightsController::kill() on shutdown, and ensures tests link implementation.
Unit tests
src/SkinWeights_test.cpp
GTest suite validating nearest-bone selection, normalization, influence limits, distance capping, falloff behavior, edge cases (leaf bone, on-bone vertex, outside radii), and report JSON/text helpers.
Documentation Updates
CLAUDE.md, README.md
Adds qtmesh skin CLI docs, and a detailed SkinWeights feature section documenting heuristic, parameters, UI/MCP/CLI entry points, undo semantics, Sentry breadcrumb, and future BBW/libigl note.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Poem

A rabbit hops through mesh and bone,
Closest points and weights are sown,
Dialogs click, commands undo,
CLI tools hum and tests run through,
🐰 Skinning flows from seed to sewn.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 21.31% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(skin): inverse-distance auto skin weights (closes #402)' accurately summarizes the main feature addition and references the closed issue.
Description check ✅ Passed The PR description comprehensively covers the Summary (with background and algorithm explanation), technical surface (CLI/MCP/GUI/Library), verification with example output, Sentry breadcrumbs, extensive tests, and follow-up commits addressing undo/redo and UX improvements.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ai-assist-skin-weights

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 89393251e3

ℹ️ 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 thread src/SkinWeights.cpp Outdated
Comment thread src/SkinWeights.cpp
Two P2 correctness bugs caught by Codex review on PR #699.

## 1. Route shared-vertex weights through the Mesh assignment list

Ogre stores skin assignments on `Mesh::getBoneAssignments()` for
submeshes that use `sharedVertexData`, not on the SubMesh — the
FBX / glTF exporters follow that split (read
`Mesh::getBoneAssignments()` for shared geometry). The old code
always called `sub->addBoneAssignment`, so running `skin` on a
shared-vertex skinned mesh left the mesh-level list empty and the
export kept stale weights or lost the new ones entirely.

Fix: detect whether any submesh uses shared vertices and, if so,
process `mesh->sharedVertexData` ONCE against the mesh-level
assignment APIs (`mesh->clearBoneAssignments` /
`addBoneAssignment` / `_compileBoneAssignments`). Per-submesh
(non-shared) data continues to route to the SubMesh APIs. The
compute+commit logic is factored into a `computeAndCommit` lambda
parameterized by the four owner-specific operations so both paths
share one implementation.

Processing shared data once (rather than per-submesh) also fixes
a latent double-processing bug: N submeshes sharing one vertex
buffer would previously have recomputed and re-cleared the same
data N times.

## 2. Honor merge mode instead of appending duplicate weights

`--merge` / `replaceExisting=false` previously only skipped the
`clearBoneAssignments()` call — the emit loop still added a full
new normalized weight set for EVERY vertex. On any vertex that
already had weights, the assignment list then held both the old
and new influences, blowing past the influence cap and producing
weights that no longer sum to 1.

Fix: in merge mode, build the set of already-weighted vertex
indices from the existing assignment list up front, then skip
emitting new weights for those vertices — filling only the
unweighted ones, as the option promises.

## Verification (Rumba Dancing.fbx, fully pre-skinned)

* Replace mode: 8461 → 20129 assignments (full recompute).
* Merge mode:   8461 → 8461 assignments (every vertex already
  weighted, so merge correctly adds nothing — previously this
  doubled to ~28590).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🤖 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 `@CLAUDE.md`:
- Line 240: Add the missing "skin" subcommand to the two earlier
recognized-subcommand lists so the CLI auto-activation docs match the new
"qtmesh skin" feature; update the lists that enumerate recognized subcommands
(the sections before the detailed "qtmesh skin" description and the other
subcommand list) to include "skin" and ensure the sentence that explains CLI
mode activation ("CLI mode is activated by: (1) invoking via the `qtmesh`
symlink, (2) passing `--cli`, or (3) using a recognized subcommand as the first
argument.") remains accurate.

In `@qml/SkinWeightsDialog.qml`:
- Around line 81-178: InspectorButton and InspectorCheckbox are mouse-only; make
them keyboard-focusable and activate on Enter/Space. For InspectorButton (ids
btn and btnMa) set the interactive element (MouseArea) to be focusable (focus:
true and enableTabFocus / focusPolicy/Keys support), add Keys.onPressed handlers
to call btn.clicked() on Enter/Return/Space and update cursor/visual focus
styling when active; ensure the Text shows focus rect if needed. For
InspectorCheckbox (properties label/checked and its MouseArea + inner checkbox
Rectangle) make its MouseArea focusable the same way, add Keys.onPressed to
toggle parent.toggled() on Enter/Return/Space and update the displayed check
mark based on checked; ensure tab order / width behavior remains unchanged. Use
the existing ids (btn, btnMa, nf, ni, InspectorCheckbox, parent.toggled) to
locate and modify the components.

In `@src/CLIPipeline.cpp`:
- Around line 6951-6963: Filter the result of
Manager::getSingleton()->getEntities() to only real Ogre::Entity objects before
deciding failure/duplicate or performing a cast: after calling
MeshImporterExporter::importer(...) iterate the returned entities collection and
check each object's getMovableType() == "Entity", collect matching objects into
a new list (or vector) of Ogre::Entity*; then use that filtered list to test
emptiness, size>1 and to obtain the first Ogre::Entity* instead of using
entities.first(); this avoids miscounting ManualObject helpers and unsafe casts.
- Around line 6947-6983: Add Sentry breadcrumbs for the import and export steps
in the cmdSkin flow: before calling
MeshImporterExporter::importer({fi.absoluteFilePath()}) add
SentryReporter::addBreadcrumb(QStringLiteral("file.import"), QString("import
%1").arg(fi.absoluteFilePath())); and before calling
MeshImporterExporter::exporter(node, QFileInfo(outputPath).absoluteFilePath(),
fmt) add SentryReporter::addBreadcrumb(QStringLiteral("file.export"),
QString("export %1
fmt=%2").arg(QFileInfo(outputPath).absoluteFilePath()).arg(fmt)); keep using
SentryReporter::addBreadcrumb(category, message) and the same string
construction style as the existing ai.assist.skin_weights breadcrumb.

In `@src/MCPServer.cpp`:
- Around line 1490-1500: The arg parsing in toolComputeSkinWeights currently
trusts QJsonValue::toInt/toDouble/toBool which silently accepts wrong types;
update the parsing to explicitly validate types on args before assigning to
SkinWeightsOptions: for "max_influences" ensure
args["max_influences"].isDouble() and that the numeric value is integral (e.g.,
value.toDouble() == value.toInt()) before setting opts.maxInfluencesPerVertex,
for "falloff" and "max_distance" require args["..."].isDouble() before using
toDouble(), and for "skip_unweighted" and "replace_existing" require
args["..."].isBool() before using toBool(); on any type mismatch return an
explicit usage/error response from toolComputeSkinWeights instead of silently
using defaults.

In `@src/SkinWeights_test.cpp`:
- Around line 136-147: The test indexes low[1] and high[1] without first
asserting that the weight computations succeeded or produced at least two
elements; update the test around the two SkinWeights::computeWeights calls to
verify success and output size before accessing index 1 — for example, assert
the call returned a success status (if computeWeights has a return) or assert
low.size() >= 2 and high.size() >= 2 (or low.count/high.count where applicable)
after calling SkinWeights::computeWeights with kBarPositions, kTwoBones,
optsLow/optsHigh and before the ASSERT_GE/EXPECT_GE checks.

In `@src/SkinWeights.h`:
- Around line 65-69: The comment for the member skipUnweightedBones in
class/struct SkinWeights is inaccurate (it says "Default true" while the
initializer is false); update the comment to match the actual default (false) or
otherwise change the initializer if intended, e.g. edit the comment above
skipUnweightedBones to state "Default false" so the documentation matches the
initialized value skipUnweightedBones.

In `@src/SkinWeightsController.cpp`:
- Around line 115-120: When report.applied is false and report.error is empty
you emit the fallback "Skin weights failed" but never set result["error"],
causing callers (e.g., SkinWeightsDialog.runCompute) to see no error and replace
it with "unknown error"; update the failure path in SkinWeightsController (the
branch that checks report.applied and uses report.error) so that before emitting
the error signal you assign result["error"] = QStringLiteral("Skin weights
failed") when report.error.isEmpty(), ensuring the returned result contains the
same fallback string you emit.
- Around line 74-95: The code dereferences entities.first() and calls
entity->getName() and SkinWeights::computeAndApply without validating the
resolved entity; add a guard after obtaining Ogre::Entity* entity =
entities.first() to ensure entity is non-null (and its mesh if needed) and
handle the null/stale case by logging/reporting an error, clearing m_busy and
emitting busyChanged before returning or setting an error SkinWeightsReport;
reference the same null checks used by hasSkinnedSelection() and protect calls
to entity->getName() and SkinWeights::computeAndApply accordingly.
🪄 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: 540f1298-24bc-4269-834c-11de2de3a8c1

📥 Commits

Reviewing files that changed from the base of the PR and between 03e6067 and dd890b8.

📒 Files selected for processing (19)
  • CLAUDE.md
  • README.md
  • qml/PropertiesPanel.qml
  • qml/SkinWeightsDialog.qml
  • qml/qmldir
  • src/CLIPipeline.cpp
  • src/CLIPipeline.h
  • src/CMakeLists.txt
  • src/MCPServer.cpp
  • src/MCPServer.h
  • src/SkinWeights.cpp
  • src/SkinWeights.h
  • src/SkinWeightsController.cpp
  • src/SkinWeightsController.h
  • src/SkinWeights_test.cpp
  • src/main.cpp
  • src/mainwindow.cpp
  • src/qml_resources.qrc
  • tests/CMakeLists.txt

Comment thread CLAUDE.md Outdated
Comment thread qml/SkinWeightsDialog.qml
Comment thread src/CLIPipeline.cpp
Comment thread src/CLIPipeline.cpp Outdated
Comment thread src/MCPServer.cpp
Comment thread src/SkinWeights_test.cpp Outdated
Comment thread src/SkinWeights.h
Comment thread src/SkinWeightsController.cpp
Comment thread src/SkinWeightsController.cpp Outdated
fernandotonon and others added 3 commits June 1, 2026 01:06
Adds coverage for the edge cases, fallback paths, and the report
serializers that the initial suite missed.

New tests:

* SingleBoneGetsFullWeight — one-bone skeleton → every vert 100%
  bone 0.
* LeafBonePointDistanceWorks — head==tail bone degenerates to
  point distance (the `abLenSq < 1e-18` branch in
  distSqPointSegment).
* VertexExactlyOnBoneDoesNotDivideByZero — exercises the `+ 1e-12`
  eps guard; asserts no NaN/Inf leaks through.
* VertexOutsideAllRadiiPinsToBoneZero — the "pin to bone 0,
  weight 1.0" fallback when a vertex is beyond every bone's
  influence cap (so it still animates with the rig instead of
  going static).
* MaxInfluencesClampedToUpperBound — requesting 999 influences is
  clamped to the hard cap of 8, not overflowing the fixed-size
  VertexWeights arrays.
* FalloffClampedFromBelow — falloff below the 0.5 floor still
  produces a valid normalized weight set.
* ReportToJsonRoundTrip — full report → JSON, incl. the submeshes
  array and verticesWithMaxInfluences.
* ReportToJsonIncludesErrorOnFailure — `error` key present when
  applied=false.
* ReportToTextContainsKeyFields — text report mentions mesh /
  skeleton names and the before/after counts.

The pure-data `computeWeights` and the two serializers are now
fully covered. The Ogre-backed `computeAndApply` (incl. the
shared-vertex routing and merge-mode skip fixed in dd890b8) still
relies on integration runs since it needs a live entity +
skeleton — guarded out of headless CI via the existing Ogre-init
skip.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Batch of fixes from the CodeRabbit re-review. One critical
null-deref, several Major hardening items, and minor doc/test
polish.

## Critical — guard the resolved entity (SkinWeightsController.cpp)

`computeWeightsForSelected` dereferenced `entity->getName()` and
called `computeAndApply()` on `getResolvedEntities().first()`
without a null check, even though `hasSkinnedSelection()` already
treats that entry as nullable. A stale/null resolved entry would
crash instead of returning a user-facing error. Added a
`!entity || !entity->getMesh()` guard that returns
"Selected entity is no longer valid."

## Major — controller always returns an error string

On the generic `report.applied == false` path the controller
emitted "Skin weights failed" but left `result["error"]` unset, so
`SkinWeightsDialog.runCompute()` overwrote it with "unknown
error". Now the fallback message is written into the result map
before emitting.

## Major — MCP argument type validation (MCPServer.cpp)

`QJsonValue::toInt/toDouble/toBool` silently return the default
when the JSON type mismatches, so `"falloff": "4"` (string) or
`"replace_existing": "false"` (string) would apply unintended
settings. Added `isDouble()` / `isBool()` type checks that return
a clear usage error before reading any value.

## Major — filter to real entities in cmdSkin (CLIPipeline.cpp)

`Manager::getEntities()` can include helper ManualObjects;
treating them as mesh entities made a single-entity file look
multi-entity (and could cast wrong). Now filters on
`getMovableType() == "Entity"` before the count/first checks —
matching the documented `Manager::getEntities()` pitfall.

## Major — file.import / file.export breadcrumbs (CLIPipeline.cpp)

`cmdSkin` does both import and export but only logged the
ai.assist breadcrumb. Added the standard `file.import` /
`file.export` breadcrumbs per the project's Sentry convention.

## Major — keyboard accessibility (SkinWeightsDialog.qml)

`InspectorButton` and `InspectorCheckbox` were mouse-only — the
checkboxes especially couldn't be reached/toggled by keyboard.
Added `activeFocusOnTab`, `Accessible.role`/`name`/`checked`, and
Enter/Space/Return key handlers, plus a focus-ring border so
keyboard users can see the focused control. (The Window-level
Esc/Enter handler from the original dialog stays for the footer
buttons.)

## Minor — doc + test polish

* `SkinWeights.h`: the `skipUnweightedBones` comment said "Default
  true" but the initializer is `false` (CLI also defaults false).
  Corrected the comment.
* `CLAUDE.md`: added `retopo` and `skin` to the recognized-
  subcommand list that drives CLI auto-activation.
* `SkinWeights_test.cpp` (FalloffSharpensTheBind): assert
  `computeWeights` succeeded and the result vectors are large
  enough before indexing `[1]`, so a regression fails with a clear
  assertion instead of UB.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two follow-ups on the #402 skin-weights feature, both per user
request on PR #699.

## Undo/redo for the auto-skin operation

The auto-skin previously mutated bone assignments in place with no
undo entry — a bad result couldn't be reverted. Added
`ComputeSkinWeightsCommand` (`src/commands/`), a QUndoCommand that:

* Snapshots every submesh's `VertexBoneAssignmentList` plus the
  mesh-level shared list before the first `redo()`.
* Runs `SkinWeights::computeAndApply` on first redo and captures
  the resulting "after" state for replay on redo-after-undo.
* On `undo()` / replay, restores the snapshot list and calls
  `_compileBoneAssignments`, which re-packs the (unchanged) vertex
  buffer's BLEND_INDICES/WEIGHTS bytes and rebuilds the
  blendIndexToBoneIndexMap from the restored list. Unlike the
  UV-unwrap restore — which swapped the whole VertexData out from
  under a live SkeletonInstance — here the buffer object is the
  same one the skeleton references, so recompiling is correct and
  safe (it just rewrites the blend bytes in place).

`SkinWeightsController::computeWeightsForSelected` now pushes this
command through `UndoManager` instead of calling `computeAndApply`
directly, so Ctrl+Z reverts the auto-skin and Ctrl+Shift+Z
reapplies it. A skeleton pre-check was added before the push so a
skeleton-less mesh fails cleanly without leaving a no-op entry on
the undo stack (QUndoStack::push runs redo() unconditionally).

## Moved the button to Animation Mode

Skinning governs how the mesh deforms under animation — it's a
rigging step, not a mesh-topology edit — so the
"Compute Skin Weights…" button moved out of Edit Mode Tools into a
new "Skinning" CollapsibleSection under Animation Mode → Mode
Tools (alongside the VAT baker), gated on
`hasAnimations` + AnimationMode. The button still binds to
`hasSkinnedSelection` so it disables on static meshes.

CLAUDE.md updated to reflect the new location + the undo behavior.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/SkinWeights_test.cpp (1)

147-154: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Use a probe point that is off the bone for the falloff check.

low[1] / high[1] come from the vertex at (0, 1, 0), which lies exactly on bone 0. That makes the primary weight effectively saturated regardless of falloff, so this test doesn't really prove the "sharper bind" behavior. A dedicated probe like (0, 1.25, 0) would exercise the intended comparison much better.

Suggested test adjustment
 TEST(SkinWeightsTest, FalloffSharpensTheBind)
 {
     // With a high falloff, the weights should concentrate more
     // aggressively on the nearest bone.
     std::vector<SkinWeights::VertexWeights> low, high;
+    const std::vector<float> probe = {
+        0.0f, 1.25f, 0.0f,
+    };

     SkinWeightsOptions optsLow;
     optsLow.maxInfluencesPerVertex = 2;
     optsLow.falloff = 1.0;
     optsLow.maxInfluenceDistance = 0;
     ASSERT_TRUE(SkinWeights::computeWeights(
-        kBarPositions.data(), 4, kTwoBones, optsLow, low));
+        probe.data(), 1, kTwoBones, optsLow, low));

     SkinWeightsOptions optsHigh = optsLow;
     optsHigh.falloff = 8.0;
     ASSERT_TRUE(SkinWeights::computeWeights(
-        kBarPositions.data(), 4, kTwoBones, optsHigh, high));
+        probe.data(), 1, kTwoBones, optsHigh, high));

-    // Vertex 1 sits at y=1 — equidistant-ish from both bones.
-    // High falloff should drive its primary weight higher.
-    ASSERT_GE(low.size(), 2u);
-    ASSERT_GE(high.size(), 2u);
-    ASSERT_GE(low[1].count, 1);
-    ASSERT_GE(high[1].count, 1);
-    EXPECT_GE(high[1].weights[0], low[1].weights[0])
+    ASSERT_EQ(low.size(), 1u);
+    ASSERT_EQ(high.size(), 1u);
+    ASSERT_GE(low[0].count, 1);
+    ASSERT_GE(high[0].count, 1);
+    EXPECT_GT(high[0].weights[0], low[0].weights[0])
         << "high falloff failed to concentrate weight on the nearest bone";
 }
🤖 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/SkinWeights_test.cpp` around lines 147 - 154, The test currently compares
low[1]/high[1] which correspond to the vertex at (0,1,0) that lies on bone 0 and
masks falloff differences; change the probe to a point off the bone (e.g. (0,
1.25, 0)) and use the skinning result for that probe instead of low[1]/high[1],
then assert EXPECT_GE(highProbe.weights[0], lowProbe.weights[0]) (and keep the
existing count checks) so the sharper bind behavior is actually exercised;
update any variable names (e.g., create lowProbe/highProbe) or lookup logic to
obtain the entry for the (0,1.25,0) sample used in the test.
♻️ Duplicate comments (1)
src/MCPServer.cpp (1)

1495-1496: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject fractional max_influences values explicitly.

This still accepts JSON numbers like 4.5, and toInt(4) later will silently fall back to the default instead of preserving the caller’s value or returning a usage error. max_influences is documented as an integer, so validate integrality here before the assignment.

Suggested fix
-    if (args.contains("max_influences") && !args["max_influences"].isDouble())
-        return makeErrorResult("Error: 'max_influences' must be a number.");
+    if (args.contains("max_influences")) {
+        const QJsonValue maxInfluencesValue = args["max_influences"];
+        if (!maxInfluencesValue.isDouble())
+            return makeErrorResult("Error: 'max_influences' must be an integer.");
+        const double maxInfluences = maxInfluencesValue.toDouble();
+        if (!std::isfinite(maxInfluences) || std::floor(maxInfluences) != maxInfluences)
+            return makeErrorResult("Error: 'max_influences' must be an integer.");
+    }
In Qt 6, how does QJsonValue::toInt(int defaultValue) behave when the JSON value is a non-integral number like 4.5? Does it return the provided default value instead of converting/truncating?
🤖 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 1495 - 1496, The current check only ensures
"max_influences" is numeric but allows fractional numbers like 4.5; update
validation around args["max_influences"] in MCPServer.cpp to explicitly reject
non-integral numeric values by first checking isDouble() and then verifying that
args["max_influences"].toDouble() is an integer (e.g., compare to
std::floor/toInt-truncated value or use std::modf) before any later toInt()
call, returning makeErrorResult("Error: 'max_influences' must be an integer.")
if it’s not integral so the caller’s bad input is rejected rather than silently
falling back to a default.
🧹 Nitpick comments (1)
src/commands/ComputeSkinWeightsCommand.h (1)

18-33: ⚡ Quick win

Update the restore contract comment.

This block still says undo/redo restore paths do not call _compileBoneAssignments(), but the implementation now does so in restoreSnapshot(). Keeping the old invariant here makes the snapshot/restore logic much harder to reason about during future fixes.

🤖 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/commands/ComputeSkinWeightsCommand.h` around lines 18 - 33, The comment
block is outdated: it claims undo/redo do NOT call _compileBoneAssignments(),
but the implementation in restoreSnapshot() does. Update the comment in
ComputeSkinWeightsCommand to remove or correct the sentence stating that the
index maps are reinstalled without calling _compileBoneAssignments, and instead
document that restoreSnapshot() reinstalls the index maps and also invokes
_compileBoneAssignments() (and why, briefly) so the comment matches the actual
behavior of redo(), undo(), and restoreSnapshot().
🤖 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 310-314: The CollapsibleSection for "Skinning" is incorrectly
gated by PropertiesPanelController.hasAnimations which hides the entire section
for skinned meshes without clips; change the sectionVisible predicate in the
CollapsibleSection so it does not require hasAnimations — either call
root.modeToolSectionVisible(EditorModeController.AnimationMode) alone or OR in
SkinWeightsController.hasSkinnedSelection so the section remains visible for
skinned selections while the SkinWeightsController.hasSkinnedSelection continues
to gate the button availability.
- Around line 1133-1164: The skinning button (Rectangle with id skinLabel and
MouseArea id skinMa) is currently mouse-only; make it keyboard accessible by
allowing focus on the MouseArea (set focus: true and focusPolicy: Qt.TabFocus),
add Accessible properties (e.g., Accessible.name = skinLabel.text and
Accessible.role = Accessible.Button) on the MouseArea or parent Rectangle, and
handle Enter/Space via Keys.onPressed (or Keys.onReleased) to call
root.openSkinWeightsDialog() just like onClicked; also ensure visual focus
indication (e.g., change border.color or opacity when skinMa.activeFocus is
true) and keep enabled binding to SkinWeightsController.hasSkinnedSelection so
keyboard users cannot activate when disabled.

In `@src/commands/ComputeSkinWeightsCommand.cpp`:
- Around line 21-27: Manager::getEntities() returns all movable objects so
iterating with Ogre::Entity* causes unsafe casts; change the loop to iterate as
Ogre::MovableObject* (or auto* as Ogre::MovableObject) from
Manager::getEntities(), check obj->getMovableType() == "Entity" before casting,
then cast to Ogre::Entity* (or use dynamic_cast) and compare its name to
mEntityName; update the loop in ComputeSkinWeightsCommand.cpp to perform the
movable-type guard prior to any Entity-specific access.

In `@src/SkinWeightsController.cpp`:
- Around line 77-98: The early-return branches in the method (checking !entity,
!entity->getMesh(), and missing skeleton via entity->getMesh()->getSkeleton())
bypass the single breadcrumb so failed attempts never reach Sentry; before these
validations run (or at least inside each early-return branch) call
SentryReporter::addBreadcrumb("ui.action", "<descriptive action>") so the UI
action is recorded, then emit the error and set
result["applied"]/result["error"] as currently done; update the
SkinWeightsController method containing these checks to add the breadcrumb using
category "ui.action" (or add one breadcrumb at the start of the validation
block) so every user-facing failure is recorded.

---

Outside diff comments:
In `@src/SkinWeights_test.cpp`:
- Around line 147-154: The test currently compares low[1]/high[1] which
correspond to the vertex at (0,1,0) that lies on bone 0 and masks falloff
differences; change the probe to a point off the bone (e.g. (0, 1.25, 0)) and
use the skinning result for that probe instead of low[1]/high[1], then assert
EXPECT_GE(highProbe.weights[0], lowProbe.weights[0]) (and keep the existing
count checks) so the sharper bind behavior is actually exercised; update any
variable names (e.g., create lowProbe/highProbe) or lookup logic to obtain the
entry for the (0,1.25,0) sample used in the test.

---

Duplicate comments:
In `@src/MCPServer.cpp`:
- Around line 1495-1496: The current check only ensures "max_influences" is
numeric but allows fractional numbers like 4.5; update validation around
args["max_influences"] in MCPServer.cpp to explicitly reject non-integral
numeric values by first checking isDouble() and then verifying that
args["max_influences"].toDouble() is an integer (e.g., compare to
std::floor/toInt-truncated value or use std::modf) before any later toInt()
call, returning makeErrorResult("Error: 'max_influences' must be an integer.")
if it’s not integral so the caller’s bad input is rejected rather than silently
falling back to a default.

---

Nitpick comments:
In `@src/commands/ComputeSkinWeightsCommand.h`:
- Around line 18-33: The comment block is outdated: it claims undo/redo do NOT
call _compileBoneAssignments(), but the implementation in restoreSnapshot()
does. Update the comment in ComputeSkinWeightsCommand to remove or correct the
sentence stating that the index maps are reinstalled without calling
_compileBoneAssignments, and instead document that restoreSnapshot() reinstalls
the index maps and also invokes _compileBoneAssignments() (and why, briefly) so
the comment matches the actual behavior of redo(), undo(), and
restoreSnapshot().
🪄 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: 93ae16a8-0efa-4871-9e9d-321c134b16ad

📥 Commits

Reviewing files that changed from the base of the PR and between dd890b8 and bc4949c.

📒 Files selected for processing (12)
  • CLAUDE.md
  • qml/PropertiesPanel.qml
  • qml/SkinWeightsDialog.qml
  • src/CLIPipeline.cpp
  • src/CMakeLists.txt
  • src/MCPServer.cpp
  • src/SkinWeights.h
  • src/SkinWeightsController.cpp
  • src/SkinWeights_test.cpp
  • src/commands/ComputeSkinWeightsCommand.cpp
  • src/commands/ComputeSkinWeightsCommand.h
  • tests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (5)
  • tests/CMakeLists.txt
  • src/CLIPipeline.cpp
  • src/SkinWeights.h
  • qml/SkinWeightsDialog.qml
  • src/CMakeLists.txt

Comment thread qml/PropertiesPanel.qml Outdated
Comment thread qml/PropertiesPanel.qml
Comment thread src/commands/ComputeSkinWeightsCommand.cpp
Comment thread src/SkinWeightsController.cpp
Four findings from CodeRabbit's review of bc4949c.

## Skinning section gated on hasAnimations (PropertiesPanel.qml)

The new "Skinning" section was gated on `hasAnimations`, so it
vanished for a skeleton-bearing mesh with no clips yet — exactly
the case where you'd author weights before animating. Re-gated on
`SkinWeightsController.hasSkinnedSelection` (has a skeleton) plus
the Animation-Mode + Mode-Tools-tab check, instead of
hasAnimations.

## Skinning button not keyboard-accessible (PropertiesPanel.qml)

The "Compute Skin Weights…" button was mouse-only. Added
`activeFocusOnTab`, `Accessible.role`/`name`, Enter/Space/Return
key handlers, and a focus-ring border — matching the dialog
controls fixed in 2910c19.

## resolveEntity loop (ComputeSkinWeightsCommand.cpp)

CodeRabbit flagged binding `Ogre::Entity* e` before the
movable-type check as the unsafe cast the CLAUDE.md pitfall warns
about. In fact `Manager::getEntities()` already filters to real
Entities at collection time (`collectEntitiesRecursive` only
appends objects whose `getMovableType() == "Entity"`), so every
element is genuine. Added a comment documenting that the inner
re-check is belt-and-suspenders, not load-bearing — no behavior
change since the list is already safe.

## Validation exits skipped the breadcrumb (SkinWeightsController.cpp)

The early-return validation branches (no selection, no skeleton,
stale entity) returned before the method's only breadcrumb, so
failed UI attempts never reached Sentry. Added a `ui.action`
breadcrumb at the top of `computeWeightsForSelected` so every
attempt — successful or not — is recorded.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 1, 2026

@fernandotonon fernandotonon merged commit 4ef7760 into master Jun 1, 2026
20 checks passed
@fernandotonon fernandotonon deleted the feat/ai-assist-skin-weights branch June 1, 2026 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AI: libigl bounded biharmonic skinning weights

1 participant