Skip to content

feat(#410): AI mesh part segmentation (ONNX PointNet++ + geometric fallback)#771

Merged
fernandotonon merged 12 commits into
masterfrom
feat/ai-segment-410
Jun 28, 2026
Merged

feat(#410): AI mesh part segmentation (ONNX PointNet++ + geometric fallback)#771
fernandotonon merged 12 commits into
masterfrom
feat/ai-segment-410

Conversation

@fernandotonon

@fernandotonon fernandotonon commented Jun 27, 2026

Copy link
Copy Markdown
Owner

Implements issue #410 — AI: Mesh segmentation / part labeling (ONNX). Follows the epic #397 AI-assist pattern (#404/#408/#409). The fourth ONNX consumer.

What it does

Predicts a semantic part label (head / torso / left+right arm / left+right leg) per vertex and per face of a mesh. Powers Edit-Mode "Select by part", per-part material assignment, and auto-rig priors. Surfaced on CLI, MCP, and the Edit Mode panel.

Geometric fallback is first-class

MeshSegmenter::segmentGeometric — connected-component islands (union-find) + an up-axis/lateral spatial heuristic (top→head, lower→legs, mid-sides→arms, centre→torso), overridable per-vertex by rig bone-proximity hints — is always compiled (Ogre-free, no ONNX) and used automatically when the build lacks ONNX, the model is missing/un-downloadable, or inference fails. Result::usedModel / fallbackReason report which path ran. Verified on a Mixamo humanoid: head/torso/arms/legs all populated with sensible proportions.

ONNX path

PointNet++-style: normalise → deterministic point sample → [1,N,3] → per-point argmax over part channels → scatter labels back to all vertices by nearest sampled point. Runtime I/O-name discovery, channels-first/last handling, CoreML EP. ensureModelBlocking() downloads meshseg.onnx to AppData/ai_models/segment/ (overrides QTMESH_SEGMENT_MODEL_BASE_URL / QSettings ai/segmentModelBaseUrl; offline guard QTMESH_SEGMENT_NO_DOWNLOAD; non-ONNX #ifndef guard).

Surfaces (CLI / MCP / GUI)

  • CLI: qtmesh segment <file> [--json] [--no-model] [--up-axis x|y|z] — per-part vertex/face counts (text) or full vertex/face→label arrays + per-part summary (stable JSON schema).
  • MCP: segment_mesh tool — {entity_name?, no_model?}, returns per-part vertex counts + which path ran (registered heavy).
  • GUI: Edit Mode → "Select by Part (AI)" — selects all faces matching the selected face's part (or the largest part if none selected); pushes via selectFace so the existing highlight refreshes.

Model (trained + hosted)

The standard part-seg datasets (ShapeNet-Part, PartNet) are non-commercial — same wall as #408/#409 — so the shipped model is being trained on synthetic, permissively-derived data: per-vertex labels read from rigged-humanoid bone weights (a CC0 derivation we own). scripts/export-meshseg-onnx.py (offline, not shipped) synthesises the data → trains → exports. The model is trained (98% per-point acc on held-out synthetic) and HOSTED at fernandotonon/QtMeshEditor-models/segment/meshseg.onnx (~0.3 MB) — validated on a real Mixamo character via the C++ runtime ("via model", symmetric head/torso/arms/legs, better than the geometric fallback). The geometric fallback remains for non-ONNX builds / inference failure.

Acceptance criteria

  • Humanoid test mesh → head/torso/limb labels (geometric fallback today; ONNX when hosted).
  • "Select by part" populates the selection set.
  • CLI JSON output documented + stable.
  • Sentry breadcrumb ai.assist.segment.

Version bumped to 3.13.0. 11 unit tests for the pure-data core. Attribution: THIRD_PARTY_AI_MODELS.md.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added AI mesh part segmentation via a new segment CLI command and an MCP tool (segment_mesh), including optional JSON output and an --no-model geometric fallback.
    • Introduced Edit Mode “Select by Part (AI)” with download/segment progress, progress bar, and cancellation.
  • Bug Fixes
    • Improved CLI --up-axis validation to return exit code 2 when the value is missing.
  • Documentation
    • Updated CI/reproducible-build documentation to version 3.13.0 and expanded AI model attribution/workflow notes for mesh segmentation.

Update — segmentation quality + responsive UI

  • Worker-threaded "Select by Part" + progress bar. The action ran the
    first-use model download + ONNX inference synchronously and froze the app.
    Now (mirroring AI: RigNet auto-rigging (ONNX, ML) #408 UniRig) it gathers geometry on the main thread, runs the
    download + inference on a worker thread (with a "Downloading model…" phase, a
    determinate progress bar, and a Cancel button), and applies the selection
    back on the main thread.
  • Rig-prior segmentation (the big quality win). When the mesh is SKINNED,
    each vertex is now labeled by the part of the bone it's most-weighted to
    exact, instant, no model/download. MeshSegmenter::partForBoneName handles
    non-human anatomy the humanoid coordinate-model can't: ears/snout/muzzle →
    head, tail → torso, paws/hooves/toes → leg, wings/fingers → arm (guards the
    "ear" ⊂ "forearm" collision; unit-tested). Used when it covers ≥70% of
    vertices; unrigged/sparse-rig meshes fall through to the model, then the
    geometric fallback. Status reads "(from rig)".
  • Retrained ML fallback model → v1.1.0 (for unrigged meshes). The original
    model put arms DOWN (real characters are T/A-pose), used heavy 3-axis
    augmentation (scrambled left/right), and a flat PointNet (couldn't separate
    adjacent parts → cat-ear-as-arm). Retrained on T/A-pose arms, upright +
    yaw-only augmentation, head-protrusion samples, wider proportions, and a
    PointNet++-style local kNN feature. 99% per-part on a held-out T-pose test.
    Hosted at the combined repo + a dedicated QtMeshEditor-mesh-segmentation repo (tagged v1.1.0).

Verified on real meshes: a Mixamo humanoid and a non-humanoid (big-eared) cat
now segment into clean head/torso/arm/leg via the rig path.

fernandotonon and others added 4 commits June 27, 2026 16:03
… path

First slice of AI mesh part segmentation (epic #397, 4th ONNX consumer). Ogre-free,
unit-testable core (src/MeshSegmenter.{h,cpp}):

- Part taxonomy: unknown/head/torso/left+right arm/left+right leg (stable string
  ids; index order = the model's output channels).
- **Geometric fallback (always compiled, no ONNX/Ogre)** — connected-component
  islands + an up-axis/lateral spatial heuristic (top→head, lower→legs,
  mid-sides→arms, centre→torso), overridable per-vertex by rig bone-proximity
  hints. Used automatically when the build lacks ONNX, the model is missing, or
  inference fails. Produces reasonable head/torso/limb labels on upright
  humanoids and a stable component labeling otherwise.
- **PointNet++ ONNX path** (#ifdef ENABLE_ONNX) — normalise → deterministic
  point sample → [1,N,3] tensor → per-point argmax → scatter back to vertices by
  nearest sampled point. Runtime I/O-name discovery, channels-first/last
  handling, CoreML EP on macOS. Self-contained ensureModelBlocking() (the #408
  pattern: AppData/ai_models/segment/, QTMESH_SEGMENT_* overrides + offline
  guard).
- Pure-data helpers (connectedComponents, facesFromVertexLabels) + 11 unit
  tests covering taxonomy, islands, majority face labels, the spatial heuristic,
  bone-proximity override, and the predict→fallback path.

Permissive-data note (THIRD_PARTY follows): ShapeNet-Part / PartNet are
non-commercial, so the shipped model will be trained on synthetic,
permissively-derived labels (rig bone-weight → part). The plumbing + fallback
ship now; the ONNX path lights up when that model is hosted.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- CLI: `qtmesh segment <file> [--json] [--no-model] [--up-axis x|y|z]`
  (CLIPipeline::cmdSegment). Loads the mesh, gathers geometry via
  AutoRig::gatherGeometry, runs MeshSegmenter::predict, and emits per-part
  vertex/face counts (text) or the full vertex/face→label arrays + per-part
  summary (--json, stable schema). Registered in the CLI dispatch +
  AppLaunchHandler's recognized-subcommand list.
- MCP: `segment_mesh` tool (MCPServer::toolSegmentMesh) — args
  {entity_name?, no_model?}; returns per-part vertex counts + which path ran.
  Registered + marked heavy + schema added.
- Sentry breadcrumb ai.assist.segment on both surfaces.

Verified end-to-end on a Mixamo humanoid (geometric fallback, no model hosted
yet): head/torso/arms/legs all populated with sensible proportions; JSON output
parses cleanly.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
EditModeController::selectByPart() (Q_INVOKABLE): gathers the edit entity's
geometry, runs MeshSegmenter::predict to label each face (head/torso/arm/leg),
then selects all faces whose predicted part matches any currently-selected
face's part — or, when nothing is selected, the single largest predicted part
so one click is useful. Switches to Face mode, applies via selectFace() so the
existing viewport highlight + editSelectionChanged() refresh kick in, and
returns a status string (parts + face count, or why it couldn't run). Uses the
ONNX model when available, else the geometric fallback (status notes which).

QML: "Select by Part (AI)" button + status line in the Edit Mode tools panel
(qml/PropertiesPanel.qml), below the selection count.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- CLAUDE.md: `qtmesh segment` CLI usage + MeshSegmenter architecture entry.
- THIRD_PARTY_AI_MODELS.md: segmentation model section — synthetic
  bone-weight-derived labels (ShapeNet-Part/PartNet rejected as non-commercial),
  geometric fallback, hosting + override env vars.
- Version 3.12.0 → 3.13.0 (next after the released 3.12.0); doc refs synced.

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

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown

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 40 minutes. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ 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.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 79b16ea2-5f51-401d-b8ed-3cab7cde0408

📥 Commits

Reviewing files that changed from the base of the PR and between c528549 and 80814f6.

📒 Files selected for processing (10)
  • .gitignore
  • CLAUDE.md
  • THIRD_PARTY_AI_MODELS.md
  • scripts/export-meshseg-onnx.py
  • scripts/fetch-training-rigs.sh
  • scripts/mine-training-data.sh
  • src/AutoRig.cpp
  • src/AutoRig.h
  • src/CLIPipeline.cpp
  • src/EditModeController.cpp
📝 Walkthrough

Walkthrough

Adds MeshSegmenter part-segmentation APIs and implementations, wires them into CLI, MCP, and Edit Mode entry points, adds an ONNX export script and build/test support, and updates related docs plus 3.13.0 version references.

Changes

Mesh segmentation feature

Layer / File(s) Summary
MeshSegmenter contract and taxonomy
src/MeshSegmenter.h
Defines Part, Options, Result, and prediction/model helper APIs, plus part naming, bone-name mapping, and model-path helpers.
Segmentation algorithms and tests
src/MeshSegmenter.cpp, src/MeshSegmenter_test.cpp, src/CMakeLists.txt, tests/CMakeLists.txt, scripts/export-meshseg-onnx.py
Implements connected components, face-label voting, geometric fallback, model download/inference paths, and tests, and adds the model export/build wiring.
CLI, MCP, and Edit Mode integration
src/AppLaunchHandler.cpp, src/CLIPipeline.h, src/CLIPipeline.cpp, src/MCPServer.h, src/MCPServer.cpp, src/EditModeController.h, src/EditModeController.cpp, qml/PropertiesPanel.qml
Registers segment and segment_mesh, adds the Edit Mode selection workflow and progress/cancel UI, and routes all three entry points through MeshSegmenter.
Docs and version references
CMakeLists.txt, README.md, website/src/hooks/useQtmeshActionRef.js, CLAUDE.md, THIRD_PARTY_AI_MODELS.md
Updates feature docs, model attribution, and 3.13.0 version references across project, README, website fallback, and AI model notes.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

Poem

🐇 I hopped through meshes, ears held high,
Found heads and paws as I zipped by.
With ONNX or fallback, the labels will gleam,
In CLI, MCP, and UI — a tidy dream.
Thump! 3.13.0 shines bright today.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 29.23% 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 clearly names the main feature and implementation approach, matching the PR's mesh segmentation change.
Description check ✅ Passed The description covers the feature, technical details, surfaces, model hosting, and acceptance criteria, though it doesn't follow the template headings exactly.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ai-segment-410

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.

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

Copy link
Copy Markdown

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: 589babb555

ℹ️ 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/MeshSegmenter.cpp
std::vector<float> pts(static_cast<size_t>(N) * 3);
std::vector<int> srcVert(N);
for (int i = 0; i < N; ++i) {
const int v = (vertexCount >= N) ? (i < vertexCount ? i : pick(rng)) : pick(rng);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Sample ONNX points across the whole mesh

When the ONNX path runs, this branch samples randomly with replacement for meshes smaller than the default 4096 points, then the scatter path below copies only the first vertexCount sampled labels, leaving any unsampled vertices at the default torso label; for meshes larger than N, it samples only vertices [0, N) instead of the whole mesh. This makes model-backed segmentation depend on vertex order and leaves many vertices mislabeled, so sample every vertex once when vertexCount <= N and downsample across the full [0, vertexCount) range when it is larger.

Useful? React with 👍 / 👎.

Comment thread src/MeshSegmenter.cpp Outdated
Comment on lines +337 to +339
pts[3*i+0] = (positions[3*v+0]-centre[0])*inv;
pts[3*i+1] = (positions[3*v+1]-centre[1])*inv;
pts[3*i+2] = (positions[3*v+2]-centre[2])*inv;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Apply the requested up axis before ONNX inference

--up-axis is parsed into opts, but the ONNX input tensor always receives raw x/y/z coordinates here. When a model is present, a Z-up or X-up mesh passed with qtmesh segment --up-axis z is still inferred as if Y were up, so the advertised axis correction only works in the geometric fallback; remap the coordinates to the model's expected up axis before filling pts or explicitly fall back/reject non-Y-up model inference.

Useful? React with 👍 / 👎.

Comment thread src/MeshSegmenter.cpp
// upN in [0,1] along up axis; lateral = signed offset from mid (>0 one side).
if (upN > 0.82f) return Part::Head;
if (upN < 0.45f) { // lower body → legs
return lateral >= 0.0f ? Part::RightLeg : Part::LeftLeg;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Map positive lateral coordinates to left limbs

For the default +Y humanoid case, side is X and the existing auto-rig convention in this repo places the model's left side at +X, but this check assigns positive lateral values to RightLeg (and the same sign convention is used for arms below). The geometric fallback therefore swaps left/right labels on upright humanoids whenever the fallback is used, causing select-by-part and any downstream priors to target the wrong side.

Useful? React with 👍 / 👎.

Trained the mesh part-segmentation model on SYNTHETIC, permissively-derived data
(ShapeNet-Part/PartNet are non-commercial, so rejected — same wall as #408/#409):
procedurally-generated humanoids from primitive parts (head sphere, torso box,
four limb capsules) in randomised proportions/poses/orientations, where each
point's label is known by construction (CC0 — we generate it). A small
PointNet-style segmenter learns the humanoid configuration and transfers to real
meshes.

- scripts/export-meshseg-onnx.py (one-time, offline, NOT shipped): synthesises
  the data → trains → exports meshseg.onnx ([1,N,3]→[1,N,7]).
- Trained to 98% per-point accuracy on held-out synthetic; validated on a real
  Mixamo character via the C++ runtime — labels into symmetric head/torso/arms/
  legs ("via model"), an improvement over the geometric fallback's lopsided legs.
- Hosted meshseg.onnx (~0.3 MB) at fernandotonon/QtMeshEditor-models/segment/;
  the ensureModelBlocking() URL now resolves, so the ML path runs on first use.
- THIRD_PARTY_AI_MODELS.md updated (architecture, validation, hosting).

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 14

🤖 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 1963-1975: The new “Select by Part (AI)” control in
PropertiesPanel.qml is mouse-only because it is implemented as a bare Rectangle
with a MouseArea. Update this control to follow the existing keyboard-accessible
button pattern used elsewhere in PropertiesPanel.qml, or replace it with a
Button, so it can receive tab focus and activate on Enter/Space while still
calling EditModeController.selectByPart() and updating
editToolsCol.selectByPartStatus.

In `@scripts/export-meshseg-onnx.py`:
- Around line 100-105: Reject invalid training arguments immediately in
export-meshseg-onnx.py by validating the parsed values from parse_args before
the training loop starts. In the main flow around ap.parse_args and the
subsequent training/evaluation logic that computes loss, require --samples to be
at least 2 and --epochs to be greater than 0, and fail fast with a clear message
if not. This should be handled near the argument parsing and loop setup so the
code path that references loss cannot run with an empty training batch or an
untrained model.
- Around line 104-136: The `--seed` value is only applied to NumPy, so
`PointSeg` initialization and the `torch.randperm` training order remain
nondeterministic. In `export-meshseg-onnx.py`, use the parsed seed in the main
setup before creating `PointSeg` and the training loop to seed PyTorch’s RNG
(and any backend-specific RNGs if available), so repeated runs with the same
`a.seed` produce the same exported weights.
- Around line 77-84: Apply Options::upAxis in the ONNX preprocessing path inside
the mesh normalization/augmentation logic before inference so the model sees the
mesh in the correct frame. Update the code around the point where P is rotated
and centered to remap coordinates based on upAxis for both the ONNX branch and
the geometric fallback, ensuring raw XYZ is not passed through unchanged for
x/z-up meshes.

In `@src/CLIPipeline.cpp`:
- Around line 8440-8446: The --up-axis handling in CLIPipeline’s argument
parsing should reject a missing value instead of silently continuing with the
default axis. Update the branch that checks arg == "--up-axis" to validate that
the next argv entry exists before consuming it, and if it is missing, emit a
usage/error message and return a nonzero exit code rather than falling through.
Keep the existing validation logic for valid values in the same parsing block.

In `@src/EditModeController.cpp`:
- Around line 816-820: The polygon-selection loop in
EditModeController::selectFacesByLabel is using triangle hits and then calling
selectFace(), which expands selection to the entire owning n-gon and can
over-select neighboring triangles with different labels. Change this path to
select by polygon/face label directly using the owning face or polygon index
from the raycast/label data instead of per-triangle hits, and make selectedCount
reflect the actual number of polygons selected rather than triangle hits.

In `@src/EditModeController.h`:
- Around line 642-649: Expose the mesh up-axis in the EditModeController
selection path by updating selectByPart() so the GUI can pass an explicit
up-axis into MeshSegmenter instead of always relying on the default +Y. Thread
this through the EditModeController interface and the call site(s) that invoke
selectByPart(), using the same up-axis source already used by the CLI/MCP path
so GUI and CLI stay in parity for X/Z-up assets.

In `@src/MCPServer.cpp`:
- Around line 3389-3401: `toolSegmentMesh()` is returning only a human-readable
summary and discarding the actual segmentation data; update the result
construction in `MCPServer::toolSegmentMesh` to include `r.vertexLabels` and
`r.faceLabels` in a structured payload, while still keeping the summary if
needed. Ensure the MCP response exposes the label map programmatically so
clients can consume per-vertex/per-face labels instead of parsing text.
- Around line 6459-6475: The segment_mesh MCP contract is missing the up_axis
option, so toolSegmentMesh() always uses the default +Y orientation and can
mislabel X/Z-up meshes. Update the segment_mesh schema and appendTool metadata
to expose an up_axis property alongside entity_name and no_model, then thread
that value through toolSegmentMesh so it overrides opts.upAxis when provided,
keeping the MCP surface in parity with the CLI.

In `@src/MeshSegmenter.cpp`:
- Line 266: The first-use model download in MeshSegmenter currently starts
without a Sentry breadcrumb, so add one immediately before the dl->startDownload
call using SentryReporter::addBreadcrumb with an appropriate category and
message. Update the MeshSegmenter flow that handles the blocking external
download so direct API callers get a breadcrumb describing the download action,
keeping the breadcrumb aligned with this user-facing operation.
- Around line 346-356: Replace the platform guards in MeshSegmenter’s session
setup with the project’s Qt macros, using Q_OS_MACOS for the CoreML block and
Q_OS_WIN for the Windows-specific path/session branch instead of __APPLE__ and
_WIN32. Keep the existing logic in the same MeshSegmenter initialization code,
only updating the conditional compilation symbols so the platform-specific
sections follow the project guideline.
- Around line 381-398: The ONNX logits handling in MeshSegmenter should validate
the output shape before indexing, because the current elems check alone still
allows unsupported tensors like [1, N] to be read out of bounds. Update the
logic around outputs[0].GetTensorTypeAndShapeInfo(), oshape, chan, and
channelsLast to explicitly accept only the expected per-point logits layouts
([1, N, C] or [1, C, N]) and reject anything else by routing to fallback()
before the loop that reads d[c*N+i]. Ensure the indexing in the argmax loop only
runs after this shape validation.
- Around line 181-185: The bone hint override in MeshSegmenter::segmentMesh is
too aggressive: invalid or unknown entries in boneProximity currently force
r.vertexLabels[v] to Part::Torso, which erases the geometric label. Update the
haveBone loop in MeshSegmenter.cpp so only valid bone hints (within Part::Count)
overwrite an existing label, and leave r.vertexLabels[v] unchanged for -1 or
other invalid values.
- Around line 328-412: The ONNX sampling/scatter logic in MeshSegmenter’s
segmentation path is not covering vertices consistently: the sampling loop fills
srcVert and point labels in a way that leaves many vertices defaulting to Torso
when vertexCount and N differ. Update the sampling policy and the scatter-back
step together so every vertex is either sampled or assigned by nearest sampled
point, and do not assume the first vertexCount entries form a 1:1 cover; fix the
logic around the sampling loop and the vertexLabels assignment/nearest-neighbor
fallback in the same function.
🪄 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: 5b0af4c0-9b72-4041-beb6-6633b0be81fe

📥 Commits

Reviewing files that changed from the base of the PR and between df53b58 and 7ab44f9.

📒 Files selected for processing (19)
  • CLAUDE.md
  • CMakeLists.txt
  • README.md
  • THIRD_PARTY_AI_MODELS.md
  • qml/PropertiesPanel.qml
  • scripts/export-meshseg-onnx.py
  • src/AppLaunchHandler.cpp
  • src/CLIPipeline.cpp
  • src/CLIPipeline.h
  • src/CMakeLists.txt
  • src/EditModeController.cpp
  • src/EditModeController.h
  • src/MCPServer.cpp
  • src/MCPServer.h
  • src/MeshSegmenter.cpp
  • src/MeshSegmenter.h
  • src/MeshSegmenter_test.cpp
  • tests/CMakeLists.txt
  • website/src/hooks/useQtmeshActionRef.js

Comment thread qml/PropertiesPanel.qml
Comment thread scripts/export-meshseg-onnx.py Outdated
Comment thread scripts/export-meshseg-onnx.py Outdated
Comment on lines +100 to +105
ap.add_argument("--samples", type=int, default=4000)
ap.add_argument("--points", type=int, default=4096)
ap.add_argument("--epochs", type=int, default=40)
ap.add_argument("--out", default="meshseg.onnx")
ap.add_argument("--seed", type=int, default=0)
a = ap.parse_args()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Reject degenerate training arguments up front.

--samples 1 leaves no training batch, so Line 147 can read loss before assignment. --epochs 0 also exports an untrained random model.

Proposed fix
     ap.add_argument("--seed", type=int, default=0)
     a = ap.parse_args()
+    if a.samples < 2:
+        ap.error("--samples must be at least 2")
+    if a.points < 1:
+        ap.error("--points must be at least 1")
+    if a.epochs < 1:
+        ap.error("--epochs must be at least 1")
 
     import torch

Also applies to: 134-147

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/export-meshseg-onnx.py` around lines 100 - 105, Reject invalid
training arguments immediately in export-meshseg-onnx.py by validating the
parsed values from parse_args before the training loop starts. In the main flow
around ap.parse_args and the subsequent training/evaluation logic that computes
loss, require --samples to be at least 2 and --epochs to be greater than 0, and
fail fast with a clear message if not. This should be handled near the argument
parsing and loop setup so the code path that references loss cannot run with an
empty training batch or an untrained model.

Comment on lines +104 to +136
ap.add_argument("--seed", type=int, default=0)
a = ap.parse_args()

import torch
import torch.nn as nn

print("generating synthetic data…")
P, L = gen_dataset(a.samples, a.points, a.seed)
P = torch.tensor(P); L = torch.tensor(L)
n = P.shape[0]; N = P.shape[1]
nval = max(1, n // 10)
dev = "mps" if torch.backends.mps.is_available() else "cpu"
print(f"data N={n} points={N} dev={dev}")

class PointSeg(nn.Module):
def __init__(s, d=128):
super().__init__()
s.mlp1 = nn.Sequential(nn.Linear(3,64), nn.GELU(), nn.Linear(64,d), nn.GELU())
s.mlp2 = nn.Sequential(nn.Linear(d,d), nn.GELU(), nn.Linear(d,d), nn.GELU())
s.head = nn.Sequential(nn.Linear(d+d,d), nn.GELU(), nn.Linear(d,C))
def forward(s, pts):
f = s.mlp1(pts); g = s.mlp2(f)
glob = g.max(dim=1, keepdim=True).values.expand(-1, g.shape[1], -1)
return s.head(torch.cat([f, glob], dim=-1))

net = PointSeg().to(dev)
opt = torch.optim.AdamW(net.parameters(), lr=2e-3, weight_decay=1e-4)
sch = torch.optim.lr_scheduler.CosineAnnealingLR(opt, a.epochs)
lossf = nn.CrossEntropyLoss()
bs = 32
for ep in range(a.epochs):
net.train()
perm = torch.randperm(n - nval) + nval

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- search other scripts for seeding patterns ---'
rg -n "manual_seed|randperm|seed" scripts -g '*.py' || true

Repository: fernandotonon/QtMeshEditor

Length of output: 754


Seed PyTorch as well

--seed only reaches np.random.default_rng; the model init and torch.randperm still use PyTorch’s default RNG, so rerunning with the same command can produce different exported weights.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/export-meshseg-onnx.py` around lines 104 - 136, The `--seed` value is
only applied to NumPy, so `PointSeg` initialization and the `torch.randperm`
training order remain nondeterministic. In `export-meshseg-onnx.py`, use the
parsed seed in the main setup before creating `PointSeg` and the training loop
to seed PyTorch’s RNG (and any backend-specific RNGs if available), so repeated
runs with the same `a.seed` produce the same exported weights.

Comment thread src/CLIPipeline.cpp Outdated
Comment thread src/MeshSegmenter.cpp Outdated
Comment thread src/MeshSegmenter.cpp
QObject::connect(&timeout, &QTimer::timeout, &loop, [&]() { timedOut = true; loop.quit(); });
timeout.start(300000); // 5 min — the model is small (~MBs)

dl->startDownload(url, dest, label);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a breadcrumb for the first-use model download.

This starts a blocking external model download, but the operation itself has no Sentry breadcrumb for direct API callers. As per coding guidelines, src/**/*.cpp: “All user-facing actions and significant operations must add a Sentry breadcrumb via SentryReporter::addBreadcrumb(category, message).”

🤖 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/MeshSegmenter.cpp` at line 266, The first-use model download in
MeshSegmenter currently starts without a Sentry breadcrumb, so add one
immediately before the dl->startDownload call using
SentryReporter::addBreadcrumb with an appropriate category and message. Update
the MeshSegmenter flow that handles the blocking external download so direct API
callers get a breadcrumb describing the download action, keeping the breadcrumb
aligned with this user-facing operation.

Source: Coding guidelines

Comment thread src/MeshSegmenter.cpp
Comment thread src/MeshSegmenter.cpp
Comment on lines +346 to +356
#ifdef __APPLE__
try { std::unordered_map<std::string,std::string> c; so.AppendExecutionProvider("CoreML", c); }
catch (const Ort::Exception&) {}
#endif
#ifdef _WIN32
const std::wstring wpath = modelPath.toStdWString();
Ort::Session session(env, wpath.c_str(), so);
#else
const std::string p = modelPath.toStdString();
Ort::Session session(env, p.c_str(), so);
#endif

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use the project’s Qt platform macros.

These platform guards use __APPLE__ and _WIN32; the project guideline requires Q_OS_MACOS / Q_OS_WIN for platform-specific code. As per coding guidelines, platform-specific code must be guarded with the appropriate macros.

Suggested fix
-#ifdef __APPLE__
+#ifdef Q_OS_MACOS
         try { std::unordered_map<std::string,std::string> c; so.AppendExecutionProvider("CoreML", c); }
         catch (const Ort::Exception&) {}
 `#endif`
-#ifdef _WIN32
+#ifdef Q_OS_WIN
         const std::wstring wpath = modelPath.toStdWString();
         Ort::Session session(env, wpath.c_str(), so);
📝 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
#ifdef __APPLE__
try { std::unordered_map<std::string,std::string> c; so.AppendExecutionProvider("CoreML", c); }
catch (const Ort::Exception&) {}
#endif
#ifdef _WIN32
const std::wstring wpath = modelPath.toStdWString();
Ort::Session session(env, wpath.c_str(), so);
#else
const std::string p = modelPath.toStdString();
Ort::Session session(env, p.c_str(), so);
#endif
`#ifdef` Q_OS_MACOS
try { std::unordered_map<std::string,std::string> c; so.AppendExecutionProvider("CoreML", c); }
catch (const Ort::Exception&) {}
`#endif`
`#ifdef` Q_OS_WIN
const std::wstring wpath = modelPath.toStdWString();
Ort::Session session(env, wpath.c_str(), so);
`#else`
const std::string p = modelPath.toStdString();
Ort::Session session(env, p.c_str(), so);
`#endif`
🤖 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/MeshSegmenter.cpp` around lines 346 - 356, Replace the platform guards in
MeshSegmenter’s session setup with the project’s Qt macros, using Q_OS_MACOS for
the CoreML block and Q_OS_WIN for the Windows-specific path/session branch
instead of __APPLE__ and _WIN32. Keep the existing logic in the same
MeshSegmenter initialization code, only updating the conditional compilation
symbols so the platform-specific sections follow the project guideline.

Source: Coding guidelines

Comment thread src/MeshSegmenter.cpp
Comment on lines +381 to +398
// Output is per-point logits [1, N, C] (or [1, C, N]); argmax → label.
auto ti = outputs[0].GetTensorTypeAndShapeInfo();
const auto oshape = ti.GetShape();
const size_t elems = ti.GetElementCount();
const int C = static_cast<int>(Part::Count);
if (elems < static_cast<size_t>(N))
return fallback(QStringLiteral("Segmentation output too small — used the geometric fallback."));
const int chan = (!oshape.empty() && oshape.back() > 0)
? static_cast<int>(oshape.back()) : C;
const bool channelsLast = (chan == C);
const float* d = outputs[0].GetTensorData<float>();

std::vector<int> pointLabel(N, static_cast<int>(Part::Torso));
for (int i = 0; i < N; ++i) {
int best = 0; float bestv = -1e30f;
for (int c = 0; c < C; ++c) {
const float val = channelsLast ? d[static_cast<size_t>(i)*chan + c]
: d[static_cast<size_t>(c)*N + i];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Validate the ONNX logits shape before indexing.

The guard only checks elems >= N; an output like [1, N] makes channelsLast == false, then Line 398 reads d[c*N+i] past the tensor. Reject unsupported shapes before indexing so bad/custom models fall back instead of hitting UB.

Suggested fix
         const int C = static_cast<int>(Part::Count);
-        if (elems < static_cast<size_t>(N))
-            return fallback(QStringLiteral("Segmentation output too small — used the geometric fallback."));
-        const int chan = (!oshape.empty() && oshape.back() > 0)
-                             ? static_cast<int>(oshape.back()) : C;
-        const bool channelsLast = (chan == C);
+        const size_t needed = static_cast<size_t>(N) * static_cast<size_t>(C);
+        if (elems < needed)
+            return fallback(QStringLiteral("Segmentation output too small — used the geometric fallback."));
+        const bool channelsLast = oshape.size() >= 2 && oshape.back() == C;
+        const bool channelsFirst = oshape.size() >= 2
+            && oshape[oshape.size() - 2] == C
+            && oshape.back() == N;
+        if (!channelsLast && !channelsFirst)
+            return fallback(QStringLiteral("Segmentation output shape is unsupported — used the geometric fallback."));
         const float* d = outputs[0].GetTensorData<float>();
@@
-                const float val = channelsLast ? d[static_cast<size_t>(i)*chan + c]
+                const float val = channelsLast ? d[static_cast<size_t>(i)*C + c]
                                                : d[static_cast<size_t>(c)*N + i];
📝 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
// Output is per-point logits [1, N, C] (or [1, C, N]); argmax → label.
auto ti = outputs[0].GetTensorTypeAndShapeInfo();
const auto oshape = ti.GetShape();
const size_t elems = ti.GetElementCount();
const int C = static_cast<int>(Part::Count);
if (elems < static_cast<size_t>(N))
return fallback(QStringLiteral("Segmentation output too small — used the geometric fallback."));
const int chan = (!oshape.empty() && oshape.back() > 0)
? static_cast<int>(oshape.back()) : C;
const bool channelsLast = (chan == C);
const float* d = outputs[0].GetTensorData<float>();
std::vector<int> pointLabel(N, static_cast<int>(Part::Torso));
for (int i = 0; i < N; ++i) {
int best = 0; float bestv = -1e30f;
for (int c = 0; c < C; ++c) {
const float val = channelsLast ? d[static_cast<size_t>(i)*chan + c]
: d[static_cast<size_t>(c)*N + i];
// Output is per-point logits [1, N, C] (or [1, C, N]); argmax → label.
auto ti = outputs[0].GetTensorTypeAndShapeInfo();
const auto oshape = ti.GetShape();
const size_t elems = ti.GetElementCount();
const int C = static_cast<int>(Part::Count);
const size_t needed = static_cast<size_t>(N) * static_cast<size_t>(C);
if (elems < needed)
return fallback(QStringLiteral("Segmentation output too small — used the geometric fallback."));
const bool channelsLast = oshape.size() >= 2 && oshape.back() == C;
const bool channelsFirst = oshape.size() >= 2
&& oshape[oshape.size() - 2] == C
&& oshape.back() == N;
if (!channelsLast && !channelsFirst)
return fallback(QStringLiteral("Segmentation output shape is unsupported — used the geometric fallback."));
const float* d = outputs[0].GetTensorData<float>();
std::vector<int> pointLabel(N, static_cast<int>(Part::Torso));
for (int i = 0; i < N; ++i) {
int best = 0; float bestv = -1e30f;
for (int c = 0; c < C; ++c) {
const float val = channelsLast ? d[static_cast<size_t>(i)*C + c]
: d[static_cast<size_t>(c)*N + i];
🤖 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/MeshSegmenter.cpp` around lines 381 - 398, The ONNX logits handling in
MeshSegmenter should validate the output shape before indexing, because the
current elems check alone still allows unsupported tensors like [1, N] to be
read out of bounds. Update the logic around
outputs[0].GetTensorTypeAndShapeInfo(), oshape, chan, and channelsLast to
explicitly accept only the expected per-point logits layouts ([1, N, C] or [1,
C, N]) and reject anything else by routing to fallback() before the loop that
reads d[c*N+i]. Ensure the indexing in the argmax loop only runs after this
shape validation.

fernandotonon and others added 3 commits June 27, 2026 17:22
The Edit-Mode "Select by Part (AI)" action ran ensureModelBlocking() (first-use
model download) + ONNX inference synchronously on the UI thread, freezing the
app — especially on the first run while the model downloads.

Mirror the #408 UniRig worker pattern:
- EditModeController::selectByPart now gathers geometry on the main thread
  (Ogre buffer locks), then runs the download + predict on a std::thread and
  applies the face selection back on the main thread via finishSegmentOnMain().
- New segmentBusy / segmentDownloading / segmentProgress(/Total) properties +
  segmentProgressChanged / segmentFinished(status,isError) signals, and
  cancelSegment() (flips a shared atomic the predict ProgressFn checks).
- PropertiesPanel.qml: button disables + shows "Segmenting…" while busy; a
  progress block shows the "Downloading model…" phase, a determinate inference
  bar, and a Cancel button; the final status comes from segmentFinished.

CLI/MCP paths are unchanged (headless — blocking is fine there).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…++ local feats

Improve "Select by Part" quality, especially on non-humanoid meshes (the cat:
its ears were being grabbed as "arm" by the coordinate-only model).

Rig-prior fast path (the real fix for rigged meshes):
- MeshSegmenter::partForBoneName maps a skeleton bone name → a segmentation part,
  handling NON-human anatomy the humanoid model can't: ears/snout/muzzle/jaw →
  head, tail → torso, paws/hooves/toes → leg, wings/fingers → arm. Guards the
  "ear" ⊂ "forEARm" substring collision. Exhaustively unit-tested.
- EditModeController::selectByPart now, when the mesh is SKINNED, labels each
  vertex by the part of the bone it's most-weighted to (exact), in
  gatherGeometry's vertex order (shared then per-submesh). When that covers ≥70%
  of vertices it's used directly — no model, no download, instant — and the
  status reads "(from rig)". Unrigged / sparse-rig meshes fall through to the
  worker (model / geometric fallback).

ML fallback model improvements (export-meshseg-onnx.py, already retrained +
re-hosted as v1.1.0):
- T/A-pose arms (horizontal — the real rigged-character case) instead of
  always-down; upright + yaw-only augmentation (consistent left/right);
  head-protrusion samples; wider proportions; per-point jitter.
- PointNet++-style local kNN feature added to the net so adjacent parts
  separate. 99% per-part on a held-out T-pose test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeRabbit review on PR #771:

- MeshSegmenter::predict (ONNX): apply Options::upAxis. The model is trained
  on +Y-up point clouds; an x/z-up mesh was fed raw XYZ and segmented in the
  wrong frame (head/legs mislabelled). Now remap the sampled coords so the up
  axis lands on Y before inference. The geometric fallback already honoured it.
- MeshSegmenter::segmentGeometric: run the spatial classification for ALL
  vertices first, then let only VALID bone-proximity hints override. An unknown
  hint (-1) no longer collapses unhinted vertices to Torso.
- CLIPipeline (segment + rig): `--up-axis` with a missing value now errors
  instead of silently falling through to the default.
- MCPServer toolSegmentMesh: return the full label map as JSON (per-part
  vertex+face counts, per-face label array) instead of just a prose sentence,
  and accept an `up_axis` arg; schema updated with the `up_axis` enum prop.
- EditModeController::selectByPart: accept an up-axis arg (GUI parity with
  CLI/MCP); count DISTINCT polygons and de-dup the n-gon expansion so the
  overlay rebuild + signal fire ONCE instead of per-triangle.
- PropertiesPanel.qml "Select by Part" button: keyboard-accessible
  (activeFocusOnTab, Space/Return activation, Accessible role/name, focus ring).

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

Copy link
Copy Markdown
Owner Author

Thanks @coderabbitai — addressed all findings in ca38ce8:

  1. Up-axis in the ONNX path (key one). You were right that Options::upAxis only affected the geometric fallback — the ML branch fed raw XYZ, so x/z-up meshes were segmented in the wrong frame. Fixed in MeshSegmenter::predict(): the sampled point cloud is now axis-remapped so the mesh up axis lands on +Y (the frame the model was trained on) before inference. (The fix is in the C++ preprocessing, not the export script — the model contract stays +Y-up.)
  2. segmentGeometric bone-proximity. Now runs the spatial classification for all vertices first, then lets only valid bone hints (bp >= 0) override. An unknown hint (-1) keeps the spatial label instead of collapsing to Torso.
  3. CLI --up-axis missing value. Both the segment and rig handlers now error (requires a value) instead of silently falling through to the default.
  4. MCP segment_mesh return. Now returns the full label map as JSON — per-part vertex+face counts, the per-face label array, used-model/fallback flag, and the resolved up-axis — alongside the summary. Added the up_axis enum prop to the schema.
  5. Edit-Mode parity + perf. selectByPart accepts an up-axis arg (CLI/MCP parity) and now counts distinct polygons, de-duping the n-gon expansion so the overlay rebuild + editSelectionChanged fire once rather than once per triangle.
  6. QML button a11y. "Select by Part" is now keyboard-accessible: activeFocusOnTab, Space/Return activation, Accessible.role/name, and a focus ring.

Built (GUI + UnitTests) locally on macOS arm64; CI will run the suite under Xvfb.

@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@fernandotonon Thanks for the thorough follow-up — these changes address my findings. The +Y-up remap in MeshSegmenter::predict(), the boneProximity >= 0 override guard in segmentGeometric, and the CLI/MCP/Edit Mode parity fixes were the key pieces I was looking for. Nice touch on the QML accessibility improvements too, and thanks for validating with a local GUI + UnitTests build.

🐇✨


✏️ Learnings added
Learnt from: fernandotonon
URL: https://github.com/fernandotonon/QtMeshEditor/pull/771

Timestamp: 2026-06-28T02:40:45.954Z
Learning: In `src/MeshSegmenter.cpp`, `MeshSegmenter::predict()` keeps the ONNX model contract as +Y-up and remaps sampled mesh coordinates from the user-selected `Options::upAxis` into +Y during C++ preprocessing before inference.

Learnt from: fernandotonon
URL: https://github.com/fernandotonon/QtMeshEditor/pull/771

Timestamp: 2026-06-28T02:40:45.954Z
Learning: In `src/MeshSegmenter.cpp`, `MeshSegmenter::segmentGeometric()` first computes spatial labels for all vertices and only overrides them with bone hints when `boneProximity >= 0`; a `-1` hint means unknown and preserves the spatial classification.

Learnt from: fernandotonon
URL: https://github.com/fernandotonon/QtMeshEditor/pull/771

Timestamp: 2026-06-28T02:40:45.954Z
Learning: In `src/MCPServer.cpp`, `MCPServer::toolSegmentMesh()` now returns structured segmentation JSON including per-part vertex and face counts, per-face labels, model-vs-fallback metadata, and the resolved `up_axis`, and its tool schema exposes an `up_axis` enum argument.

Learnt from: fernandotonon
URL: https://github.com/fernandotonon/QtMeshEditor/pull/771

Timestamp: 2026-06-28T02:40:45.954Z
Learning: In `src/EditModeController.cpp`, `EditModeController::selectByPart()` accepts an up-axis argument and de-duplicates expanded n-gon triangles so selection counts and overlay rebuilds operate on distinct polygons rather than per-triangle expansions.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 6

♻️ Duplicate comments (1)
qml/PropertiesPanel.qml (1)

1993-1998: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Pass the selected up-axis into selectByPart().

This still calls EditModeController.selectByPart() with the default "y", so the Edit Mode path remains wrong for X/Z-up assets even though CLI/MCP already expose axis control. This file already has root.rigUpAxes[root.rigUpAxisIndex], so the existing selection can be threaded through here.

Suggested fix
-                    EditModeController.selectByPart()
+                    EditModeController.selectByPart(
+                        root.rigUpAxes[root.rigUpAxisIndex])

As per coding guidelines, "When adding new major user-facing features or commands, ensure the corresponding CLI/MCP/GUI surfaces are kept in parity where that feature already has multiple entry points."

🤖 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 1993 - 1998, The Edit Mode activation
path in PropertiesPanel.qml still calls EditModeController.selectByPart()
without the selected up-axis, so it defaults to y and breaks X/Z-up assets.
Update the activate() flow to pass the current axis from
root.rigUpAxes[root.rigUpAxisIndex] into EditModeController.selectByPart(),
keeping the GUI in parity with the CLI/MCP axis-aware behavior.

Source: Coding guidelines

🧹 Nitpick comments (1)
src/MeshSegmenter.cpp (1)

43-58: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Make side detection token-aware before stripping separators.

After separators are removed, startsWith('l') / endsWith('l') and the equivalent r checks misclassify common center bones such as tail, lowerback, root, and ribcage, so the torso rules at Lines 96-98 are skipped and these rig hints become Unknown. This also fails common Bip01 L UpperArm-style names because the bip01 prefix is not stripped before side inference.

🤖 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/MeshSegmenter.cpp` around lines 43 - 58, Update boneSide and the
name-normalization flow in MeshSegmenter.cpp so side detection works on tokens
before separators are stripped; the current prefix/suffix character checks in
boneSide misclassify center names like tail, lowerback, root, and ribcage, and
also miss names like Bip01 L UpperArm because the bip01 prefix is not removed
early enough. Adjust the normalization helper used by boneSide to remove known
prefixes first, then detect left/right using token-aware matching or word
boundaries instead of raw startsWith/endsWith single letters, so the torso rules
can still map these rig hints correctly.
🤖 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/EditModeController.cpp`:
- Around line 955-974: The batch-selection loop in EditModeController is still
triggering overlay rebuilds and editSelectionChanged() from selectFace() on
every polygon. Update selectFace() to take a notify flag (or equivalent) in both
its declaration and definition, and guard the final
updateSelectionOverlay()/signal emission block with it so the loop can call
selectFace(..., false) while preserving a single overlay/signal update after the
batch.
- Around line 858-899: The async segmentation completion path in
EditModeController::segment should not apply results after cancellation, because
MeshSegmenter::predict may return an OK "cancelled" fallback that still reaches
finishSegmentOnMain(). Update the thread lambda to track cancellation explicitly
via the existing cancel flag and, before invoking the main-thread completion
handler, short-circuit if cancelled so m_segmentBusy/m_segmentDownloading are
cleared and segmentFinished reports cancellation instead of mutating selection.
Keep the check near the MeshSegmenter::predict result handling and the
QMetaObject::invokeMethod callback.
- Around line 992-994: `EditModeController::cancelSegment()` is a user-facing
cancellation action but currently only flips `m_segmentCancel` without recording
telemetry. Update this method to add a Sentry breadcrumb via
`SentryReporter::addBreadcrumb(category, message)` before or when the
cancellation is requested, using a cancellation-related category/message, while
keeping the existing `m_segmentCancel->store(true)` behavior unchanged.

In `@src/EditModeController.h`:
- Around line 668-669: Add a Sentry breadcrumb to the new cancel flow so the
user-facing action is visible in telemetry. Update
EditModeController::cancelSegment in EditModeController.cpp to call
SentryReporter::addBreadcrumb with the appropriate category and a message
describing the cancel action before or alongside the atomic flag update. Keep
the existing cancel behavior intact while ensuring the breadcrumb is emitted on
every call.

In `@src/MCPServer.cpp`:
- Around line 6505-6516: Update the `segment_mesh` tool schema description for
`up_axis` so it clearly states this axis is used by both
`MeshSegmenter::predict()` model inference and the fallback geometric heuristic,
not just the fallback path. Keep the wording aligned with `toolSegmentMesh()`
behavior so MCP clients know to supply the correct axis for X/Z-up meshes
regardless of whether ONNX is available.

In `@src/MeshSegmenter.cpp`:
- Around line 432-435: The nearest-point scatter in MeshSegmenter is comparing
sampled points stored with the axis remap from axisFor against vertex positions
without that remap, which can pick the wrong nearest sample for X-up/Z-up
meshes. Update the scatter logic to apply the same axis mapping consistently
when computing distances and assigning labels, using the same axisFor-based
coordinate transform used when filling pts so the nearest-neighbor search
operates in the same space.

---

Duplicate comments:
In `@qml/PropertiesPanel.qml`:
- Around line 1993-1998: The Edit Mode activation path in PropertiesPanel.qml
still calls EditModeController.selectByPart() without the selected up-axis, so
it defaults to y and breaks X/Z-up assets. Update the activate() flow to pass
the current axis from root.rigUpAxes[root.rigUpAxisIndex] into
EditModeController.selectByPart(), keeping the GUI in parity with the CLI/MCP
axis-aware behavior.

---

Nitpick comments:
In `@src/MeshSegmenter.cpp`:
- Around line 43-58: Update boneSide and the name-normalization flow in
MeshSegmenter.cpp so side detection works on tokens before separators are
stripped; the current prefix/suffix character checks in boneSide misclassify
center names like tail, lowerback, root, and ribcage, and also miss names like
Bip01 L UpperArm because the bip01 prefix is not removed early enough. Adjust
the normalization helper used by boneSide to remove known prefixes first, then
detect left/right using token-aware matching or word boundaries instead of raw
startsWith/endsWith single letters, so the torso rules can still map these rig
hints correctly.
🪄 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: 55d25f8a-c71a-4d1f-b8f7-9d89ba0d29e7

📥 Commits

Reviewing files that changed from the base of the PR and between 7ab44f9 and ca38ce8.

📒 Files selected for processing (9)
  • qml/PropertiesPanel.qml
  • scripts/export-meshseg-onnx.py
  • src/CLIPipeline.cpp
  • src/EditModeController.cpp
  • src/EditModeController.h
  • src/MCPServer.cpp
  • src/MeshSegmenter.cpp
  • src/MeshSegmenter.h
  • src/MeshSegmenter_test.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/MeshSegmenter.h
  • src/CLIPipeline.cpp

Comment thread src/EditModeController.cpp
Comment thread src/EditModeController.cpp
Comment thread src/EditModeController.cpp
Comment thread src/EditModeController.h
Comment thread src/MCPServer.cpp Outdated
Comment thread src/MeshSegmenter.cpp
… scatter

CodeRabbit follow-up review on the review-fix commit:

- MeshSegmenter::predict (ONNX scatter): apply the SAME up-axis remap to the
  per-vertex nearest-point comparison. The sampled points are in model
  (+Y-up) coordinates but the scatter compared them against raw vertex order
  — for X/Z-up meshes with vertexCount > N this assigned labels from the
  wrong nearest sample.
- EditModeController::selectByPart completion: a cancelled run can still
  return an OK "cancelled" fallback from predict(); the main-thread handler
  now short-circuits on cancel (flag OR fallbackReason) and reports
  cancellation instead of mutating the selection.
- EditModeController::selectFace: add a `notify` param (default true). The
  select-by-part batch loop now passes notify=false per polygon and rebuilds
  the overlay + emits editSelectionChanged() ONCE — previously every polygon
  triggered a full overlay rebuild (the de-dup landed but the per-call notify
  did not).
- EditModeController::cancelSegment: add the ai.assist.segment Sentry
  breadcrumb (user-facing cancel action was invisible to telemetry).
- MCPServer segment_mesh schema: clarify up_axis affects the ML model too
  (point cloud is remapped to the +Y-up training frame), not just the
  geometric fallback.

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

Copy link
Copy Markdown
Owner Author

Thanks @coderabbitai — second round addressed in 3ccbd2f:

  1. ONNX scatter axis mismatch (Major) — good catch. The nearest-point scatter compared model-frame sampled points against raw-order vertex coords, so X/Z-up meshes with vertexCount > N could pull labels from the wrong sample. The per-vertex comparison now applies the same axisFor remap.
  2. Apply after cancel (Major) — the completion handler now short-circuits when cancel is set OR r.fallbackReason == "cancelled", reporting cancellation instead of mutating the selection.
  3. Per-polygon overlay rebuild (Major) — you're right, the de-dup landed but selectFace still notified per call. Added a notify param (default true); the batch loop passes notify=false and rebuilds the overlay + emits editSelectionChanged() once afterward.
  4. Cancel breadcrumb (Major/Minor)cancelSegment() now records the ai.assist.segment breadcrumb.
  5. MCP up_axis schema wording (Minor) — clarified it affects the ML model too (point cloud remapped to the +Y-up training frame), so clients set it on model-backed calls.

Built GUI + UnitTests clean on macOS arm64; CI re-running.

@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@fernandotonon Thanks — this second round addresses the remaining concerns cleanly.

The scatter-side axis remap fix in MeshSegmenter::predict(), the cancel short-circuit before applying selection, and the batched notify=false selection path were the big ones I was hoping to see. Nice follow-through as well on the cancel breadcrumb and the MCP schema wording clarification.

Great to hear GUI + UnitTests are still clean locally on macOS arm64.

🐇✨


✏️ Learnings added
Learnt from: fernandotonon
URL: https://github.com/fernandotonon/QtMeshEditor/pull/771

Timestamp: 2026-06-28T03:25:45.548Z
Learning: In `src/MeshSegmenter.cpp`, `MeshSegmenter::predict()` must use the same up-axis remap for nearest-point scatter back to full-resolution vertices as it uses for sampled points sent to the ONNX model; otherwise x-up or z-up meshes with `vertexCount > sampleCount` can receive labels from the wrong sample.

Learnt from: fernandotonon
URL: https://github.com/fernandotonon/QtMeshEditor/pull/771

Timestamp: 2026-06-28T03:25:45.548Z
Learning: In `src/EditModeController.cpp`, the segmentation completion path must not apply selection results after cancellation; it now short-circuits when the atomic cancel flag is set or when `MeshSegmenter::Result::fallbackReason` equals `"cancelled"`.

Learnt from: fernandotonon
URL: https://github.com/fernandotonon/QtMeshEditor/pull/771

Timestamp: 2026-06-28T03:25:45.548Z
Learning: In `src/EditModeController.cpp`, batched polygon selection uses a `notify` parameter on `selectFace()` so expansion can run with `notify=false`, followed by a single overlay rebuild and one `editSelectionChanged()` emission after the batch.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

fernandotonon and others added 3 commits June 28, 2026 00:14
Adds the "train the model further as we gather data" path the unrigged-cat
case needs. Rigged meshes carry exact part labels in their bone weights, so
every rigged asset is a free, perfectly-labelled training sample.

- AutoRig::rigPriorPartLabels(entity, vertexCount, &resolved): shared static
  that labels each vertex by the part of the bone it's most-weighted to
  (MeshSegmenter::partForBoneName), in gatherGeometry's vertex order. Factored
  out of EditModeController so the in-app select-by-part fast path and the
  miner produce bit-identical labels.
- EditModeController::selectByPart: rig-prior block now calls the shared
  helper (no behaviour change; -34 lines of duplicated bone-walk).
- CLI `qtmesh segment <mesh> --dump-training-data out.json`: runs the rig-prior
  path on a SKINNED mesh and writes the normalised point cloud + exact
  per-vertex labels (schema "qtmesh-meshseg-training-v1"). Errors on a static
  mesh or a too-sparse rig (<50% resolved).
- scripts/export-meshseg-onnx.py: `--real-data <dirs/files>` loads mined JSONs
  (with yaw/tilt/jitter aug copies) and MIXES them into the synthetic set
  before training. Gains land on the MODEL path used for UNrigged meshes
  (rigged meshes already use the exact rig-prior path in-app).

Verified: mined paint/Rumba Dancing.fbx -> 5828 verts, 100% resolved, all 6
parts present, left/right split correct, points = verts*3, schema contract OK.

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

Mixamo is a good source of rigged humanoids BUT its Adobe EULA does not clearly
permit training a redistributable commercial model, and the shipped weights are
a derived work of the training data — so per the project's permissive bar (the
same wall ShapeNet-Part / PartNet / LAFAN1 / RigNet hit) we mine CC0/CC-BY
sources instead, which give the same standard-bone-name rigged humanoids with
clean redistribution rights.

- scripts/mine-training-data.sh: runs `qtmesh segment --dump-training-data`
  over a directory of rigged meshes -> per-mesh JSON samples. Skips static /
  sparse-rig meshes gracefully. Fully automated.
- scripts/fetch-training-rigs.sh: curated CC0 source ledger (Quaternius /
  Poly Pizza / Kenney) + a provenance SOURCES.md; fetches stable-URL sources
  and guides manual download for the rest (those distribute via Drive/itch.io
  with no rot-proof raw URLs). CC-BY allowed with recorded attribution.
- THIRD_PARTY_AI_MODELS.md: document the CC0/CC-BY-only mined-data policy and
  the explicit Mixamo exclusion + rationale.
- .gitignore: training_rigs/ + mined_training_data/ (local corpus, never
  committed; license provenance travels in SOURCES.md).

Verified: mine-training-data.sh mines 3/3 rigged repo FBXes; static meshes are
correctly rejected.

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

Copy link
Copy Markdown

@fernandotonon fernandotonon merged commit dfb238b into master Jun 28, 2026
21 checks passed
@fernandotonon fernandotonon deleted the feat/ai-segment-410 branch June 28, 2026 05:19
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.

1 participant