feat(#889): Face auto-rig — 52 ARKit blendshapes on any humanoid face (consolidated epic) - #903
Merged
Merged
Conversation
…-FaceKit MIT Slice A of epic #889 (auto-generate ARKit blendshapes on any humanoid mesh). Offline feasibility prototype + decision records only; no app code. - scripts/spike-facerig.py (NOT shipped): pure numpy/scipy prototype of the two algorithms Slices C/D will port to C++ — non-rigid ICP (Amberg 2007 optimal-step, no Wrap3D) to fit the ICT-FaceKit template to an arbitrary neutral head, then deformation transfer (Sumner-Popovic 2004) of each ARKit expression onto the user topology. - Proven on a real different-topology head (26,719-vert ICT template -> 12,763-vert decimated user): NRICP surface fit mean 0.003% / max 0.59% of the head diagonal; transferred shapes anatomically correct (jawOpen drops the lower face ΔY -0.32 with the forehead still |Δ| 0.002; eyeBlink stays localized). GO. - THIRD_PARTY_AI_MODELS.md: ICT-FaceKit MIT entry (template + 52 ARKit-named shapes, shared topology; 'full model' USC tier rejected; no ML/ONNX — a deterministic sparse solve). - docs/FACE_RIG_SPIKE.md: full implementable contract (NRICP params + stiffness anneal, the deformation-transfer linear system, the ICT->ARKit name map, OBJ multi-group gotcha, humanoid/orientation/landmark risks) + the two quality upgrades for the C++ port (full deformation-gradient transfer, optional landmarks) + go/no-go. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ce B) Slice B of epic #889. - scripts/export-arkit-template.py (offline, not shipped): packs the ICT-FaceKit MIT template — generic_neutral_mesh.obj + the 52 ARKit expression meshes (same topology; shape = expr - neutral) — into one compact little-endian arkit_template.bin. Bakes the ICT->ARKit name map (FaceCap::kBlendshapeNames order; single/centered ARKit channels like browInnerUp/cheekPuff sum the ICT _L/_R halves, the rest map 1:1). Built the real bundle: 51 shapes, 17 MB, 26,719-vert neutral. - src/FaceRig/ArkitTemplate.{h,cpp}: Ogre-free loader for that binary (magic + header + neutral + faces + named delta shapes), with strict bounds/truncation checks. Model management is the house pattern: AppData/ai_models/facerig/arkit_template.bin, download-on-first-use with QTMESH_FACERIG_MODEL_BASE_URL / ai/facerigModelBaseUrl and the QTMESH_FACERIG_NO_DOWNLOAD offline guard. Shape names are the canonical ARKit-52 so the generated targets match face capture (#869). - ArkitTemplate_test.cpp: 5 headless tests (header/neutral/faces/shapes parse, bad-magic + truncation rejection, missing-file, and an env-gated test that loads the REAL 17 MB bundle and checks the 51 ARKit-named shapes incl. nonzero jawOpen). All pass. - scripts/upload-facerig-template.sh: HF hosting (facerig/arkit_template.bin + the ICT MIT LICENSE) — maintainer step, same as the mocap upload. - test_main.cpp: QTMESH_TESTS_SKIP_OGRE_PREFLIGHT so the pure-data FaceRig suites run on machines without GL (CI unaffected). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Slice C of epic #889. Pure-data, Ogre-free, ZERO new dependencies, headless-tested — the native C++ port of the NRICP the spike (#897) proved. - src/FaceRig/NonRigidICP.{h,cpp}: Amberg 2007 optimal-step non-rigid ICP. Fits a template mesh to a user neutral head of arbitrary topology, producing per-template-vertex positions on the user surface (the correspondence the deformation transfer #892 consumes). Per-vertex 3x4 affine A_i; minimizes data (A_i·v_i -> closest user-surface point) + annealed stiffness (alpha·(A_i - A_j) over template edges, 50->0.5). Self-contained: * median-split KD-tree over user-triangle centroids (broad phase), * Ericson point-to-triangle projection (exact closest point), * CSR sparse matrix + conjugate-gradient on the normal equations (AtA x = At b) — no Eigen, no scipy, nothing vendored. Reports per-vertex residual + mean/max + bbox diag so callers can gate on fit quality (the humanoid check in Slice E). Rigid centroid+bbox-scale pre-align, then the non-rigid warp. - 6 headless tests: fit-to-self near-identity, fit onto a translated target, **fit onto a DIFFERENT-topology target** (10x10 template -> 14x14 user, <5% residual, finite), stiffness-bounded under noise, degenerate input no-NaN, residual/diag reporting. All pass (~5-13 ms each). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-Rig Slice D) Transfer each ARKit template expression's per-triangle deformation onto the user-identity mesh produced by the NRICP fit (#891), in template topology. - SparseSolve.{h,cpp}: dependency-free CSR matrix (fromTriplets/mul/mulT) + conjugate-gradient on the normal equations (CGNR), factored out for reuse. No Eigen, no external solver. - DeformationTransfer.{h,cpp}: per source triangle builds the deformation gradient S = [e1' e2' n']·[e1 e2 n]⁻¹ (Sumner's 4th "normal" vertex trick), then solves one sparse least-squares over the fitted mesh for vertex positions whose per-triangle gradient matches S. The 4th vertex is a free per-triangle unknown; the gauge is fixed by anchoring a single vertex to the source's own vertex-0 displacement (anchoring every vertex fights the shape). init() precomputes the topology-fixed system once so all 52 shapes transfer without rebuilding. - DeformationTransfer_test.cpp: identity round-trip near-identity (<15% RMS), scale-covariant transfer onto a scaled identity with sign/winding preserved, zero-delta→zero, bad-input rejection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Chain the pure-data stages into an end-to-end face auto-rig: fit the ARKit template onto a user face (NRICP #892), transfer each of the 52 expressions onto the user identity (DeformationTransfer #893), resample to the real user vertices, and attach them as Ogre::Pose + VAT_POSE morph targets — named per FaceCap::kBlendshapeNames so face capture (#869) drives them with no new playback code. - FaceRigger.{h,cpp} (Ogre-free, headless-tested): buildFaceRig() runs the whole pipeline → per-user-vertex delta per shape. Humanoid-only gate on the NRICP fit residual (a non-face mesh fits poorly and is refused). Resample via a dependency-free spatial-hash grid (nearest correspondence vertex, built once for all 52 shapes). - FaceRigAttach.{h,cpp} (Ogre bridge): extract the entity's combined geometry, run buildFaceRig, split deltas back per submesh handle, and attach via AddMorphTargetCommand::redo() (the exact MorphCommands pose-build). attachFaceRigWithBundledTemplate() downloads/loads the template first. - CLI: qtmesh facerig <file> [-o out] [--max-shapes N] [--max-residual PCT] [--json] (CLIPipeline::cmdFaceRig); recognized subcommand + gamification map. - MCP: add_arkit_blendshapes tool (selected face, optional re-export), heavy. - NonRigidICP: harden the residual against diverged (NaN/inf) vertices — skip them from the mean, count them, and flag a >5% diverged fit as failed so a mostly-NaN fit can't read as a great one (surfaced by UV-sphere pole fans). Verified end-to-end: the real ICT template (26719v, 51 shapes) fits a decimated different-topology face (15755v) at mean 0.008% / max 0.61% residual and attaches 51 blendshapes; the exported glb carries all 51 morph targets on the primitive. Tests: FaceRigger_test.cpp — per-user-vertex shapes with correct semantics (jaw vs smile moved-mass regions), names/order preserved, non-face rejection, bad-input rejection. Full FaceRig suite green (18 tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
One-click face auto-rig from the Inspector's Vertex Morph Animation section: fit the ARKit template onto the selected face mesh and attach the 52 ARKit morph targets, so the #869 face-capture panel drives them (name-based hand-off — the shapes are named per FaceCap::kBlendshapeNames). - FaceRigController (QML_SINGLETON): hasMeshSelection / busy / downloading / status props; addArkitBlendshapesAsync() extracts geometry + loads the bundled template on the MAIN thread, runs the heavy Ogre-free buildFaceRig() on a WORKER (UI stays responsive; status = "Downloading…/Fitting…"), then commits the attach back on the main thread as ONE undo macro ("Add ARKit Blendshapes") so Ctrl+Z removes all shapes at once. Re-resolves the entity by name after the worker returns (selection may have changed). Gamification + Sentry breadcrumbs. - FaceRigAttach: split extractGeometry() (main-thread buffer read) and attachShapes() (main-thread pose attach) out of attachFaceRig so the GUI can run the fit off-thread; attachFaceRig/CLI/MCP now compose the same helpers (behavior-preserving — CLI end-to-end unchanged: 51 shapes, 0.008% mean). - QML: "✨ Add ARKit Blendshapes (AI)" button in the Shapes section, gated on a mesh selection, disabled + showing worker status while busy, with a result/ error line. Themed with the Inspector's highlight/border/text colors. - Registered the singleton + kill() in mainwindow.cpp. Note: the #869 Performance Capture panel lives on the mocap epic branches (not this stack); the hand-off is automatic once both reach master since the shape names already match the mocap-52 vocabulary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Finish the face auto-rig epic. Quality pass — recover ARKit names past the Assimp glTF gap: - FaceRig::writeArkitSidecar() writes a `<mesh>.arkit.json` sidecar (schema qtmesh-arkit-blendshapes-v1: ordered shape names) next to the exported mesh, because Assimp 6.0's glTF exporter drops mesh.extras.targetNames even when aiAnimMesh::mName is set. `qtmesh mocap --face` / re-import can rebind the mocap-52 vocabulary by index. AttachReport now carries the ordered shapeNames; the CLI + MCP export paths write the sidecar. Verified: 51 ordered names written alongside the glb. Docs: - docs/FACE_RIG.md — user + developer guide (GUI/CLI/MCP usage, the NRICP → deformation-transfer → resample pipeline, quality numbers, limits). - THIRD_PARTY_AI_MODELS.md — ICT-FaceKit MIT licensing verdict (MIT tier only; the USC "full model" tier is REJECTED), hosting + first-use-download notes. - CLAUDE.md — FaceRig architecture section + `qtmesh facerig` CLI reference + added `facerig` to the recognized-subcommand list. - README.md — `qtmesh facerig` quick-start lines. - printUsage() now lists `facerig`. - .gitignore allowlists docs/FACE_RIG.md. Packaging (from Slice B, verified here): scripts/export-arkit-template.py packs the ICT MIT head + 52 shapes → facerig/arkit_template.bin; scripts/upload-facerig-template.sh uploads it (+ the ICT LICENSE) to the HF models repo via `hf upload`; downloads on first use. Telemetry: Sentry `ai.assist.face_rig` on all three surfaces (CLI/MCP/GUI); gamification noteOperation on the auto_rig / morph clusters. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a deterministic ARKit FaceRig pipeline with template packaging, NRICP fitting, deformation transfer, landmark anchoring, morph-target attachment, GUI/CLI/MCP entry points, export name preservation, and supporting documentation and tests. ChangesFaceRig auto-rigging feature
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
13 tasks
![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: fbed51409f
ℹ️ 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".
| // that Assimp 6.0's glTF exporter drops (it doesn't emit targetNames even | ||
| // though we set aiAnimMesh::mName). Face capture (#869) and re-import can | ||
| // read this to rebind the mocap-52 vocabulary by index. | ||
| QFile f(meshPath + QStringLiteral(".arkit.json")); |
There was a problem hiding this comment.
For the export→re-import workflow this sidecar is the only place the ARKit target names survive, but nothing in the repo consumes it: a search for .arkit.json / qtmesh-arkit-blendshapes-v1 finds only this writer, while the importer still relies on aiAnimMesh::mName and falls back to generated names when Assimp drops targetNames. As a result, users who export a face-rigged GLB/GLTF and load it again still lose the ARKit names, so downstream face-mocap/rebinding by index cannot work despite the sidecar being written.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Addressed in e7abe01: AssimpToOgreImporter::loadModel now reads the <file>.arkit.json sidecar and passes the ordered names to MeshProcessor as name hints (applied when the scene has exactly one morphed mesh and the aiMesh carries no names). MeshImporterExporter::exporter also writes the sidecar for any exported mesh with poses, so GUI exports round-trip too. Verified: qtmesh morph reference.glb --list shows all 51 ARKit names after re-import.
Two field-reported problems on full-body character meshes: 1) Wrong fit — the ARKit template (a FACE) was fitted against the WHOLE body, so "mouth" shapes landed on an arm. Fix: isolate the head region before fitting. FaceRigAttach::extractGeometry now computes a per-vertex headMask via the rig-prior (AutoRig::rigPriorPartLabels, exact on skinned meshes — handles the fox's snout/ears and Mixamo proportions) or MeshSegmenter's geometric fallback; buildFaceRig fits ONLY the head sub-mesh and scatters deltas back to the head vertices (body stays at zero). Only isolates when the head is a real minority region (50..75% of verts) — a bare-face crop still fits whole. Verified on Hip Hop Dancing (fox): jawOpen now moves 546 head verts at the top of the mesh (height frac 0.89) vs smearing across the torso (0.50) before. 2) GUI crash during attach — the batch adds poses + a VAT_POSE clip to a LIVE entity and re-initialises it per shape; the render loop's _updateAnimation could run against half-rebuilt pose buffers mid-batch and crash (skeletal + pose combined). Fix: FaceRigController disables every enabled animation state for the attach batch and restores them after. CLI (no render loop) was already stable; this closes the GUI-only crash. Known limitation (follow-up): on meshes whose head proportions differ sharply from the ICT template (e.g. Rumba), NRICP can converge to a low-residual but mis-oriented correspondence, so some shapes attach sparsely or not at all — landmark/PCA pre-align is the next step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t step The ARKit attach adds 51 poses + VAT_POSE clips to a LIVE entity and re-initialised the entity after EACH shape (entity->_initialise(true)). On a big / multi-submesh mesh (e.g. Rumba Dancing, 11 submeshes) that is O(shapes × mesh) of main-thread work and froze the UI at the final progress step (56/57 = the attach phase). Fix: AddMorphTargetCommand gains setDeferInit(). FaceRigController builds all per-shape commands, defers the re-init on every one EXCEPT the last, and lets that last command do a single _initialise — so redo (and Ctrl+Shift+Z redo) rebuilds the pose/animation buffers exactly once at the end instead of 51 times. undo is unchanged (per-command removal is cheap). The CLI/MCP attachShapes path is untouched (no render loop, process exits). Verified: Rumba attaches all 51 shapes at 0.086% residual with every moved vertex on the head (height fraction 0.73–1.00, mean 0.82) — no body smear. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ong shape placement) The NRICP fit had no anatomical anchoring, so it converged to a low-residual but MIS-ORIENTED drape — ARKit "mouth" shapes landed on the wrong features (and worse on full-body / oddly-proportioned humanoids like the fox and Rumba). This adds automatic facial-landmark detection to anchor the fit onto the real eyes/nose/mouth, per the user's suggestion. Pipeline (all ENABLE_ONNX-guarded, graceful unanchored fallback): - FaceLandmarkDetector (src/FaceRig): runs MediaPipe FaceMesh V2 (face_landmarks.onnx, Apache-2.0 — the SAME model the mocap face-capture uses, reused not re-converted) on a centred 256 crop → 478 image-space landmarks. We render the head ourselves so the upstream face detector is skipped. Model downloads on first use to ai_models/facerig/. - MeshDepthRenderer::renderShadedView: renders the entity with REAL materials + a head-on light (photo-like face for the detector), with an optional focus AABB so a full-body character is framed TIGHTLY on the head — without this the face is a few pixels and MediaPipe can't detect it (7 landmarks → 166 once framed on the head). - FaceRigLandmarks: render + detect + back-project (Möller–Trumbore ray from the render camera to the mesh surface) on BOTH the template (temp entity, cached) and the user head, pair by MediaPipe index → NRICP landmark anchors. - NonRigidICP: landmark constraint rows (weight rides alpha so orientation/scale lock first, then relax). FaceRigger/FaceRigAttach/FaceRigController thread the anchors through; detection runs on the MAIN thread (Ogre render), the fit on the worker. headSubmesh() gives the detector the head region to frame/raycast. Also lands the previously-uncommitted GUI responsiveness work: worker-thread progress bar + Cancel (FaceRigController progress/progressTotal/cancel) and the batch-attach deferred re-init. Verified via CLI on both problem meshes: Hip Hop (fox) 151 anchors, Rumba (human) 166 anchors; both fit at <0.15% mean / <11% max with all moved verts on the head. Full FaceRig suite green (18 tests). Docs + THIRD_PARTY entry + upload-script hook added; the landmark model hosts under facerig/ (falls back to the unanchored fit until hosted). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two field-reported issues + the groundwork for the fix you asked for (auto-place markers, user adjusts): - Duplicate morph-target rows: a target that spans N submeshes has one Ogre pose PER submesh, all same-named (Rumba's face = 11 submeshes → 11 "jawOpen" rows). morphTargetsFor() now coalesces by name (every by-name op already operates across all matching poses), so the Inspector lists each target once. - Attach robustness: hide the entity for the pose-attach batch so the render loop can't touch its pose/skin buffers mid-rebuild (belt-and-braces with the animation-state disable + the single deferred _initialise). - Face-marker foundation (FaceRigLandmarks): a canonical marker catalog (nose, chin, eye corners, mouth corners, brows, …) mapped to stable MediaPipe FaceMesh indices; seedFaceMarkers() resolves each marker's TEMPLATE vertex from the (reliable) template detection and seeds the editable USER position from auto-detect when confident, else a head-box-projected default for the user to drag; anchorsFromMarkers() turns the (edited) markers into the same NRICP landmark anchors. This is the base for the click-to-adjust UI — needed because MediaPipe FaceMesh returns a garbage point-cloud on cartoon/stylized faces (verified via a landmark overlay), so auto-detect must be a SEED the user corrects, not the final answer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…faces) MediaPipe FaceMesh returns garbage on cartoon/stylized faces (verified via a landmark overlay), so pure auto-detection can't anchor them. Per the chosen design, auto-detection now SEEDS a small set of draggable markers that the user corrects — the reliable path for the stylized characters people actually rig. - FaceRigController marker session: beginFaceMarkers() extracts geometry, loads the template, and seeds the markers (template detection resolves each marker's template vertex reliably — the ICT template is a real face; user detection seeds positions when confident, else a head-box-projected default). Draggable PT_SPHERE overlays (cyan = selected, yellow = placed, grey = default). selectMarker/handleMarkerClick reposition a marker on the mesh surface (Ogre::Math ray/tri); rigFromMarkers() builds NRICP anchors from the edited markers and runs the SAME fit+attach (shared runRigAsync, refactored out of addArkitBlendshapesAsync). TransformOperator routes viewport clicks (same priority as AutoRig markers). - QML: "Place / adjust face markers…" button + an in-session panel — marker chips (click to select, colour-coded by state), "Rig from markers", "Cancel". Shows whether the auto-seed was confident so the user knows how much to fix. Both paths (direct auto + marker-anchored) share runRigAsync, so all the prior fixes (head isolation, deferred attach, animation-disable, progress+cancel) apply to both. App loads with no QML errors; full FaceRig + morph suites green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Even with all markers set, transferred shapes smeared (jawOpen dragged the whole lower face down instead of hinging the jaw): markers were only SOFT constraints inside NRICP, so on faces far from the template's proportions (cartoon jowls/chins) the un-anchored regions still slid to a wrong low-residual correspondence, and the resample smeared the deltas. Fix: rbfWarpByAnchors() — a thin-plate RBF space warp (φ(r)=r + affine, small dense solve, pure data) driven by the marker pairs. buildFaceRig pre-warps the WHOLE template into the user's face proportions before the fit, so each marked feature (mouth corners, chin, eyes, nose) STARTS exactly on its marked position and the space between interpolates smoothly; NRICP then only refines locally. Applied only for small curated anchor sets (≤32 — the marker path); bulk auto-detected sets are excluded (a garbage detection would fold the template). DeformationTransfer still uses the ORIGINAL template neutral as its source rest — the fit output stays the same correspondence semantics. Unit-tested: anchors land exactly, a pure-translation anchor field translates every vertex (thin-plate is affine-exact), <4 anchors → empty (caller falls back to the unwarped fit). Full FaceRig suite green (19 tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…stacking Field report: after the marker rig, NO slider moved anything. Two root causes: 1) Ill-conditioned warp — face markers are nearly COPLANAR (all on the front of the face), so the thin-plate RBF's affine term was unconstrained along the depth axis and could shear the template's unmarked regions (back of the head) into garbage, crumpling the fit. rbfWarpByAnchors is now a SIMILARITY prealign (centroid + RMS-spread scale — robust for coplanar sets) plus a ridge-regularized GAUSSIAN RBF on the residuals, whose influence DECAYS away from the face — far vertices get the similarity only, so the warp physically cannot explode. sigma = mean nearest-neighbour marker spacing x1.5. 2) Re-rig stacked instead of replacing — attaching over an already-rigged mesh ADDED duplicate same-named poses into the existing animations, and VAT_POSE keyframes reference poses BY INDEX, so the old references corrupt — the "sliders do nothing" state. The controller now DELETES existing same-named targets first (inside the same undo macro, atomic undo/redo). Also: marker spheres now actually show their state colours (self-illumination — with lighting disabled Ogre ignores diffuse/ambient, so they rendered plain white), and the rig logs anchors/replaced/attached counts for diagnosis. CLI auto path regression-checked (51 shapes @ 0.144% on Rumba); FaceRig suite green (19 tests) incl. the reworked warp test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ltas 50x
Root cause of "no slider does anything" after the marker rig, found by
reproducing the GUI marker path headlessly (QTMESH_FACERIG_MARKER_SIM):
MediaPipe's garbage detection on cartoon faces PASSED the count-based
confidence check and pre-placed the markers at scattered-blob positions.
Clumped garbage anchors contract the similarity scale + shred the Gaussian
warp, the fit crumples, and every transferred delta collapses (measured:
jawOpen maxDisp 0.001 = 0.12% of the head — invisible). With sane marker
positions the SAME pipeline produces jawOpen maxDisp 0.053 over 931 verts —
a clearly visible jaw open, the best result so far.
- constellationResidual(): align the template marker layout onto the detected
user layout with a similarity and measure the normalised residual — a real
face detection agrees (<0.25), a garbage blob doesn't.
- seedFaceMarkers: detection seeds are used ONLY when the constellation is
consistent; otherwise EVERY marker seeds at its head-box-projected template
position, placed=true — the proportional defaults measurably produce a good
rig on their own (Rumba: jawOpen 0.053 without touching a marker), and the
user refines from there (needed for muzzles like the fox).
- buildLandmarkAnchors (auto path): same gate — a garbage anchor set is
dropped entirely (unanchored fit beats a poisoned one).
- CLI: QTMESH_FACERIG_MARKER_SIM={1,2} diagnostic that exercises the exact GUI
marker path headlessly with per-shape delta stats (how this was found).
FaceRig suite green (19 tests); CLI auto path regression-checked.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hapes fix) Field report: GUI marker rig attached shapes with amplitudes ~0.0005 (0.06% of the head — invisible) while the identical headless sim produced 0.053. Cause: the GUI render differs from headless (skybox/lighting), so MediaPipe's garbage constellation VARIES run-to-run, and the user's run scored under the 0.25 gate and got trusted — poisoning the warp/fit again. - seedFaceMarkers gate tightened to 0.12: detection must look UNAMBIGUOUSLY like a face layout to be trusted; anything else seeds the proportional defaults (measured good: jawOpen 0.053/931 verts on Rumba untouched). - buildLandmarkAnchors (auto path) gate tightened to 0.15. - Amplitude safety net in buildFaceRig: if an ANCHORED run yields max shape amplitude < 0.5% of the head diag (invisible), retry once unanchored — a plain head-isolated fit always beats an invisible one. Makes "attached but invisible" impossible regardless of where garbage sneaks in. - UI now reports the amplitudes in the success status (jawOpen amp / max amp) so a crushed rig is visible at a glance instead of masquerading as success. Sim-verified (jawOpen 0.053) + FaceRig suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The user placed "Left mouth corner" on the CHARACTER's left — correctly — but the anchor paired it with the template's RIGHT corner, a mirrored constraint that folds the template through itself and crushes the fit. Measured on the template: MediaPipe's "left"-named indices (33/133/61/105) sit at +X = character-RIGHT (MP names sides in IMAGE space, mirrored for a camera-facing subject). Also measured: MediaPipe drifts on the untextured template render (nose tip detected 4 units off the midline), skewing every template anchor. - faceMarkerCatalog relabelled to CHARACTER-space sides (what a user placing markers on a model naturally means). - seedFaceMarkers symmetrizes the detected template landmarks before resolving vertices: the ICT template is x-symmetric with midline x=0, so midline features snap to x=0 and side pairs get mirrored positions (pair-mean height/depth, mean |x|) — detection supplies only what it gets right. Verified: nose/chin/lips/forehead at x=0, eye pair at ±6.75. - anchorsFromMarkers(markers, tmpl) auto-corrects a MIRRORED placement: scores the as-placed vs left/right-swapped pairings against the template constellation and keeps the better one — either side convention works. - QML hint states the convention explicitly. Sim: default-marker rig on Rumba now uses the CORRECT template anchors (jawOpen 0.0177 over 819 verts — clearly visible). FaceRig suite green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Field feedback on the (now working) marker rig: - OFFSET: the deformation landed slightly BELOW the marked lips — the anchors loosened at the fine anneal levels (weight 10 x alpha drops under the data terms once alpha < 1) and the fitted lip line drifted. landmarkWeight 10→30 pins the marked features through the whole anneal; measured jawOpen amplitude nearly doubles (0.018 → 0.033) with the same default markers. - Markers now auto-advance IN CATALOG ORDER after each placement (the old "next unplaced" logic stuck on one marker since defaults mark all placed), so the user can walk the whole set click-by-click. - Chip highlight fixed: markerPlaced() is an invokable, so its QML binding never re-evaluated (stale placed state); it now depends on the notifying selectedMarker property. Selected chip gets a bold white outline. - "Strength" slider (0.5–3.0x, default 1.5): amplitude multiplier applied to every transferred delta (FaceRigOptions.amplitude) — the exaggeration knob the user asked for, legitimate now that the fit itself is correct. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d mask) User-reported: "this mesh has the eyes as a separated submesh, couldn't it animate the eyes as well?" — measured and confirmed: Rumba's eye/teeth submeshes are skinned to non-body-region bones (eye bones) that the rig-prior labeler can't classify, so they were SILENTLY EXCLUDED from the head mask (414-vert submesh with 0 masked verts) and could never receive blendshape deltas — eyeBlink moved 7 stray verts. Fix: after the label-based mask, geometrically expand it — any vertex inside the labeled head's slightly-padded AABB joins the face rig regardless of its skinning. Measured on Rumba: eyeBlinkLeft 7→205 verts (amplitude 0.0005→0.026, 50x), jawOpen 0.033→0.041, fit max residual 10%→2.9% (the fuller head helps the whole fit). Also adds per-submesh mask-coverage stats to the marker-sim diagnostic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rification Enables a named/first target at weight 1 on the imported mesh, runs the software animation update, and prints the measured vertex displacement — distinguishes 'targets dead' from 'targets subtle' without a GUI. Used to verify that re-imported glb morphs play at FULL amplitude (jawOpen 1.186 on the ARKit reference), i.e. export/import/playback are all healthy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ICT template is 191 connected components (main face shell 14,016 verts; eyeballs, corneas, teeth, mouth interior ~half the vertex count). The NRICP surface fit dragged every interior component onto the outer skin, so eye shapes (eyeLook*/eyeBlink*) transferred onto collapsed geometry and never moved the user's eyes. Now: union-find the template faces into components, NRICP-fit only the main component (landmarks remapped onto it), then place each satellite component by a local least-squares affine estimated from its K=60 nearest fitted main verts. Falls back to the warped template rest on singular/insufficient neighborhoods. Reference self-rig deltas: eyeBlinkLeft 153→395 verts, eyeLookUpLeft 0→278, jawOpen 3,930→5,467 (teeth/mouth interior now follow the jaw), fit max residual 0.50%→0.34%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Assimp's glTF2 exporter drops mesh.extras.targetNames, so a rigged glb re-imported into the editor degraded to Shape_N morph names — losing the ARKit vocabulary face capture matches on. Import: Importer::loadModel reads the <file>.arkit.json sidecar (schema qtmesh-arkit-blendshapes-v1) and passes the ordered names to MeshProcessor as name hints, applied only when the scene has exactly one morphed mesh (the unambiguous case) and the aiMesh carries no names of its own. Export: MeshImporterExporter::exporter writes the same sidecar next to any exported mesh that has poses (GUI parity with the CLI/MCP face-rig paths). Verified: qtmesh morph arkit_reference2.glb --list shows all 51 ARKit names (zero Shape_N); playtest jawOpen plays at the exported amplitude (0.751). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…landmark detection Root cause of 'auto-detect finds nothing' and the checkerboard renders: export-arkit-template.py parsed only the first 3 indices of each face line, but ICT-FaceKit meshes are QUADS — the shipped template (and everything rendered or fit from it) was missing every quad's second triangle. Fan- triangulate in load_obj (26,384 → 52,220 tris) and repack. Detection fixes stacked on top, each verified end-to-end: - Multi-view: render front/back/left/right (+ a fog depth-map fallback pass that is immune to broken normals/winding) and keep the most confident view. Nothing guarantees an asset faces the renderer's front — glTF assets commonly face +Z while front() puts the camera on -Z, and the LH-flip asymmetry between import and glTF export flips facing on every round-trip. - Tight-crop search: MediaPipe FaceMesh is trained on detector-cropped faces (measured presence logit: -27 full-frame, -8.6 head-crop, +19.8 face-crop). detect() now scans silhouette-guided face-square candidates (subject-on-black renders) and refines around the winning landmark bbox. - The template's throwaway render entity now gets smooth vertex normals (it was POSITION-only — undefined normals turned the shaded render into per-triangle noise). - The flat depth material is now double-sided so inconsistently-wound assets don't render as culled-speckle. - MeshDepthRenderer: save/restore the REAL showBoundingBox state (the unconditional restore-to-true leaked debug boxes into later captures), hide other nodes' boxes during capture, and renderDepthMapView gains the same focusAabb head-framing as the shaded path. - Detection debug prints are now runtime-gated only (QTMESH_FACERIG_DEBUG worked only in Debug builds). With the repacked template the full auto-detect chain passes for the first time: template + user views detect at conf 1.00, 13/13 markers seeded, constellation residual 0.061 (trusted), anchored fit max residual 0.23%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hting, soft anchors Three GUI-only failures made in-editor auto-detect useless while the headless CLI path passed (reproduced by driving the live editor via MCP): 1. Frozen RTT: MeshDepthRenderer restores the editor render target after every capture, but GL3+ kept the offscreen viewport cached as active — the next update() skipped the FBO re-bind and rendered into the editor window. All 16 multi-view captures came back bit-identical (the first frame). restoreEditorRenderTarget() now clears the cached viewport (rs->_setViewport(nullptr)) so each capture re-binds. 2. Blown-out capture: scene/user lights stacked on the capture's ambient 0.75 + headlight and saturated the render to a pure-white silhouette. MediaPipe false-positives (presence >= 0.85) on the featureless blob, and because template and user renders degrade to the SAME canonical garbage layout, the constellation gate passed it — garbage anchors, fit max residual 3.65%, and the crumpled eyelid/mouth deltas the user saw. renderShadedView now disables all existing scene lights for the capture (deterministic ambient 0.35 + 0.65 headlight, restored after), and detectMeshLandmarks skips renders whose subject has near-zero intensity variance (a silhouette carries no facial features). 3. Hard anchors: landmark weight only rode alpha, so at the finest anneal level a slightly-off user mark still outweighed the surface term and dragged its neighbourhood off the mesh (spiky eyeBlink/mouthClose deltas). The weight now ALSO decays linearly to zero by the last level — markers steer the coarse alignment, pure surface snapping finishes. Markers act as an initialization, not a constraint. Live-editor rig of the ARKit reference: fit max residual 3.65% -> 0.37%, detection confident (conf 1.00 on the correct view, early-out after 2 renders), simulated imperfect markers stay smooth (max 0.35%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An active marker session (chips, viewport overlays, Rig-from-markers state) belongs to the entity it was started on; switching to another model kept it alive and the stale status line read as if it applied to the new selection. On selectionChanged: cancel the marker session when the selected entity differs from the session's entity (or nothing is selected), and clear the status line when idle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…of-head lock-on MediaPipe's presence sigmoid saturates near 1.0 for both a true face (logit ~+20) and a convincing false positive — measured: the smooth back/ side of a head scores up to 0.87 sigmoid but only ~+2 logit. The old first-hit-above-0.85 early-out could therefore lock onto the BACK view of a backwards-facing import (glb round-trips flip facing) and anchor the whole rig there. detectMeshLandmarks now evaluates ALL four views per render mode and keeps the highest RAW logit — effectively detecting the mesh orientation before the fit, via the detector itself. The crop-candidate and refine passes inside detect() rank by logit too. The depth-mode fallback still only runs when no shaded view reaches a strong-face logit (>= 6). Reference verification (backwards-facing glb): views score front -3.2 / back +19.8 / left -3.2 / right +1.9 — the true face wins by an order of magnitude; live-editor rig fit max residual 0.299%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the head When detection is too weak to trust (stylized/covered faces), the seeded markers fall back to head-box-projected template positions — but the box mapping assumed the mesh faces the template's +Z, so a backwards-facing import got every default on the BACK of the head. detectMeshLandmarks now exposes the mesh's facing (MESH-LOCAL) resolved by a priority ladder: 1. presence-logit winner view — when any view shows positive face evidence; 2. FEET direction — full-body characters only (body > 2.5x head height): horizontal centroid of the feet slab (lowest 8%) vs the ankle slab above it; toes extend forward. Verified on the masked-face bandit where every MediaPipe logit is negative and depth-detail mis-ranks (braids + hat out-detail a covered face); 3. depth-map Laplacian detail winner — busts/heads with no body. seedFaceMarkers yaw-rotates the template coordinates to the resolved cardinal facing before the head-box mapping, so proportional defaults land on the face whichever way the model faces. The trusted-detection path and the ICT reference are unchanged (13/13, residual 0.034, fit max 0.29%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…solidated branch Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> # Conflicts: # .gitignore # THIRD_PARTY_AI_MODELS.md
- NonRigidICP: correspondence search now takes the K=4 nearest triangle CENTROIDS and picks by exact point-triangle distance — a single centroid winner mis-corresponds next to large/sliver triangles (PR #899). - DeformationTransfer: reject malformed buffers (trailing floats, indices outside [0,N)) before dereferencing; anchor the translation gauge of EVERY connected component, not just vertex 0 — the ICT template is dozens of islands (eyeballs, corneas, teeth) and each needs its own anchor row + rhs (PR #900). - FaceRigger: '--max-residual' now gates the MAX fit residual directly (it silently allowed 6x the supplied value); mean gated at a quarter of it. Healthy fits (max <= ~4%) pass the default 8% unchanged (PR #901). - FaceRigAttach::extractGeometry: skip a sharedVertexData pool no submesh references — orphan vertices joined the fit with no triangles (PR #901). - ArkitTemplate::ensureModelBlocking: a synchronous startDownload failure no longer blocks for the full 5-minute timeout ('done' guard, the LLM CLI pattern); Sentry breadcrumbs on download start/ok/fail (PR #898). - export-arkit-template.py: document that tongueOut is deliberately absent — ICT-FaceKit has no tongue expression, 51 real shapes (PR #898). - docs/FACE_RIG_SPIKE.md: escape |Δ| pipes that broke the results table (PR #897). Already addressed by earlier commits (noted for the record): the .arkit.json sidecar is now consumed on import (PR #903, commit e7abe01) and re-rigging replaces existing same-named targets instead of stacking (PR #902). FaceRig sources build into UnitTests via src/CMakeLists.txt — the 34 FaceRig/NRICP/DT tests run green. Verified: reference rig max residual 0.35% under the stricter gate; anchored Rumba sim 3.27%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fernandotonon
changed the title
fernandotonon
changed the base branch from
feat/facerig-slice-f-gui-895
to
master
July 18, 2026 03:33
This was referenced Jul 18, 2026
…aster Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
src/CLIPipeline.cpp-873-873 (1)
873-873: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the reference to the unavailable
mocapcommand.The supplied dispatch table has no
mocapsubcommand, so this help text directs users to a command they cannot run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CLIPipeline.cpp` at line 873, Update the help text containing the `qtmesh mocap --face` reference in the CLI usage output to remove the unavailable mocap command, while preserving the surrounding downloads-on-first-use guidance.src/CLIPipeline.cpp-867-872 (1)
867-872: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse one authoritative ARKit target count. The PR contract specifies 51 targets, while both CLI documentation sites advertise 52.
src/CLIPipeline.cpp#L867-L872: update help from the packaged template's authoritative count.src/CLIPipeline.h#L228-L230: keep the API documentation synchronized with that value.🤖 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/CLIPipeline.cpp` around lines 867 - 872, The CLI help text in src/CLIPipeline.cpp lines 867-872 and the API documentation in src/CLIPipeline.h lines 228-230 both advertise 52 ARKit blendshape targets; update both documentation sites to the authoritative count of 51, keeping the descriptions synchronized.src/test_main.cpp-142-150 (1)
142-150: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winOnly skip preflight for
QTMESH_TESTS_SKIP_OGRE_PREFLIGHT=1.qEnvironmentVariableIsSet()treats any defined value, including0, as enabled, so parse the value explicitly to preserve the documented=1behavior insrc/test_main.cpp:142-150.🤖 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/test_main.cpp` around lines 142 - 150, Update the preflight condition in the test startup flow around tryInitOgre() to skip only when QTMESH_TESTS_SKIP_OGRE_PREFLIGHT has the explicit value "1", rather than whenever the variable is merely defined. Preserve the existing skip message and Ogre preflight behavior for all other values.src/CLIPipeline.cpp-9494-9499 (1)
9494-9499: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject invalid numeric face-rig options.
QString::toInt()/toDouble()fall back to0on bad input, so--max-shapes abcsilently switches to the default “use all shapes” path and--max-residual abcforces a zero residual threshold. Parse withokand reject invalid or out-of-range values before running the rig.🤖 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/CLIPipeline.cpp` around lines 9494 - 9499, Update the --max-shapes and --max-residual parsing in the CLI option handling to use QString::toInt/toDouble with ok flags, reject non-numeric input and values outside their valid ranges, and terminate with the existing argument-error behavior before running the rig. Do not allow parse failures to become zero-valued options or continue with defaults.src/FaceRig/ArkitTemplate.cpp-159-194 (1)
159-194: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the prescribed
file.importbreadcrumb category.Downloading the template is an input/file operation; both lifecycle breadcrumbs currently bypass the repository telemetry taxonomy.
As per coding guidelines, “use
file.import/file.exportfor I/O.”🤖 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/FaceRig/ArkitTemplate.cpp` around lines 159 - 194, Update both SentryReporter::addBreadcrumb calls in the template download flow to use the prescribed “file.import” category instead of “ai.assist.face_rig”. Preserve the existing download-start and success/failure breadcrumb messages and logic.Source: Coding guidelines
src/FaceRig/ArkitTemplate.h-4-7 (1)
4-7: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse 51 consistently for the shipped ARKit template contract.
The exporter deliberately omits unsupported
tongueOut, but several comments and documents still describe 52 generated or stored shapes.
src/FaceRig/ArkitTemplate.h#L4-L7: state that the binary contains 51 expression deltas.docs/FACE_RIG_SPIKE.md#L22-L23: distinguish ICT source expressions from the 51-channel shipped vocabulary.docs/FACE_RIG_SPIKE.md#L70-L70: change the pipeline output to 51 deltas.scripts/spike-facerig.py#L10-L11: describe the production output as 51 supported ARKit channels.scripts/spike-facerig.py#L21-L23: update the contract count and mention the omittedtongueOut.🤖 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/FaceRig/ArkitTemplate.h` around lines 4 - 7, Update the shipped ARKit template documentation to consistently specify 51 expression deltas, reflecting the omitted tongueOut channel. In src/FaceRig/ArkitTemplate.h lines 4-7, state that the binary contains 51 expression deltas; in docs/FACE_RIG_SPIKE.md lines 22-23, distinguish the ICT source expressions from the 51-channel shipped vocabulary, and in line 70 describe pipeline output as 51 deltas; in scripts/spike-facerig.py lines 10-11, describe production output as 51 supported ARKit channels, and in lines 21-23 update the contract count while mentioning omitted tongueOut.docs/FACE_RIG_SPIKE.md-48-48 (1)
48-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced pipeline block.
Use
textto satisfy MD040.Proposed fix
-``` +```text🤖 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 `@docs/FACE_RIG_SPIKE.md` at line 48, Add the text language identifier to the fenced pipeline code block in FACE_RIG_SPIKE.md by changing its opening fence to ```text, while preserving the block contents and closing fence.Source: Linters/SAST tools
docs/FACE_RIG.md-10-13 (1)
10-13: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDescribe FaceRig as a native core with optional ONNX landmark anchoring. The current categorical “no ONNX” wording contradicts the MediaPipe landmark stage.
docs/FACE_RIG.md#L10-L13: state that fitting and deformation transfer require no ONNX, while landmark anchoring optionally does.CLAUDE.md#L384-L384: apply the same distinction in the architecture overview.🤖 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 `@docs/FACE_RIG.md` around lines 10 - 13, Update the FaceRig descriptions in docs/FACE_RIG.md lines 10-13 and CLAUDE.md line 384 to identify it as a native core: fitting and deformation transfer require no ONNX, while landmark anchoring optionally uses ONNX/MediaPipe. Replace the categorical “no ONNX” wording consistently at both sites without changing the remaining architecture details.CLAUDE.md-143-144 (1)
143-144: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStandardize the public contract on 51 generated expression targets. The PR objective and measured result specify 51, while several new passages advertise 52.
CLAUDE.md#L143-L144: change the CLI example to 51 ARKit blendshapes.CLAUDE.md#L384-L384: change the FaceRig overview to 51 expressions.README.md#L190-L192: update the heading and command comment to 51.THIRD_PARTY_AI_MODELS.md#L276-L299: make the asset and verification counts agree on 51.docs/FACE_RIG.md#L1-L8: update the introduction and GUI result count.docs/FACE_RIG.md#L39-L60: update the template and resampling diagram to 51.docs/FACE_RIG.md#L85-L88: retain 51 as the measured attachment count and align earlier claims with it.🤖 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 `@CLAUDE.md` around lines 143 - 144, Standardize all public FaceRig documentation on 51 generated expression targets: update CLAUDE.md lines 143-144 and 384, README.md lines 190-192, THIRD_PARTY_AI_MODELS.md lines 276-299, and docs/FACE_RIG.md lines 1-8, 39-60, and 85-88 so CLI examples, headings, asset and verification counts, introductions, diagrams, and measured attachment claims consistently state 51.src/FaceRig/DeformationTransfer.cpp-189-199 (1)
189-199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve rest geometry for degenerate source triangles.
When
invert3(Vs, Vinv)fails,m_srcRestInvbecomes zero; transfer then computesS = 0and constrains the corresponding fitted triangle to collapse. Track invalid source frames and skip those rows or use an identity gradient instead.Also applies to: 226-240
🤖 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/FaceRig/DeformationTransfer.cpp` around lines 189 - 199, Update the source-frame initialization around m_srcRestInv and the corresponding transfer-row construction around the later shape-gradient logic to handle invert3 failure explicitly: track each degenerate source triangle as invalid, then skip its fitting row or use an identity gradient instead of retaining a zero inverse that forces collapse. Preserve normal transfer behavior for valid source frames.docs/FACE_RIG.md-39-39 (1)
39-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language identifier to the pipeline fence.
Use
textto satisfy MD040.Proposed fix
-``` +```text ArkitTemplate (ICT-FaceKit neutral + 52 expression deltas, one topology)🤖 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 `@docs/FACE_RIG.md` at line 39, Update the code fence in FACE_RIG.md surrounding the ArkitTemplate text to specify the text language identifier, using ```text instead of an untagged fence while preserving the existing content.Source: Linters/SAST tools
src/MCPServer.cpp-2305-2315 (1)
2305-2315: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnforce the advertised numeric constraints.
max_shapesis documented as an integer, but1.5is accepted and truncated; values below zero are also accepted. Validate an integral value of at least zero, and requiremax_residual_pctto be positive.🤖 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 2305 - 2315, Update validation in the FaceRigOptions argument parsing: require max_shapes to be an integral numeric value greater than or equal to zero before assigning it to opts.maxShapes, rejecting fractional and negative values. Require max_residual_pct to be strictly positive before assigning opts.maxFitResidualPct, while preserving the existing error-result behavior.src/FaceRig/FaceRigger.cpp-622-654 (1)
622-654: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdvance progress to the declared total.
The final shape starts at
fitLevels + shapeTotal - 1, and no callback reportstotal, leaving GUI progress one step short. Report the completed shape count or emit a finaltick(total, ...).🤖 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/FaceRig/FaceRigger.cpp` around lines 622 - 654, The shape-transfer loop in the visible `tick` call reports progress before processing each shape, so the final completion count is never reported. Update the progress calculation to use the declared shape total and report completion through the final shape or an additional final `tick(total, ...)`, ensuring the callback reaches the full total without changing cancellation behavior.
🤖 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-arkit-template.py`:
- Around line 115-131: The ARKit export loop currently packages missing or
topology-incompatible expressions. Update the processing around ict_stems,
load_obj, and the neutral mesh to abort immediately when any required OBJ is
missing, when vertex counts differ, or when its face topology/order does not
match the neutral mesh; do not append zero-delta shapes for missing inputs.
In `@scripts/spike-facerig.py`:
- Around line 74-85: Update load_obj to fan-triangulate faces with four or more
vertices instead of retaining only the first three indices: preserve the first
vertex and emit one triangle for each adjacent vertex pair, including both
triangles for ICT quads. Keep OBJ index conversion and existing vertex parsing
unchanged.
In `@scripts/upload-facerig-template.sh`:
- Around line 29-38: Update the upload flow around the upload function and its
arkit_template.bin/ICT license calls to validate both required artifacts before
uploading; exit nonzero with a clear error if either is missing. Keep
missing-file skipping only for the optional FACE_LMK artifact, and allow the
existing done message only after all required uploads succeed.
In `@src/Assimp/Importer.cpp`:
- Around line 243-257: The flat ARKit sidecar name list must become a
per-mesh/primitive mapping. In src/Assimp/Importer.cpp lines 243-257, parse
stable mesh/primitive identifiers with each target’s ordered names and counts,
then pass that mapping through the API; update src/Assimp/MeshProcessor.h lines
36-42 to replace the global vector API with the per-mesh mapping, and update
src/Assimp/MeshProcessor.cpp lines 138-156 to apply and validate each mapping
independently for every morphed mesh, preserving target order and rejecting
mismatched counts.
In `@src/CLIPipeline.cpp`:
- Around line 9525-9526: Update the file.import breadcrumb in the CLIPipeline
import flow, and the corresponding file.export breadcrumb, to preserve their
existing categories while logging only the file basename or extension instead of
fi.absoluteFilePath().
In `@src/FaceRig/ArkitTemplate.cpp`:
- Around line 95-112: Update the template parsing flow around m_neutral,
m_faces, and m_shapes to parse geometry into temporary containers before
modifying members. Validate every face index is within [0, V) and every neutral
position and shape delta is finite; return fail for invalid data. Commit the
temporary geometry to m_neutral, m_faces, and m_shapes only after all validation
succeeds.
In `@src/FaceRig/DeformationTransfer.cpp`:
- Around line 242-253: Update the anchor RHS construction in the m_anchors loop
to transform each component’s template displacement through its
template-to-fitted transform before adding it to m_fitted. Use the existing
per-island transform symbols and apply the mapping independently for each anchor
coordinate, preserving the anchor weight and fitted-space output.
In `@src/FaceRig/FaceLandmarkDetector.cpp`:
- Around line 62-120: Update FaceLandmarkDetector::ensureModelBlocking to record
file-I/O breadcrumbs with SentryReporter::addBreadcrumb when the model download
starts and when it completes. Use the file.import/file.export categories as
appropriate, and add outcome breadcrumbs covering success, download failure, and
timeout while preserving the existing download and cancellation flow.
- Around line 127-166: Clear d->inputNames, d->outputNames, d->inputNamesC, and
d->outputNamesC at the start of FaceLandmarkDetector::load before repopulating
them. Ensure each reload rebuilds the ONNX name vectors solely from the newly
created session, preventing duplicate names and stale c_str pointers.
In `@src/FaceRig/FaceRigAttach.cpp`:
- Around line 316-325: Update writeArkitSidecar and the corresponding importer
to store and consume ordered ARKit morph-name mappings keyed by mesh or pose
handle, rather than a single global names array. Preserve the existing flat
schema only for single-morphed-mesh scenes, and ensure multi-submesh exports
retain each mesh’s names independently through re-import.
- Around line 212-218: Validate a, b, and c against the valid vertex-count bound
nv before converting them to size_t and indexing fullToSub in the
triangle-processing loop. Skip any triangle with an index below zero or greater
than or equal to nv, while preserving the existing submesh-index checks and
insertion behavior for valid triangles.
In `@src/FaceRig/FaceRigger.cpp`:
- Around line 657-672: Update the amplitude safety check in buildFaceRig so
anchor health is evaluated independently of the user amplitude: compare unscaled
transfer displacement, or normalize maxDisp by the configured amplitude before
applying the 0.005 * diag threshold. Preserve the existing invisible-shape retry
behavior while avoiding false retries for low-amplitude or intentionally subtle
rigs.
- Around line 522-528: The satellite fallback in the neighbor-fitting logic must
remain in the fitted coordinate frame instead of copying original tn
coordinates. Update both the insufficient-neighbor branch around k < 4 and the
singular-neighbor fallback around the referenced later block to apply the
regularized/local rigid transform, or at minimum add the nearby main-component
displacement, before assigning fitted satellite vertices.
In `@src/FaceRig/FaceRigLandmarks.cpp`:
- Around line 704-745: Add rotational alignment to constellationResidual in
src/FaceRig/FaceRigLandmarks.cpp (lines 704-745), solving the best global
rotation together with centroid and scale before calculating residuals; retain
the existing degenerate-input safeguards and normalization. Apply the same
global rotational similarity solve in src/FaceRig/FaceRigger.cpp (lines 150-179)
before computing local Gaussian residuals, so pre-warping supports meshes
oriented toward ±X and ±Z.
In `@src/FaceRig/NonRigidICP.cpp`:
- Around line 229-233: Validate all imported face indices at both geometry
boundaries before dereferencing them. In src/FaceRig/NonRigidICP.cpp lines
229-233, update the input checks surrounding the NonRigidICP entry point to
reject malformed template and user face buffers, including indices outside their
corresponding vertex ranges, before constructing triangles or edges. In
src/FaceRig/FaceRigger.cpp lines 288-325, validate every userF index before
indexing fullToSub, and reject invalid input using the existing failure path.
- Around line 402-405: Update the ICP flow around the progress callback to track
whether cancellation occurred when progress returns false, and ensure the final
res.ok assignment also requires that the operation was not aborted. Preserve the
best-fit result and finite-residual handling, while keeping the public callback
cancellation contract reflected in the returned status.
- Around line 304-323: Replace the fixed four-centroid candidate search in the
correspondence loop with an exact triangle-AABB BVH nearest-distance query, or
expand candidates using a proven lower-bound distance criterion until the global
closest point is guaranteed. Update the tree/nearest-query integration around
nearestK and closestPointTriangle so target[i] always uses the true nearest
surface point, while preserving the X[i] fallback when no triangles exist.
In `@src/MCPServer.cpp`:
- Around line 2351-2352: Remove the FaceRig::writeArkitSidecar call from the
export flow so it cannot overwrite the complete morph-name sidecar produced by
MeshImporterExporter::exporter(). Preserve the exporter’s deduplicated pose-name
output and the alignment between sidecar names and exported target indices.
In `@src/MeshDepthRenderer.cpp`:
- Around line 426-442: Move OgreRenderTargetUtil::restoreEditorRenderTarget()
from the normal render path into the restore lambda used by Restorer, ensuring
it executes during RAII cleanup after any update or readRenderTarget early exit.
Remove the standalone call after readRenderTarget while preserving the existing
scene-state restoration order.
---
Minor comments:
In `@CLAUDE.md`:
- Around line 143-144: Standardize all public FaceRig documentation on 51
generated expression targets: update CLAUDE.md lines 143-144 and 384, README.md
lines 190-192, THIRD_PARTY_AI_MODELS.md lines 276-299, and docs/FACE_RIG.md
lines 1-8, 39-60, and 85-88 so CLI examples, headings, asset and verification
counts, introductions, diagrams, and measured attachment claims consistently
state 51.
In `@docs/FACE_RIG_SPIKE.md`:
- Line 48: Add the text language identifier to the fenced pipeline code block in
FACE_RIG_SPIKE.md by changing its opening fence to ```text, while preserving the
block contents and closing fence.
In `@docs/FACE_RIG.md`:
- Around line 10-13: Update the FaceRig descriptions in docs/FACE_RIG.md lines
10-13 and CLAUDE.md line 384 to identify it as a native core: fitting and
deformation transfer require no ONNX, while landmark anchoring optionally uses
ONNX/MediaPipe. Replace the categorical “no ONNX” wording consistently at both
sites without changing the remaining architecture details.
- Line 39: Update the code fence in FACE_RIG.md surrounding the ArkitTemplate
text to specify the text language identifier, using ```text instead of an
untagged fence while preserving the existing content.
In `@src/CLIPipeline.cpp`:
- Line 873: Update the help text containing the `qtmesh mocap --face` reference
in the CLI usage output to remove the unavailable mocap command, while
preserving the surrounding downloads-on-first-use guidance.
- Around line 867-872: The CLI help text in src/CLIPipeline.cpp lines 867-872
and the API documentation in src/CLIPipeline.h lines 228-230 both advertise 52
ARKit blendshape targets; update both documentation sites to the authoritative
count of 51, keeping the descriptions synchronized.
- Around line 9494-9499: Update the --max-shapes and --max-residual parsing in
the CLI option handling to use QString::toInt/toDouble with ok flags, reject
non-numeric input and values outside their valid ranges, and terminate with the
existing argument-error behavior before running the rig. Do not allow parse
failures to become zero-valued options or continue with defaults.
In `@src/FaceRig/ArkitTemplate.cpp`:
- Around line 159-194: Update both SentryReporter::addBreadcrumb calls in the
template download flow to use the prescribed “file.import” category instead of
“ai.assist.face_rig”. Preserve the existing download-start and success/failure
breadcrumb messages and logic.
In `@src/FaceRig/ArkitTemplate.h`:
- Around line 4-7: Update the shipped ARKit template documentation to
consistently specify 51 expression deltas, reflecting the omitted tongueOut
channel. In src/FaceRig/ArkitTemplate.h lines 4-7, state that the binary
contains 51 expression deltas; in docs/FACE_RIG_SPIKE.md lines 22-23,
distinguish the ICT source expressions from the 51-channel shipped vocabulary,
and in line 70 describe pipeline output as 51 deltas; in
scripts/spike-facerig.py lines 10-11, describe production output as 51 supported
ARKit channels, and in lines 21-23 update the contract count while mentioning
omitted tongueOut.
In `@src/FaceRig/DeformationTransfer.cpp`:
- Around line 189-199: Update the source-frame initialization around
m_srcRestInv and the corresponding transfer-row construction around the later
shape-gradient logic to handle invert3 failure explicitly: track each degenerate
source triangle as invalid, then skip its fitting row or use an identity
gradient instead of retaining a zero inverse that forces collapse. Preserve
normal transfer behavior for valid source frames.
In `@src/FaceRig/FaceRigger.cpp`:
- Around line 622-654: The shape-transfer loop in the visible `tick` call
reports progress before processing each shape, so the final completion count is
never reported. Update the progress calculation to use the declared shape total
and report completion through the final shape or an additional final
`tick(total, ...)`, ensuring the callback reaches the full total without
changing cancellation behavior.
In `@src/MCPServer.cpp`:
- Around line 2305-2315: Update validation in the FaceRigOptions argument
parsing: require max_shapes to be an integral numeric value greater than or
equal to zero before assigning it to opts.maxShapes, rejecting fractional and
negative values. Require max_residual_pct to be strictly positive before
assigning opts.maxFitResidualPct, while preserving the existing error-result
behavior.
In `@src/test_main.cpp`:
- Around line 142-150: Update the preflight condition in the test startup flow
around tryInitOgre() to skip only when QTMESH_TESTS_SKIP_OGRE_PREFLIGHT has the
explicit value "1", rather than whenever the variable is merely defined.
Preserve the existing skip message and Ogre preflight behavior for all other
values.
🪄 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: 573bb247-7568-4ad9-bda3-27c8e73b3065
📒 Files selected for processing (52)
.gitignoreCLAUDE.mdREADME.mdTHIRD_PARTY_AI_MODELS.mddocs/FACE_RIG.mddocs/FACE_RIG_SPIKE.mdqml/PropertiesPanel.qmlscripts/export-arkit-template.pyscripts/spike-facerig.pyscripts/upload-facerig-template.shsrc/AppLaunchHandler.cppsrc/Assimp/Importer.cppsrc/Assimp/MeshProcessor.cppsrc/Assimp/MeshProcessor.hsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/FaceRig/ArkitTemplate.cppsrc/FaceRig/ArkitTemplate.hsrc/FaceRig/ArkitTemplate_test.cppsrc/FaceRig/DeformationTransfer.cppsrc/FaceRig/DeformationTransfer.hsrc/FaceRig/DeformationTransfer_test.cppsrc/FaceRig/FaceLandmarkDetector.cppsrc/FaceRig/FaceLandmarkDetector.hsrc/FaceRig/FaceRigAttach.cppsrc/FaceRig/FaceRigAttach.hsrc/FaceRig/FaceRigLandmarks.cppsrc/FaceRig/FaceRigLandmarks.hsrc/FaceRig/FaceRigger.cppsrc/FaceRig/FaceRigger.hsrc/FaceRig/FaceRigger_test.cppsrc/FaceRig/NonRigidICP.cppsrc/FaceRig/NonRigidICP.hsrc/FaceRig/NonRigidICP_test.cppsrc/FaceRig/SparseSolve.cppsrc/FaceRig/SparseSolve.hsrc/FaceRigController.cppsrc/FaceRigController.hsrc/MCPServer.cppsrc/MCPServer.hsrc/MeshDepthRenderer.cppsrc/MeshDepthRenderer.hsrc/MeshImporterExporter.cppsrc/MorphAnimationManager.cppsrc/OgreRenderTargetUtil.hsrc/TransformOperator.cppsrc/TransformOperator.hsrc/commands/MorphCommands.cppsrc/commands/MorphCommands.hsrc/mainwindow.cppsrc/test_main.cpp
Comment on lines
+243
to
+257
| // ARKit blendshape name sidecar (`<file>.arkit.json`, schema | ||
| // qtmesh-arkit-blendshapes-v1): Assimp's glTF2 exporter drops | ||
| // `targetNames`, so shapes in a re-imported glb arrive nameless and would | ||
| // degrade to "Shape_N". Restore the authored ARKit names from the sidecar | ||
| // the face-rig exporters write next to the mesh. | ||
| { | ||
| QFile sidecar(QString::fromStdString(path) + ".arkit.json"); | ||
| if (sidecar.exists() && sidecar.open(QIODevice::ReadOnly)) { | ||
| const QJsonObject root = QJsonDocument::fromJson(sidecar.readAll()).object(); | ||
| if (root.value("schema").toString().startsWith("qtmesh-arkit-blendshapes")) { | ||
| std::vector<std::string> names; | ||
| for (const auto& v : root.value("names").toArray()) | ||
| names.push_back(v.toString().toStdString()); | ||
| if (!names.empty()) | ||
| meshProcessor.setMorphNameHints(std::move(names)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The flat sidecar contract breaks multi-submesh FaceRig round-trips. Separate head, eye, and teeth primitives can expose different morph subsets, but the importer stores one global list and then disables it when multiple meshes morph.
src/Assimp/Importer.cpp#L243-L257: parse a sidecar mapping names to stable mesh/primitive identifiers instead of one global array.src/Assimp/MeshProcessor.cpp#L138-L156: apply and validate the mapping independently for every morphed mesh.src/Assimp/MeshProcessor.h#L36-L42: replace the global vector API with a per-mesh mapping that includes target order/count.
📍 Affects 3 files
src/Assimp/Importer.cpp#L243-L257(this comment)src/Assimp/MeshProcessor.cpp#L138-L156src/Assimp/MeshProcessor.h#L36-L42
🤖 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/Assimp/Importer.cpp` around lines 243 - 257, The flat ARKit sidecar name
list must become a per-mesh/primitive mapping. In src/Assimp/Importer.cpp lines
243-257, parse stable mesh/primitive identifiers with each target’s ordered
names and counts, then pass that mapping through the API; update
src/Assimp/MeshProcessor.h lines 36-42 to replace the global vector API with the
per-mesh mapping, and update src/Assimp/MeshProcessor.cpp lines 138-156 to apply
and validate each mapping independently for every morphed mesh, preserving
target order and rejecting mismatched counts.
Owner
Author
There was a problem hiding this comment.
Acknowledged as a real limitation — the flat v1 sidecar can't map per-primitive target subsets. The import-side guard (apply only when exactly one mesh morphs) makes it safe rather than wrong; a v2 per-submesh schema is planned as a follow-up issue (multi-submesh characters like Mixamo rigs need it). Single-submesh rigs — the reference asset and typical facerig outputs — round-trip correctly today. Fixed in 1ecd88e on the adjacent finding: the MCP path no longer overwrites the exporter's full sidecar.
There was a problem hiding this comment.
@fernandotonon Understood. The single-morphed-mesh guard makes v1 safe for its supported round-trip path; it just intentionally declines name restoration for multi-primitive rigs rather than applying potentially incorrect names.
I’ll treat per-submesh/primitive target mappings as a known v2 follow-up, and won’t consider the current single-submesh behavior a blocker. Would you like me to open the follow-up issue for the v2 sidecar schema?
✏️ Learnings added
Learnt from: fernandotonon
Repo: fernandotonon/QtMeshEditor PR: 903
File: src/Assimp/Importer.cpp:243-257
Timestamp: 2026-07-18T04:55:48.606Z
Learning: In the FaceRig ARKit blendshape import/export workflow, the v1 `<mesh>.arkit.json` sidecar contains one flat ordered morph-name list and is deliberately applied only when exactly one imported mesh has morph targets. This safely supports single-submesh FaceRig round-trips but cannot restore independently ordered target subsets for multi-submesh/primitive rigs (such as head, eyes, and teeth); a v2 per-submesh schema is planned as follow-up work.
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.
Comment on lines
+9525
to
+9526
| SentryReporter::addBreadcrumb(QStringLiteral("file.import"), | ||
| QString("Importing %1").arg(fi.absoluteFilePath())); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not send absolute local paths in telemetry breadcrumbs.
These messages can expose usernames, client names, and directory structure to Sentry. Retain the required file.import/file.export categories, but log only the extension or basename.
Proposed redaction
SentryReporter::addBreadcrumb(QStringLiteral("file.import"),
- QString("Importing %1").arg(fi.absoluteFilePath()));
+ QString("Importing %1").arg(fi.fileName()));
SentryReporter::addBreadcrumb(QStringLiteral("file.export"),
- QString("Exporting %1").arg(QFileInfo(outputPath).absoluteFilePath()));
+ QString("Exporting %1").arg(QFileInfo(outputPath).fileName()));As per coding guidelines, track I/O using file.import and file.export; keep those categories while redacting paths.
Also applies to: 9685-9686
🤖 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/CLIPipeline.cpp` around lines 9525 - 9526, Update the file.import
breadcrumb in the CLIPipeline import flow, and the corresponding file.export
breadcrumb, to preserve their existing categories while logging only the file
basename or extension instead of fi.absoluteFilePath().
Source: Coding guidelines
Comment on lines
+229
to
+233
| NricpResult res; | ||
| const int Nt = int(tmplV.size()/3); | ||
| const int Fu = int(userF.size()/3); | ||
| if (Nt < 3 || Fu < 1 || userV.size() < 9) | ||
| return res; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Validate imported triangle indices at every public geometry boundary. Both paths dereference face indices without confirming they reference existing vertices.
src/FaceRig/NonRigidICP.cpp#L229-L233: reject malformed template and user face buffers before constructing triangles or edges.src/FaceRig/FaceRigger.cpp#L288-L325: validateuserFbefore indexingfullToSubduring head isolation.
📍 Affects 2 files
src/FaceRig/NonRigidICP.cpp#L229-L233(this comment)src/FaceRig/FaceRigger.cpp#L288-L325
🤖 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/FaceRig/NonRigidICP.cpp` around lines 229 - 233, Validate all imported
face indices at both geometry boundaries before dereferencing them. In
src/FaceRig/NonRigidICP.cpp lines 229-233, update the input checks surrounding
the NonRigidICP entry point to reject malformed template and user face buffers,
including indices outside their corresponding vertex ranges, before constructing
triangles or edges. In src/FaceRig/FaceRigger.cpp lines 288-325, validate every
userF index before indexing fullToSub, and reject invalid input using the
existing failure path.
Comment on lines
+304
to
+323
| // find closest surface point per current X_i: broad-phase K | ||
| // nearest centroids, then the EXACT point-triangle distance picks | ||
| // among them — the centroid-nearest triangle alone mis-corresponds | ||
| // next to large/sliver triangles on non-uniform meshes. | ||
| std::vector<Vec3> target(Nt); | ||
| std::vector<int> cand; | ||
| for (int i = 0; i < Nt; ++i) { | ||
| tree.nearestK(X[i], 4, cand); | ||
| double bestD2 = std::numeric_limits<double>::max(); | ||
| Vec3 bestP{0,0,0}; | ||
| for (int cf : cand) { | ||
| const Vec3 p = closestPointTriangle( | ||
| X[i], utri[cf][0], utri[cf][1], utri[cf][2]); | ||
| const Vec3 d = vsub(p, X[i]); | ||
| const double d2 = vdot(d, d); | ||
| if (d2 < bestD2) { bestD2 = d2; bestP = p; } | ||
| } | ||
| target[i] = cand.empty() | ||
| ? X[i] | ||
| : bestP; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not treat four centroid candidates as the nearest surface.
The nearest point on a large or sliver triangle can be closer even when its centroid is not among the nearest four. This produces incorrect correspondences on the arbitrary/non-uniform topologies this API supports. Use a triangle-AABB BVH or expand candidates with a proven distance bound.
🤖 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/FaceRig/NonRigidICP.cpp` around lines 304 - 323, Replace the fixed
four-centroid candidate search in the correspondence loop with an exact
triangle-AABB BVH nearest-distance query, or expand candidates using a proven
lower-bound distance criterion until the global closest point is guaranteed.
Update the tree/nearest-query integration around nearestK and
closestPointTriangle so target[i] always uses the true nearest surface point,
while preserving the X[i] fallback when no triangles exist.
Owner
Author
There was a problem hiding this comment.
K=4-of-centroids is a deliberate approximation: the fit anneals from a rigid prealign, so queries sit near the surface and the candidate set is locally dense; the exact point-triangle test then picks within it. A BVH-exact query is the correct general answer and a good follow-up, but on the meshes this API targets (heads at 10-50k tris) we measured no correspondence regressions vs the previous single-centroid version — this PR already strictly improves it. Noted for the follow-up issue.
There was a problem hiding this comment.
@fernandotonon, understood. Given the intended head-mesh workload, rigid pre-alignment/annealing, and the measured improvement over the prior single-centroid lookup, I agree this is a reasonable bounded approximation for this PR. An exact triangle-AABB BVH query remains a worthwhile generalization, but it need not block this change.
Would you like me to open a GitHub follow-up issue for the exact nearest-surface query?
✏️ Learnings added
Learnt from: fernandotonon
Repo: fernandotonon/QtMeshEditor PR: 903
File: src/FaceRig/NonRigidICP.cpp:311-330
Timestamp: 2026-07-18T04:56:02.897Z
Learning: In `src/FaceRig/NonRigidICP.cpp`, the `KDTree::nearestK(..., 4, ...)` centroid candidate lookup is an intentional performance/quality approximation for 10–50k triangle head meshes. Rigid pre-alignment and annealed fitting keep queries near the surface; exact point-to-triangle distance is evaluated over the local candidate set. Replacing it with an exact triangle-AABB BVH nearest-surface query is deferred follow-up work, not required for the FaceRig PR.
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.
Comment on lines
+426
to
+442
| auto restore = [&]() { | ||
| sm->setAmbientLight(savedAmbient); | ||
| sm->setFog(savedFogMode, savedFogColour, 0.0f, savedFogStart, savedFogEnd); | ||
| if (lightNode) { lightNode->detachAllObjects(); | ||
| sm->getRootSceneNode()->removeAndDestroyChild(lightNode); } | ||
| if (light) sm->destroyLight(light); | ||
| for (auto* l : disabledLights) l->setVisible(true); | ||
| if (gridNode) gridNode->setVisible(gridWasVisible); | ||
| if (targetNode) targetNode->showBoundingBox(targetBoxWasShown); | ||
| for (auto* n : hiddenBoxes) n->showBoundingBox(true); | ||
| for (auto& [n, wasVisible] : hiddenNodes) n->setVisible(wasVisible); | ||
| }; | ||
| struct Restorer { std::function<void()> fn; ~Restorer() { fn(); } } restorer{restore}; | ||
|
|
||
| st.renderTarget->update(); | ||
| QImage rgba = readRenderTarget(size); | ||
| OgreRenderTargetUtil::restoreEditorRenderTarget(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant symbols
rg -n "restoreEditorRenderTarget|readRenderTarget|renderTarget->update|Restorer|setAmbientLight|setFog" src/MeshDepthRenderer.cpp src -S
# Show the surrounding function body around the reported lines
sed -n '360,470p' src/MeshDepthRenderer.cppRepository: fernandotonon/QtMeshEditor
Length of output: 14343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '110,170p' src/MeshDepthRenderer.cpp
printf '\n----\n'
sed -n '250,315p' src/MeshDepthRenderer.cpp
printf '\n----\n'
sed -n '1,120p' src/OgreRenderTargetUtil.h
printf '\n----\n'
sed -n '1,120p' src/MeshDepthRenderer_coverage_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 11522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '270,320p' src/ModelTurntableRenderer.cpp
printf '\n----\n'
sed -n '280,320p' src/MaterialPreviewRenderer.cpp
printf '\n----\n'
sed -n '420,450p' src/MaterialPreviewRenderer.cppRepository: fernandotonon/QtMeshEditor
Length of output: 4426
🌐 Web query:
Ogre 14 RenderTarget update copyContentsToMemory throws Ogre::Exception restoreEditorRenderTarget exception safety
💡 Result:
In Ogre 14, the Ogre::RenderTarget::copyContentsToMemory method does not offer built-in exception safety in the sense of avoiding exceptions; rather, Ogre strictly follows an exception-based error handling model [1]. When an error occurs—such as invalid parameters, GPU state issues, or unsupported operations—Ogre throws an Ogre::Exception [1]. Regarding the specific error mention of "restoreEditorRenderTarget," this is not a standard, publicly documented Ogre engine exception. It likely originates from custom editor-related logic, a plugin, or a specific application-level implementation that wraps or manages RenderTargets. To ensure exception safety when working with Ogre::RenderTarget operations: 1. Use Try-Catch Blocks: The Ogre architecture requires that all engine function calls occur within try{} catch(Ogre::Exception& e) {} blocks to handle errors gracefully [1]. Since Ogre does not use return values to indicate failure, catching these exceptions is the only way to prevent application crashes [1]. 2. Verify Parameters: Ensure you are using the overload of copyContentsToMemory that takes explicitly specified source and destination boxes [2][3]. The older, deprecated overload—which omits these boxes—is known for inconsistent behavior when the destination size does not match the RenderTarget size, which can trigger errors [2][3]. 3. Check Target Capabilities: Be aware that certain RenderTarget types, such as Ogre::MultiRenderTarget, may throw exceptions when copyContentsToMemory is called because they do not support being written to memory or disk [4][5]. If you are encountering a crash specifically related to "restoreEditorRenderTarget," investigate the stack trace to determine if it is being thrown by your own editor framework or a third-party tool rather than the core Ogre library. If the exception is escaping your application code, you must wrap the rendering call that triggers it in a try-catch block to handle the error at the application layer [1].
Citations:
- 1: https://ogrecave.github.io/ogre/api/14/class_ogre_1_1_exception.html
- 2: https://ogrecave.github.io/ogre/api/14/class_ogre_1_1_render_target.html
- 3: https://ogrecave.github.io/ogre/api/latest/class_ogre_1_1_render_target.html
- 4: https://ogrecave.github.io/ogre/api/14/class_ogre_1_1_multi_render_target.html
- 5: https://ogrecave.github.io/ogre-next/api/2.2/class_ogre_1_1_multi_render_target.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "renderDepthMapView\\(" src -S
printf '\n----\n'
rg -n "try \\{|catch \\(" src/MeshDepthRenderer.cpp src -SRepository: fernandotonon/QtMeshEditor
Length of output: 48183
Move restoreEditorRenderTarget() into the RAII cleanup at src/MeshDepthRenderer.cpp:438-442
If update() or readRenderTarget() exits early, the editor viewport stays bound to the offscreen RTT. Add the restore call to restore so it runs on every exit path.
🤖 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/MeshDepthRenderer.cpp` around lines 426 - 442, Move
OgreRenderTargetUtil::restoreEditorRenderTarget() from the normal render path
into the restore lambda used by Restorer, ensuring it executes during RAII
cleanup after any update or readRenderTarget early exit. Remove the standalone
call after readRenderTarget while preserving the existing scene-state
restoration order.
… failure The per-suite test executables link against tests/CMakeLists.txt's own qtmesh_test_common, whose explicit source list didn't include the FaceRig sources while MCPServer.cpp / CLIPipeline.cpp / mainwindow.cpp in that same list now reference them (undefined references on unit-tests-linux). The UnitTests target was unaffected (it GLOBs src recursively), which is why the local build masked it. This was PR #902's P1 review finding — confirmed real by CI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- NonRigidICP: reject out-of-range face indices in BOTH the template and user buffers at the fit() boundary (critical finding); a cancelled fit now reports ok=false even when the partial residuals look finite. - ArkitTemplate::load: validate face indices and reject non-finite positions/deltas — a corrupt bundle can no longer reach the solvers. - FaceRigAttach::headSubmesh: bound-check triangle indices before indexing fullToSub (only negatives were rejected). - FaceRigger: normalise the amplitude out of the invisible-shapes safety net so a healthy rig at low amplitude doesn't trigger the unanchored rerun. - DeformationTransfer: scale island gauge-anchor deltas by the global template->fitted size ratio (template-space deltas had the wrong amplitude on scaled fits; rotationless fits are the pipeline contract). - MCPServer: drop the redundant sidecar write after export — the exporter already writes the FULL deduplicated pose-name list; overwriting it with only the newly-attached names dropped pre-existing morph targets. - FaceLandmarkDetector: download breadcrumbs + the synchronous-failure done-guard (same pattern as ArkitTemplate). - CLIPipeline: facerig import breadcrumb logs the extension only, not the absolute path (telemetry privacy). - scripts: spike-facerig.py fan-triangulates ICT quads (same bug as the packer); export-arkit-template.py aborts on missing expressions or a face-list mismatch instead of packaging dead zero shapes; upload-facerig-template.sh refuses to publish without the template and ICT license. All 34 FaceRig/NRICP/DT tests pass; reference rig unchanged (max residual 0.35%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The CI harness treats ANY skipped test as a suite failure (the sole remaining red in unit-tests-linux: 462/463 suites). Follow the SkinEvaluate env-gated convention: SUCCEED() + return when QTMESH_FACERIG_TEMPLATE is unset. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/MCPServer.cpp (1)
2327-2329: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the correct Sentry breadcrumb category for MCP tools.
The Sentry breadcrumb category used here is
"ai.assist.face_rig", but as per coding guidelines, all MCP tool calls must be tracked using the"ai.tool_call"category.🛠️ Proposed fix
- SentryReporter::addBreadcrumb(QStringLiteral("ai.assist.face_rig"), + SentryReporter::addBreadcrumb(QStringLiteral("ai.tool_call"), QStringLiteral("add_arkit_blendshapes entity=%1 max_shapes=%2") .arg(QString::fromStdString(entity->getName())).arg(opts.maxShapes));🤖 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 2327 - 2329, Update the SentryReporter::addBreadcrumb call for the add_arkit_blendshapes MCP tool to use the "ai.tool_call" category instead of "ai.assist.face_rig". Preserve the existing breadcrumb message and arguments.Source: Coding guidelines
src/FaceRig/FaceRigger.cpp (2)
318-324: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winValidate upper face-index bounds before indexing
fullToSub.When
headMaskis active, a malformed face containinga,b, orc >= nuFullreachesfullToSub[size_t(...)]after only negative-index checks. This can crash beforeNonRigidICP::fit()performs its own validation.const int a = userF[f], b = userF[f+1], c = userF[f+2]; - if (a < 0 || b < 0 || c < 0) continue; + if (a < 0 || b < 0 || c < 0 || + a >= nuFull || b >= nuFull || c >= nuFull) continue;🤖 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/FaceRig/FaceRigger.cpp` around lines 318 - 324, Update the face-processing loop that builds subF to validate a, b, and c are less than nuFull before indexing fullToSub. Keep the existing negative-index and sub-index checks, skipping any face with an out-of-range upper index so malformed faces cannot access fullToSub.
46-87: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not cap nearest-neighbor search without accounting for query distance.
rMaxis derived only from the populated grid span. If a user vertex lies farther outside that span,nearest()returns-1; Lines 609-610 then silently skip its transferred delta. Use a distance-aware termination bound or fall back to an exact scan when no candidate was found.🤖 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/FaceRig/FaceRigger.cpp` around lines 46 - 87, Update FaceRigger::nearest so queries outside the populated grid cannot terminate at the span-based rMax without finding a point. Replace the current fixed cap with a query-distance-aware bound, or add an exact scan over all points when the shell search finds no candidate, ensuring valid nearest indices are returned whenever m_pts contains points and only empty inputs return -1.src/FaceRig/ArkitTemplate.cpp (1)
94-128: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winParse into temporary containers to prevent a partially loaded state.
While the added validation logic is correct, mutating member variables directly during parsing leaves the object in a partially populated, inconsistent state if a validation check fails midway through the file. Please complete the previous review's suggestion: parse the geometry into temporary containers, validate them, and only commit them to members once all checks have passed.
📦 Proposed fix
- m_vertexCount = V; - m_faceCount = F; - m_neutral.resize(size_t(V) * 3); - for (auto& v : m_neutral) { + std::vector<float> tmpNeutral(size_t(V) * 3); + for (auto& v : tmpNeutral) { v = rdF32(p); if (!std::isfinite(v)) return fail(QStringLiteral("non-finite neutral position")); } - m_faces.resize(size_t(F) * 3); - for (auto& i : m_faces) { + std::vector<int32_t> tmpFaces(size_t(F) * 3); + for (auto& i : tmpFaces) { i = rdI32(p); // out-of-range indices would be dereferenced by the fit / renders if (i < 0 || i >= V) return fail(QStringLiteral("face index %1 out of range (V=%2)") .arg(i).arg(V)); } - m_shapes.clear(); - m_shapes.reserve(S); + std::vector<ArkitShape> tmpShapes; + tmpShapes.reserve(S); for (int s = 0; s < S; ++s) { if (p + kNameLen > end) return fail(QStringLiteral("shape %1 name overruns").arg(s)); ArkitShape shape; shape.name = QString::fromLatin1(p, qstrnlen(p, kNameLen)); p += kNameLen; shape.deltas.resize(size_t(V) * 3); for (auto& d : shape.deltas) { d = rdF32(p); if (!std::isfinite(d)) return fail(QStringLiteral("non-finite delta in shape %1") .arg(shape.name)); } - m_shapes.push_back(std::move(shape)); + tmpShapes.push_back(std::move(shape)); } + + m_vertexCount = V; + m_faceCount = F; + m_neutral = std::move(tmpNeutral); + m_faces = std::move(tmpFaces); + m_shapes = std::move(tmpShapes); + return true;🤖 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/FaceRig/ArkitTemplate.cpp` around lines 94 - 128, Update the parsing routine containing m_vertexCount, m_faceCount, m_neutral, m_faces, and m_shapes to build vertex/face/shape data in local temporary containers while reading and validating the file. Only after every check succeeds should the validated counts and containers be moved into the corresponding members; any fail(...) path must leave the existing object state unchanged.
🤖 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.
Outside diff comments:
In `@src/FaceRig/ArkitTemplate.cpp`:
- Around line 94-128: Update the parsing routine containing m_vertexCount,
m_faceCount, m_neutral, m_faces, and m_shapes to build vertex/face/shape data in
local temporary containers while reading and validating the file. Only after
every check succeeds should the validated counts and containers be moved into
the corresponding members; any fail(...) path must leave the existing object
state unchanged.
In `@src/FaceRig/FaceRigger.cpp`:
- Around line 318-324: Update the face-processing loop that builds subF to
validate a, b, and c are less than nuFull before indexing fullToSub. Keep the
existing negative-index and sub-index checks, skipping any face with an
out-of-range upper index so malformed faces cannot access fullToSub.
- Around line 46-87: Update FaceRigger::nearest so queries outside the populated
grid cannot terminate at the span-based rMax without finding a point. Replace
the current fixed cap with a query-distance-aware bound, or add an exact scan
over all points when the shell search finds no candidate, ensuring valid nearest
indices are returned whenever m_pts contains points and only empty inputs return
-1.
In `@src/MCPServer.cpp`:
- Around line 2327-2329: Update the SentryReporter::addBreadcrumb call for the
add_arkit_blendshapes MCP tool to use the "ai.tool_call" category instead of
"ai.assist.face_rig". Preserve the existing breadcrumb message and arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a57d848-a847-444a-ac11-653396a05e9a
📒 Files selected for processing (14)
scripts/export-arkit-template.pyscripts/spike-facerig.pyscripts/upload-facerig-template.shsrc/CLIPipeline.cppsrc/FaceRig/ArkitTemplate.cppsrc/FaceRig/ArkitTemplate_test.cppsrc/FaceRig/DeformationTransfer.cppsrc/FaceRig/DeformationTransfer.hsrc/FaceRig/FaceLandmarkDetector.cppsrc/FaceRig/FaceRigAttach.cppsrc/FaceRig/FaceRigger.cppsrc/FaceRig/NonRigidICP.cppsrc/MCPServer.cpptests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- src/FaceRig/DeformationTransfer.h
- src/FaceRig/ArkitTemplate_test.cpp
- src/FaceRig/DeformationTransfer.cpp
- src/FaceRig/FaceRigAttach.cpp
- src/FaceRig/NonRigidICP.cpp
- src/CLIPipeline.cpp
- src/FaceRig/FaceLandmarkDetector.cpp
…crumb category, atomic template load - FaceRigger: upper-bound face-index check in the headMask subset path (critical — malformed faces reached fullToSub before NRICP's own validation); the delta-resample grid's nearest() now clamps its SEARCH ORIGIN into the populated bounds so a query far outside the grid can't exhaust the shell cap and silently drop that vertex's transferred delta (distances still measured to the real query — result unchanged). - MCPServer: add_arkit_blendshapes breadcrumb uses the ai.tool_call category per the MCP house rule (and drops the entity name). - ArkitTemplate::load: parse into temporaries and commit only after all validation passes — no half-populated object on a corrupt bundle. All FaceRig suites pass; reference rig unchanged (max residual 0.35%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
fernandotonon
added a commit
that referenced
this pull request
Jul 19, 2026
…ink failure The per-suite test executables link tests/CMakeLists.txt's own qtmesh_test_common, whose explicit source list didn't include the Mocap sources while MCPServer.cpp / mainwindow.cpp in that same list reference them (undefined references to MocapController, FaceCapPredictor, RecordMocapClipCommand, … on unit-tests-linux). Add the Mocap sources (they self-guard with #ifdef ENABLE_MOCAP → empty TU when off) and link Qt6::Multimedia into the test lib under ENABLE_MOCAP (the frame sources include QCamera/QMediaPlayer headers). Mirrors how the main build wires UnitTests, and how the FaceRig sources were added for #903. The UnitTests target was unaffected (it GLOBs src recursively) — which is why the local build masked it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 22, 2026
Closed
Closed
Closed
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidated PR for the face auto-rig epic (#889) — supersedes the stacked slice PRs #897, #898, #899, #900, #901, #902 (all closed in favour of this one; their review findings are addressed here, see commit
d234722d).What this does
✨ Add ARKit Blendshapes (AI)on any humanoid face mesh: fit the ICT-FaceKit template (MIT) to the user's neutral head via native non-rigid ICP (Amberg 2007), transfer the 51 ARKit expressions (ICT has no tongueOut) via deformation transfer (Sumner-Popović 2004), and attach them as namedOgre::Posemorph targets — so face performance capture (#869) can drive any rigged character. Deterministic geometry, zero new dependencies, no GPL.Surfaces
qtmesh facerig <file> -o out [--max-shapes N] [--max-residual PCT] [--json]add_arkit_blendshapesRobustness highlights (the part that took the tuning)
.arkit.jsonsidecar written on export and consumed on import.Models / hosting
facerig/arkit_template.bin(ICT-FaceKit, MIT — repacked with full quad triangulation) andfacerig/face_landmarks.onnx(MediaPipe Face Mesh V2, Apache-2.0) hosted onfernandotonon/QtMeshEditor-models; standalone model card atfernandotonon/QtMeshEditor-facemesh-onnx. Both download on first use.THIRD_PARTY_AI_MODELS.md; docs indocs/FACE_RIG.md+docs/FACE_RIG_SPIKE.md.Verification
🤖 Generated with Claude Code
Summary by CodeRabbit
qtmesh facerigCLI for ARKit blendshape generation (including--max-shapes,--max-residual, and--json) plus an MCP action to attach ARKit blendshapes.