feat(skinning): Skinning v2 — DQS display toggle, evaluation suite, Slice-C decision (#819 Slices C/D/E)#830
Conversation
…ion (#819 Slices C/D/E) Slice D — dual-quaternion skinning display: - SkinningDisplay toggles RTSS hardware skinning per entity: Dual Quaternion (prepareEntityForSkinning ST_DUAL_QUATERNION + technique invalidation, kills candy-wrapper collapse on twists) / Linear (erases the HS_SRS_DATA imprint, back to the default path) - HardwareSkinningFactory + dormant template SRS registered in RTShaderHelper (bone cap 96; above-cap entities keep the default path), factory unregistered+deleted on shutdown - Surfaces: "Display: Linear | Dual Quaternion" row in the Animation-mode Skinning section, MCP set_skinning_display, SkinWeightsController get/set, Sentry render.skinning - Display only: exported weights unchanged (documented in tooltips) - Live-verified: toggle DQS/linear on real assets renders correctly; known pre-existing AppleScript-quit abort is unrelated (reproduces with the factory disabled) Slice E — skin-quality metrics + acceptance suite: - SkinMetrics (pure-data): influence histogram, Laplacian weight- smoothness energy, LBS deform + rotation transforms, signed mesh volume - SkinEvaluate: extract EXISTING weights from an entity; --evaluate report (histogram, smoothness, geodesic bleed of existing weights); --compare vs a reference-skinned copy with position-matched vertices (equidistant duplicates tie-break on min weight diff — self-compare is exactly 0) and name-matched bones - CLI: qtmesh skin --evaluate [--json], --compare <ref> [--json] - Acceptance fixtures (hermetic, procedural): 90° elbow capsule volume >= 0.9 (measured 0.911), proximity bleed 0 for GVB vs >0.1 for inverse-distance, smoothing strictly reduces energy - docs/SKINNING_QUALITY.md: metric definitions, thresholds, Mixamo comparison protocol; env-gated reference test (QTMESH_SKIN_OURS_FBX / QTMESH_SKIN_REF_FBX) Slice C — decision record (THIRD_PARTY_AI_MODELS.md): - UniRig's skin head is blocked on spconv/PTv3 (no ONNX lowering; verified upstream) — a faithful export is a research task, not an export chore - SkinTokens (MIT, Qwen3-0.6B AR, no spconv) evaluated per the issue's decision gate: the preferred ML-skinning follow-up - Algorithm::UniRigML keeps its clean GeodesicVoxel fallback on all surfaces meanwhile Co-Authored-By: Claude Fable 5 <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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds skin-quality metrics, asset evaluation and comparison commands, runtime linear/dual-quaternion display controls, MCP and QML integrations, RTSS lifecycle handling, tests, and documentation for skinning workflows. ChangesSkinning quality tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLIPipeline
participant SkinEvaluate
participant OgreEntity
User->>CLIPipeline: Run skin evaluation or comparison
CLIPipeline->>OgreEntity: Import asset
CLIPipeline->>SkinEvaluate: Evaluate or compare
SkinEvaluate->>OgreEntity: Extract skin data
SkinEvaluate-->>CLIPipeline: Return report
CLIPipeline-->>User: Print text or JSON
sequenceDiagram
participant PropertiesPanel
participant SkinWeightsController
participant SkinningDisplay
participant OgreRTSS
PropertiesPanel->>SkinWeightsController: Select display mode
SkinWeightsController->>SkinningDisplay: Apply mode to selected entity
SkinningDisplay->>OgreRTSS: Prepare or clear hardware skinning
OgreRTSS-->>SkinningDisplay: Regenerate material techniques
SkinningDisplay-->>PropertiesPanel: Report mode state
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 |
![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: 78f87b6aa7
ℹ️ 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".
| mat->getTechnique(0)->getPass(0) | ||
| ->getUserObjectBindings().eraseUserAny(kOgreHsDataKey); |
There was a problem hiding this comment.
When the user toggles Dual Quaternion back to Linear, this erases the key from the pass user bindings, but Ogre's prepareEntityForSkinning stores HS_SRS_DATA on Technique(0) and the RTSS hardware-skinning SRS reads it from the technique. The DQS imprint therefore survives invalidation, so the regenerated material can keep rendering in DQS while the UI/MCP report linear; erase mat->getTechnique(0)->getUserObjectBindings() instead.
Useful? React with 👍 / 👎.
| Ogre::RTShader::HardwareSkinningFactory::prepareEntityForSkinning( | ||
| entity, Ogre::RTShader::ST_DUAL_QUATERNION, | ||
| /*correctAntidpodalityHandling*/ true, /*shearScale*/ false); |
There was a problem hiding this comment.
When two skinned entities share the same Ogre material, this call imprints the shared material resource, while the selected mode is tracked on only one entity. Toggling one character to DQS (or later back to Linear after the erase bug is fixed) changes the generated shader for every other entity using that material and leaves their current() mode stale, so this is not actually per-entity unless the material is made unique before applying the imprint.
Useful? React with 👍 / 👎.
The test harness never runs Manager::loadResources() (the GUI/CLI path that brings RTSS up), so ShaderGenerator::getSingletonPtr() was null in the isolated CI run. Initialize it in SetUp like MaterialPresetLibrary_test does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-material entities Codex review on PR #830: - P1: Ogre stores/reads HS_SRS_DATA on getTechnique(0)'s user bindings (verified v14.5.2 imprintSkeletonData/preAddToRenderState), not on a pass — Linear mode was erasing the wrong object, so the DQS imprint survived and the material kept rendering DQS while reporting linear. Test's imprint probe fixed to match. - P2: the imprint is material-level, so entities sharing a material switch together — apply() now stamps the tracked mode on every entity sharing one of the affected materials, keeping current() truthful (new SharedMaterialEntitiesTrackTheSameMode test). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both Codex findings were valid — fixed in the latest commit:
Also fixed the CI failure: the test harness never runs 🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CLIPipeline.cpp (1)
8921-8942: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd validation for
--evaluate/--comparemutual exclusivity and missing reference path.Two edge cases are silently mishandled:
- If both
--evaluateand--compareare passed,--comparesilently takes priority (line 8992 checks!comparePath.isEmpty()first), which is confusing.- If
--compareis the last argument (no reference path), thei + 1 < argcguard at line 8922 fails silently,comparePathstays empty, and the user gets a misleading "-o required" error or unintended weight computation.🔧 Proposed validation
if (inputPath.isEmpty()) { err() << "Error: No input file specified." << Qt::endl; err() << "Usage: qtmesh skin <file> " "[--algo geodesic-voxel|inverse-distance|unirig] " "[--max-influences N] [--falloff F] [--max-distance D] " "[--voxel-res N] [--smooth-iterations N] " "[--skip-unweighted] [--merge] -o <out> [--json]\n" " qtmesh skin <file> --evaluate [--voxel-res N] [--json]\n" " qtmesh skin <file> --compare <reference> [--json]" << Qt::endl; return 2; } + if (evaluateMode && !comparePath.isEmpty()) { + err() << "Error: --evaluate and --compare are mutually exclusive." << Qt::endl; + return 2; + } + if (evaluateMode && comparePath.isEmpty()) { + // --evaluate mode: no reference needed + } else if (!evaluateMode && !comparePath.isEmpty() && false) { + // handled below + } + // Detect --compare without a following path argument const bool analysisMode = evaluateMode || !comparePath.isEmpty();A simpler approach: add a
compareRequestedbool set when--compareis seen (regardless of whether a path follows), then validate after the loop:+ bool compareRequested = false; for (int i = 1; i < argc; ++i) { ... if (arg == "--compare") { + compareRequested = true; if (i + 1 < argc) { comparePath = QString::fromLocal8Bit(argv[++i]); } continue; } ... } ... + if (compareRequested && comparePath.isEmpty()) { + err() << "Error: --compare requires a reference file path." << Qt::endl; + return 2; + } + if (evaluateMode && !comparePath.isEmpty()) { + err() << "Error: --evaluate and --compare are mutually exclusive." << Qt::endl; + return 2; + } const bool analysisMode = evaluateMode || !comparePath.isEmpty();🤖 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 8921 - 8942, Add a compareRequested flag in the argument-parsing logic and set it whenever --compare is encountered, even without a following value. After parsing, reject simultaneous evaluateMode and compareRequested with a clear error, and reject compareRequested when comparePath is empty with a missing-reference error; return before analysis mode or output validation so these cases cannot fall through.
🧹 Nitpick comments (1)
src/SkinningDisplay_test.cpp (1)
91-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest entity not destroyed at the end.
entcreated viacreateAnimatedTestEntityisn't destroyed here, unlikeGuardsRejectInvalidEntitieswhich callssceneMgr->destroyEntity(ent). Minor test-hygiene inconsistency; low practical impact givenuniqueName()avoids collisions, but leaves entities accumulating in the shared SceneManager across the test binary's lifetime.🤖 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/SkinningDisplay_test.cpp` around lines 91 - 122, Add explicit cleanup for the entity created by createAnimatedTestEntity in DqsAppliesAndLinearReverts, ensuring sceneMgr->destroyEntity(ent) is called after the assertions (including on assertion failures if the test fixture supports a scoped cleanup mechanism), matching the cleanup pattern used by GuardsRejectInvalidEntities.
🤖 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/SKINNING_QUALITY.md`:
- Around line 52-61: Add the text language identifier to the fenced
sample-output block in the Skin Evaluation section by changing the opening fence
to ```text, while leaving the report contents unchanged.
- Around line 16-18: The documented bleed metric contradicts the GeodesicVoxel
sample output. Update the “Bleed fraction” definition and related acceptance
threshold to state whether post-processing or smoothing can create non-local
weights, then revise the GeodesicVoxel sample value or explanation so the
definition, threshold, and sample output are consistent.
In `@qml/PropertiesPanel.qml`:
- Around line 2345-2382: Make the skinning display options in the dispBtn
delegate keyboard- and screen-reader-accessible by replacing the raw
Rectangle/MouseArea interaction with the project’s existing accessible
segmented-control pattern, or add equivalent activeFocusOnTab, Accessible
role/name/checked state, visible focus styling, and keyboard activation for the
Space/Enter keys. Preserve the existing setSkinningDisplayMode and activeMode
behavior, and ensure the controls expose mutually exclusive radio-style
selection.
- Around line 2321-2328: The active mode state in skinDispRow is refreshed only
by onSelectionChanged, so runtime changes from MCPServer.cpp can leave the UI
stale. Add an observable SkinWeightsController mode-changed signal and connect
it alongside onSelectionChanged, or otherwise trigger the same
skinningDisplayMode() refresh after shared runtime mode updates; ensure both
user and MCP changes update skinDispRow.activeMode.
In `@src/CLIPipeline.cpp`:
- Around line 9002-9008: Add a multi-entity validation after the reference
import and entity-discovery loop associated with refEntity: count all newly
imported Ogre::Entity objects not present in meshEntities, reject the reference
file when more than one is found, and report the same error/cleanup outcome used
by the existing input-file multi-entity check. Ensure refEntity is assigned only
for the single valid entity and prevent comparison from continuing with silently
ignored entities.
In `@src/SkinEvaluate.cpp`:
- Around line 196-198: Document in the public static evaluate() API that calling
it resets the entity’s shared skeleton to bind pose as part of geodesic voxel
field generation, or save and restore all bone transforms around
skel->reset(true) to prevent the caller’s pose from being mutated.
In `@src/SkinMetrics.cpp`:
- Around line 81-106: Guard against a zero-length rotation axis in
SkinMetrics::rotationAbout before normalizing it. Define the API’s intended
behavior for this invalid input—preferably reject it explicitly using the
project’s established error-handling convention, or return an identity transform
if that is the documented contract—instead of dividing by zero; preserve
existing behavior for nonzero axes.
In `@src/SkinWeightsController.cpp`:
- Line 231: Update the mode handling in SkinWeightsController using
SkinningDisplay::modeFromString so unknown or unsupported display-mode strings
are detected instead of defaulting to Linear; validate the input against the
accepted mode names and return an appropriate error without changing the entity
when validation fails.
- Around line 232-238: Move the SentryReporter::addBreadcrumb call to after
SkinningDisplay::apply succeeds, or update it to include the operation outcome;
ensure failed applications do not emit a breadcrumb claiming the mode was set.
In `@src/SkinWeightsController.h`:
- Around line 53-60: Clarify the API documentation near skinningDisplayMode()
and setSkinningDisplayMode(): state that skinningDisplayMode() returns the
current mode for the first selected entity, while setSkinningDisplayMode()
returns a bool indicating whether the requested mode was successfully applied.
---
Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Around line 8921-8942: Add a compareRequested flag in the argument-parsing
logic and set it whenever --compare is encountered, even without a following
value. After parsing, reject simultaneous evaluateMode and compareRequested with
a clear error, and reject compareRequested when comparePath is empty with a
missing-reference error; return before analysis mode or output validation so
these cases cannot fall through.
---
Nitpick comments:
In `@src/SkinningDisplay_test.cpp`:
- Around line 91-122: Add explicit cleanup for the entity created by
createAnimatedTestEntity in DqsAppliesAndLinearReverts, ensuring
sceneMgr->destroyEntity(ent) is called after the assertions (including on
assertion failures if the test fixture supports a scoped cleanup mechanism),
matching the cleanup pattern used by GuardsRejectInvalidEntities.
🪄 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: c8001557-e739-4497-8dcb-2132c67aacd5
📒 Files selected for processing (22)
.gitignoreCLAUDE.mdTHIRD_PARTY_AI_MODELS.mddocs/SKINNING_QUALITY.mdqml/PropertiesPanel.qmlsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/RTShaderHelper.cppsrc/SkinEvaluate.cppsrc/SkinEvaluate.hsrc/SkinEvaluate_test.cppsrc/SkinMetrics.cppsrc/SkinMetrics.hsrc/SkinMetrics_test.cppsrc/SkinWeightsController.cppsrc/SkinWeightsController.hsrc/SkinningDisplay.cppsrc/SkinningDisplay.hsrc/SkinningDisplay_test.cpptests/CMakeLists.txt
| | Influence histogram | vertices per influence count (0–8), average, max | max ≤ 4 (hardware-skinning convention) | | ||
| | Weight smoothness | Laplacian energy: mean over mesh edges of ‖w_u − w_v‖² | lower = smoother falloffs; hard 0/1 borders score ~2 per edge | | ||
| | Bleed fraction | share of committed (vertex, bone) weights whose bone is **not geodesically local** to the vertex (GeodesicVoxelBind field) | 0 — geodesic-voxel weights are 0 by construction | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reconcile the bleed metric definition with the sample output.
The table says GeodesicVoxel bleed is 0 “by construction,” but the documented GeodesicVoxel sample reports 0.0055. Clarify whether post-processing/smoothing can introduce non-local weights, then align the definition, acceptance threshold, and sample output.
Also applies to: 59-60
🤖 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/SKINNING_QUALITY.md` around lines 16 - 18, The documented bleed metric
contradicts the GeodesicVoxel sample output. Update the “Bleed fraction”
definition and related acceptance threshold to state whether post-processing or
smoothing can create non-local weights, then revise the GeodesicVoxel sample
value or explanation so the definition, threshold, and sample output are
consistent.
| ``` | ||
| Skin Evaluation | ||
| =============== | ||
| Vertices: 90573 | ||
| Bones: 119 | ||
| Avg influences: 3.47 | ||
| Max influences: 4 | ||
| Smoothness energy: 0.004... (lower = smoother falloffs) | ||
| Bleed fraction: 0.0055 (weights not geodesically local) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the sample-output fence.
The unlabelled fence triggers MD040. Use ```text for this report block.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 52-52: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/SKINNING_QUALITY.md` around lines 52 - 61, Add the text language
identifier to the fenced sample-output block in the Skin Evaluation section by
changing the opening fence to ```text, while leaving the report contents
unchanged.
Source: Linters/SAST tools
| // Re-read the active mode when the selection changes. | ||
| property string activeMode: SkinWeightsController.skinningDisplayMode() | ||
| Connections { | ||
| target: SkinWeightsController | ||
| function onSelectionChanged() { | ||
| skinDispRow.activeMode = | ||
| SkinWeightsController.skinningDisplayMode() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Refresh the active mode when runtime/MCP changes it.
This handler only reacts to selection changes, but MCPServer.cpp applies the mode directly without changing selection. The viewport can therefore be in Dual Quaternion mode while the Linear button remains highlighted until the user reselects the entity. Add an observable mode-changed notification or refresh this state after shared runtime changes.
🤖 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 `@qml/PropertiesPanel.qml` around lines 2321 - 2328, The active mode state in
skinDispRow is refreshed only by onSelectionChanged, so runtime changes from
MCPServer.cpp can leave the UI stale. Add an observable SkinWeightsController
mode-changed signal and connect it alongside onSelectionChanged, or otherwise
trigger the same skinningDisplayMode() refresh after shared runtime mode
updates; ensure both user and MCP changes update skinDispRow.activeMode.
| Rectangle { | ||
| id: dispBtn | ||
| required property var modelData | ||
| readonly property bool active: | ||
| skinDispRow.activeMode === modelData.mode | ||
| width: dispLabel.implicitWidth + 14 | ||
| height: 22 | ||
| radius: 3 | ||
| color: active ? PropertiesPanelController.highlightColor | ||
| : (dispMa.containsMouse | ||
| ? PropertiesPanelController.headerColor | ||
| : PropertiesPanelController.inputColor) | ||
| border.color: PropertiesPanelController.borderColor | ||
| border.width: 1 | ||
|
|
||
| Text { | ||
| id: dispLabel | ||
| anchors.centerIn: parent | ||
| text: dispBtn.modelData.label | ||
| color: PropertiesPanelController.textColor | ||
| font.pixelSize: 10 | ||
| } | ||
| MouseArea { | ||
| id: dispMa | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| cursorShape: Qt.PointingHandCursor | ||
| onClicked: { | ||
| if (SkinWeightsController.setSkinningDisplayMode( | ||
| dispBtn.modelData.mode)) | ||
| skinDispRow.activeMode = dispBtn.modelData.mode | ||
| } | ||
| ToolTip.visible: containsMouse | ||
| ToolTip.delay: 500 | ||
| ToolTip.text: dispBtn.modelData.mode === "dual-quaternion" | ||
| ? "Render with dual-quaternion hardware skinning — preserves volume on twists (no candy-wrapper). Display only: exported weights are unchanged; engines re-skin with their own blend." | ||
| : "Render with the default linear-blend skinning path." | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the display controls keyboard- and screen-reader-accessible.
These new controls are raw Rectangle/MouseArea elements with no activeFocusOnTab, Accessible role/name/checked state, or keyboard activation. Users who cannot use a mouse cannot change the skinning mode. Use accessible radio/button controls or add the same focus and key handling used by the existing segmented controls.
🤖 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 `@qml/PropertiesPanel.qml` around lines 2345 - 2382, Make the skinning display
options in the dispBtn delegate keyboard- and screen-reader-accessible by
replacing the raw Rectangle/MouseArea interaction with the project’s existing
accessible segmented-control pattern, or add equivalent activeFocusOnTab,
Accessible role/name/checked state, visible focus styling, and keyboard
activation for the Space/Enter keys. Preserve the existing
setSkinningDisplayMode and activeMode behavior, and ensure the controls expose
mutually exclusive radio-style selection.
| for (Ogre::Entity* e : Manager::getSingleton()->getEntities()) { | ||
| if (e && e->getMovableType() == "Entity" | ||
| && !meshEntities.contains(e)) { | ||
| refEntity = e; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Add multi-entity guard for the reference file.
The reference entity is found by picking the first new Ogre::Entity* not in meshEntities. If the reference file imports multiple entities, only the first is used and the rest are silently ignored, potentially producing incorrect comparison results. The input file already has a multi-entity check (lines 8978-8984), but the reference does not.
🛡️ Proposed multi-entity guard
Ogre::Entity* refEntity = nullptr;
+ int newCount = 0;
for (Ogre::Entity* e : Manager::getSingleton()->getEntities()) {
if (e && e->getMovableType() == "Entity"
&& !meshEntities.contains(e)) {
- refEntity = e;
- break;
+ if (!refEntity) refEntity = e;
+ ++newCount;
}
}
+ if (newCount > 1) {
+ err() << "Error: reference " << comparePath
+ << " contains multiple mesh entities. `qtmesh skin --compare` "
+ "currently supports one entity per file."
+ << Qt::endl;
+ return 1;
+ }
if (!refEntity) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (Ogre::Entity* e : Manager::getSingleton()->getEntities()) { | |
| if (e && e->getMovableType() == "Entity" | |
| && !meshEntities.contains(e)) { | |
| refEntity = e; | |
| break; | |
| } | |
| } | |
| Ogre::Entity* refEntity = nullptr; | |
| int newCount = 0; | |
| for (Ogre::Entity* e : Manager::getSingleton()->getEntities()) { | |
| if (e && e->getMovableType() == "Entity" | |
| && !meshEntities.contains(e)) { | |
| if (!refEntity) refEntity = e; | |
| +newCount; | |
| } | |
| } | |
| if (newCount > 1) { | |
| err() << "Error: reference " << comparePath | |
| << " contains multiple mesh entities. `qtmesh skin --compare` " | |
| "currently supports one entity per file." | |
| << Qt::endl; | |
| return 1; | |
| } | |
| if (!refEntity) { |
🤖 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 9002 - 9008, Add a multi-entity validation
after the reference import and entity-discovery loop associated with refEntity:
count all newly imported Ogre::Entity objects not present in meshEntities,
reject the reference file when more than one is found, and report the same
error/cleanup outcome used by the existing input-file multi-entity check. Ensure
refEntity is assigned only for the single valid entity and prevent comparison
from continuing with silently ignored entities.
| { | ||
| Ogre::Skeleton* skel = entity->getMesh()->getSkeleton().get(); | ||
| skel->reset(true); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Document the skeleton reset side effect in evaluate().
skel->reset(true) mutates the skeleton's shared state to obtain bind-pose bone positions for the geodesic voxel field. This is correct for CLI usage, but evaluate() is a public static API — a future GUI or MCP caller would see the entity's pose silently reset. Consider adding a brief note to the header doc or saving/restoring bone transforms.
📝 Suggested doc addition in SkinEvaluate.h
// when the mesh encloses volume — the geodesic bleed fraction
// (GeodesicVoxelBind field at `voxelResolution`).
+ // Note: resets the skeleton to bind pose to compute bone
+ // positions for the voxel field; callers displaying the entity
+ // should restore the pose afterwards if needed.
static QJsonObject evaluate(Ogre::Entity* entity,🤖 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/SkinEvaluate.cpp` around lines 196 - 198, Document in the public static
evaluate() API that calling it resets the entity’s shared skeleton to bind pose
as part of geodesic voxel field generation, or save and restore all bone
transforms around skel->reset(true) to prevent the caller’s pose from being
mutated.
| SkinMetrics::Transform SkinMetrics::rotationAbout(const double axis[3], | ||
| const double pivot[3], | ||
| double angleRad) | ||
| { | ||
| // Normalize the axis. | ||
| const double len = std::sqrt(axis[0] * axis[0] + axis[1] * axis[1] | ||
| + axis[2] * axis[2]); | ||
| const double x = axis[0] / len, y = axis[1] / len, z = axis[2] / len; | ||
| const double c = std::cos(angleRad), s = std::sin(angleRad); | ||
| const double t = 1.0 - c; | ||
|
|
||
| // Rodrigues rotation matrix. | ||
| const double r[9] = { | ||
| t * x * x + c, t * x * y - s * z, t * x * z + s * y, | ||
| t * x * y + s * z, t * y * y + c, t * y * z - s * x, | ||
| t * x * z - s * y, t * y * z + s * x, t * z * z + c, | ||
| }; | ||
| // Translation = pivot − R·pivot so the pivot is a fixed point. | ||
| const double tx = pivot[0] - (r[0] * pivot[0] + r[1] * pivot[1] + r[2] * pivot[2]); | ||
| const double ty = pivot[1] - (r[3] * pivot[0] + r[4] * pivot[1] + r[5] * pivot[2]); | ||
| const double tz = pivot[2] - (r[6] * pivot[0] + r[7] * pivot[1] + r[8] * pivot[2]); | ||
|
|
||
| return { r[0], r[1], r[2], tx, | ||
| r[3], r[4], r[5], ty, | ||
| r[6], r[7], r[8], tz }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against zero-length axis in rotationAbout.
Line 88 divides by len without checking for zero. A zero-length axis produces NaN/Inf values that silently propagate through the transform. Callers currently pass valid axes, but this is a public API.
🛡️ Proposed fix
const double len = std::sqrt(axis[0] * axis[0] + axis[1] * axis[1]
+ axis[2] * axis[2]);
- const double x = axis[0] / len, y = axis[1] / len, z = axis[2] / len;
+ if (len < 1e-12)
+ return identityTransform();
+ const double x = axis[0] / len, y = axis[1] / len, z = axis[2] / len;📝 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.
| SkinMetrics::Transform SkinMetrics::rotationAbout(const double axis[3], | |
| const double pivot[3], | |
| double angleRad) | |
| { | |
| // Normalize the axis. | |
| const double len = std::sqrt(axis[0] * axis[0] + axis[1] * axis[1] | |
| + axis[2] * axis[2]); | |
| const double x = axis[0] / len, y = axis[1] / len, z = axis[2] / len; | |
| const double c = std::cos(angleRad), s = std::sin(angleRad); | |
| const double t = 1.0 - c; | |
| // Rodrigues rotation matrix. | |
| const double r[9] = { | |
| t * x * x + c, t * x * y - s * z, t * x * z + s * y, | |
| t * x * y + s * z, t * y * y + c, t * y * z - s * x, | |
| t * x * z - s * y, t * y * z + s * x, t * z * z + c, | |
| }; | |
| // Translation = pivot − R·pivot so the pivot is a fixed point. | |
| const double tx = pivot[0] - (r[0] * pivot[0] + r[1] * pivot[1] + r[2] * pivot[2]); | |
| const double ty = pivot[1] - (r[3] * pivot[0] + r[4] * pivot[1] + r[5] * pivot[2]); | |
| const double tz = pivot[2] - (r[6] * pivot[0] + r[7] * pivot[1] + r[8] * pivot[2]); | |
| return { r[0], r[1], r[2], tx, | |
| r[3], r[4], r[5], ty, | |
| r[6], r[7], r[8], tz }; | |
| } | |
| SkinMetrics::Transform SkinMetrics::rotationAbout(const double axis[3], | |
| const double pivot[3], | |
| double angleRad) | |
| { | |
| // Normalize the axis. | |
| const double len = std::sqrt(axis[0] * axis[0] + axis[1] * axis[1] | |
| axis[2] * axis[2]); | |
| if (len < 1e-12) | |
| return identityTransform(); | |
| const double x = axis[0] / len, y = axis[1] / len, z = axis[2] / len; | |
| const double c = std::cos(angleRad), s = std::sin(angleRad); | |
| const double t = 1.0 - c; | |
| // Rodrigues rotation matrix. | |
| const double r[9] = { | |
| t * x * x + c, t * x * y - s * z, t * x * z + s * y, | |
| t * x * y + s * z, t * y * y + c, t * y * z - s * x, | |
| t * x * z - s * y, t * y * z + s * x, t * z * z + c, | |
| }; | |
| // Translation = pivot − R·pivot so the pivot is a fixed point. | |
| const double tx = pivot[0] - (r[0] * pivot[0] + r[1] * pivot[1] + r[2] * pivot[2]); | |
| const double ty = pivot[1] - (r[3] * pivot[0] + r[4] * pivot[1] + r[5] * pivot[2]); | |
| const double tz = pivot[2] - (r[6] * pivot[0] + r[7] * pivot[1] + r[8] * pivot[2]); | |
| return { r[0], r[1], r[2], tx, | |
| r[3], r[4], r[5], ty, | |
| r[6], r[7], r[8], tz }; | |
| } |
🤖 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/SkinMetrics.cpp` around lines 81 - 106, Guard against a zero-length
rotation axis in SkinMetrics::rotationAbout before normalizing it. Define the
API’s intended behavior for this invalid input—preferably reject it explicitly
using the project’s established error-handling convention, or return an identity
transform if that is the documented contract—instead of dividing by zero;
preserve existing behavior for nonzero axes.
| return false; | ||
| } | ||
|
|
||
| const auto m = SkinningDisplay::modeFromString(mode); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject invalid display-mode strings instead of silently selecting Linear.
SkinningDisplay::modeFromString() maps every unknown value to Linear, so a typo or unsupported MCP/QML value reports success while changing the entity’s mode. Validate accepted values and return an error for unknown modes.
🤖 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/SkinWeightsController.cpp` at line 231, Update the mode handling in
SkinWeightsController using SkinningDisplay::modeFromString so unknown or
unsupported display-mode strings are detected instead of defaulting to Linear;
validate the input against the accepted mode names and return an appropriate
error without changing the entity when validation fails.
| SentryReporter::addBreadcrumb(QStringLiteral("render.skinning"), | ||
| QStringLiteral("display mode %1 on %2") | ||
| .arg(SkinningDisplay::modeToString(m), | ||
| QString::fromStdString(entity->getName()))); | ||
|
|
||
| QString err; | ||
| if (!SkinningDisplay::apply(entity, m, &err)) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Record the breadcrumb after applying the mode, or include the outcome.
The breadcrumb currently says the mode was set before SkinningDisplay::apply() can fail, producing misleading telemetry for skeleton-less entities or uninitialized RTSS. As per coding guidelines, significant operations must be tracked with breadcrumbs that accurately represent the operation.
🤖 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/SkinWeightsController.cpp` around lines 232 - 238, Move the
SentryReporter::addBreadcrumb call to after SkinningDisplay::apply succeeds, or
update it to include the operation outcome; ensure failed applications do not
emit a breadcrumb claiming the mode was set.
Source: Coding guidelines
| /// #819 Slice D: dual-quaternion display toggle for the selected | ||
| /// entity. Modes: "linear" (default LBS path) or | ||
| /// "dual-quaternion" (RTSS hardware DQS — kills the candy-wrapper | ||
| /// collapse on twists). Runtime shading only; exported weights | ||
| /// are unchanged. Returns the mode applied to / read from the | ||
| /// first selected entity. | ||
| Q_INVOKABLE QString skinningDisplayMode() const; | ||
| Q_INVOKABLE bool setSkinningDisplayMode(const QString& mode); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the getter and setter return contracts.
The documentation says these methods “return the mode,” but setSkinningDisplayMode() returns bool and reports application success/failure. Document the getter as returning the mode and the setter as returning success.
🤖 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/SkinWeightsController.h` around lines 53 - 60, Clarify the API
documentation near skinningDisplayMode() and setSkinningDisplayMode(): state
that skinningDisplayMode() returns the current mode for the first selected
entity, while setSkinningDisplayMode() returns a bool indicating whether the
requested mode was successfully applied.
The CI harness counts ANY skipped test as a suite failure — CompareAgainstReference now passes as a no-op when QTMESH_SKIN_OURS_FBX / QTMESH_SKIN_REF_FBX are unset. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Ogre 14.x registers HardwareSkinningFactory as a ShaderGenerator BUILT-IN (mBuiltinSRSFactories): our initialize() guard (!getSingletonPtr()) therefore never fired — the bone cap and the template SRS were never applied — while shutdown() deleted the built-in factory that ShaderGenerator::destroy() then deleted again. The double free segfaulted teardown of every suite that runs the full RTSS lifecycle on CI (MainWindowTest, MCPServerTest, RTShaderHdrIblTest, SceneSaveLoadTest, ...), failing unit-tests-linux with 7 crashed suites. Follow the DualQuaternion sample instead: use the built-in factory, set the bone cap, and add the template SRS to the generated scheme (guarded by TYPE — addTemplateSubRenderState only dedupes by instance, and tests re-run initialize()). Shutdown no longer touches the factory; ShaderGenerator::destroy() owns it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
CI is green — 2418ed0 fixes the unit-tests-linux failure. Root cause: Ogre 14.x registers Now per the Ogre DualQuaternion sample: use the built-in factory, set the bone cap, add the template hardware-skinning SRS to the generated scheme (deduped by type — 🤖 Generated with Claude Code |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Completes the remaining slices of #819 on top of the merged Slices A+B (PR #829).
Slice D — Dual-quaternion skinning display toggle
No weight map fixes linear-blend volume collapse on twists — that needs a different blend at render time.
SkinningDisplay(src/SkinningDisplay.{h,cpp}): per-entity toggle via Ogre's RTSSHardwareSkinningFactory. Dual Quaternion =prepareEntityForSkinning(ST_DUAL_QUATERNION, antipodality-corrected)+ technique invalidation; Linear = erase the material'sHS_SRS_DATAimprint (mirrors Ogre's file-local constant, stable since 1.8) and regenerate. Mode is tracked on the entity'sUserObjectBindings.RTShaderHelper::initialize(Ogre ShaderSystem-sample pattern — plain materials are unaffected) and unregistered + deleted on shutdown. Bone cap 96: entities above it stay safely on the default path (verified: 119-bone bandit renders unchanged).set_skinning_display(modeenum),SkinWeightsController::skinningDisplayMode/setSkinningDisplayMode, Sentryrender.skinning.Slice E — Metrics, acceptance suite, Mixamo protocol
SkinMetrics(pure-data, unit-tested): influence histogram, Laplacian weight-smoothness energy, LBS deform (rotationAbout+deformLBS), signed mesh volume.SkinEvaluate: extracts an entity's existing weights (whoever authored them) and reports metrics — including the geodesic bleed of existing weights against a freshGeodesicVoxelBindfield — plus a reference comparison with position-matched vertices and name-matched bones. Equidistant duplicate vertices (seams, hand-on-prop contact points) tie-break on minimum weight difference, so self-compare is exactly 0 (verified on the 90k-vert bandit; the naive matcher reported spurious 1.45 max-diffs at contact points).qtmesh skin <file> --evaluate [--voxel-res N] [--json]andqtmesh skin <file> --compare <reference> [--json].docs/SKINNING_QUALITY.md: metric definitions, thresholds, the manual Mixamo comparison protocol (no Adobe assets in-repo), and the env-gated regression test (QTMESH_SKIN_OURS_FBX/QTMESH_SKIN_REF_FBX— skipped in CI, suite has unconditional tests so it never goes skip-only).Slice C — UniRig skinning head: decision record
Per the issue's own decision gate ("also evaluate SkinTokens … if it exports cleaner, prefer it"):
src/model/unirig_skin.py— the mesh geometry path runs through a PTv3 backbone on spconv sparse convolutions (no ONNX operator lowering) plus flash-attn MHA. The AI: RigNet auto-rigging (ONNX, ML) #408 skeleton export only worked because that stage never executes the PTv3 path. A faithfulskin.onnxmeans re-implementing PTv3 densely — a research task.THIRD_PARTY_AI_MODELS.md;Algorithm::UniRigMLkeeps its clean, tested fallback to GeodesicVoxel on every surface meanwhile, so the ML path can light up later without changing any app surface.Verification
--evaluateon the 90k-vert bandit: avg 3.44 influences, bleed 0.028, smoothness 0.0124.--compareself-test: 90,573/90,573 matched, mean/max L1 diff 0.0000.Tracks #819 (Slices C/D/E; A+B merged in #829).
🤖 Generated with Claude Code
Summary by CodeRabbit
qtmesh skin --evaluate(quality metrics) andqtmesh skin --compare <reference>(per-vertex weight diffs) with JSON or text output.