feat(#407): native auto-rigging (Pinocchio is LGPL → native template embedding)#754
feat(#407): native auto-rigging (Pinocchio is LGPL → native template embedding)#754fernandotonon wants to merge 12 commits into
Conversation
Pinocchio (Baran & Popović 2007) is LGPL-2.1, which conflicts with the project's statically-linked permissive-distribution stance — so, like #401 (Instant Meshes) and #402 (libigl/TetGen), this is a native from-scratch implementation of the published *algorithm* (skeleton-template embedding), zero new deps. - AutoRig (src/AutoRig.h/.cpp): Ogre-free pure-data core — built-in templates (humanoid 19-bone / biped / quadruped / generic), fitTemplate() maps a template's normalised joint graph into the mesh AABB then recentres flagged joints toward per-height-slab centroids (spine→medial line, limb roots inside the silhouette). rigEntity() builds an Ogre::Skeleton (parent-relative bone positions, setBindingPose), binds via mesh->_notifySkeleton + entity ->_initialise(true) — the _initialise is REQUIRED or the exporters (both gate on entity->hasSkeleton()) silently drop the new rig. - AutoRigController (QML singleton, mirrors SkinWeightsController) for the GUI. - CLI: `qtmesh rig <file> [--skeleton T] [--skin] [--up-axis x|y|z] -o out` (cmdRig) — import, rig, optionally chain SkinWeights::computeAndApply, export. Registered in run() dispatch + AppLaunchHandler subcommand list. - AutoRig_test.cpp: pure-data unit tests (template well-formedness, AABB containment, vertical ordering, degenerate-input robustness, string/JSON). - Sentry breadcrumb ai.assist.auto_rig. Verified end-to-end: static OBJ -> 19-bone humanoid + skin -> glTF export with 1 skin / 17 joints; FBX export carries the skeleton too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- MCP: `auto_rig` { template, skin?, up_axis?, output_path? }
(MCPServer::toolAutoRig) — rigs the selected static mesh, optional skin chain
+ optional re-export. Registered + advertised. Breadcrumb ai.assist.auto_rig.
- GUI: AutoRigDialog.qml (template + up-axis pickers, "also skin" checkbox)
driven by AutoRigController; new "Rigging" CollapsibleSection in Animation
Mode → Mode Tools, gated on AutoRigController.hasRiggableSelection (a static
mesh — already-rigged meshes show "Skinning" instead). Lazy-loaded Loader +
openAutoRigDialog(), registered in qml_resources.qrc.
- Tests: CLIPipeline_cmdrig_coverage_test.cpp (arg-validation + file-missing
branches need no GL; success path skips gracefully without Xvfb).
- CLAUDE.md: CLI examples (skin + rig), recognized-subcommand list, and a full
AutoRig architecture entry (incl. the LGPL→native rationale, the
_initialise(true) export gotcha, and documented quality limits).
App + UnitTests build clean on macOS arm64.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 59 minutes and 16 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a native automatic rigging feature that fits predefined skeleton templates (humanoid, biped, quadruped, generic) onto unrigged Ogre mesh entities using AABB-based joint placement with optional vertex-centroid recentering. The pipeline is exposed via a new ChangesAuto-Rig Pipeline
macOS CI Cache Improvements
Sequence DiagramsequenceDiagram
participant User
participant AutoRigDialog
participant AutoRigController
participant AutoRig
participant OgreSkeleton
User->>AutoRigDialog: click Auto-Rig (template=humanoid, alsoSkin=true)
AutoRigDialog->>AutoRigController: autoRigSelected("humanoid", "y", true)
AutoRigController->>AutoRigController: emit busyChanged(true)
AutoRigController->>AutoRig: rigEntity(entity, opts)
AutoRig->>AutoRig: fitTemplate (AABB fit + recenter joints)
AutoRig->>OgreSkeleton: create bones, parent/child hierarchy
OgreSkeleton->>OgreSkeleton: setBindingPose()
AutoRig->>OgreSkeleton: _notifySkeleton, _initialise(true)
AutoRig-->>AutoRigController: Report{applied=true, boneCount=19}
AutoRigController->>AutoRigController: emit busyChanged(false)
AutoRigController-->>AutoRigDialog: QVariantMap result
AutoRigDialog-->>User: display success status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: db6937ebe1
ℹ️ 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".
| sectionVisible: root.currentTab === root.modeToolsTab | ||
| && root.modeToolMatches(EditorModeController.AnimationMode) | ||
| && AutoRigController.hasRiggableSelection |
There was a problem hiding this comment.
This binding references AutoRigController, but the app uses the QRC/manual-registration path for PropertiesPanel singletons: the neighboring controllers such as QuadRetopoController and SkinWeightsController are explicitly registered in mainwindow.cpp, and no AutoRigController registration was added. With qt_add_qml_module disabled, QML_ELEMENT alone will not make this singleton available, so the Rigging section/dialog hits ReferenceError: AutoRigController is not defined and the GUI auto-rig entry point cannot work.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bede436 — registered AutoRigController via qmlRegisterSingletonType under the 'PropertiesPanel' URI in mainwindow.cpp (next to SkinWeights/QuadRetopo) + added its kill(). Verified at runtime: no ReferenceError when the Rigging section/dialog loads.
| const r = AutoRigController.autoRigSelected( | ||
| dialog.templates[dialog.templateIndex], | ||
| dialog.alsoSkin) |
There was a problem hiding this comment.
When a user selects x or z in the dialog's Up axis picker, runRig() still calls autoRigSelected() with only the template and skin flag. The controller constructs AutoRig::Options with the default +Y axis, so the GUI always rigs as Y-up even though the UI advertises X/Z support, which misplaces skeletons for meshes using those axes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bede436 — autoRigSelected now takes upAxis; the dialog passes dialog.upAxes[dialog.upAxisIndex] and the controller maps x/y/z → Options::upAxis.
unit-tests-linux failed to link: AutoRig::* symbols undefined in libqtmesh_test_common.a (MCPServer::toolAutoRig and CLIPipeline::cmdRig reference them). The test target has its own TEST_SRC_FILES list separate from the app's src/CMakeLists.txt — add AutoRig.cpp + AutoRigController.cpp there, next to SkinWeights (same omission class as #738's PbrMapSynth gap). Co-Authored-By: Claude Opus 4.8 (1M context) <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: 9
🧹 Nitpick comments (2)
src/AutoRigController.cpp (1)
88-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider the
selectionChangedemit at line 114.After rigging completes,
hasRiggableSelection()will returnfalsebecause the entity now has a skeleton. The manualemit selectionChanged()refreshes QML property bindings so the dialog button disables correctly.This works but couples entity-state changes to the selection-change signal. If this pattern causes confusion later, consider adding a dedicated
riggabilityChanged()signal.🤖 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/AutoRigController.cpp` around lines 88 - 114, The current implementation uses emit selectionChanged() at the end of the AutoRig::rigEntity operation to refresh QML bindings after the entity rigging state changes, but this semantically misuses the selection-change signal for a rigging-state change. Add a dedicated riggabilityChanged() signal to the class header, then replace the emit selectionChanged() call at the end of the try block with emit riggabilityChanged() to make the intent clearer and decouple entity-state changes from the selection-change signal.src/MCPServer.cpp (1)
6412-6421: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPublish enum constraints for finite string options.
templateandup_axisare documented as fixed value sets, but the schema exposes free-form strings. Addenumso MCP clients can validate calls before runtime.Proposed fix
props["template"] = QJsonObject{{"type", "string"}, + {"enum", QJsonArray{"humanoid", "biped", "quadruped", "generic"}}, {"description", "Skeleton template: 'humanoid' (19-bone, default), 'biped', " "'quadruped', or 'generic' (3-joint spine fallback)."}}; @@ props["up_axis"] = QJsonObject{{"type", "string"}, + {"enum", QJsonArray{"x", "y", "z"}}, {"description", "Mesh up axis: 'x', 'y' (default), or 'z'."}};🤖 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 6412 - 6421, The `template` and `up_axis` properties in the schema definitions have documented finite value sets but lack `enum` constraints, preventing MCP clients from validating input before runtime. Add an `enum` field to each QJsonObject to specify the allowed values: for the `template` property include the enum values ['humanoid', 'biped', 'quadruped', 'generic'], and for the `up_axis` property include the enum values ['x', 'y', 'z']. This allows clients to validate calls against these constraints before execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qml/AutoRigDialog.qml`:
- Around line 41-58: In the runRig() function, the call to
AutoRigController.autoRigSelected() is missing the upAxis parameter that the
user selected via the up axis picker. Add dialog.upAxes[dialog.upAxisIndex] as a
parameter to the autoRigSelected() method call between the template parameter
and the alsoSkin parameter to pass the user's selected up axis value to the
controller.
In `@qml/PropertiesPanel.qml`:
- Around line 4200-4205: The openAutoRigDialog() function does not handle the
case where autoRigLoader enters an error state, which leaves the loader in a
permanent dead state where active is true but item is null, preventing any
subsequent retries. Add a third condition to check if autoRigLoader is in an
error state (when active is true but item is null, indicating a failed load),
and in that case, reset active to false to allow the loader to be retried on the
next click. Reference the error-retry pattern already implemented in the
openIsometricSpritesDialog() function for consistency.
In `@src/AutoRig.cpp`:
- Around line 325-335: When entity initialization fails with an Ogre exception,
the mesh still references the skeleton even though the skeleton resource is
being removed. In the catch block that handles the Ogre::Exception, after
calling skelMgr.remove(skelName), you must also detach the skeleton from the
mesh to clear its reference. Add a call to mesh->_notifySkeleton with a
null/empty skeleton value (similar to how it was attached earlier) to reset the
mesh's skeleton reference and ensure hasSkeleton() returns false on subsequent
operations.
- Around line 221-237: The vbuf->lock() call can return nullptr if the buffer is
write-only or the lock fails, but the code does not check for this before
dereferencing base in the loop that calls baseVertexPointerToElement. Add a null
check after the lock() call for the base pointer, and if it is null, shrink the
out vector back to base0 (using out.resize(base0)) to avoid leaving
uninitialized slots, then return false to indicate the operation failed.
In `@src/AutoRigController.h`:
- Around line 37-38: The autoRigSelected method in AutoRigController.h is
missing the upAxis parameter that the AutoRigDialog.qml UI is collecting from
the user's segmented control selection. Add a const QString& upAxis parameter to
the autoRigSelected method signature in AutoRigController.h, and then update the
corresponding method call in AutoRigDialog.qml to pass
dialog.upAxes[dialog.upAxisIndex] as the third argument so the user's selected
axis choice is actually transmitted to the backend instead of being ignored.
In `@src/MCPServer.cpp`:
- Around line 1709-1721: The try/catch block surrounding AutoRig::rigEntity and
SkinWeights::computeAndApply only catches Ogre::Exception, but export operations
occur outside this block and std::runtime_error exceptions are not caught,
allowing them to bypass the MCP error response path. Either extend the catch
block to also catch std::runtime_error in addition to Ogre::Exception to match
the error handling pattern used by the runOgreOp helper, or expand the try block
to wrap the complete mutating and export section to ensure all potential errors
(including export failures and std::runtime_error) are properly caught and
converted to MCP error responses.
- Around line 1711-1716: When the SkinWeights computeAndApply() method fails in
the code block where report.applied is true and alsoSkin is requested, the
current implementation only sets report.error but does not prevent the handler
from returning success. To fix this, in addition to setting the error message
when sw.applied is false, also set report.applied to false so that the handler
properly returns an error instead of continuing with an unskinned export. This
ensures that a failed skin operation (when requested with skin=true) causes the
overall operation to fail rather than silently exporting without proper
skinning.
- Around line 1734-1735: The SentryReporter::addBreadcrumb call for the auto_rig
export operation includes the full outputPath in the message, which may contain
sensitive information such as usernames or private project directories. Modify
the message parameter passed to addBreadcrumb to redact or remove the path
details while keeping the breadcrumb itself intact for I/O operation tracking.
Either omit the path from the message entirely or replace the arg(outputPath)
with a generic indicator that does not expose the actual file system location.
- Around line 1726-1728: The code in MCPServer.cpp does not validate the type of
the `output_path` argument before processing it. When
`args["output_path"].toString()` is called on a non-string value, it silently
converts to an empty string, allowing malformed MCP calls to report success
without exporting anything. Add a type validation check for the `output_path`
argument similar to how `skin` and `template` parameters are validated, to
ensure the argument is a string before proceeding with the export logic, and
handle malformed inputs appropriately by returning an error instead of silently
succeeding.
---
Nitpick comments:
In `@src/AutoRigController.cpp`:
- Around line 88-114: The current implementation uses emit selectionChanged() at
the end of the AutoRig::rigEntity operation to refresh QML bindings after the
entity rigging state changes, but this semantically misuses the selection-change
signal for a rigging-state change. Add a dedicated riggabilityChanged() signal
to the class header, then replace the emit selectionChanged() call at the end of
the try block with emit riggabilityChanged() to make the intent clearer and
decouple entity-state changes from the selection-change signal.
In `@src/MCPServer.cpp`:
- Around line 6412-6421: The `template` and `up_axis` properties in the schema
definitions have documented finite value sets but lack `enum` constraints,
preventing MCP clients from validating input before runtime. Add an `enum` field
to each QJsonObject to specify the allowed values: for the `template` property
include the enum values ['humanoid', 'biped', 'quadruped', 'generic'], and for
the `up_axis` property include the enum values ['x', 'y', 'z']. This allows
clients to validate calls against these constraints before execution.
🪄 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: 24f2ce69-7806-4adc-9d43-fed3cb63eae8
📒 Files selected for processing (16)
CLAUDE.mdqml/AutoRigDialog.qmlqml/PropertiesPanel.qmlsrc/AppLaunchHandler.cppsrc/AutoRig.cppsrc/AutoRig.hsrc/AutoRigController.cppsrc/AutoRigController.hsrc/AutoRig_test.cppsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CLIPipeline_cmdrig_coverage_test.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/qml_resources.qrc
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Status (automated): The CI hasn't re-run yet: GitHub Actions appears to have stalled repo-wide (no workflow run created for any branch since ~15:50 UTC, despite Actions being enabled — pushes, an empty re-trigger commit |
# Conflicts: # CLAUDE.md
Code review (Codex + CodeRabbit) on the auto-rig PR: - CRITICAL: register AutoRigController as a QML singleton in mainwindow.cpp (PropertiesPanel URI) like the sibling controllers + add its kill(). With qt_add_qml_module disabled, QML_SINGLETON alone doesn't expose it, so the Rigging section/dialog would ReferenceError. Verified no error at runtime now. - CRITICAL: the dialog's Up-axis picker was ignored — autoRigSelected() didn't take upAxis. Added a `const QString& upAxis` param (controller maps x/y/z → Options::upAxis) and pass dialog.upAxes[dialog.upAxisIndex] from QML. - AutoRig::appendPositions: guard a null vbuf->lock() (shrink `out` back, return false) instead of dereferencing. - AutoRig::rigEntity: on _initialise failure, detach the half-built skeleton (mesh->_notifySkeleton(null)) before removing it, so hasSkeleton() resets and a retry / exporter doesn't pick up a partial rig. - MCP toolAutoRig: validate output_path type; a requested skin that fails is now a hard error (no unskinned export reported as success); export wrapped in the try/catch (also catches std::exception); Sentry breadcrumb no longer logs the full output path. - PropertiesPanel openAutoRigDialog(): handle Loader.Error to allow retry. App + UnitTests build clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…atch) build-macos kept failing with "No rule to make target '.../Xcode_26.5.../libz.tbd'" even after the Pin-Xcode step: the producer (build-n-cache-ogre-macos) resolved "newest" to Xcode 26.3 on its runner image and cached OGRE with 26.3's absolute libz.tbd path, while the consumer (build-macos) resolved 26.5 and linked against the missing path. "newest" (sort -V | tail -1) is NOT deterministic across the per-job runner images. Fold the resolved Xcode app name into XCODE_TAG (exported by the Pin step) and append it to all macOS assimp/ogre cache keys + restore-keys. A consumer on a different Xcode now cache-misses and rebuilds OGRE/Assimp against its own SDK instead of linking a stale path. (Belongs on master too — same deploy.yml.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
build-macos (incl. the 3.9.1 release deploy) failed:
No rule to make target '.../Xcode_26.5/.../libz.tbd', needed by QtMeshEditor
Diagnosis: the Pin-Xcode step reliably selects Xcode 26.3 on ALL macOS jobs
(verified across runs), so compilation is consistent — but the restored OGRE
cache was built earlier under Xcode 26.5 and its CMake export hardcodes 26.5's
libz.tbd path. Restoring that into a 26.3 build breaks the link.
Fix: bump MACOS_CACHE_VERSION xcode26b → xcode263 so OGRE/Assimp are rebuilt
under the currently-pinned Xcode (26.3) and the stale 26.5 cache is discarded.
Also reverted the earlier XCODE_TAG-in-cache-key experiment: build-macos only
RESTORES the OGRE cache (no rebuild step), so a per-job Xcode-keyed miss would
leave it with no OGRE at all ("Could not find OGRE"). With Xcode pinned
consistently, a plain version bump is the correct, sufficient fix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The real cause of the build-macos libz.tbd failure: the producer (build-n-cache-ogre-macos) and consumer (build-macos) run on DIFFERENT runner images whose "newest Xcode" differs — producer resolved Xcode 26.5 and cached OGRE with 26.5's absolute libz.tbd path baked into its CMake export; consumer resolved 26.3 and linked against the missing 26.5 path. Just pinning "newest" or bumping the cache version doesn't help because the two images disagree. Fix (self-healing): - Fold the resolved Xcode app name into XCODE_TAG and append it to all macOS assimp/ogre cache keys + restore-keys, so a job only restores a cache built under its OWN Xcode. - Give build-macos (consumer) the same "check out + build OGRE on cache miss" steps the producer has. When the consumer's Xcode differs from the producer's (cache miss), it rebuilds OGRE under its own SDK instead of failing on a stale libz.tbd path. This makes the macOS build robust regardless of which Xcode each runner image ships. (Bigger than the earlier one-line bump, but that couldn't fix a cross-image Xcode disagreement.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Previous commit Xcode-keyed BOTH the assimp and ogre macOS caches. That broke build-macos on a runner whose Xcode differed from the producer's: assimp cache-missed (no assimp-build-on-miss exists) so find_package(assimp) failed with "Could not find a package configuration file provided by assimp". Assimp is a plain static lib that doesn't bake absolute SDK paths, so one assimp cache is valid across Xcode versions — revert XCODE_TAG on the 3 assimp keys, keeping it ONLY on the 2 ogre keys (ogre's CMake export DOES bake an absolute libz.tbd path, which is why ogre needs per-Xcode keying + the consumer's rebuild-on-miss). The shared assimp cache is then always present for the ogre rebuild to link against. Verified on the failing run: Qt + OGRE now resolve and link (no libz.tbd error); this removes the remaining assimp-not-found failure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/deploy.yml:
- Around line 1806-1812: In the "Check out ogre repo (cache miss)" step, replace
the unpinned `actions/checkout@master` reference with a pinned commit SHA (e.g.,
`actions/checkout@<full-commit-sha>`), and add `persist-credentials: false` to
the `with` block to prevent unnecessary token exposure. This applies the same
security practices to the OGRE cache-miss checkout that should be used across
all checkout actions in the workflow.
🪄 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: d3c65e2e-ed51-47fd-91f1-97b380bfe674
📒 Files selected for processing (11)
.github/workflows/deploy.ymlCLAUDE.mdqml/AutoRigDialog.qmlqml/PropertiesPanel.qmlsrc/AutoRig.cppsrc/AutoRigController.cppsrc/AutoRigController.hsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/mainwindow.cpp
✅ Files skipped from review due to trivial changes (2)
- src/mainwindow.cpp
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/MCPServer.h
- qml/PropertiesPanel.qml
- qml/AutoRigDialog.qml
- src/AutoRig.cpp
- src/MCPServer.cpp
- src/CLIPipeline.cpp
| - if: steps.cache-ogre-macos.outputs.cache-hit != 'true' | ||
| name: Check out ogre repo (cache miss) | ||
| uses: actions/checkout@master | ||
| with: | ||
| repository: OGRECave/ogre | ||
| ref: v${{ env.OGRE_VERSION }} | ||
| path: ${{github.workspace}}/ogre |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "deploy.yml" -o -name "*.yml" | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 164
🏁 Script executed:
fd -t f "deploy.yml" 2>/dev/nullRepository: fernandotonon/QtMeshEditor
Length of output: 164
🏁 Script executed:
git ls-files | grep -E "(workflow|deploy)" | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 263
🏁 Script executed:
sed -n '1800,1820p' .github/workflows/deploy.ymlRepository: fernandotonon/QtMeshEditor
Length of output: 1393
🌐 Web query:
GitHub Actions checkout action best practices security pinning commit SHA
💡 Result:
Pinning GitHub Actions—including the checkout action—to a full-length commit SHA is a critical security best practice to prevent supply chain attacks [1][2][3]. Why Pinning is Necessary GitHub Action tags (e.g., @v4) are mutable references, meaning a repository maintainer or an attacker who compromises an action's repository can reassign the tag to a malicious commit [2][4][5]. By pinning to a specific, immutable full-length commit SHA (a 40-character hash), you ensure that your workflow executes only the exact code you have vetted [6][5][7]. Best Practices for SHA Pinning 1. Use Full-Length SHAs: Always use the full 40-character SHA. Short SHAs are vulnerable to collisions and are not recommended [6]. 2. Include Human-Readable Comments: Because SHAs are not human-readable, append the original version tag as a comment [6][8]. This maintains context for updates and is required by many automation tools to function correctly [9][8]. Example: - uses: actions/checkout@b4ffde6 # v4.1.1 3. Automate Updates: Use tools like Dependabot or Renovate to keep your pinned SHAs up to date [1][2][10]. These tools can automatically open pull requests when new releases are available, mitigating the maintenance burden of manual pinning [1][9]. 4. Enforce via Policy: Organizations can mandate SHA pinning through GitHub's built-in policy features, which prevent workflows from running if they use unpinned actions [11][5][7]. 5. Audit Your Workflows: Use security auditing tools such as OSSF Scorecard, zizmor, or other dedicated pinning tools (e.g., pinact, ratchet, frizbee) to detect and remediate unpinned actions in your repositories [3][10][8]. While GitHub-owned actions (actions/*) are generally considered lower risk due to internal supply chain controls, current security guidance emphasizes a uniform "pin-everything" approach for all third-party dependencies to maximize protection [9][7][3].
Citations:
- 1: https://emmer.dev/blog/pin-your-github-actions-to-protect-against-mutability/
- 2: https://asecurityengineer.com/posts/hardening-github-actions-workflows/
- 3: https://github.com/github/docs/blob/962a1c8dccb8c0f66548b324e5b921b5e4fbc3d6/content/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions.md
- 4: https://nearform.com/insights/why-you-should-pin-your-github-actions-by-commit-hash/
- 5: https://www.romainlespinasse.dev/posts/github-actions-commit-sha-pinning/
- 6: https://adaptive-enforcement-lab.com/secure/github-actions-security/action-pinning/sha-pinning/
- 7: https://docs.github.com/en/actions/reference/security/secure-use
- 8: https://devopsil.com/articles/2026-03-22-github-actions-security-hardening
- 9: https://safeguard.sh/resources/blog/how-to-pin-github-actions-to-shas-correctly
- 10: https://www.cncf.io/blog/2026/05/04/securing-github-actions-ci-dependencies-recipe-card/
- 11: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
🌐 Web query:
GitHub Actions checkout persist-credentials security third-party repositories
💡 Result:
The persist-credentials input in the GitHub Actions actions/checkout action controls whether the repository's authentication token (typically GITHUB_TOKEN) is configured in the local Git configuration of the runner [1][2]. By default, this is set to true, which allows subsequent steps in the same job to execute authenticated Git commands (e.g., git push or git fetch) without needing to manually re-authenticate [1][2][3]. Security Implications: 1. Credential Exposure: When set to true, any subsequent step in the workflow job—including those running third-party actions or scripts—can access the persisted credentials [4][5]. If an attacker compromises a third-party dependency used in the workflow, they could potentially steal this token [6][3]. 2. Artifact Leakage: A known attack vector involves the inadvertent inclusion of the.git directory or files containing these credentials in build artifacts [6]. If these artifacts are uploaded and made public (e.g., in a public repository), the credentials can be extracted, potentially leading to supply chain compromises [6][7]. 3. Principle of Least Privilege: Because many workflows only require read access to check out code and do not need to perform Git operations (like pushing commits), leaving this enabled violates the principle of least privilege [3]. Recommendations: - Default to False: It is a widely recommended security best practice to set persist-credentials: false by default in all workflows [8][3][5]. - Explicitly Enable When Needed: Only set persist-credentials: true in specific jobs that genuinely require authenticated Git operations [3]. - Token Permissions: Regardless of this setting, always ensure that your GITHUB_TOKEN has the minimum permissions necessary for the job's requirements [4][9]. - Modern Guardrails: While recent versions of actions/checkout have improved security by storing credentials in a separate file under $RUNNER_TEMP rather than directly in.git/config, setting persist-credentials: false remains the standard recommendation to minimize the attack surface [1][2][10].
Citations:
- 1: https://github.com/actions/checkout
- 2: https://github.com/actions/checkout/blob/main/README.md
- 3: https://masteringlaravel.io/daily/2026-06-18-the-security-default-we-change-in-githubs-checkout-action
- 4: Remove
persist-credentialsor change the default tofalseactions/checkout#485 - 5: https://github.com/suzuki-shunsuke/ghalint/blob/main/docs/policies/013.md
- 6: https://www.bleepingcomputer.com/news/security/github-actions-artifacts-found-leaking-auth-tokens-in-popular-projects/
- 7: https://nvd.nist.gov/vuln/detail/cve-2026-40313
- 8: https://cheatsheetseries.owasp.org/cheatsheets/GitHub_Actions_Security_Cheat_Sheet.html
- 9: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 10: [security] Escalate concerning default
persist-credentials=trueplease actions/checkout#2312
Pin the checkout action and disable persisted credentials in the OGRE cache-miss step.
Line 1808 uses actions/checkout@master, an unpinned reference that is mutable and vulnerable to supply chain attacks. Additionally, credentials are persisted by default, unnecessarily exposing the GitHub token to any subsequent step or third-party action. Pin the action to a full-length commit SHA and set persist-credentials: false.
Suggested patch
- - if: steps.cache-ogre-macos.outputs.cache-hit != 'true'
- name: Check out ogre repo (cache miss)
- uses: actions/checkout@master
- with:
- repository: OGRECave/ogre
- ref: v${{ env.OGRE_VERSION }}
- path: ${{github.workspace}}/ogre
+ - if: steps.cache-ogre-macos.outputs.cache-hit != 'true'
+ name: Check out ogre repo (cache miss)
+ uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4
+ with:
+ persist-credentials: false
+ repository: OGRECave/ogre
+ ref: v${{ env.OGRE_VERSION }}
+ path: ${{github.workspace}}/ogre📝 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.
| - if: steps.cache-ogre-macos.outputs.cache-hit != 'true' | |
| name: Check out ogre repo (cache miss) | |
| uses: actions/checkout@master | |
| with: | |
| repository: OGRECave/ogre | |
| ref: v${{ env.OGRE_VERSION }} | |
| path: ${{github.workspace}}/ogre | |
| - if: steps.cache-ogre-macos.outputs.cache-hit != 'true' | |
| name: Check out ogre repo (cache miss) | |
| uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4 | |
| with: | |
| persist-credentials: false | |
| repository: OGRECave/ogre | |
| ref: v${{ env.OGRE_VERSION }} | |
| path: ${{github.workspace}}/ogre |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 1806-1812: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 1808-1808: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/deploy.yml around lines 1806 - 1812, In the "Check out
ogre repo (cache miss)" step, replace the unpinned `actions/checkout@master`
reference with a pinned commit SHA (e.g., `actions/checkout@<full-commit-sha>`),
and add `persist-credentials: false` to the `with` block to prevent unnecessary
token exposure. This applies the same security practices to the OGRE cache-miss
checkout that should be used across all checkout actions in the workflow.
Source: Linters/SAST tools
… first) This test failed intermittently on CI (Xvfb) with: Value of: window->m_modeBarShell->isHidden() Actual: true Expected: false The fixture constructs MainWindow but never show()s it. QToolBar::isHidden() reflects effective visibility, which is only realized once the parent window is mapped — so under Xvfb the shell reports hidden and the assertion is racy. It hit BOTH this branch and the unrelated CI-only PR #756 (which has no source changes), confirming it's a pre-existing flake, not a regression. Fix: show() the window and processEvents() before the visibility assertion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
build-macos still failed with the Xcode_26.5 libz.tbd path even after pinning DEVELOPER_DIR=Xcode_26.3 and rebuilding OGRE: CMake's find_package(ZLIB) resolved to the SDK that `xcrun` defaults to (26.5 on these images) rather than the xcode-select'd one, so the OGRE SDK's CMake export baked a 26.5 libz.tbd path that the cache then carried forward. Fix: export SDKROOT (from `xcrun --sdk macosx --show-sdk-path` under the pinned Xcode) in the Pin step, so clang AND CMake resolve system libs under the SAME pinned SDK on every macOS job. Bump MACOS_CACHE_VERSION → sdkpin1 to discard the OGRE caches that still carry the 26.5 path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
1 participant
Closes #407.
Summary
Predicts a skeleton for an unrigged mesh. The issue proposed wrapping Pinocchio (Baran & Popović, SIGGRAPH 2007), but Pinocchio's core library is LGPL-2.1-or-later (only its demo CLI is MIT). Statically vendoring LGPL imposes relink/object-file obligations that conflict with this project's statically-linked, permissively-redistributed binaries (Homebrew/Snap/WinGet/Docker) and its permissive-license stance — the same call #401 (Instant Meshes) and #402 (libigl/TetGen) made. Pinocchio's algorithm is published and unencumbered; only its code is LGPL, so this is a from-scratch native implementation with zero new dependencies.
What's new
AutoRig(src/AutoRig.h/.cpp) — Ogre-free, unit-tested core:fitTemplate(): map the template into the mesh AABB, then recentre flagged joints (spine, limb roots) toward the centroid of vertices in a thin slab at the joint's up-height → spine onto the medial line, limb roots inside the silhouette.rigEntity(): build anOgre::Skeleton(parent-relative bone positions,setBindingPose), bind viamesh->_notifySkeleton+entity->_initialise(true)— the re-initialise is required or both exporters (which gate onentity->hasSkeleton()) silently drop the rig.qtmesh rig <file> [--skeleton humanoid|biped|quadruped|generic] [--skin] [--up-axis x|y|z] -o out—--skinchainsSkinWeights::computeAndApplyfor one-click rig+skin.auto_rig{ template, skin?, up_axis?, output_path? }.AutoRigDialog.qml+AutoRigController), gated on a static-mesh selection.ai.assist.auto_rig.AutoRig_test.cpp) + CLI coverage (CLIPipeline_cmdrig_coverage_test.cpp).Acceptance criteria
Ogre::Skeleton, exported to FBX/glTF/.skeleton).ai.assist.auto_rig.Notes
App + UnitTests build clean on macOS arm64. The full rig→skin→export path is verified locally on a static mesh; the GL-dependent unit tests run on CI Linux + Xvfb. Decision rationale (LGPL → native) recorded in
CLAUDE.md.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation
rigsubcommand and added AutoRig usage notes.Tests
rigcommand.