fix(#410): segmentation works in-app — sampling bug + flat model (3.13.1)#774
fix(#410): segmentation works in-app — sampling bug + flat model (3.13.1)#774fernandotonon wants to merge 1 commit into
Conversation
…odel The shipped segmentation was broken in the app (≈0.33 acc even on a correctly oriented mesh). Two root causes, both fixed: 1. **Inverted point sampling (MeshSegmenter::predict).** For meshes with fewer verts than `samplePoints` (the COMMON case), the sample loop drew EVERY point at random instead of covering verts 0..vc-1 in order. The scatter then only labelled the randomly-hit verts (~63% coverage) and left the rest at the Torso default — wrecking the output. Now the first min(N,vc) points are verts in order (1:1 coverage); N is also capped at the vertex count so a small mesh isn't padded with noisy random duplicates. 2. **Model architecture incompatible with the linked ONNX Runtime.** The model used an in-graph kNN (cdist/topk/gather); those ops did not execute correctly in the app's ONNX Runtime build (Python ORT gave 0.98 on the same model+input, the C++ app gave 0.33). Reworked the exported model to a flat PointNet segmentation network (per-point MLP + global max-pool + per-point head) using only Linear/GELU/max — ops every ORT build runs identically. Verified in the C++ app: 0.33 → 0.99 on a correctly-oriented rigged mesh. Training (scripts/export-meshseg-onnx.py, dev-only, not shipped): - flat PointSeg architecture (no kNN). - `--real-data` loader + `--real-weight` rebalance: mix CC0-mined real rigs (rig-prior labels) with synthetic at a controlled ratio; per-source val acc reporting (synth vs real) so real-mesh quality is visible, not hidden in a blended average. - full 360° yaw augmentation (was ±0.25rad) so facing direction matters less. Model: meshseg v1.2.0 (flat) trained on synthetic + ~89 CC0 rigged characters (Quaternius CC0 + Khronos glTF-Sample-Assets + ToxSam/Polygonal-Mind xyz CC0), hosted on the HF repos. **Known limitation:** left/right limb assignment is inherently orientation-dependent — best on upright, front-facing meshes; a backward-facing mesh can mislabel L/R. Rigged meshes are unaffected (the exact rig-prior path is used for them). Bumps version to 3.13.1 (3.13.0 shipped the broken model). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughBumps QtMeshEditor from 3.13.0 to 3.13.1 in ChangesVersion bump to 3.13.1
MeshSegmenter fix and ONNX training improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba74197ee9
ℹ️ 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".
| # the old ±0.25rad yaw made the model brittle — a 180°-yawed mesh got its | ||
| # left/right mirrored and head mislabelled). We keep it UPRIGHT (only a tiny | ||
| # tilt) and DO NOT mirror, so true left/right handedness is preserved. | ||
| yaw = rng.uniform(0.0, 2*np.pi) |
There was a problem hiding this comment.
When retraining with the default synthetic data, this full-360° yaw rotates the primitive humanoid while leaving left_*/right_* labels unchanged. Because these generated bodies have no consistent front/back signal, integrating yaw over [0, 2π) makes the left- and right-side point distributions identical (a π yaw puts the right arm where the left arm normally is), so the labels become contradictory and the exported ONNX model cannot learn stable left/right segmentation; in-app results can arbitrarily swap arms/legs. Keep yaw within a fixed facing range or add a reliable orientation cue before using full yaw for side-labeled data.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
scripts/export-meshseg-onnx.py (1)
286-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the unused
Bin the shape unpack.Nis the only value used here, so simplify this line to_, N, _ = f.shape(orN = f.shape[1]).🤖 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` at line 286, The shape unpack in the mesh segmentation export code is assigning an unused batch dimension, so simplify the unpack in the relevant block of the export script by dropping the unused B and keeping only the needed N value; update the assignment near the f.shape handling in the export logic to use a form like _, N, _ = f.shape or equivalent so the code matches the actual usage.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/export-meshseg-onnx.py`:
- Line 216: The --knn CLI option is still presented as an active model parameter
even though PointSeg ignores k, so update the argument handling in
export-meshseg-onnx.py to mark --knn as deprecated/no-op. Adjust the argparse
definition and any related status output around the point where the model
configuration is printed so it no longer suggests the value affects
training/export, and ensure the same treatment is applied wherever the knn
setting is surfaced.
- Around line 249-257: The current shuffle-and-index split in
export-meshseg-onnx.py can leak augmented copies of the same real mesh across
train and validation. Update the dataset construction around the combined `P`,
`L`, and `src` arrays so that samples are grouped by source mesh before any
augmentation/oversampling, or keep a group identifier through `srcT` and perform
a group-aware split before `nval` is computed. Make sure the split logic in the
block that prints the combined dataset stats no longer relies on a random
permutation alone, and that all siblings from one real mesh stay in the same
partition.
- Around line 242-246: The oversampling path in load_real_data() appends
augmented real replicas from _augment_upright() directly into extraP without
re-centering/re-scaling them, so they can drift from the normalized distribution
used at inference. Normalize each augmented replica immediately after
_augment_upright() and before appending it to extraP, keeping the existing label
handling in extraL unchanged.
In `@src/MeshSegmenter.cpp`:
- Around line 469-486: The new capped sampling logic in
MeshSegmenter::segmentMesh (the point-selection loop that fills pts/srcVert) is
a significant user-visible operation but still lacks Sentry breadcrumb tracking.
Add a SentryReporter::addBreadcrumb(...) call in this sampling path using the
established breadcrumb category/message pattern so regressions in the new
inference-path behavior are traceable in production.
---
Nitpick comments:
In `@scripts/export-meshseg-onnx.py`:
- Line 286: The shape unpack in the mesh segmentation export code is assigning
an unused batch dimension, so simplify the unpack in the relevant block of the
export script by dropping the unused B and keeping only the needed N value;
update the assignment near the f.shape handling in the export logic to use a
form like _, N, _ = f.shape or equivalent so the code matches the actual usage.
🪄 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: 108f39e7-1d94-4242-817d-2e40ca6912f0
📒 Files selected for processing (5)
CMakeLists.txtREADME.mdscripts/export-meshseg-onnx.pysrc/MeshSegmenter.cppwebsite/src/hooks/useQtmeshActionRef.js
| help="oversample factor for real samples so they aren't drowned " | ||
| "out by synthetic. 0 = auto-balance to ~50/50 real:synthetic") | ||
| ap.add_argument("--dim", type=int, default=128, help="model feature width") | ||
| ap.add_argument("--knn", type=int, default=12, help="local kNN neighbours") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Mark --knn as deprecated/no-op.
PointSeg now ignores k, but the CLI help and status print still present --knn as an active model parameter. That can mislead training runs.
Proposed cleanup
- ap.add_argument("--knn", type=int, default=12, help="local kNN neighbours")
+ ap.add_argument("--knn", type=int, default=12,
+ help="deprecated/no-op; accepted for compatibility with older commands")
...
- print(f"data N={n} points={N} dev={dev} dim={a.dim} knn={a.knn}")
+ print(f"data N={n} points={N} dev={dev} dim={a.dim} flat-PointNet (--knn ignored)")Also applies to: 259-259
🤖 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` at line 216, The --knn CLI option is still
presented as an active model parameter even though PointSeg ignores k, so update
the argument handling in export-meshseg-onnx.py to mark --knn as
deprecated/no-op. Adjust the argparse definition and any related status output
around the point where the model configuration is printed so it no longer
suggests the value affects training/export, and ensure the same treatment is
applied wherever the knn setting is surfaced.
| for _ in range(reps_i - 1): | ||
| for i in range(len(rP)): | ||
| extraP.append(_augment_upright(rP[i], rng)); extraL.append(rL[i]) | ||
| rP = np.concatenate([rP, np.stack(extraP)]) | ||
| rL = np.concatenate([rL, np.stack(extraL)]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Normalize re-augmented real replicas.
load_real_data() re-centers/re-scales augmented real samples, but this oversampling path rotates/jitters replicas and appends them directly. Normalize each replica after _augment_upright() so training matches the app’s normalized inference distribution.
Proposed fix
extraP, extraL = [], []
for _ in range(reps_i - 1):
for i in range(len(rP)):
- extraP.append(_augment_upright(rP[i], rng)); extraL.append(rL[i])
+ pp = _augment_upright(rP[i], rng)
+ c = 0.5 * (pp.min(0) + pp.max(0))
+ h = float(np.max(0.5 * (pp.max(0) - pp.min(0))))
+ extraP.append(((pp - c) / (h + 1e-9)).astype(np.float32))
+ extraL.append(rL[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.
| for _ in range(reps_i - 1): | |
| for i in range(len(rP)): | |
| extraP.append(_augment_upright(rP[i], rng)); extraL.append(rL[i]) | |
| rP = np.concatenate([rP, np.stack(extraP)]) | |
| rL = np.concatenate([rL, np.stack(extraL)]) | |
| extraP, extraL = [], [] | |
| for _ in range(reps_i - 1): | |
| for i in range(len(rP)): | |
| pp = _augment_upright(rP[i], rng) | |
| c = 0.5 * (pp.min(0) + pp.max(0)) | |
| h = float(np.max(0.5 * (pp.max(0) - pp.min(0)))) | |
| extraP.append(((pp - c) / (h + 1e-9)).astype(np.float32)) | |
| extraL.append(rL[i]) | |
| rP = np.concatenate([rP, np.stack(extraP)]) | |
| rL = np.concatenate([rL, np.stack(extraL)]) |
🤖 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 242 - 246, The oversampling path
in load_real_data() appends augmented real replicas from _augment_upright()
directly into extraP without re-centering/re-scaling them, so they can drift
from the normalized distribution used at inference. Normalize each augmented
replica immediately after _augment_upright() and before appending it to extraP,
keeping the existing label handling in extraL unchanged.
| sh = np.random.default_rng(a.seed + 1).permutation(len(P)) | ||
| P = P[sh]; L = L[sh] | ||
| P = P[sh]; L = L[sh]; src = src[sh] | ||
| print(f"combined dataset: {len(P)} samples " | ||
| f"({a.samples} synthetic + {len(rP)} real)") | ||
| f"({a.samples} synthetic + {len(rP)} real @ {reps_i}x = " | ||
| f"{100*len(rP)/len(P):.0f}% real)") | ||
| P = torch.tensor(P); L = torch.tensor(L) | ||
| srcT = torch.tensor(src) | ||
| n = P.shape[0]; N = P.shape[1] | ||
| nval = max(1, n // 10) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Avoid train/validation leakage for real meshes.
Real samples include augmented copies from the same source mesh, then the combined sample list is shuffled and split by index. This can put sibling augmentations of one real mesh into both train and validation, making the new real accuracy optimistic. Split real data by source mesh before augmentation/oversampling, or carry group IDs and group-split before computing nval.
🤖 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 249 - 257, The current
shuffle-and-index split in export-meshseg-onnx.py can leak augmented copies of
the same real mesh across train and validation. Update the dataset construction
around the combined `P`, `L`, and `src` arrays so that samples are grouped by
source mesh before any augmentation/oversampling, or keep a group identifier
through `srcT` and perform a group-aware split before `nval` is computed. Make
sure the split logic in the block that prints the combined dataset stats no
longer relies on a random permutation alone, and that all siblings from one real
mesh stay in the same partition.
| // Cap N at the vertex count: padding a small mesh up to samplePoints with | ||
| // random duplicate points only adds noise to the model's global max-pool | ||
| // and never improves coverage (every real vert is already point i). Sample | ||
| // DOWN only when the mesh is larger than samplePoints. | ||
| const int N = std::min(std::max(256, opts.samplePoints), vertexCount); | ||
| // Deterministic point sample (with replacement if the mesh is small). | ||
| std::mt19937 rng(0x5e6u); // NOSONAR — non-crypto, fixed for reproducibility | ||
| std::uniform_int_distribution<int> pick(0, vertexCount - 1); | ||
| 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); | ||
| // The first min(N, vertexCount) points are verts 0..vc-1 IN ORDER so | ||
| // every vertex is covered 1:1 (the scatter's `vertexCount <= N` | ||
| // branch relies on this). Only the padding beyond vc (when N > vc) is | ||
| // random. (Previously this was inverted: when vc < N — the common | ||
| // case — every point was random, so ~37% of verts never got a real | ||
| // label and fell back to the Torso default, wrecking the result.) | ||
| const int v = (i < vertexCount) ? i : pick(rng); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a breadcrumb for this inference-path sampling change.
This alters a significant, user-visible operation, but the new capped-sampling path still has no SentryReporter::addBreadcrumb(...) instrumentation. That makes production regressions on small meshes much harder to diagnose.
As per coding guidelines, "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message) using the established breadcrumb categories."
🤖 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 469 - 486, The new capped sampling logic
in MeshSegmenter::segmentMesh (the point-selection loop that fills pts/srcVert)
is a significant user-visible operation but still lacks Sentry breadcrumb
tracking. Add a SentryReporter::addBreadcrumb(...) call in this sampling path
using the established breadcrumb category/message pattern so regressions in the
new inference-path behavior are traceable in production.
Source: Coding guidelines
|
|
Closing — after extensive experimentation (8 model iterations + connectivity/smoothing/auto-rig + a Mixamo-vs-CC0 training comparison), we concluded the segmentation rework wasn't a clear improvement over the shipped model, and reverting to keep the known-good state. Key findings documented for the future: the in-app model load path is nested (QtMeshEditor/QtMeshEditor/), the sampling loop had an inversion bug worth revisiting, and (settled empirically) Mixamo training data does NOT beat CC0/synthetic. Parking this rather than shipping a regression. |
1 participant
Problem
The mesh part segmentation shipped in 3.13.0 (#771) was effectively broken in the app — it scored ≈0.33 per-vertex accuracy even on a correctly-oriented rigged mesh (the model itself was fine: 0.98 in Python). Two independent root causes, found by feeding the C++ path's exact points back through Python ORT:
1. Inverted point sampling (
MeshSegmenter::predict)For meshes with fewer vertices than
samplePoints(4096 — i.e. most meshes), the sampling loop drew every point at random instead of covering verts0..vc-1in order. The nearest-point scatter then only labelled the randomly-hit verts (~63% coverage) and left the rest at theTorsodefault. Fixed: the firstmin(N, vc)points are verts in order (1:1 coverage), andNis capped at the vertex count so small meshes aren't padded with noisy duplicates.2. In-graph kNN incompatible with the linked ONNX Runtime
The model used an in-graph kNN (
cdist/topk/gather). Those ops did not execute correctly in the app's ONNX Runtime build — identical model + input gave 0.98 in Python ORT but 0.33 in the C++ app. Reworked the exported model to a flat PointNet segmentation network (per-point MLP → global max-pool → per-point head) using onlyLinear/GELU/max, which every ORT build runs identically.Verified in the C++ app: 0.33 → 0.99 on a correctly-oriented rigged mesh.
Model — meshseg v1.2.0 (flat)
Retrained on synthetic + ~89 CC0 rigged characters (Quaternius CC0, Khronos glTF-Sample-Assets, ToxSam/Polygonal-Mind
xyzCC0). Hosted on the HF repos (combined + dedicated). Training script gains: flat arch,--real-data/--real-weightrebalance with per-source (synth vs real) val reporting, and full 360° yaw augmentation.Known limitation (documented): left/right limb assignment is inherently orientation-dependent — best on upright, front-facing meshes; a backward-facing mesh may mislabel L/R. Rigged meshes are unaffected — they use the exact rig-prior path, not the model.
Version
Bumps to 3.13.1 (3.13.0 shipped the broken model).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation