Visitar URL original
Fix FBX animation refinement growing across bones by Popov72 · Pull Request #19050 · BabylonJS/Babylon.js · GitHub
Skip to content

Fix FBX animation refinement growing across bones - #19050

Open
Popov72 wants to merge 2 commits into
BabylonJS:masterfrom
Popov72:popov72-fix-fbx-animation-sampling
Open

Popov72 wants to merge 2 commits into
BabylonJS:masterfrom
Popov72:popov72-fix-fbx-animation-sampling

Conversation

@Popov72

@Popov72 Popov72 commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

🤖 This PR was created by the create-pr skill.

Summary

Fix the out-of-memory failure when importing animated Mixamo FBX files reported in this forum topic.

_buildInheritedRigBoneAnimations() previously fed each bone's refined sample-time array into the next bone's refinement pass. Although each pass limits recursion to two levels, that limit restarted for each bone, allowing subdivisions to compound across the rig.

Each bone now refines the same original sample intervals. The results are merged and sorted into a shared sampling grid. This preserves authored key times and adaptive refinement while bounding subdivisions to the existing depth limit independently of the number of animated bones.

Normalize the decomposed and interpolated quaternions used for the angular-error comparison. Small length errors, including the near-linear branch of quaternion interpolation, must not be interpreted as angular differences.

No public API or loader options change.

Regression coverage

  • A four-bone inherited rig previously generated 1,649 keys per rotation track; the test now requires no more than 121 keys while retaining adaptive samples and the complete clip range.
  • A small constant-speed rotation no longer gets extra samples from quaternion length error.
  • Constant rotations remain unrefined, and nonlinear translation retains authored times and the existing subdivision depth.

Validation

  • All 113 FBX unit tests pass.
  • Changed-file Prettier and ESLint checks pass (ESLint reports two pre-existing warnings outside the change).
  • A focused TypeScript check of the changed loader and tests passes with the core ambient declarations and generated shader prerequisites.
  • Loader-only tree-shaking invariants and side-effects synchronization checks pass.
  • The reporter's mixamo.fbx imports with the patched loader against Babylon.js 9.32.0 in a WebGL browser, without disabling refinement: 2 meshes, 1 skeleton, 1 animation group, 49 channels, approximately frames 0–505, and at most 2,499 keys per track. Animation advancement was confirmed after 120 rendered frames.
  • Before the fix, instrumented loading of the same asset grew from 999 sample times to 21,422 within eleven bone passes; the diagnostic stopped there to avoid exhausting memory.

Validation is scoped to the affected loader; no full-repository build or test run was performed.

Copilot AI balanced review requested due to automatic review settings October 25, 2026 15:46
Refine each inherited rig bone from the original sample grid and merge the results so subdivision depth cannot compound across bones. Normalize quaternions for the angular-error comparison and add regressions for bounded rig sampling and animation fidelity.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@bjsplat

bjsplat commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The localized changes address compounded sampling growth with focused regression coverage and no unresolved review findings.

No open issues 🎉

What changed in this PR

This PR fixes excessive animation sampling growth in the FBX loader by preventing refinement from compounding across bones, without changing public APIs or loader options.

Changes:

  • Refine each bone from the original sample intervals, then merge results into a shared, sorted sampling grid.
  • Normalize quaternions before measuring angular error to avoid unnecessary subdivisions.
  • Add regression coverage for refinement bounds, rotation stability, and authored sample-time preservation.
File Description
packages/​dev/​loaders/​test/​unit/​FBX/​skeletalAnimation.test.ts Adds a four-bone regression test enforcing bounded refinement and complete clip coverage.
packages/​dev/​loaders/​test/​unit/​FBX/​animationSampling.test.ts Tests stable rotations, authored times, and the subdivision depth limit.
packages/​dev/​loaders/​src/​FBX/​fbxFileLoader.pure.ts Prevents compounded refinement and normalizes quaternion error comparisons.

Review effort: Balanced

@bjsplat

bjsplat commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

Snapshot stored with reference name:
refs/pull/19050/merge

Test environment:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/19050/merge/index.html

To test a playground add it to the URL, for example:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/19050/merge/index.html#WGZLGJ#4600

Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves):

https://playground.babylonjs.com/?snapshot=refs/pull/19050/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/19050/merge
https://gui.babylonjs.com/?snapshot=refs/pull/19050/merge
https://nme.babylonjs.com/?snapshot=refs/pull/19050/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/19050/merge#BCU1XR#0

If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools.

@bjsplat

bjsplat commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

@bjsplat

bjsplat commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

🟢 Memory Leak Test Results

4 passed, 0 leaked out of 4 scenarios

🟢 All memory leak tests passed — no leaks detected.

Passed Scenarios (4)
Scenario Package
Core Playground #2FDQT5#1508 @babylonjs/core
Core Playground #T90MQ4#14 @babylonjs/core
Core Playground #8EDB5N#2 @babylonjs/core
Core Playground #LL5BIQ#636 @babylonjs/core

@bjsplat

bjsplat commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

@bjsplat

bjsplat commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

⚡ Performance Test Results

🟢 All performance tests passed — no regressions detected.

@bjsplat

bjsplat commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@bjsplat

bjsplat commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

@bjsplat

bjsplat commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

🟢 Memory Leak Test Results

4 passed, 0 leaked out of 4 scenarios

🟢 All memory leak tests passed — no leaks detected.

Passed Scenarios (4)
Scenario Package
Core Playground #2FDQT5#1508 @babylonjs/core
Core Playground #T90MQ4#14 @babylonjs/core
Core Playground #8EDB5N#2 @babylonjs/core
Core Playground #LL5BIQ#636 @babylonjs/core

@bjsplat

bjsplat commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

@bjsplat

bjsplat commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

⚡ Performance Test Results

🟢 All performance tests passed — no regressions detected.

@bjsplat

bjsplat commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

@deltakosh deltakosh self-assigned this Oct 26, 2026

@deltakosh deltakosh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hello @Popov72,

#19042 has already merged the same _buildInheritedRigBoneAnimations fix, and this branch now has merge conflicts. Please resolve the overlap while retaining the quaternion-normalization change and complementary tests; this PR is not entirely redundant.

Thanks!

@deltakosh
deltakosh enabled auto-merge (squash) October 26, 2026 17:15
@deltakosh
deltakosh disabled auto-merge October 28, 2026 12:39
@deltakosh
deltakosh requested a review from sebavan October 28, 2026 14:02

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants