Skip to content

fix(loader): always create GLTF_ROOT container for consistent animation paths#2943

Merged
cptbtptpbcptdtptp merged 2 commits into
galacean:dev/2.0from
luzhuang:fix/gltf-scene-root-container
Mar 26, 2026
Merged

fix(loader): always create GLTF_ROOT container for consistent animation paths#2943
cptbtptpbcptdtptp merged 2 commits into
galacean:dev/2.0from
luzhuang:fix/gltf-scene-root-container

Conversation

@luzhuang
Copy link
Copy Markdown
Contributor

@luzhuang luzhuang commented Mar 26, 2026

Summary

Fixes #2942

  • Remove single-root vs multi-root branching in GLTFSceneParser, always creating a GLTF_ROOT wrapper entity
  • Ensures animation bone paths are consistent across different glTF files, fixing cross-file animation clip retargeting
  • Matches the behavior of Three.js (wrapper Group), Babylon.js (__root__ node), and Cocos Creator

Breaking Change

Single-root glTF files now have one extra nesting level (GLTF_ROOT → original root). Code that references entities by path or index from instantiateSceneRoot() may need adjustment.

Test plan

  • Added test verifying single-root scene creates GLTF_ROOT container
  • Updated Animator layer mask test paths for new nesting level
  • Full test suite passes (1057 passed, 0 failed)

Summary by CodeRabbit

  • Refactor

    • Consistently create a standard GLTF_ROOT container as the scene root to unify hierarchy structure.
  • Tests

    • Added a test validating single-root glTF scene structure.
    • Updated animator unit tests to match restructured entity paths.
    • Adjusted end-to-end cases to locate renderers differently and apply initial transforms to child nodes where appropriate.

…on paths (galacean#2942)

Remove single-root vs multi-root branching in GLTFSceneParser, always
creating a GLTF_ROOT wrapper entity. This ensures animation bone paths
are consistent across different glTF files, fixing cross-file animation
clip retargeting.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 9914a2bb-33ec-4a7e-84d2-1ef5794e38f7

📥 Commits

Reviewing files that changed from the base of the PR and between 60c88be and 42362d6.

📒 Files selected for processing (2)
  • e2e/case/animator-blendShape.ts
  • e2e/case/animator-multiSubMeshBlendShape.ts

Walkthrough

The GLTFSceneParser now always creates a "GLTF_ROOT" container entity and attaches all scene nodes as its children; tests and e2e code were updated to account for the added container level and to verify the new defaultSceneRoot structure.

Changes

Cohort / File(s) Summary
Scene parser restructuring
packages/loader/src/gltf/parser/GLTFSceneParser.ts
Removed conditional reuse of a single scene node as the root. Always instantiate a "GLTF_ROOT" Entity, mark it as template, and add each scene node via context.get as children.
Animator test path updates
tests/src/core/Animator.test.ts
Adjusted layer mask path strings from "_rootJoint/..." to root/_rootJoint/... to reflect the extra container root in path resolution.
Loader tests: root verification
tests/src/loader/GLTFLoader.test.ts
Added Vitest suite asserting glTFResource.defaultSceneRoot is named "GLTF_ROOT" and contains the expected single child for single-root glTF files.
e2e adjustments
e2e/case/animator-blendShape.ts, e2e/case/animator-multiSubMeshBlendShape.ts
Adjusted component lookup and which entity receives initial transforms/Animator to account for the new container-level defaultSceneRoot.children[0] usage.

Sequence Diagram(s)

sequenceDiagram
    participant Parser as GLTFSceneParser
    participant Context as GLTFParserContext
    participant Root as "Entity\n(GLTF_ROOT)" rect rgba(100,150,250,0.5)
    participant Node as "Entity\n(scene node)" rect rgba(150,200,150,0.5)
    participant Animator as Animator

    Parser->>Context: parse sceneNodes[]
    Parser->>Root: new Entity("GLTF_ROOT") / _markAsTemplate(glTFResource)
    loop for each sceneNode
        Parser->>Context: context.get(Entity, sceneNode)
        Context-->>Node: return Entity for node
        Root->>Node: addChild(node)
    end
    Note right of Root: defaultSceneRoot -> GLTF_ROOT
    Animator->>Root: findByPath("skeletonRoot/...")
    Root-->>Animator: resolve child paths (consistent since skeleton root included)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I stitched a root both snug and neat,

GLTF_ROOT now holds each little feet.
No more missing hops along the way,
Bones find their paths and dance all day. 🎀

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: implementing a consistent GLTF_ROOT container structure for animation paths.
Linked Issues check ✅ Passed The PR successfully implements all coding requirements from issue #2942: always creates GLTF_ROOT container regardless of scene node count, ensuring consistent animation bone paths across different glTF files.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the objective: GLTFSceneParser modification and test/e2e updates to account for the new nesting level introduced by the GLTF_ROOT wrapper.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 26, 2026

Codecov Report

❌ Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.26%. Comparing base (0597e61) to head (42362d6).
⚠️ Report is 1 commits behind head on dev/2.0.

Files with missing lines Patch % Lines
e2e/case/animator-multiSubMeshBlendShape.ts 0.00% 2 Missing ⚠️
e2e/case/animator-blendShape.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           dev/2.0    #2943      +/-   ##
===========================================
+ Coverage    77.07%   77.26%   +0.18%     
===========================================
  Files          899      899              
  Lines        98229    98292      +63     
  Branches      9683     9682       -1     
===========================================
+ Hits         75706    75941     +235     
+ Misses       22355    22184     -171     
+ Partials       168      167       -1     
Flag Coverage Δ
unittests 77.26% <62.50%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@augmentcode
Copy link
Copy Markdown

augmentcode Bot commented Mar 26, 2026

🤖 Augment PR Summary

Summary: Always wrap parsed glTF scenes in a GLTF_ROOT entity so animation paths are stable across assets.

Changes: GLTFSceneParser now unconditionally creates GLTF_ROOT and attaches all scene nodes beneath it.

Updated Animator layer-mask tests to include the new leading root-node segment for path lookups.

Added a GLTFLoader test to assert single-root scenes also produce a GLTF_ROOT container.

Why: Consistent bone/track paths improves cross-file animation clip retargeting and matches other engines’ root wrapping behavior.

Note: Single-root scenes gain one extra nesting level (GLTF_ROOT → original root).

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

- animator-blendShape: use getComponentsIncludeChildren to find
  SkinnedMeshRenderer on child entity
- animator-multiSubMeshBlendShape: place Animator on actual model
  root (children[0]) so curve bindings with empty path resolve correctly
@GuoLei1990 GuoLei1990 self-requested a review March 26, 2026 06:03
Copy link
Copy Markdown
Collaborator

@cptbtptpbcptdtptp cptbtptpbcptdtptp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@cptbtptpbcptdtptp cptbtptpbcptdtptp merged commit c3d2160 into galacean:dev/2.0 Mar 26, 2026
11 of 12 checks passed
@cptbtptpbcptdtptp cptbtptpbcptdtptp added bug Something isn't working glTF loader labels Mar 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working glTF loader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants