Face-Rig Slice B (#890): ArkitTemplate loader + bundle export + HF hosting - #898
Closed
fernandotonon wants to merge 1 commit into
Closed
Face-Rig Slice B (#890): ArkitTemplate loader + bundle export + HF hosting#898fernandotonon wants to merge 1 commit into
fernandotonon wants to merge 1 commit into
Conversation
…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>
13 tasks
📝 WalkthroughWalkthroughAdds an offline ICT-FaceKit-to-ARKit template exporter, a validated native binary loader with download support, publishing automation, build integration, and tests for valid, invalid, missing, and exported template files. ChangesARKit template pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant ArkitTemplate
participant ModelDownloader
participant TemplateFile
Application->>ArkitTemplate: ensureModelBlocking()
ArkitTemplate->>TemplateFile: check modelPath()
ArkitTemplate->>ModelDownloader: startDownload() when missing
ModelDownloader->>TemplateFile: write arkit_template.bin
ArkitTemplate->>TemplateFile: verify downloaded file
Application->>ArkitTemplate: load(template path)
Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: e61ead6fab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+74
to
+75
| ("noseSneerLeft", ["noseSneer_L"]), ("noseSneerRight", ["noseSneer_R"]), | ||
| ] |
There was a problem hiding this comment.
Because this exporter list ends at noseSneerRight, it produces only 51 records even though the surrounding comments and the FaceCap/ARKit vocabulary are the 52-channel set. In any face-capture flow that emits the tongueOut coefficient, rigs generated from this bundle will have no corresponding morph target, so that channel can never animate; if ICT has no mesh for it, adding a zero tongueOut record would still preserve the expected ordering/name set.
Useful? React with 👍 / 👎.
Comment on lines
+174
to
+175
| dl->startDownload(url, dest, label); | ||
| loop.exec(); |
There was a problem hiding this comment.
When ModelDownloader::startDownload() fails synchronously (for example another model download is already active, or the .part file cannot be opened), it emits downloadError before returning. The connected lambda then calls loop.quit() before loop.exec() starts, so this helper still enters the event loop and blocks the caller until the 5-minute timeout; users can hit this by invoking face-rig model loading while the shared downloader is busy or the destination is unwritable.
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/FaceRig/ArkitTemplate.cpp`:
- Around line 55-183: Add SentryReporter::addBreadcrumb calls to
ArkitTemplate::load for the template file.import operation and to
ensureModelBlocking for the significant model download, using the appropriate
breadcrumb categories and including relevant path or download context. Keep the
existing load and download behavior unchanged.
- Around line 127-183: Add QTMESH_FACERIG_NO_DOWNLOAD to the existing
kNoDownloadGuards collection used by test or download suppression logic. Ensure
the guard is recognized before ArkitTemplate::ensureModelBlocking() can call
ModelDownloader::startDownload, preserving the early-return behavior when the
environment variable is set.
🪄 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: a0da9580-4b5f-4e28-887a-8aa7452f6b08
📒 Files selected for processing (7)
scripts/export-arkit-template.pyscripts/upload-facerig-template.shsrc/CMakeLists.txtsrc/FaceRig/ArkitTemplate.cppsrc/FaceRig/ArkitTemplate.hsrc/FaceRig/ArkitTemplate_test.cppsrc/test_main.cpp
Comment on lines
+55
to
+183
| bool ArkitTemplate::load(const QString& path, QString* error) | ||
| { | ||
| auto fail = [&](const QString& msg) { | ||
| if (error) | ||
| *error = msg; | ||
| return false; | ||
| }; | ||
|
|
||
| QFile f(path); | ||
| if (!f.open(QIODevice::ReadOnly)) | ||
| return fail(QStringLiteral("cannot open %1").arg(path)); | ||
| const QByteArray blob = f.readAll(); | ||
| f.close(); | ||
|
|
||
| // header: magic(8) + 3*int32 | ||
| if (blob.size() < kMagicLen + 12) | ||
| return fail(QStringLiteral("template too small / truncated")); | ||
| if (std::memcmp(blob.constData(), kMagic, kMagicLen) != 0) | ||
| return fail(QStringLiteral("bad magic (not an arkit_template.bin)")); | ||
|
|
||
| const char* p = blob.constData() + kMagicLen; | ||
| const char* end = blob.constData() + blob.size(); | ||
| const int V = rdI32(p); | ||
| const int F = rdI32(p); | ||
| const int S = rdI32(p); | ||
| if (V <= 0 || F <= 0 || S <= 0 || V > 5'000'000 || S > 128) | ||
| return fail(QStringLiteral("implausible header (V=%1 F=%2 S=%3)") | ||
| .arg(V).arg(F).arg(S)); | ||
|
|
||
| // exact byte budget check up front so a corrupt file can't over-read | ||
| const qint64 need = qint64(kMagicLen) + 12 + qint64(V) * 3 * 4 | ||
| + qint64(F) * 3 * 4 | ||
| + qint64(S) * (kNameLen + qint64(V) * 3 * 4); | ||
| if (blob.size() < need) | ||
| return fail(QStringLiteral("template truncated (need %1 bytes, have %2)") | ||
| .arg(need).arg(blob.size())); | ||
|
|
||
| m_vertexCount = V; | ||
| m_faceCount = F; | ||
| m_neutral.resize(size_t(V) * 3); | ||
| for (auto& v : m_neutral) | ||
| v = rdF32(p); | ||
| m_faces.resize(size_t(F) * 3); | ||
| for (auto& i : m_faces) | ||
| i = rdI32(p); | ||
|
|
||
| m_shapes.clear(); | ||
| m_shapes.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); | ||
| m_shapes.push_back(std::move(shape)); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| QString ArkitTemplate::modelPath() | ||
| { | ||
| const QString dataPath = | ||
| QStandardPaths::writableLocation(QStandardPaths::AppDataLocation); | ||
| return QDir(dataPath).filePath(QStringLiteral("ai_models/facerig/") | ||
| + QString::fromLatin1(kModelFile)); | ||
| } | ||
|
|
||
| bool ArkitTemplate::present() { return QFileInfo::exists(modelPath()); } | ||
|
|
||
| QString ArkitTemplate::ensureModelBlocking() | ||
| { | ||
| const QString dest = modelPath(); | ||
| if (QFileInfo::exists(dest)) | ||
| return dest; | ||
| if (!qEnvironmentVariableIsEmpty("QTMESH_FACERIG_NO_DOWNLOAD")) | ||
| return {}; | ||
|
|
||
| QString base; | ||
| { | ||
| QSettings s; | ||
| base = s.value(QString::fromLatin1(kBaseUrlSettingsKey)).toString(); | ||
| if (base.isEmpty()) { | ||
| const QByteArray env = qgetenv("QTMESH_FACERIG_MODEL_BASE_URL"); | ||
| base = env.isEmpty() ? QString::fromLatin1(kDefaultModelBaseUrl) | ||
| : QString::fromUtf8(env); | ||
| } | ||
| } | ||
| if (base.isEmpty()) | ||
| return {}; | ||
| if (!base.endsWith('/')) | ||
| base += '/'; | ||
|
|
||
| auto* dl = ModelDownloader::instance(); | ||
| if (!dl) | ||
| return {}; | ||
|
|
||
| QDir().mkpath(QFileInfo(dest).absolutePath()); | ||
| const QString url = base + QString::fromLatin1(kModelFile); | ||
| const QString label = QStringLiteral("ARKit face template"); | ||
|
|
||
| QEventLoop loop; | ||
| bool ok = false, timedOut = false; | ||
| auto onDone = QObject::connect(dl, &ModelDownloader::downloadCompleted, &loop, | ||
| [&](const QString& name, const QString&) { | ||
| if (name == label) { ok = true; loop.quit(); } | ||
| }); | ||
| auto onErr = QObject::connect(dl, &ModelDownloader::downloadError, &loop, | ||
| [&](const QString& name, const QString&) { | ||
| if (name == label) { ok = false; loop.quit(); } | ||
| }); | ||
| QTimer timeout; | ||
| timeout.setSingleShot(true); | ||
| QObject::connect(&timeout, &QTimer::timeout, &loop, | ||
| [&]() { timedOut = true; loop.quit(); }); | ||
| timeout.start(300000); // 5 min — the template is ~17 MB | ||
|
|
||
| dl->startDownload(url, dest, label); | ||
| loop.exec(); | ||
|
|
||
| QObject::disconnect(onDone); | ||
| QObject::disconnect(onErr); | ||
| if (timedOut && dl) | ||
| dl->cancelDownload(); | ||
|
|
||
| return (ok && !timedOut && QFileInfo::exists(dest)) ? dest : QString(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
No SentryReporter::addBreadcrumb calls for template load/download.
load() performs a file.import-style read and ensureModelBlocking() performs a significant first-use network download, but neither adds a breadcrumb. As per coding guidelines, "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb; use ui.action for toolbar/menu clicks, ai.tool_call for MCP calls, and file.import/file.export for 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 55 - 183, Add
SentryReporter::addBreadcrumb calls to ArkitTemplate::load for the template
file.import operation and to ensureModelBlocking for the significant model
download, using the appropriate breadcrumb categories and including relevant
path or download context. Keep the existing load and download behavior
unchanged.
Source: Coding guidelines
Comment on lines
+127
to
+183
| QString ArkitTemplate::ensureModelBlocking() | ||
| { | ||
| const QString dest = modelPath(); | ||
| if (QFileInfo::exists(dest)) | ||
| return dest; | ||
| if (!qEnvironmentVariableIsEmpty("QTMESH_FACERIG_NO_DOWNLOAD")) | ||
| return {}; | ||
|
|
||
| QString base; | ||
| { | ||
| QSettings s; | ||
| base = s.value(QString::fromLatin1(kBaseUrlSettingsKey)).toString(); | ||
| if (base.isEmpty()) { | ||
| const QByteArray env = qgetenv("QTMESH_FACERIG_MODEL_BASE_URL"); | ||
| base = env.isEmpty() ? QString::fromLatin1(kDefaultModelBaseUrl) | ||
| : QString::fromUtf8(env); | ||
| } | ||
| } | ||
| if (base.isEmpty()) | ||
| return {}; | ||
| if (!base.endsWith('/')) | ||
| base += '/'; | ||
|
|
||
| auto* dl = ModelDownloader::instance(); | ||
| if (!dl) | ||
| return {}; | ||
|
|
||
| QDir().mkpath(QFileInfo(dest).absolutePath()); | ||
| const QString url = base + QString::fromLatin1(kModelFile); | ||
| const QString label = QStringLiteral("ARKit face template"); | ||
|
|
||
| QEventLoop loop; | ||
| bool ok = false, timedOut = false; | ||
| auto onDone = QObject::connect(dl, &ModelDownloader::downloadCompleted, &loop, | ||
| [&](const QString& name, const QString&) { | ||
| if (name == label) { ok = true; loop.quit(); } | ||
| }); | ||
| auto onErr = QObject::connect(dl, &ModelDownloader::downloadError, &loop, | ||
| [&](const QString& name, const QString&) { | ||
| if (name == label) { ok = false; loop.quit(); } | ||
| }); | ||
| QTimer timeout; | ||
| timeout.setSingleShot(true); | ||
| QObject::connect(&timeout, &QTimer::timeout, &loop, | ||
| [&]() { timedOut = true; loop.quit(); }); | ||
| timeout.start(300000); // 5 min — the template is ~17 MB | ||
|
|
||
| dl->startDownload(url, dest, label); | ||
| loop.exec(); | ||
|
|
||
| QObject::disconnect(onDone); | ||
| QObject::disconnect(onErr); | ||
| if (timedOut && dl) | ||
| dl->cancelDownload(); | ||
|
|
||
| return (ok && !timedOut && QFileInfo::exists(dest)) ? dest : QString(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the test guard list plus the helper.
git ls-files | rg '(^|/)(test_main\.cpp|ArkitTemplate\.cpp)$'
echo '--- test_main.cpp guard list ---'
sed -n '1,220p' src/test_main.cpp | nl -ba | sed -n '1,220p'
echo '--- ArkitTemplate.cpp around ensureModelBlocking ---'
sed -n '100,240p' src/FaceRig/ArkitTemplate.cpp | nl -ba | sed -n '100,240p'Repository: fernandotonon/QtMeshEditor
Length of output: 285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '(^|/)(test_main\.cpp|ArkitTemplate\.cpp)$'
echo '--- test_main.cpp guard list ---'
sed -n '1,220p' src/test_main.cpp | nl -ba | sed -n '1,220p'
echo '--- ArkitTemplate.cpp around ensureModelBlocking ---'
sed -n '100,240p' src/FaceRig/ArkitTemplate.cpp | nl -ba | sed -n '100,240p'Repository: fernandotonon/QtMeshEditor
Length of output: 285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ensureModelBlocking|QTMESH_FACERIG_NO_DOWNLOAD|kNoDownloadGuards" src/test_main.cpp src/FaceRig/ArkitTemplate.cpp src -g '!**/build/**'Repository: fernandotonon/QtMeshEditor
Length of output: 7767
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo 'src/test_main.cpp'; [ -f src/test_main.cpp ] && grep -n "kNoDownloadGuards\|QTMESH_FACERIG_NO_DOWNLOAD" -n src/test_main.cpp || true
echo 'src/FaceRig/ArkitTemplate.cpp'; grep -n "ensureModelBlocking\|QTMESH_FACERIG_NO_DOWNLOAD" -n src/FaceRig/ArkitTemplate.cpp || trueRepository: fernandotonon/QtMeshEditor
Length of output: 443
Add QTMESH_FACERIG_NO_DOWNLOAD to kNoDownloadGuards.
Without it, ArkitTemplate::ensureModelBlocking() can still try a real download during tests and sit in the 5-minute event loop.
🤖 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 127 - 183, Add
QTMESH_FACERIG_NO_DOWNLOAD to the existing kNoDownloadGuards collection used by
test or download suppression logic. Ensure the guard is recognized before
ArkitTemplate::ensureModelBlocking() can call ModelDownloader::startDownload,
preserving the early-return behavior when the environment variable is set.
|
This was referenced Jul 14, 2026
fernandotonon
added a commit
that referenced
this pull request
Jul 18, 2026
- 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>
Owner
Author
|
Consolidated into #903 (single epic PR, retargeted to master). Review findings from this PR are addressed there — see commit d234722d. |
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.
Part of epic #889. Closes #890. Follows the Slice A spike (#897).
What this adds
The ARKit-blendshape template the deformation-transfer face-rig (#889) copies from — loaded, packed, and hosted.
scripts/export-arkit-template.py(offline, not shipped): packs the ICT-FaceKit MIT template (neutral + 52 ARKit expression meshes) into one compact little-endianarkit_template.bin. Bakes the ICT→ARKit name map inFaceCap::kBlendshapeNamesorder (single/centered channels likebrowInnerUp/cheekPuffsum the ICT_L/_Rhalves; the rest 1:1). Real bundle: 51 shapes, 17 MB, 26,719-vert neutral.src/FaceRig/ArkitTemplate.{h,cpp}— Ogre-free loader for that binary with strict bounds/truncation checks; the NonRigidICP (Face-Rig Slice B — ArkitTemplate loader + template hosting (ICT-FaceKit MIT head + 52 shapes) #891) / DeformationTransfer (Face-Rig Slice C — NonRigidICP (pure-data, headless-tested): template -> user neutral fit #892) stages consume it. House model-management:AppData/ai_models/facerig/, download-on-first-use (QTMESH_FACERIG_MODEL_BASE_URL/ai/facerigModelBaseUrl/QTMESH_FACERIG_NO_DOWNLOAD).ArkitTemplate_test.cpp): header/neutral/faces/shapes parse, bad-magic + truncation + missing-file rejection, and an env-gated test against the real 17 MB bundle (checks the 51 ARKit names + nonzero jawOpen). All pass.scripts/upload-facerig-template.sh— HF hosting (template + ICT MIT LICENSE).test_main.cpp:QTMESH_TESTS_SKIP_OGRE_PREFLIGHTso the pure-data FaceRig suites run without GL (CI unaffected).Verification
facerig/arkit_template.binresolves at HTTP 200 with the exact byte size (16,990,916); the ICT MIT LICENSE sits beside it.ensureModelBlocking()runs.Next: Slice C (#891) — NonRigidICP (the native template→user fit).
🤖 Generated with Claude Code
Summary by CodeRabbit