Face-Rig Slice A (#896): spike — NRICP + deformation transfer proven, ICT-FaceKit MIT - #897
Face-Rig Slice A (#896): spike — NRICP + deformation transfer proven, ICT-FaceKit MIT#897fernandotonon wants to merge 1 commit into
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>
📝 WalkthroughWalkthroughAdds ICT-FaceKit licensing and face-rig spike documentation, plus an offline Python prototype that fits a template with NRICP, transfers expression deformations, exports ARKit targets, and records quality metrics. ChangesFace auto-rig feasibility spike
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant OBJLoader
participant NRICP
participant DeformationTransfer
participant ReportWriter
CLI->>OBJLoader: Load template and user OBJ meshes
CLI->>NRICP: Fit template to user surface
NRICP-->>CLI: Return fitted correspondence
CLI->>DeformationTransfer: Transfer expression deformation
DeformationTransfer-->>CLI: Return user-vertex morph deltas
CLI->>ReportWriter: Export targets and JSON metrics
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 896e112460
ℹ️ 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".
| ev, _ = load_obj(ep) | ||
| # express the template expression through the SAME correspondence: | ||
| # correspondence of the expression = corr + (expr - neutral) template delta | ||
| expr_corr = corr + (ev - tv) |
There was a problem hiding this comment.
When the user head is in a different scale/unit than the ICT template, this adds raw template-space expression deltas directly to the fitted user-space correspondence. The NRICP fit scales/transforms the neutral template onto uv, but (ev - tv) is never put through that fitted scale/local affine, so a mesh authored in meters vs centimeters will get blendshape motions that are orders of magnitude too large or too small while the fit report still looks normalized to the user diagonal. The spike should transform these deltas through the fitted correspondence/local affine before resampling them to user vertices.
Useful? React with 👍 / 👎.
| # correspondence of the expression = corr + (expr - neutral) template delta | ||
| expr_corr = corr + (ev - tv) | ||
| user_delta = deformation_transfer(uv, uf, corr, expr_corr) | ||
| arkit = ICT_TO_ARKIT.get(stem, stem) |
There was a problem hiding this comment.
With the default --shapes path this processes every ICT expression OBJ, but any stem missing from this partial map falls back to the raw ICT filename. For the full FaceKit set, expressions such as additional mouth/nose/tongue shapes will therefore be exported/reported under non-FaceCap::kBlendshapeNames names instead of the canonical ARKit targets, so the spike does not actually prove or provide the promised 52-name contract for Slice B/D. Please either complete the map or fail loudly on unmapped stems.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/spike-facerig.py (1)
160-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove dead code and unused variables from prototyping.
Several functions and variables from earlier prototyping iterations remain in the code but are never used. Removing this vestigial code reduces unnecessary overhead and prevents the C++ implementers (Slices C/D) from accidentally porting dead math.
scripts/spike-facerig.py#L160-L161: remove the unusedDmatrix (which currently triggers a redundantcsr_matrixallocation inside the inner loop).scripts/spike-facerig.py#L144-L145: remove the unusedGandkronvariables.scripts/spike-facerig.py#L92-L104: remove the unusedumeyamarigid alignment function.scripts/spike-facerig.py#L244-L251: remove the unusedtri_framefunction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/spike-facerig.py` around lines 160 - 161, Remove the unused D sparse matrix allocation inside the inner loop, and remove the unused G and kron variables. Delete the unused umeyama rigid-alignment function and tri_frame function; apply these removals at scripts/spike-facerig.py lines 160-161, 144-145, 92-104, and 244-251 respectively.
🤖 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 `@docs/FACE_RIG_SPIKE.md`:
- Line 91: Update the jawOpen row in the Markdown table to escape the pipe
characters around Δ, preserving the displayed “|Δ|” text while preventing
Markdown from treating them as column separators.
In `@scripts/spike-facerig.py`:
- Around line 154-156: Update the comment immediately above the target
assignment using closest_on_tris to describe an exact point-to-triangle
projection, and remove the centroid/nearest-vertex approximation wording. Keep
the implementation unchanged so the C++ port follows the exact projection
behavior.
- Around line 162-165: Hoist the constant build_data(Th) result and the
alpha-dependent build_stiffness(M, alpha) plus vstack assembly out of the
innermost iters_per loop. Construct Amat once per alpha before that loop, then
reuse it for every iteration while preserving the existing solve behavior.
---
Nitpick comments:
In `@scripts/spike-facerig.py`:
- Around line 160-161: Remove the unused D sparse matrix allocation inside the
inner loop, and remove the unused G and kron variables. Delete the unused
umeyama rigid-alignment function and tri_frame function; apply these removals at
scripts/spike-facerig.py lines 160-161, 144-145, 92-104, and 244-251
respectively.
🪄 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: f7f16672-44bb-4800-8439-f4b452fe4b65
📒 Files selected for processing (4)
.gitignoreTHIRD_PARTY_AI_MODELS.mddocs/FACE_RIG_SPIKE.mdscripts/spike-facerig.py
| | ARKit shape | max displacement | verts moved | semantics check | | ||
| |---|---|---|---| | ||
| | NRICP surface fit | mean **0.003%** / max **0.59%** of diag | — | template lands on user surface | | ||
| | jawOpen | 9.0% | 6,771 | lower face ΔY −0.32 (drops), forehead |Δ| 0.002 (still) ✅ | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the pipe characters to fix table rendering.
The pipe characters in |Δ| are interpreted as table column separators by Markdown, breaking the table rendering for this row.
📝 Proposed formatting fix
-| jawOpen | 9.0% | 6,771 | lower face ΔY −0.32 (drops), forehead |Δ| 0.002 (still) ✅ |
+| jawOpen | 9.0% | 6,771 | lower face ΔY −0.32 (drops), forehead \|Δ\| 0.002 (still) ✅ |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | jawOpen | 9.0% | 6,771 | lower face ΔY −0.32 (drops), forehead |Δ| 0.002 (still) ✅ | | |
| | jawOpen | 9.0% | 6,771 | lower face ΔY −0.32 (drops), forehead \|Δ\| 0.002 (still) ✅ | |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 91-91: Table column count
Expected: 4; Actual: 6; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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 91, Update the jawOpen row in the Markdown
table to escape the pipe characters around Δ, preserving the displayed “|Δ|”
text while preventing Markdown from treating them as column separators.
| # closest point = project X onto that triangle (approx: centroid-nudged | ||
| # to nearest vertex of the tri for a cheap point-to-point target) | ||
| target = closest_on_tris(X, user_v, user_f, tri_idx) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Correct the misleading documentation about the projection method.
The comment explicitly states the projection is an approximation ("centroid-nudged to nearest vertex"), but the implementation of closest_on_tris performs the exact point-to-triangle projection mathematically.
It is critical to fix this comment so the C++ port (Slices C/D) doesn't mistakenly implement a cheap vertex-snap, which would severely degrade the reported sub-1% fit quality.
📝 Proposed comment fix
- # closest point = project X onto that triangle (approx: centroid-nudged
- # to nearest vertex of the tri for a cheap point-to-point target)
+ # closest point = project X exactly onto that triangle for an
+ # accurate point-to-surface target📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # closest point = project X onto that triangle (approx: centroid-nudged | |
| # to nearest vertex of the tri for a cheap point-to-point target) | |
| target = closest_on_tris(X, user_v, user_f, tri_idx) | |
| # closest point = project X exactly onto that triangle for an | |
| # accurate point-to-surface target | |
| target = closest_on_tris(X, user_v, user_f, tri_idx) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/spike-facerig.py` around lines 154 - 156, Update the comment
immediately above the target assignment using closest_on_tris to describe an
exact point-to-triangle projection, and remove the centroid/nearest-vertex
approximation wording. Keep the implementation unchanged so the C++ port follows
the exact projection behavior.
| # data rows: for each vertex, th_i (1x4) times A_i (4x3) -> point | ||
| data_rows = build_data(Th) # (n, 4n) | ||
| stiff = build_stiffness(M, alpha) # (4E, 4n) | ||
| Amat = vstack([stiff, data_rows]).tocsr() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Hoist constant matrix constructions out of the inner loop.
The data matrix data_rows is entirely constant (derived from the resting template Th), and stiff only changes when alpha changes. Consequently, the entire left-hand side matrix Amat is constant for any given alpha.
Rebuilding these csr_matrix systems in the innermost iters_per loop causes massive unnecessary allocation and assembly overhead. Hoisting them is a critical optimization for both this script and the eventual C++ port to ensure they don't repeat the redundant assembly.
⚡ Proposed performance fix
+ data_rows = build_data(Th) # (n, 4n)
for alpha in stiffness:
+ stiff = build_stiffness(M, alpha) # (4E, 4n)
+ Amat = vstack([stiff, data_rows]).tocsr()
for _ in range(iters_per):
# data term: A_i * th_i ~= closest user point to current X_i
_, tri_idx = tree.query(X)
# closest point = project X onto that triangle (approx: centroid-nudged
# to nearest vertex of the tri for a cheap point-to-point target)
target = closest_on_tris(X, user_v, user_f, tri_idx)
# Build sparse system: [ alpha * (M kron G) ; D ] A = [ 0 ; target ]
# A is stacked as (4n x 3). D picks th_i per row.
D = csr_matrix((np.repeat(1.0, n),
(np.arange(n), np.arange(n))), shape=(n, n))
- # data rows: for each vertex, th_i (1x4) times A_i (4x3) -> point
- data_rows = build_data(Th) # (n, 4n)
- stiff = build_stiffness(M, alpha) # (4E, 4n)
- Amat = vstack([stiff, data_rows]).tocsr()
rhs = np.vstack([np.zeros((stiff.shape[0], 3)), target])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # data rows: for each vertex, th_i (1x4) times A_i (4x3) -> point | |
| data_rows = build_data(Th) # (n, 4n) | |
| stiff = build_stiffness(M, alpha) # (4E, 4n) | |
| Amat = vstack([stiff, data_rows]).tocsr() | |
| data_rows = build_data(Th) # (n, 4n) | |
| for alpha in stiffness: | |
| stiff = build_stiffness(M, alpha) # (4E, 4n) | |
| Amat = vstack([stiff, data_rows]).tocsr() | |
| for _ in range(iters_per): | |
| # data term: A_i * th_i ~= closest user point to current X_i | |
| _, tri_idx = tree.query(X) | |
| # closest point = project X onto that triangle (approx: centroid-nudged | |
| # to nearest vertex of the tri for a cheap point-to-point target) | |
| target = closest_on_tris(X, user_v, user_f, tri_idx) | |
| # Build sparse system: [ alpha * (M kron G) ; D ] A = [ 0 ; target ] | |
| # A is stacked as (4n x 3). D picks th_i per row. | |
| D = csr_matrix((np.repeat(1.0, n), | |
| (np.arange(n), np.arange(n))), shape=(n, n)) | |
| rhs = np.vstack([np.zeros((stiff.shape[0], 3)), target]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/spike-facerig.py` around lines 162 - 165, Hoist the constant
build_data(Th) result and the alpha-dependent build_stiffness(M, alpha) plus
vstack assembly out of the innermost iters_per loop. Construct Amat once per
alpha before that loop, then reuse it for every iteration while preserving the
existing solve behavior.
|
- 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>
|
Consolidated into #903 (single epic PR, retargeted to master). Review findings from this PR are addressed there — see commit d234722d. |
1 participant
Part of epic #889. Closes #896. The de-risk spike for auto-generating ARKit blendshapes on any humanoid mesh — offline prototype + decision records, no app code.
Verdict: GO
Both algorithms proven on a real, different-topology head with a permissive (MIT) template. Slices B–G are an engineering port of a working prototype, not open research.
What was proven (
scripts/spike-facerig.py, not shipped)Fit the MIT ICT-FaceKit generic-neutral template (26,719 verts) to a 12,763-vert, 60%-decimated user head (genuinely different topology), then transfer ICT's ARKit expressions onto the user's topology:
It's a deterministic sparse-linear-algebra pipeline — no ML, no ONNX, no model download at inference (only the MIT template asset downloads on first use, like other bundled assets). Same class as
GeodesicVoxelBind/QuadRetopo.Deliverables
scripts/spike-facerig.py— the prototype (NRICP + deformation transfer + quality measurement + the ICT→ARKit name map).docs/FACE_RIG_SPIKE.md— the implementable contract for Slices C/D: NRICP params + stiffness anneal, the deformation-transfer linear system, the OBJ multi-group gotcha, the humanoid/orientation/landmark risks, and the two quality upgrades the C++ port must make over the spike (full deformation-gradient transfer; optional landmark constraints).THIRD_PARTY_AI_MODELS.md— ICT-FaceKit MIT entry (the "full model" USC tier rejected).Acceptance criteria (from #896)
THIRD_PARTY_AI_MODELS.md(MIT — PASS).docs/FACE_RIG_SPIKE.mdwith the implementable NRICP + deformation-transfer contract and go/no-go.Next: Slice B (#890) — bundle + host the ICT template.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation