fix(loader): compute glTF skin bounds in rootBone space#3027
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors glTF skin root-bone resolution when explicit skeleton indices are absent, replacing an approximate heuristic with an LCA-based algorithm, and updates bounds computation to use the inverted rootBone world transform. ChangesGLTF Skinning and Scene Loading
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@packages/loader/src/gltf/parser/GLTFSkinParser.test.ts`:
- Around line 4-8: The test mutates globalThis.window inside createParser which
can leak state; update tests to save the original value (const _origWindow =
globalThis.window) and set the stubbed value in a beforeEach hook (or inside
createParser but only after saving), then restore the original in an afterEach
hook (globalThis.window = _origWindow). Refer to createParser and
globalThis.window when locating the change and ensure restoration occurs even if
createParser throws.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9fae284d-d92d-4bbc-a0ea-a2592e03f586
📒 Files selected for processing (5)
packages/loader/src/gltf/extensions/GLTFExtensionSchema.tspackages/loader/src/gltf/parser/GLTFSceneParser.tspackages/loader/src/gltf/parser/GLTFSkinParser.test.tspackages/loader/src/gltf/parser/GLTFSkinParser.tstests/src/loader/GLTFLoader.test.ts
|
补充说明 当前 PR 里这个文件最终只保留 这里要解决的是：
旧逻辑在 新逻辑改成直接用 const inverseRootBoneWorld = new Matrix();
Matrix.invert(rootBone.transform.worldMatrix, inverseRootBoneWorld);
BoundingBox.transform(mesh.bounds, inverseRootBoneWorld, skinnedMeshRenderer.localBounds);这和 |
|
补充说明 这里现在不是要扩展
这个边界来自 CI visual diff 的 RCA：之前把 mesh node 纳入 LCA，会把某些现有资产的 rootBone 从 因此当前代码有意保留 joints-only fallback： const rootBone = this._findSkinRootBoneByLCA(joints, entities);如果资产需要表达 “rootBone 是 joints 外面的某个父节点”，正确来源应是显式 |
|
补充整理了测试覆盖（已按
本次整理后的当前 head： 验证：
|
luzhuang
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3027 +/- ##
===========================================
+ Coverage 77.48% 77.52% +0.03%
===========================================
Files 914 914
Lines 101783 101678 -105
Branches 10430 10431 +1
===========================================
- Hits 78862 78821 -41
+ Misses 22738 22673 -65
- Partials 183 184 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/src/loader/GLTFLoader.test.ts (1)
97-149: ⚡ Quick winConsider adding a mesh node that uses the skin to fully exercise the updated LCA algorithm.
The fixture defines joints
[0, 1]but no node has askinproperty, meaning no mesh uses this skin. According to the PR objectives and context snippet 1, the updated LCA algorithm collects "joint indices plus nodes whosenode.skin === skinIndex". Without a mesh node referencing the skin, the test validates only the joints-only path, not the full algorithm.For comparison:
testSkinRoot.gltf(line 57): node 0 hasskin: 0testSkinRootBounds.gltf(line 179): node 1 hasmesh: 0, skin: 0🔧 Suggested enhancement
Add a mesh reference and skin property to one of the nodes, for example:
nodes: [ { name: "Character_Root", - children: [1] + children: [1, 3] }, { name: "mixamorig:Hips" }, { name: "Light" + }, + { + name: "Character_Man", + mesh: 0, + skin: 0 } ], +meshes: [ + { + primitives: [ + { + attributes: { POSITION: 1 }, + mode: 4 + } + ] + } +],And add a position accessor pointing to the buffer. This would ensure the test exercises the full LCA calculation including mesh nodes.
🤖 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 `@tests/src/loader/GLTFLoader.test.ts` around lines 97 - 149, The test fixture for context.glTFResource.url.endsWith("testSingleSkeleton.gltf") defines skins with joints [0,1] but no node references that skin, so update the mocked glTF to include a mesh node that uses the skin (e.g., set one of the nodes to include mesh: 0 and skin: 0), add a corresponding meshes entry with a primitive that references a POSITION accessor, and add a POSITION accessor (and any necessary bufferView) pointing into the existing buffer so the loader's LCA logic (which looks for joint indices plus nodes where node.skin === skinIndex) is exercised fully; modify the nodes array, meshes array, and accessors/bufferViews to reflect the mesh+accessor relationship.
🤖 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.
Nitpick comments:
In `@tests/src/loader/GLTFLoader.test.ts`:
- Around line 97-149: The test fixture for
context.glTFResource.url.endsWith("testSingleSkeleton.gltf") defines skins with
joints [0,1] but no node references that skin, so update the mocked glTF to
include a mesh node that uses the skin (e.g., set one of the nodes to include
mesh: 0 and skin: 0), add a corresponding meshes entry with a primitive that
references a POSITION accessor, and add a POSITION accessor (and any necessary
bufferView) pointing into the existing buffer so the loader's LCA logic (which
looks for joint indices plus nodes where node.skin === skinIndex) is exercised
fully; modify the nodes array, meshes array, and accessors/bufferViews to
reflect the mesh+accessor relationship.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 733075d1-1a8a-4cc4-b892-1fef080893f1
📒 Files selected for processing (1)
tests/src/loader/GLTFLoader.test.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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 `@packages/loader/src/gltf/parser/GLTFSkinParser.ts`:
- Line 45: The LCA fallback currently calls _findSkinRootBoneByLCA(joints,
entities) using only the joints list, which drops mesh nodes that reference this
skin; update the input to _findSkinRootBoneByLCA to include both the joint nodes
and any entity nodes whose glTF node has node.skin === skinIndex (i.e. merge
joints with entities.filter(e => e.gltfNode?.skin === skinIndex)) so the LCA
calculation considers mesh participants as well; apply the same change to the
other occurrence that computes the no-skeleton LCA (the other call site around
the current fallback logic) so both rootBone computations include
skin-referencing nodes.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 29c8e4a4-950e-460e-833d-c04f9658ecef
📒 Files selected for processing (4)
notes/loader/2026-06-11-gltf-skin-rootbone-visual-ci.mdpackages/loader/src/gltf/parser/GLTFSkinParser.test.tspackages/loader/src/gltf/parser/GLTFSkinParser.tstests/src/loader/GLTFLoader.test.ts
✅ Files skipped from review due to trivial changes (1)
- notes/loader/2026-06-11-gltf-skin-rootbone-visual-ci.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/src/loader/GLTFLoader.test.ts
- packages/loader/src/gltf/parser/GLTFSkinParser.test.ts
luzhuang
changed the title
|
Update after CI RCA on The PR no longer expands the missing- Current boundary:
I updated the PR title/body and corrected the earlier explanatory comments to match this current behavior. CI is green on head |
|
Follow-up cleanup across
Local verification for this cleanup:
GitHub checks are green on head |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
已按这轮 review 收敛到当前 head
验证：
远端状态：GitHub Actions 全绿， |
|
已处理这两条 non-blocking 建议，当前 head
本地验证：
远端 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
force-pushed
the
fix/gltf-loader-shaderlab-split
branch
from
d9614ca to
12757c3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
复审 HEAD c3f279dfc，自上轮 approve 后无新提交、作者无新增回复。核心修复不变：GLTFSceneParser._computeLocalBounds else 分支（rootBone ∉ skin.joints）用 inverse(rootBone.worldMatrix) 把 mesh bounds 映射到 rootBone 空间，替代旧的"平均子 joint IBM"近似。
核心不变量再确认成立：运行时出口 SkinnedMeshRenderer._updateBounds 固定乘 rootBone.transform.worldMatrix，所以加载入口必须用同一根 rootBone 的逆世界矩阵归零——rootBone ∈ joints 走 IBM_root、rootBone ∉ joints 现场算 inverse(rootBone.worldMatrix)，两者同一公式，一进一出精确抵消。对规范资产（导出器生成的 IBM 自洽）这是 bind-pose 下的精确解而非近似。测试覆盖了"显式 rootBone 在 joints 外 + 非均匀 rootBone 缩放"，断言 rootBone-space localBounds 与最终 world bounds，能守住对称性。
无新问题，维持 approve。
3 participants
Summary
This is the glTF Loader split from #3014, narrowed to the skin/rootBone bounds path.
skin.skeletonas the authoritativeSkin.rootBone, including the valid glTF case where that root is not part ofskin.joints.skin.skeleton: inferSkin.rootBonefrom the lowest common ancestor ofskin.jointsonly. Skinned mesh nodes are owners/users of the skin, not skeleton roots, and are intentionally not used as LCA inputs.localBoundsdirectly in rootBone space when the rootBone is not one of the skin bones.tests/src/loader/GLTFLoader.test.ts.Why
There are two separate cases in this loading path:
skin.skeleton: the loader needs a stable fallback rootBone. The safest fallback is the joints' own LCA. Including mesh nodes that merely reference the skin can promote the rootBone above the skeleton hierarchy and change renderer transform space for existing assets.skin.skeletonoutsideskin.joints: this is allowed by glTF and should be preserved. In that case the old bounds fallback averaged child joint inverse bind matrices, which is only an approximation and can shiftSkinnedMeshRenderer.localBounds. The bounds should instead be transformed byinverse(rootBone.worldMatrix), matching the rootBone-space definition used by runtime skin matrix updates.Changes
GLTFSceneParserrootBoneis one of the skin bones.inverse(rootBone.transform.worldMatrix), so mesh bounds are transformed into rootBone local space directly.Tests
GLTF_ROOT.localBoundsand final worldbounds.Verification
pnpm -F @galacean/engine-loader run b:typespnpm vitest run tests/src/loader/GLTFLoader.test.ts --testNamePattern "Multi-root skins without skeleton|Skinned mesh bounds"pnpm vitest run tests/src/loader/GLTFLoader.test.ts --testNamePattern "Skinned mesh bounds" --coveragepnpm exec eslint --no-eslintrc --config .eslintrc.js --resolve-plugins-relative-to . tests/src/loader/GLTFLoader.test.tsgit diff --checkSummary by CodeRabbit
Bug Fixes
skin.joints.Tests