feat(render): shaded thumbnails + upright Blender-rig framing + rigid-part binding (#933) - #945
Conversation
…art binding (#933) Three fixes for marketplace-thumbnail rendering: 1. DCC-style shaded lighting. The turntable/isometric renderers set full- white ambient, so untextured models (Quaternius/Kenney packs) rendered as flat silhouettes with zero depth cues. Default is now low ambient (0.35) + key + fill — shaped gray renders like every DCC viewport. New 'qtmesh turntable --studio': three-point preset (warm key / cool fill / rim) for thumbnail-quality output. RTSS schemes are invalidated around the light changes so the new lights actually reach the cached shaders. 2. Upright bind pose for Blender-style FBX rigs. These files stamp Y-up METADATA but carry the standing orientation on the armature/mesh NODES (-90degX), which the importer dropped — the bind pose rendered lying down ('T-pose seen from above'). The reference skinned mesh node's world rotation is now baked the same way as the existing Z-up path (root bones + vertices, binding pose re-snapshot). Identity for Mixamo-style rigs — verified identical silhouette coverage on Rumba. 3. Rigid bone-parented parts (Quaternius Robot) crashed EVERY render with Ogre's softwareVertexBlend assertion (no blend elements). Such parts are now bound to their nearest ancestor bone with weight 1, with the node-chain placement baked into the vertices — they render assembled and follow the bone during animation. Multi-skinned-mesh scenes also gain per-mesh frame alignment into the reference mesh's frame (Assimp offsets are per-mesh-node). Known limitation: rigs whose skin bind diverges from the node tree (the Robot's arms) keep a residual global tilt. Verified: Knight/Trex turntables upright + shaded; Robot renders instead of aborting; Rumba (Mixamo) silhouette coverage bit-identical; knight isometric --animation still walks; 83 unit tests green. Fixes #933 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 Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe importer now aligns node-relative skeletons and meshes, including rigid meshes. Turntable rendering adds an optional three-point studio-lighting preset. Isometric rendering uses shaded ambient, key, and fill lighting. ChangesImporter alignment
Lighting presets
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Importer
participant BoneProcessor
participant MeshProcessor
Importer->>BoneProcessor: detect first skinned mesh node rotation
Importer->>BoneProcessor: bake rotation into skeleton roots
Importer->>MeshProcessor: process meshes with bake rotation
MeshProcessor->>MeshProcessor: align frames and bind rigid meshes
sequenceDiagram
participant CLI
participant TurntableOptions
participant ModelTurntableRenderer
participant RTSS
CLI->>TurntableOptions: parse --studio
TurntableOptions->>ModelTurntableRenderer: pass studio option
ModelTurntableRenderer->>RTSS: invalidate shader generation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 34262a3716
ℹ️ 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".
| if (rigidUnderSkeleton) | ||
| bindRigidMeshToParentBone(subMeshData, node); |
There was a problem hiding this comment.
When a bone-parented rigid submesh has tangents or morph targets, this call runs before either collection is populated, so bindRigidMeshToParentBone() transforms only its vertices and normals while its tangent basis and morph positions are added later in the original node-local frame. A rotated or translated rigid part will therefore render normal maps incorrectly, and activating a morph computes deltas against an already-transformed base and can displace the shape drastically; perform the placement after these attributes are collected (including bitangents), or apply the same placement while collecting them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 768f6ae — the rigid-part binding now runs LAST in processMesh, so tangents and morph targets (filled later) receive the placement transform too.
| if (!st.fillLight) { | ||
| st.fillLight = sm->createLight("ModelIsometricFillLight"); | ||
| st.fillLight->setType(Ogre::Light::LT_DIRECTIONAL); | ||
| st.fillLightNode = sm->getRootSceneNode()->createChildSceneNode("ModelIsometricFillLightNode"); | ||
| st.fillLightNode->attachObject(st.fillLight); |
There was a problem hiding this comment.
After an isometric export from IsometricSpritesController, ModelIsometricRenderer::shutdown() destroys only st.light; this newly attached root-scene fill light and its node remain visible. Consequently the editor scene stays illuminated by ModelIsometricFillLight after the export, and the retained state pointer can become stale if the scene manager is recreated. Destroy and null the fill light/node alongside the existing key light.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 768f6ae — both renderers' shutdown paths now destroy the fill (and turntable rim) lights and their nodes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/Assimp/BoneProcessor.cpp (1)
70-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local
R_x90in the generalized helper.
bakeRootRotationnow accepts any rotation. The nameR_x90describes only the old Z-up case. Use the parameter directly.♻️ Proposed rename
// Bake a rest-pose rotation into the root bones so no scene-node rotation // is needed. Only root bones (no parent in the Ogre skeleton) need it; // child bones' local transforms are parent-relative and correct as-is. - const Ogre::Quaternion R_x90 = rotation; for (unsigned short i = 0; i < skeleton->getNumBones(); ++i) { Ogre::Bone* bone = skeleton->getBone(i); if (bone->getParent() == nullptr) { - bone->setPosition(R_x90 * bone->getPosition()); - bone->setOrientation(R_x90 * bone->getOrientation()); + bone->setPosition(rotation * bone->getPosition()); + bone->setOrientation(rotation * bone->getOrientation()); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Assimp/BoneProcessor.cpp` around lines 70 - 76, In BoneProcessor::bakeRootRotation, remove the misleading local R_x90 alias and use the rotation parameter directly throughout the helper. Preserve the existing generalized rotation behavior for all callers.src/Assimp/MeshProcessor.cpp (1)
489-496: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the root bone list for the fallback bone selection.
getRootBoneIterator()returns a fresh iterator on each call. Call it once, or prefer the directSkeleton::getRootBones()access already used elsewhere in this repo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Assimp/MeshProcessor.cpp` around lines 489 - 496, Update the fallback logic in the bone-selection block to obtain the root-bone iterator once before checking and retrieving its first element, or use Skeleton::getRootBones() directly. Avoid calling getRootBoneIterator() separately for hasMoreElements() and getNext(), while preserving the existing return when no root bone exists.
🤖 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/Assimp/Importer.cpp`:
- Around line 234-266: Restrict the m_nodeBakeRotation path to Blender-style FBX
metadata indicating Y-up orientation, or otherwise validate the first skinned
mesh node transform is a non-mirrored pure rotation before baking. Update the
branch around BoneProcessor::nodeWorldTransform, decomposition, and
bakeRootRotation so authored/non-Y-up or negative-determinant frames retain
their existing transforms.
In `@src/Assimp/MeshProcessor.cpp`:
- Around line 149-157: Move the bindRigidMeshToParentBone call in processMesh
from its current position to immediately after the morph-target population loop
and before return subMeshData. Keep the rigidUnderSkeleton condition unchanged
so tangents and morph target positions are populated before the helper
transforms all mesh data.
In `@src/CLIPipeline.cpp`:
- Around line 4585-4588: Add Google Test coverage for the --studio option:
create a CLI test that runs turntable with --studio and asserts successful,
non-empty output, plus a renderer test that constructs TurntableOptions with
studio set to true. Keep the tests focused on the existing turntable execution
and rendering paths.
In `@src/ModelIsometricRenderer.cpp`:
- Around line 138-162: Call RTShaderHelper::invalidateShadergenScheme() after
the lighting cleanup performed by restoreIsometricLighting() and during
ModelIsometricRenderer::shutdown() after restoring ambient lighting or removing
lights, ensuring later renders regenerate the RTSS scheme for the current light
configuration.
In `@src/ModelTurntableRenderer.cpp`:
- Around line 34-37: Update ModelTurntableRenderer::shutdown() to destroy and
null fillLight, fillLightNode, rimLight, and rimLightNode; also update
ModelIsometricRenderer::shutdown() to destroy and null fillLight and
fillLightNode. Apply the cleanup at src/ModelTurntableRenderer.cpp lines 34-37
and src/ModelIsometricRenderer.cpp lines 46-47, alongside the existing key-light
shutdown handling.
---
Nitpick comments:
In `@src/Assimp/BoneProcessor.cpp`:
- Around line 70-76: In BoneProcessor::bakeRootRotation, remove the misleading
local R_x90 alias and use the rotation parameter directly throughout the helper.
Preserve the existing generalized rotation behavior for all callers.
In `@src/Assimp/MeshProcessor.cpp`:
- Around line 489-496: Update the fallback logic in the bone-selection block to
obtain the root-bone iterator once before checking and retrieving its first
element, or use Skeleton::getRootBones() directly. Avoid calling
getRootBoneIterator() separately for hasMoreElements() and getNext(), while
preserving the existing return when no root bone exists.
🪄 Autofix
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 Plus
Run ID: 23eac556-8ce6-43cb-867c-7586a13e7dbd
📒 Files selected for processing (11)
CLAUDE.mdsrc/Assimp/BoneProcessor.cppsrc/Assimp/BoneProcessor.hsrc/Assimp/Importer.cppsrc/Assimp/Importer.hsrc/Assimp/MeshProcessor.cppsrc/Assimp/MeshProcessor.hsrc/CLIPipeline.cppsrc/ModelIsometricRenderer.cppsrc/ModelTurntableRenderer.cppsrc/ModelTurntableRenderer.h
- null-guard node/scene in MeshProcessor::processNode (S2259 — the gate bug) - extract detectNodeBakeRotation + computeFrameAlign helpers (nesting / cognitive-complexity criticals), drop the temporary bisect env guards - getRootBones() instead of the deprecated iterator; dedicated declarations Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- bindRigidMeshToParentBone runs LAST in processMesh so tangents and morph targets receive the placement transform too (Codex P2 + CodeRabbit) - destroy the fill/rim lights in both renderers' shutdown paths - invalidate the RTSS scheme after isometric lighting restore - skip the node-orientation bake for mirrored (negative-determinant) frames Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Assimp/MeshProcessor.cpp (1)
96-98: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftBind only meshes that resolve to an ancestor bone.
rigidUnderSkeletonis true for every unweighted mesh whenskeletonexists. If no ancestor node matches a bone,bindRigidMeshToParentBoneassigns the mesh to the first root bone. A static sibling mesh can then move with the animated skeleton.Resolve an ancestor bone before enabling this path. If no bone exists, preserve the mesh as static or import it through a separate non-skeletal path.
Also applies to: 153-154
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Assimp/MeshProcessor.cpp` around lines 96 - 98, Update the rigidUnderSkeleton decision in MeshProcessor so it is enabled only when the unweighted mesh has an ancestor node that resolves to a skeleton bone. Ensure bindRigidMeshToParentBone cannot fall back to the first root bone for meshes without such an ancestor; preserve those meshes as static or route them through the existing non-skeletal import path.
🤖 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/Assimp/MeshProcessor.cpp`:
- Around line 470-480: The frame-alignment path must account for non-uniform
scale when transforming direction vectors. Update
MeshProcessor::computeFrameAlign and processMesh to produce and propagate a
normal-transform matrix derived from the full linear frame transform, use it for
normals, tangents, and bitangents instead of frameAlignRot, and recompute or
preserve tangent-space handedness after that full transform. Apply the
corresponding signature and call-site changes in src/Assimp/MeshProcessor.cpp at
lines 470-480 and 529-532.
---
Outside diff comments:
In `@src/Assimp/MeshProcessor.cpp`:
- Around line 96-98: Update the rigidUnderSkeleton decision in MeshProcessor so
it is enabled only when the unweighted mesh has an ancestor node that resolves
to a skeleton bone. Ensure bindRigidMeshToParentBone cannot fall back to the
first root bone for meshes without such an ancestor; preserve those meshes as
static or route them through the existing non-skeletal import path.
🪄 Autofix
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 Plus
Run ID: c3b8def5-1740-4fcc-9865-14c134c12f56
📒 Files selected for processing (3)
src/Assimp/Importer.cppsrc/Assimp/MeshProcessor.cppsrc/Assimp/MeshProcessor.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Assimp/MeshProcessor.h
…iew) frameAlign can carry non-uniform scale; normals now use the inverse- transpose and tangent-space directions the linear part (rotation-only misoriented lighting on scaled mesh nodes). Handedness was already recomputed after the transform. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ModelIsometricRenderer.cpp (1)
153-158: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake fill-light creation atomic.
If
createLight()succeeds but a later Ogre call throws,st.fillLightcan remain set whilest.fillLightNoderemains null. A later call then skips creation and dereferencesst.fillLightNodeat Line 161.Create both objects in local variables and commit them to
IsometricStateonly after attachment succeeds, or destroy partial objects in the exception path. Verify that callers always invokeModelIsometricRenderer::shutdown()before retrying after an Ogre exception.🤖 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/ModelIsometricRenderer.cpp` around lines 153 - 158, Make fill-light initialization in ModelIsometricRenderer atomic: create and configure the light and scene node in local variables, attach the light successfully, then assign both to IsometricState (st.fillLight and st.fillLightNode) only after all Ogre calls succeed. Ensure partial Ogre objects are cleaned up on exceptions, and verify callers invoke ModelIsometricRenderer::shutdown() before retrying.
🤖 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/Assimp/MeshProcessor.cpp`:
- Around line 482-486: Update the frame-alignment logic around frameAlignLinear
and frameAlignNormal to check the linear matrix determinant before calling
Inverse(). For singular matrices, avoid producing zero normals by skipping frame
alignment with identity outputs or rejecting the import using a clear error;
retain the existing inverse-transpose calculation for non-singular matrices.
---
Nitpick comments:
In `@src/ModelIsometricRenderer.cpp`:
- Around line 153-158: Make fill-light initialization in ModelIsometricRenderer
atomic: create and configure the light and scene node in local variables, attach
the light successfully, then assign both to IsometricState (st.fillLight and
st.fillLightNode) only after all Ogre calls succeed. Ensure partial Ogre objects
are cleaned up on exceptions, and verify callers invoke
ModelIsometricRenderer::shutdown() before retrying.
🪄 Autofix
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 Plus
Run ID: f3eaea83-d601-4b06-a8a8-c42bc6584f46
📒 Files selected for processing (7)
CLAUDE.mdsrc/Assimp/Importer.cppsrc/Assimp/MeshProcessor.cppsrc/Assimp/MeshProcessor.hsrc/CLIPipeline.cppsrc/ModelIsometricRenderer.cppsrc/ModelTurntableRenderer.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/ModelTurntableRenderer.cpp
- CLAUDE.md
- src/Assimp/MeshProcessor.h
- src/Assimp/Importer.cpp
- src/CLIPipeline.cpp
…#933 review) Matrix3::Inverse() returns ZERO for a singular linear part, which would null every normal — guard the determinant and fall back to identity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
1 participant
Summary
Implements #933 — all three reported problems plus the crash from the issue comment.
1. Shaded rendering (the white-blob fix)
The turntable/isometric renderers used full-white ambient, so untextured models rendered as flat silhouettes with zero depth cues. The default is now DCC-viewport-style: low ambient (0.35) + key directional + fill from the opposite side. New
qtmesh turntable --studioadds a three-point preset (warm key / cool fill / rim) for thumbnail-quality output. RTSS schemes are invalidated around the light changes (the cached-shader lesson from the MCP screenshot fix) so the lights actually take effect.2. Upright framing for Blender-style FBX (the top-down fix)
These rigs stamp Y-up metadata but carry the standing orientation on the armature/mesh nodes (−90°X), which the importer dropped — the bind pose rendered lying down, so thumbnails read top-down at any elevation. The reference skinned mesh node's world rotation is now baked exactly like the existing Z-up-metadata path (root bones + vertices + binding-pose re-snapshot). Identity for Mixamo-style rigs — Rumba's silhouette coverage is bit-identical.
3. Robot.fbx crash → assembled render (issue comment)
Blender bone-parented rigid parts (no vertex weights) crashed every render with Ogre's
softwareVertexBlendassertion. They're now bound to their nearest ancestor bone (weight 1) with the node-chain placement baked into the vertices — parts render assembled and follow the bone during animation, exceeding the requested bind-pose fallback. Multi-skinned-mesh scenes also get per-mesh frame alignment (Assimp bone offsets are per-mesh-node).Known limitation (documented in-code): rigs whose skin bind diverges from their node tree (the Robot's arm chain) keep a residual global tilt — no node-derived rotation is exact for those.
Acceptance check
qtmesh turntable KnightCharacter.fbx -o thumb.png --frames 1→ upright, shaded, recognizable character (skin-toned head/hands, gray armor). 12-frame sheet remains the default.Test plan
isometric --animationstill walks (the isometric --animation renders empty frames (turntable --animation also missing) #936 fix intact)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--studiooption for turntable renders with warm key, cool fill, and rim lighting.Documentation