fix(animation, loader): 从 #2983 抽离动画与 GLTF 加载器修复#2984
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:
WalkthroughAdds per-instance animator playback handles with speed overrides and lifecycle; updates Animator APIs and playback/crossfade to use per-instance play-data; supports self-name-prefixed Entity.findByPath; records GLTF scene roots for skin resolution; configures PhysX post-filtering to skip initial overlaps; updates runtime PhysX URLs and tests/e2e examples. ChangesAnimation Speed & Entity Path Resolution
GLTF Scene Root Tracking & Skin Detection
PhysX Query Filtering & Runtime URLs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
luzhuang
force-pushed
the
feat/animation-physics
branch
from
7e7591b to
6ab7437
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/animation/Animator.ts (1)
774-776:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCross-fade still uses shared
AnimatorState.speedinstead of per-instance speed.
Line 774 and Line 775 should usesrcPlayData.speed/destPlayData.speed; otherwise per-instance speed changes are ignored during cross-fade.💡 Proposed fix
- const srcPlaySpeed = srcState.speed * speed; - const dstPlaySpeed = destState.speed * speed; + const srcPlaySpeed = srcPlayData.speed * speed; + const dstPlaySpeed = destPlayData.speed * speed;🤖 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 `@packages/core/src/animation/Animator.ts` around lines 774 - 776, The cross-fade calculation incorrectly reads shared AnimatorState.speed (srcState.speed / destState.speed) instead of the per-instance play speeds, so replace usages that set srcPlaySpeed and dstPlaySpeed to use srcPlayData.speed and destPlayData.speed respectively and recompute dstPlayDeltaTime from dstPlaySpeed and deltaTime; update any downstream calculations that use srcPlaySpeed/dstPlaySpeed (e.g., destPlayDeltaTime) so the per-instance speed values from srcPlayData and destPlayData are honored during cross-fade.
🤖 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/core/src/animation/Animator.ts`:
- Around line 228-232: The method findAnimatorState currently declares return
type AnimatorStatePlayData but returns null on failure; change its signature to
return AnimatorStatePlayData | null (or | undefined) and update the function
implementation as needed so the type matches the null-returning behavior (refer
to findAnimatorState and its helper _getAnimatorStateInfo and use
_animatorLayersData to determine null cases); then update call sites that assume
non-null (e.g., tests or any code that dereferences .clip) to handle the
nullable return (add null checks or early returns) so no unsafe dereference
occurs.
In `@packages/loader/src/gltf/parser/GLTFSkinParser.ts`:
- Line 42: The single-line declaration combining the nullish coalescing call
makes formatting fail; split it into two lines with proper indentation by first
assigning rootBone from this._findSceneRootBone(context, joints, entities) on
its own line, then if that returns null/undefined assign rootBone to
this._findSkeletonRootBone(joints, entities), ensuring consistent indentation
and a line break between calls so ESLint/Prettier accept it; reference the
rootBone variable and the methods _findSceneRootBone and _findSkeletonRootBone
when applying the change.
In `@packages/physics-physx/src/PhysXPhysicsScene.ts`:
- Around line 53-55: _pxFilterData currently includes QueryFlag.POST_FILTER
though overlap queries (_overlapMultiple) only implement a preFilter and no
postFilter, which can cause binding failures; remove QueryFlag.POST_FILTER from
the shared this._pxFilterData flags initialization (update the call setting
this._pxFilterData.flags in PhysXPhysicsScene to use QueryFlag.STATIC |
QueryFlag.DYNAMIC | QueryFlag.PRE_FILTER only) and ensure any code that expects
postFilter uses a different filterData or explicitly adds POST_FILTER where both
preFilter and postFilter are implemented.
In `@tests/src/core/AnimatorHang.test.ts`:
- Around line 10-20: The test currently performs async setup inside the describe
callback (creating WebGLEngine via WebGLEngine.create, loading GLTFResource,
getting Animator) which runs at collection time; move that async setup into a
beforeAll hook and move cleanup into an afterAll hook that calls
engine.destroy() to free WebGL resources. Specifically, create a beforeAll(async
() => { engine = await WebGLEngine.create(...); scene =
engine.sceneManager.activeScene; rootEntity = scene.createRootEntity();
rootEntity.addComponent(Camera); resource = await
engine.resourceManager.load<GLTFResource>(glbResource); defaultSceneRoot =
resource.defaultSceneRoot; rootEntity.addChild(defaultSceneRoot); animator =
defaultSceneRoot.getComponent(Animator); }) and an afterAll(() =>
engine.destroy()); keep the describe callback synchronous and keep the
it("loaded", ...) assertion referencing the shared animator variable.
---
Outside diff comments:
In `@packages/core/src/animation/Animator.ts`:
- Around line 774-776: The cross-fade calculation incorrectly reads shared
AnimatorState.speed (srcState.speed / destState.speed) instead of the
per-instance play speeds, so replace usages that set srcPlaySpeed and
dstPlaySpeed to use srcPlayData.speed and destPlayData.speed respectively and
recompute dstPlayDeltaTime from dstPlaySpeed and deltaTime; update any
downstream calculations that use srcPlaySpeed/dstPlaySpeed (e.g.,
destPlayDeltaTime) so the per-instance speed values from srcPlayData and
destPlayData are honored during cross-fade.
🪄 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: d027c66a-6998-449f-8191-f2275769b9e7
⛔ Files ignored due to path filters (4)
e2e/.dev/physx.release.simd.wasmis excluded by!**/*.wasme2e/.dev/physx.release.wasmis excluded by!**/*.wasmpackages/physics-physx/libs/physx.release.simd.wasmis excluded by!**/*.wasmpackages/physics-physx/libs/physx.release.wasmis excluded by!**/*.wasm
📒 Files selected for processing (17)
e2e/.dev/physx.release.jse2e/.dev/physx.release.simd.jspackages/core/src/Entity.tspackages/core/src/animation/Animator.tspackages/core/src/animation/index.tspackages/core/src/animation/internal/AnimatorStatePlayData.tspackages/loader/src/gltf/parser/GLTFParserContext.tspackages/loader/src/gltf/parser/GLTFSceneParser.tspackages/loader/src/gltf/parser/GLTFSkinParser.tspackages/physics-physx/libs/physx.release.jspackages/physics-physx/libs/physx.release.simd.jspackages/physics-physx/src/PhysXPhysics.tspackages/physics-physx/src/PhysXPhysicsScene.tstests/src/core/Animator.test.tstests/src/core/AnimatorHang.test.tstests/src/core/Entity.test.tstests/src/loader/GLTFLoader.test.ts
| describe("Canvas 1024 test", async function () { | ||
| const engine = await WebGLEngine.create({ canvas: canvasDOM }); | ||
| const scene = engine.sceneManager.activeScene; | ||
| const rootEntity = scene.createRootEntity(); | ||
| rootEntity.addComponent(Camera); | ||
| const resource = await engine.resourceManager.load<GLTFResource>(glbResource); | ||
| const defaultSceneRoot = resource.defaultSceneRoot; | ||
| rootEntity.addChild(defaultSceneRoot); | ||
| const animator = defaultSceneRoot.getComponent(Animator); | ||
| it("loaded", () => { expect(animator).not.eq(null); }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Vitest support an async callback passed directly to describe(), or should async setup be done in beforeAll/afterAll?
💡 Result:
No—Vitest does not support an async callback passed directly to describe as a reliable way to do setup. Reason: Vitest’s describe callback is not part of the test execution lifecycle; it’s used to define/collect the suite. Vitest runs suites by collecting tests first and executing hooks/tests later [1]. In contrast, async setup belongs in suite-level hooks such as beforeAll/afterAll, which explicitly wait for returned promises: beforeAll “If the function returns a promise, Vitest waits until the promise resolve before running tests” [2] and afterAll has the same promise-waiting behavior [2]. If you try to put async work in the describe callback itself, that work will happen during the collection/definition phase (or can be effectively ignored), which can lead to race conditions with the later test execution [1]. Recommendation: do async initialization in beforeAll/afterAll (or in hooks provided by test.extend/fixtures), and keep describe callback synchronous (just defining tests/suites and registering hooks).
Citations:
- 1: Using an async function in describe instead of beforeAll()? vitest-dev/vitest#4858
- 2: https://main.vitest.dev/api/hooks
Move async setup out of describe and add teardown.
The async callback passed to describe at line 10 runs during test collection, not execution, causing potential race conditions. Vitest requires async setup in beforeAll/afterAll hooks. Additionally, add engine.destroy() in afterAll to clean up WebGL resources and prevent leaks across test runs.
💡 Proposed refactor
-import { describe, expect, it } from "vitest";
+import { afterAll, beforeAll, describe, expect, it } from "vitest";
@@
-describe("Canvas 1024 test", async function () {
- const engine = await WebGLEngine.create({ canvas: canvasDOM });
- const scene = engine.sceneManager.activeScene;
- const rootEntity = scene.createRootEntity();
- rootEntity.addComponent(Camera);
- const resource = await engine.resourceManager.load<GLTFResource>(glbResource);
- const defaultSceneRoot = resource.defaultSceneRoot;
- rootEntity.addChild(defaultSceneRoot);
- const animator = defaultSceneRoot.getComponent(Animator);
- it("loaded", () => { expect(animator).not.eq(null); });
+describe("Animator hang regression", () => {
+ let engine: WebGLEngine;
+ let animator: Animator | null;
+
+ beforeAll(async () => {
+ engine = await WebGLEngine.create({ canvas: canvasDOM });
+ const scene = engine.sceneManager.activeScene;
+ const rootEntity = scene.createRootEntity();
+ rootEntity.addComponent(Camera);
+ const resource = await engine.resourceManager.load<GLTFResource>(glbResource);
+ const defaultSceneRoot = resource.defaultSceneRoot;
+ rootEntity.addChild(defaultSceneRoot);
+ animator = defaultSceneRoot.getComponent(Animator);
+ });
+
+ afterAll(async () => {
+ await engine.destroy();
+ });
+
+ it("loaded", () => {
+ expect(animator).not.eq(null);
+ });
});📝 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.
| describe("Canvas 1024 test", async function () { | |
| const engine = await WebGLEngine.create({ canvas: canvasDOM }); | |
| const scene = engine.sceneManager.activeScene; | |
| const rootEntity = scene.createRootEntity(); | |
| rootEntity.addComponent(Camera); | |
| const resource = await engine.resourceManager.load<GLTFResource>(glbResource); | |
| const defaultSceneRoot = resource.defaultSceneRoot; | |
| rootEntity.addChild(defaultSceneRoot); | |
| const animator = defaultSceneRoot.getComponent(Animator); | |
| it("loaded", () => { expect(animator).not.eq(null); }); | |
| }); | |
| import { afterAll, beforeAll, describe, expect, it } from "vitest"; | |
| describe("Animator hang regression", () => { | |
| let engine: WebGLEngine; | |
| let animator: Animator | null; | |
| beforeAll(async () => { | |
| engine = await WebGLEngine.create({ canvas: canvasDOM }); | |
| const scene = engine.sceneManager.activeScene; | |
| const rootEntity = scene.createRootEntity(); | |
| rootEntity.addComponent(Camera); | |
| const resource = await engine.resourceManager.load<GLTFResource>(glbResource); | |
| const defaultSceneRoot = resource.defaultSceneRoot; | |
| rootEntity.addChild(defaultSceneRoot); | |
| animator = defaultSceneRoot.getComponent(Animator); | |
| }); | |
| afterAll(async () => { | |
| await engine.destroy(); | |
| }); | |
| it("loaded", () => { | |
| expect(animator).not.eq(null); | |
| }); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 19-19: Replace ·expect(animator).not.eq(null); with ⏎····expect(animator).not.eq(null);⏎·
(prettier/prettier)
🤖 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/core/AnimatorHang.test.ts` around lines 10 - 20, The test currently
performs async setup inside the describe callback (creating WebGLEngine via
WebGLEngine.create, loading GLTFResource, getting Animator) which runs at
collection time; move that async setup into a beforeAll hook and move cleanup
into an afterAll hook that calls engine.destroy() to free WebGL resources.
Specifically, create a beforeAll(async () => { engine = await
WebGLEngine.create(...); scene = engine.sceneManager.activeScene; rootEntity =
scene.createRootEntity(); rootEntity.addComponent(Camera); resource = await
engine.resourceManager.load<GLTFResource>(glbResource); defaultSceneRoot =
resource.defaultSceneRoot; rootEntity.addChild(defaultSceneRoot); animator =
defaultSceneRoot.getComponent(Animator); }) and an afterAll(() =>
engine.destroy()); keep the describe callback synchronous and keep the
it("loaded", ...) assertion referencing the shared animator variable.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@GuoLei1990 @cptbtptpbcptdtptp 已根据 review 重做，把 PlayData 升级为 per-Animator per-state 持久 handle，覆盖你们提的所有问题。 核心 lifecycle 改造：
override 模型（codex 建议的 live binding）：
cross fade speed bug：
proxy 清理：
附带修复：
测试： Animator 40/40，PhysicsScene 49/49 都过。请 review。 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/animation/Animator.ts (1)
1410-1453:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCross-fading to the currently-playing state aliases
srcPlayDataanddestPlayDatato the same instance, corrupting the cross-fade calculation.
layerData.getOrCreatePlayData(crossState)is keyed byAnimatorState, so whencrossState === layerData.srcPlayData?.state(e.g., user callsanimator.crossFade("CurrentState", ...)orcrossFadeInFixedDurationwith the current state name), the lookup returns the same instance that is currentlysrcPlayData.resetForPlay(...)then overwrites that shared instance's runtime fields (playedTime,clipTime,playState,currentEventIndex,isForward,offsetFrameTime), and line 1426 assigns it back asdestPlayData— leavingsrcPlayData === destPlayData.Downstream in
_updateCrossFadeState(lines 785–786), bothsrcPlayData.update(srcPlayCostTime)anddestPlayData.update(dstPlayCostTime)then mutate the same fields, causing the source side to accumulate both cost times instead of just its own. This corruptscrossWeight = Math.abs(destPlayData.playedTime) / transitionDurationat line 795. On completion,promoteDest()(line 1061) becomes a no-op since src and dest are the same object, anddestPlayData = nullthen loses the reference.In the previous implementation
srcPlayDataanddestPlayDatawere separateAnimatorStatePlayDatainstances managed byswitchPlayData(), so this scenario was safe. There is nocanTransitionToSelf-style guard on the manualcrossFade*path (the_checkNoExitTimeTransitionsguard withexcludeDestStateonly applies to stateMachine transitions).Suggested fix: Early-return when crossing to self:
private _prepareCrossFadeByTransition(transition: AnimatorStateTransition, layerIndex: number): boolean { const crossState = transition.destinationState; if (!crossState) { return false; } if (!crossState.clip) { Logger.warn(`The state named ${crossState.name} has no AnimationClip data.`); return false; } const animatorLayerData = this._getAnimatorLayerData(layerIndex); + // Cross-fading to the currently-playing state would alias src/dest to the same + // AnimatorStatePlayData instance (Map keyed by AnimatorState). Skip until + // canTransitionToSelf semantics are supported. + if ( + animatorLayerData.layerState === LayerState.Playing && + animatorLayerData.srcPlayData?.state === crossState + ) { + return false; + } const animatorStateData = this._getAnimatorStateData(crossState.name, crossState, animatorLayerData, layerIndex);🤖 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 `@packages/core/src/animation/Animator.ts` around lines 1410 - 1453, The bug is that _prepareCrossFadeByTransition can alias srcPlayData and destPlayData when crossState equals the currently playing state, corrupting cross-fade math; fix it in _prepareCrossFadeByTransition by detecting if animatorLayerData.srcPlayData?.state === crossState (or animatorLayerData.srcPlayData === destPlayData after getOrCreatePlayData) and early-return false (or abort the cross-fade) before calling resetForPlay and assigning destPlayData, so srcPlayData and destPlayData remain distinct; update any related state transitions (layerState, crossFadeTransition) only when a true cross-fade will occur.
🤖 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 `@tests/src/core/Animator.test.ts`:
- Around line 1316-1418: Add a regression test that exercises crossFade to the
currently-playing state to ensure src and dest PlayData are not aliased: create
a test in Animator.test.ts that calls animator.play("Walk"), advances a frame
(increment engine.time._frameCount and call animator.update), capture the
layer's srcPlayData (via animator._animatorLayersData[0] or
_getAnimatorLayerData(0)), then call animator.crossFade("Walk", 0.3) and advance
another frame/update, and assert that if layerData.destPlayData exists it is not
equal to layerData.srcPlayData and that layerData.srcPlayData remains === the
original captured instance; this locks down behavior around
getOrCreatePlayData/_prepareCrossFadeByTransition when dest equals the currently
playing AnimatorState.
In `@tests/src/core/physics/PhysicsScene.test.ts`:
- Around line 472-478: The test's ray origin is currently at (3,3,3) which lies
on the boundary for boxShape.size = new Vector3(6,6,6); update the test so the
origin is unambiguously inside the collider (e.g., use new Ray(new
Vector3(0,0,0), ...) or new Vector3(2.9,2.9,2.9)) before calling
physicsScene.raycast(ray, outHitResult) so that the assertions about skipped
initial overlap (expect(...).to.eq(false), outHitResult.distance === 0,
outHitResult.entity === null) reliably exercise the inside-origin case; adjust
the Ray construction in this test block accordingly.
---
Outside diff comments:
In `@packages/core/src/animation/Animator.ts`:
- Around line 1410-1453: The bug is that _prepareCrossFadeByTransition can alias
srcPlayData and destPlayData when crossState equals the currently playing state,
corrupting cross-fade math; fix it in _prepareCrossFadeByTransition by detecting
if animatorLayerData.srcPlayData?.state === crossState (or
animatorLayerData.srcPlayData === destPlayData after getOrCreatePlayData) and
early-return false (or abort the cross-fade) before calling resetForPlay and
assigning destPlayData, so srcPlayData and destPlayData remain distinct; update
any related state transitions (layerState, crossFadeTransition) only when a true
cross-fade will occur.
🪄 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: 863f88a5-ff41-42a7-a1b2-4962deff4c0b
📒 Files selected for processing (11)
packages/core/src/Entity.tspackages/core/src/animation/Animator.tspackages/core/src/animation/AnimatorStatePlayData.tspackages/core/src/animation/index.tspackages/core/src/animation/internal/AnimatorLayerData.tspackages/core/src/animation/internal/AnimatorStatePlayData.tspackages/loader/src/gltf/parser/GLTFSkinParser.tspackages/physics-physx/src/PhysXPhysicsScene.tstests/src/core/Animator.test.tstests/src/core/Entity.test.tstests/src/core/physics/PhysicsScene.test.ts
💤 Files with no reviewable changes (1)
- packages/core/src/animation/internal/AnimatorStatePlayData.ts
✅ Files skipped from review due to trivial changes (2)
- packages/core/src/animation/index.ts
- tests/src/core/Entity.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/loader/src/gltf/parser/GLTFSkinParser.ts
- packages/core/src/Entity.ts
- packages/physics-physx/src/PhysXPhysicsScene.ts
PR galacean#2984 changed Animator.findAnimatorState() to return AnimatorStatePlayData instead of AnimatorState. Unit tests were already updated to access shared-asset members via `.state.xxx`; e2e cases were missed and would TypeError at runtime when playwright loaded them. Convert each shared-asset access on findAnimatorState() results: - .clip -> .state.clip (animator-event, animator-additive) - .addTransition / .addExitTransition / ._getDuration -> .state.xxx (animator-stateMachine) - .addStateMachineScript -> .state.addStateMachineScript (animator-stateMachineScript) .speed reads/writes are intentionally preserved on the per-instance handle (the whole point of the API change).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2984 +/- ##
===========================================
+ Coverage 78.14% 78.40% +0.25%
===========================================
Files 900 900
Lines 99255 99419 +164
Branches 10213 10232 +19
===========================================
+ Hits 77563 77948 +385
+ Misses 21521 21300 -221
Partials 171 171
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/case/animator-stateMachine.ts (1)
57-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard nullable
findAnimatorState()results before.statedereference.At lines 57–59,
findAnimatorState()results are used without null checks and subsequently dereferenced with.statedownstream. If any of these state names (idle/walk/run) are not found, the code will crash at runtime.Add an early guard to fail fast with a clear error message:
✅ Minimal fix pattern
const idleState = animator.findAnimatorState("idle"); const walkState = animator.findAnimatorState("walk"); const runState = animator.findAnimatorState("run"); + if (!idleState || !walkState || !runState) { + throw new Error("Required animator states (idle/walk/run) were not found."); + }🤖 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 `@e2e/case/animator-stateMachine.ts` around lines 57 - 59, The code calls animator.findAnimatorState("idle"/"walk"/"run") and later dereferences .state without checking for null, so add an early guard after obtaining idleState, walkState, and runState to verify none are null/undefined and throw or assert with a clear message (e.g., "Animator state 'idle' not found") before any usage of .state; reference the animator.findAnimatorState calls and the idleState/walkState/runState variables when adding the checks so the failure is fast and the error message identifies the missing state.
🤖 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.
Outside diff comments:
In `@e2e/case/animator-stateMachine.ts`:
- Around line 57-59: The code calls
animator.findAnimatorState("idle"/"walk"/"run") and later dereferences .state
without checking for null, so add an early guard after obtaining idleState,
walkState, and runState to verify none are null/undefined and throw or assert
with a clear message (e.g., "Animator state 'idle' not found") before any usage
of .state; reference the animator.findAnimatorState calls and the
idleState/walkState/runState variables when adding the checks so the failure is
fast and the error message identifies the missing state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 707726dd-fb6f-4d45-8ce6-aa21b99d5950
📒 Files selected for processing (4)
e2e/case/animator-additive.tse2e/case/animator-event.tse2e/case/animator-stateMachine.tse2e/case/animator-stateMachineScript.ts
|
Round-2 review 修复： P1：crossFade-to-self alias bug ( P2： P2： P2：PhysX inside-collider raycast 边界点改严格 inside（ |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/core/src/animation/Animator.ts`:
- Around line 625-626: When computing playSpeed/playDeltaTime (using
srcPlayData.speed and this.speed) guard against playSpeed === 0 to avoid 0/0 →
NaN: if playSpeed is zero, treat playDeltaTime as 0 and any subsequent division
like playCostTime / playSpeed should short‑circuit to 0 (or skip applying
remaining time) instead of performing the division. Update the logic around the
symbols playSpeed, playDeltaTime and any use of playCostTime / playSpeed (also
the same pattern around the block at the other occurrence referenced by the
reviewer) to check for playSpeed === 0 and handle that branch explicitly.
- Around line 1428-1431: Reformat the if guard around
animatorLayerData.srcPlayData?.state and animatorLayerData.destPlayData?.state
comparing to crossState so Prettier stops failing: update the condition in
Animator.ts (the if that checks animatorLayerData.srcPlayData?.state ===
crossState || animatorLayerData.destPlayData?.state === crossState) to a
single-line expression or to a Prettier-friendly multiline layout (e.g., put the
|| at the start of the second line or align each comparison on its own line)
while keeping the same logic and references to animatorLayerData, srcPlayData,
destPlayData, and crossState.
- Around line 225-228: The method findAnimatorState should return null for
out-of-range layerIndex values instead of calling _getAnimatorStateInfo which
assumes a valid index; add an early bounds check (e.g., if layerIndex >= 0 and
(layerIndex < this._layers.length) is false) and return null before calling
_getAnimatorStateInfo, or alternatively update _getAnimatorStateInfo to handle
invalid layer indexes safely; ensure you reference the same layer storage
(this._layers) and still call _getAnimatorLayerData(foundLayer) only after a
validated foundLayer.
🪄 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: 18b4e6a1-07fc-425d-b9a0-8fd6878018b6
📒 Files selected for processing (6)
packages/core/src/Entity.tspackages/core/src/animation/Animator.tspackages/core/src/animation/AnimatorStatePlayData.tstests/src/core/Animator.test.tstests/src/core/Entity.test.tstests/src/core/physics/PhysicsScene.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/Entity.ts
- tests/src/core/Entity.test.ts
- packages/core/src/animation/AnimatorStatePlayData.ts
- tests/src/core/physics/PhysicsScene.test.ts
PR galacean#2984 changed findAnimatorState to return AnimatorStatePlayData | null. Update both EN and ZH docs to reflect: - Per-instance speed override (playData.speed) - Shared asset access (playData.state.xxx) - Nullable return guard - clearSpeedOverride() to resume live binding to state.speed
github-actions
Bot
added
the
documentation
|
Round-4 review 处理： P1 已修
P2 已修zero-speed
e2e camelCase ( 不修（设计决策）
CI 全绿（lint / build × 3 / codecov / e2e × 4）。 |
关于 lazy
|
| 场景 | 分配次数 | 占帧预算 |
|---|---|---|
| 单 transition 暖 1 个新 state | 1 × ~400ns | 0.0024% |
| 同帧暖 10 个新 state | 10 × ~400ns | 0.024% |
| 同帧暖 100 个 state（不现实） | 100 × ~400ns | 0.24% |
| 暖好后任意 transition crossFade | 0 | 0% |
实际 transition 有方向，一帧最多切到 1 个目标 state；即使极端构造同帧暖 100 个 state 也只占帧预算 0.24%，不可能掉帧。
4. 每帧 hot path 零分配
crossFade 进行中 _updateCrossFadeState (Animator.ts:785-) 每帧执行的操作：
srcPlayData.update(srcPlayCostTime); // 累加 _playedTime
destPlayData.update(dstPlayCostTime);
_evaluatePlayingState(srcPlayData, ...); // 读 curve 写 component
_evaluatePlayingState(destPlayData, ...);全部操作已有 PlayData 引用，0 PlayData 分配。promoteDest() 也只是 src = dest; dest = null 两次引用赋值，0 分配。
5. GC 不会被触发
- PlayData 被
map.set立即引用 → 下次 minor GC 时对 map 可达 → V8 晋升到 old space（O(1)） - old space GC 触发条件是 heap 压力 (MB 级)，N 个 ~80 字节对象 (N=100 也只 8KB) 远低于阈值
- 暖机期间 N 次分配本身也不触发 minor GC：new space 通常 1–8MB 容量，PlayData 总量 N×80B 完全在容量内
6. vs 旧 swap 模式
| 阶段 | 新 promote 模式 | 旧 swap 模式 |
|---|---|---|
第一次 play(X) |
new PlayData(X) + map.set |
reset srcSlot 字段 |
后续 play(X) |
map.get(X) 命中复用，0 分配 |
reset srcSlot 字段 |
| transition crossFade(X→Y) 首次 Y | new PlayData(Y) + map.set |
reset destSlot 字段 |
| 后续 transition crossFade | map.get 复用，0 分配 |
reset destSlot 字段 |
| 每帧 update / evaluate | 0 分配 | 0 分配 |
| crossFade 完成 | promoteDest 赋值，0 分配 |
switchPlayData 赋值，0 分配 |
暖机后两者完全等价。新模式的额外内存代价是每个被访问过的 state 一个轻量对象 (N × ~80B)，换取 _speedOverride 等 per-instance 持久能力。
结论
- lazy 暖机：每个 state 一生只 new 一次，~200–400ns
- 稳态 0 分配：暖好后 transition crossFade 全走
map.get复用 - 不可能掉帧：最坏极端 100 个 state 同帧暖机占帧预算 0.24%
- 不触发 GC spike：长寿对象进 old space，N 个轻量对象远低于 GC 阈值
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 11:51Z 的总结提到「目前无新问题」，跟 11:23Z 的 CHANGES_REQUESTED 状态似乎不同步。简要确认下 11:23Z 唯一的 P2（ 1. 本 PR 分支从来没有这个文件: $ git log dev/2.0..HEAD -- tests/src/core/AnimatorHang.test.ts
# (empty, 本 PR 分支无任何提交触碰此路径)
$ find tests/ -name "AnimatorHang.test.ts"
# (empty, 工作树不存在)该文件唯一来源是 2. 本 PR 的 // tests/src/core/Animator.test.ts:40
describe("Animator test", function () { // ← 同步函数,不是 async
...
beforeAll(async function () { ... }); // ← async 在 beforeAll 内,Vitest 正确 await
});53 个 Animator 测试在本 PR HEAD (e5b881c) 全部通过,没有"静默跳过/时序不确定"。 如果可以,请 dismiss 11:23Z 的 CHANGES_REQUESTED,本 PR 已 mergeable (冲突已解,CI 待 reverify)。 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@GuoLei1990 12:15Z 评论里说的 1. // tests/src/core/Animator.test.ts:40-55
describe("Animator test", function () { // ← sync function declaration
let animator: Animator;
let resource: GLTFResource;
let engine: WebGLEngine;
beforeAll(async function () { // ← async 只出现在 beforeAll 内
engine = await WebGLEngine.create({ canvas: canvasDOM });
...
});Vitest 不 await describe 返回值这点是事实，但前提是 describe callback 本身是 async（返回 Promise）才会触发"await 之后的 it() 被 collect 阶段跳过"。本文件 describe callback 是 sync 2. 实测 53/53 通过，零 silent skip collect 阶段就识别到 53 个 case，全部执行通过。如果存在 async describe 导致的注册时序问题，应该会出现 3. 11:23Z 提的 前者本 PR 分支不存在（前评论已证），后者存在但无 async describe。两次 review 的 P2 都不成立。 请 dismiss 之前的 CHANGES_REQUESTED，PR 当前 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@GuoLei1990 06:23Z review 的 5 个 issue 第一性原理逐项核实，全部不存在于当前 HEAD (e5b881c)，看起来 CodeRabbit 在读 stale snapshot： [P1] cross-fade 仍用
const srcPlaySpeed = srcPlayData.speed * speed; // ✅ playData.speed
const dstPlaySpeed = destPlayData.speed * speed; // ✅ playData.speed全文件 [P2]
findAnimatorState(stateName: string, layerIndex: number = -1): AnimatorStatePlayData | null {你 11:23Z 的「已关闭清单」第二行就确认了这条已修。 [P2]
[P2] AnimatorStatePlayData 公开后 runtime 字段暴露 —— ✅ 文档已写
你要求加的那行已经在那里。 [P3] 当前文件 90 行，只有 阻塞状态
PR 当前 如果对 CodeRabbit 报告有具体疑问，建议你在 |
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.
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.
已关闭问题清单
前几轮 review 提出的所有关键问题已全部关闭：
| 问题 | 状态 |
|---|---|
findAnimatorState 返回类型破坏性变更 |
✅ 已修复：返回 AnimatorStatePlayData | null |
cross-fade 用 state.speed 忽略 per-instance override |
✅ 已修复：改用 srcPlayData.speed |
| out-of-range layerIndex 越界 | ✅ 已修复：safe no-op |
| zero playSpeed NaN | ✅ 已修复 |
| self-crossFade alias guard | ✅ 已修复 |
| GLTF skin rootBone LCA | ✅ 已修复 |
| PhysX POST_FILTER 污染 overlap | ✅ 已修复：独立 _pxRaycastSweepFilterData |
| PhysX query callback 复用 | ✅ 已修复 |
总结
从 #2983 抽离的动画 + GLTF + 物理三方向修复，方向正确，53 个 Animator 测试、GLTF wrapper/skin root 测试、PhysX initial overlap + reentrancy 测试全覆盖。breaking changes 已在 PR 描述中完整列出，文档同步更新。整体设计质量高。
问题
[P2] AnimatorStatePlayData 缺少恢复 live-bind 的公开 API
playData.speed = x 写入后 _speedOverride 被设置，此后 state.speed（asset 侧）的变化不再传递到该 instance。若用户想重新跟随 asset，文档建议 playData.speed = playData.state.speed，但这是"设置一次相同的值"而非"清除 override"——之后 asset.speed 再变化仍不传递。
建议提供 clearSpeedOverride() 或允许 speed = undefined 清回 live-bind 状态，并在 AnimatorStatePlayData 的 JSDoc 中说明这两种模式的切换方式。
[P2] Animator.ts — 文档中 per-state PlayData handle 的生命周期边界未说明
entity disable→enable 一轮后 _reset() 会被调用，per-instance speed 等 override 会丢失。建议在 AnimatorStatePlayData 的类注释中明确："handle 在 Animator 的 controller 未更换时稳定持有；entity disable/enable 后 override 状态被重置"，避免用户误以为 override 在整个对象生命周期内持久。
AnimatorState.speed is part of the shared AnimatorController asset. Modifying it at runtime pollutes all Animator instances sharing the same controller, causing animation speed corruption after cloning. - Add speed field to AnimatorStatePlayData, initialized from AnimatorState.speed on reset - Add proxy properties (name/clip/wrapMode/transitions/addStateMachineScript) - Change speed calculation to playData.speed * animator.speed - findAnimatorState now returns per-instance AnimatorStatePlayData - Export AnimatorStatePlayData for consumer code
The earlier transition-out-of-speed-0 test went through _updateCrossFadeState (manual crossFade). The 0/0 NaN guard actually lives in _updatePlayingState's state-machine transition branch. Add a state-machine no-exit transition test that fires from a paused (speed override = 0) source state, ensuring dest receives the preserved remaining deltaTime and no NaN propagates.
… fields The public class previously exposed engine-managed runtime fields (stateData/playedTime/playState/clipTime/currentEventIndex/isForward/ offsetFrameTime) as plain public properties, relying on @internal JSDoc for visibility hiding. With underscore prefix the API surface is unambiguous: state (readonly), speed (getter/setter), and clearSpeedOverride() — anything else is implementation detail. User code that mutated runtime fields was already breaking Animator invariants; underscore-renaming makes the boundary explicit.
- Replace this._children.some(callback) with explicit for-loop to avoid the per-call callback allocation; matches core/animation iteration style for small caches and hot-ish paths. - Annotate _findChildByPathDown return type as Entity | null since the helper genuinely returns null on miss.
…miss The previous helper short-circuited on the first same-name child match and never tried later same-name siblings, regressing vs the old _findChildByName behavior. When a path like "root/a/leaf" needed to fall through the first 'a' subtree (no leaf) into the second 'a' subtree (has leaf), the helper returned null instead of finding the leaf. Continue the for-loop on miss so subsequent same-name siblings are attempted. Subtree containment is preserved: the helper still doesn't backtrack into entity.parent or beyond the entity's subtree.
statePlayDataMap was keyed by state.name; if a user removed and re-added a state with the same name (creating a fresh AnimatorState instance), the next getOrCreatePlayData would return the cached handle whose .state still pointed at the removed state. Validate identity on lookup and rebuild on mismatch. This addresses dynamic controller mutation patterns; it doesn't depend on AnimatorController dispatching an update flag for state-level mutations (which it currently doesn't).
play() and update() guarded against stale _animatorLayersData by checking _controllerUpdateFlag.flag and calling _reset(). findAnimatorState didn't, so handles returned right after a controller mutation (addLayer/removeLayer/clearLayers) were backed by pre-mutation layer data. Add the same guard.
Same-name removeState + addState would hit the stateName-keyed cache and return the previous state's curveLayerOwner / event handlers, so playing the new state evaluated with the old curve owners (e.g. position curve applied to rotation owner). Track the source state on AnimatorStateData and rebuild on identity miss; detach the prior clipChangedListener so the old state's UpdateFlagManager doesn't keep mutating discarded data. Adds a play+evaluate regression locking down curveLayerOwner rebinding.
…ulation _reset() dropped the entire animatorLayersData array, which meant the clipChangedListener installed on each surviving AnimatorState's UpdateFlagManager was leaked. Repeated controller mutations would register a fresh listener per play() while the previous ones lived on, mutating discarded stateData on every clip change. Walk the cached stateData maps before clearing the array and remove the listener via the handle now stored on AnimatorStateData. Adds a regression locking down listener count after three controller mutation cycles.
Round-11 plugged the same listener leak on _reset() but destroy() never went through that path, so destroying an Animator while its controller or any AnimatorState outlived it left the clipChangedListener attached to that state's UpdateFlagManager. The closure kept the destroyed entity reachable via clip dispatch. Call _reset() at the top of _onDestroy() so destroy reuses the listener detach pass we already added on controller mutation. Adds a regression that builds an Animator on a shared controller, plays once, destroys it, and asserts the listener count returns to its baseline.
The self-prefix branch added in 0273ebf assumed Animator could sit directly on a GLTF sceneRoot (single-root scenes), but c3d2160 in this same PR unconditionally wraps every scene in a GLTF_ROOT container 3 days earlier. After that change the Animator is always on the wrapper and binding paths always start with a real child name — the self-prefix branch is never reached on a real GLTF load. Evidence: the Animator regression added by 0273ebf itself (samples self-name-prefixed curve paths on wrapped roots) builds a "GLTF_ROOT" wrapper and a "mixamorig:Hips" child, then asserts wrappedRoot.findByPath("mixamorig:Hips"). splits[0] is "mixamorig:Hips", this.name is "GLTF_ROOT" — the self-prefix guard splits[0] === this.name is false, so the test passes via the generic child lookup with or without the branch. The four follow-up commits (c446e49 / 3ea4dd4 / da0d6a3 / 6de9e5a) were all patching edge cases introduced by the self-prefix branch itself; none of those bugs exist when the branch is gone. Drop: - findByPath self-prefix fallback (Entity.ts) - _findChildByPathDown helper, which existed only to back that fallback - six Entity tests that locked the self-prefix behaviour contract Keep: - generic findByPath sibling backtrack test (Entity.test.ts:304) which exercises _findChildByName's same-name-sibling retry - the Animator wrapper-relative regression (it now exercises the generic path, which is what it was actually testing anyway) - GLTFAnimationParser single-root prefix logic (necessary to make bindings GLTF_ROOT-relative)
… chaining
The two conditions !playData || playData.state !== state can be
expressed as a single playData?.state !== state — identical semantics
across all three cases (undefined, identity match, identity mismatch).
Removes one branch token while making the intent ("cache entry doesn't
belong to this state") read as a single thought.
clearSpeedOverride() existed solely to flip the instance back into
live-binding state.speed after a write. In practice no production flow
actually needs that: every "restore" use case is expressible as a
direct write (playData.speed = previousValue, or = playData.state.speed
to follow the current asset value). Keeping the API around forces
users to reason about a two-state model (live-bind vs override) instead
of the simpler "write = instance owns it" semantics.
Behavior:
- speed still live-binds to state.speed until first write (good editor
ergonomics: tweaking asset speed propagates to instances that haven't
claimed ownership yet)
- writing once flips the instance to own its own speed; no escape hatch
back to live-bind — by design, since the write is the signal of intent
- to re-follow the asset, simply assign playData.state.speed back
Drops 1 public API from AnimatorStatePlayData.d.ts. Surface is now
{ state, speed, get/set speed }.
Tests: removed the dedicated clearSpeedOverride regression. The
remaining 53 Animator tests still cover speed override read/write,
override survival across cross-fade, clone isolation, and speed=0
no-NaN guard.
Docs: updated zh/en animator.mdx pause example + findAnimatorState
section to use "assign the asset value back" instead of
"clearSpeedOverride()".
Two adjustments codex called out in round-9 polish review:
A) Centralize listener detach in AnimatorStateData.dispose(). The two
call sites that previously open-coded the same "destructure { state,
clipChangedListener }, null-check, removeListener" template — the
identity-mismatch branch in _getAnimatorStateData and the cleanup
loop in _reset — now both call stateData.dispose(). Future listener
sources (or rebuild paths) stay encapsulated in the data holder.
B) Make AnimatorStateData.state a readonly constructor parameter. The
runtime invariant is "a live stateData is always bound to a state",
but the previous nullable field forced every consumer to guard
`state && clipChangedListener` even though the null window only
existed for one statement between new and assignment. Construction
via `new AnimatorStateData(animatorState)` mirrors the same shape as
AnimatorStatePlayData(state) and removes the dead null branches.
No behavior change. 53 Animator tests still pass — they already cover
the lazy create, identity rebuild, listener accumulation prevention,
and destroy-time detach paths.
The signature on this PR became `AnimatorState | null` (it returns `_animatorLayersData[layerIndex]?.srcPlayData?.state ?? null`), but the 2.0 breaking-change summary only called out `findAnimatorState()`. Update JSDoc + zh/en animator.mdx to spell out the two null cases (missing layer / no state currently playing) and switch the snippet to the if-guarded form so callers see the right pattern.
…e-Skin order Strengthens the inline comment so a future "let's make the dependency explicit" refactor doesn't fall into the obvious trap of awaiting context.get(Scene) inside Skin. GLTFSceneParser._createRenderer requests context.get(Skin) for skinned renderers, so Skin → full Scene → Skin would deadlock the cached promise. The correct long-term path is exposing scene root wrapper creation as its own synchronous phase (e.g. ensureSceneRootsCreated()) that Skin can pull on demand without awaiting the entire Scene parse — but that's a loader-pipeline refactor for a follow-up PR, not this one.
…LCA only _findSceneRootBone was a fast-path classifier introduced in 9974c5b to handle multi-root scenes where joints span multiple top-level scene nodes — it short-circuited those cases to return the GLTF_ROOT wrapper. But c3d2160 (already on the branch) made GLTFSceneParser create the wrapper unconditionally and attach every top-level scene node under it. With that invariant, joint parent chains always include the wrapper at the top, so the existing LCA algorithm in _findSkeletonRootBone is already correct for both cases: Multi-root spanning: joints share only GLTF_ROOT → LCA returns wrapper. Single-root / converged: joints share a deeper ancestor → LCA returns the actual skeleton root, not the wrapper. The two-function ?? chain was masquerading a dispatch (case selection) as a fallback (try-then-retry), forcing readers into both helpers to understand that null from _findSceneRootBone meant "not my case" rather than "lookup failed". Removing it leaves a single, uniform rule: rootBone = LCA(joints). Changes: - Drop _findSceneRootBone (~50 lines) - Rename _findSkeletonRootBone to _findSkeletonRootBoneByLCA so the algorithm is self-describing - Tighten return type to Entity | null (rootNode starts as null and the function can return null when paths don't share any ancestor) - Simplify the call site to one path; the comment now states the invariant rather than guarding against a phantom fallback Existing GLTFLoader tests cover both branches and still pass: - "Multi-root skins without skeleton should use the scene wrapper as rootBone" verifies LCA → wrapper for the spanning case - "Multi-root scenes whose joints converge to a single top-level root should not use the scene wrapper" verifies LCA → Character_Root for the converged case
0623cb9 deleted _findSceneRootBone and made the Skin parser resolve rootBone via joint parent-chain LCA only. The previous comment still referenced both _findSceneRootBone and the "Skin reads _sceneRoots" mechanism, neither of which is true anymore. Restate the actual invariant: LCA needs the GLTF_ROOT wrapper present in joint parent chains, which Scene's parse head provides synchronously when it attaches top-level nodes under the wrapper. The cycle warning against "Skin awaits full Scene" stays — Scene's async tail still requests Skin via _createRenderer.
…verride mode After d0f3c35 removed clearSpeedOverride(), the "override mode" two- state concept stopped existing — speed is now simply a per-instance value that live-binds to state.speed until written. The internal _speedOverride field name still implied the old "claims override, can be cleared" semantics. Rename to _speed and clean up "override" wording from the doc header, resetForPlay comment, and five test names / inline comments so the source matches the actual model. Behavior unchanged. All 53 Animator tests still pass.
The same three lines —
if (this._controllerUpdateFlag?.flag) {
this._reset();
}
— guarded entry into play(), update(), findAnimatorState(), and
_crossFade() to drop stale layer data after a controller mutation.
Pull them into a single private helper so the intent is named once
and the call sites read as one statement.
No behavior change. All 53 Animator tests still pass.
…_correctTime
Two doc-only polish points from a second-pass review:
- The top-level class JSDoc duplicated the `speed` getter's full
semantics paragraph. Shorten the class doc to a one-line surface
pointer ("see the `speed` getter for live-bind semantics"), so the
detailed read/write behavior is documented once at the getter.
- `_correctTime` reads as "correct what?" without tracing both call
sites. Add a one-line comment explaining that reverse playback
resumed at clipTime=0 would step into negatives, so we snap to
clipEnd to keep the reverse loop seamless.
No code or behavior change. 53 Animator tests still pass.
9974c5b added a synchronous (glTFResource._sceneRoots ||= [])[index] = sceneRoot write inside GLTFSceneParser.parse. Its original consumer (the now-deleted _findSceneRootBone in GLTFSkinParser, removed in 0623cb9) is gone, so the line looks redundant against the asynchronous _handleSubAsset path that the glTFResourceMap[Scene] = "_sceneRoots" mapping triggers. It is not redundant: dev/2.0 already writes _defaultSceneRoot synchronously here, while _sceneRoots[i] would only be filled in Scene's async tail through _handleSubAsset. The two wrapper-index fields would observe different visibility timings, and any same-tick reader (e.g. a future synchronous parser consumer or editor inspector) could see _defaultSceneRoot set while _sceneRoots[i] is still undefined. Add an inline comment so a future cleanup pass doesn't delete this line under the wrong assumption that _handleSubAsset already covers it. No behavior change. GLTF loader tests still pass.
luzhuang
force-pushed
the
feat/animation-physics
branch
from
e5b881c to
4457f62
Compare
luzhuang
changed the title
|
@GuoLei1990 已将物理 raycast/sweep 修复抽离至独立 PR #2998。 本 PR 现在仅含：
物理 9 个 commits 已 rebase 到独立分支 Force-push 已完成（commits: 62 → 53）。指向旧 SHA 的 inline review comments 会标 outdated，评论本体保留。 请重新评审较小的 diff，或确认无新问题后 dismiss 之前的 CHANGES_REQUESTED。 |
|
续在 #2999：本 PR 因分支重命名（ |
2 participants
Summary
从 #2983 抽离动画 + GLTF 加载器修复，独立 PR 便于 review 与合入
dev/2.0。物理 raycast/sweep 相关修复已抽出独立 PR：#2998。
动画 — Per-state PlayData handle
把
AnimatorStatePlayData升级为"per-Animator per-state 持久 handle"，公开 API。findAnimatorState(name, layerIdx?): AnimatorStatePlayData | null— 返回稳定 handle（lazy create on first access），即使 state 从未播放也能拿到；controller mutation 后通过 update flag guard 先 reset 再返回，避免拿到 stale layerDataAnimatorLayerData.statePlayDataMap: Record<string, AnimatorStatePlayData>— 按state.name索引，Object.create(null)初始化，跟同 class 内animatorStateDataMap/curveOwnerPool风格一致；getOrCreatePlayData内做 identity 校验，同名 state 替换时重建 handleplayData.speed— getter/setter，内部_speed: number | undefined；未写入前 live-bindstate.speed（编辑器调 asset 仍能传递到未声明 override 的 instance），写入后该 instance 拥有自己的 speed（之后 asset.speed 变化不再传递）；想再次跟随 asset 直接写playData.state.speedplayData.state.xxx— 显式访问 shared 资产（删除全部 proxy properties /addStateMachineScript）_前缀（_stateData/_playedTime/_clipTime/_playState/_currentEventIndex/_isForward/_offsetFrameTime），stripInternal把@internal字段从 d.ts 剥离，npm 用户公开面只剩state/speedAnimatorStateData—state改 readonly 构造参数 +dispose()集中 detachclipChangedListener；三处生命周期边界（identity miss /_reset/_onDestroy）都调dispose()释放 listener，避免 controller mutation 或 destroy 后 listener 残留在 surviving state 上_updateCrossFadeState— cross fade 阶段使用playData.speed（修复原来用state.speed导致 cross fade 阶段忽略 per-instance override 的 bug）findAnimatorState不再读state.clip.length，避免 state 没有 clip 时崩溃_getAnimatorStateInfo加 bounds check，findAnimatorState返回 null，play/crossFadeno-op，避免layers[99].stateMachine抛错playData.speed=0时_updatePlayingState中playCostTime / playSpeed不再产生 NaN_preparePlay清理 stale 状态 — 中断 cross fade 时清空destPlayData和crossFadeTransitiongetCurrentAnimatorState— 返回类型AnimatorState | null（layer 不存在 / 无 state 播放时返回 null）加载器（动画路径相关）
fix(loader): resolve skin rootBone by joint LCA— Scene parser 同步把 top-level scene nodes 挂到GLTF_ROOTwrapper 下；没有显式skin.skeleton时，rootBone 一律通过 joints 的最近公共祖先（LCA）算出来。multi-root spanning joints 自然解析为 wrapper，converged joints 解析为真实 skeleton root（如Character_Root）。删除之前的_findSceneRootBone特化分支fix(animation): normalize single-root clip binding paths— single-root scene 的 clip binding path 前置 sceneRoot 名字，让 binding 永远相对GLTF_ROOTGLTFParserContextScene-before-Skin 注释 — 显式说明 LCA 依赖 wrapper 已挂 parent chain 的不变量；禁止 Skin parser await full Scene（避免_createRenderer反向请求 Skin 造成循环依赖）Entity
Entity.findByPath— 通用 path 解析 + 同名 sibling backtrack；null parent 不再 crash测试
_reset和destroy都 detach listener 不累积findByPath通用 path + 同名 sibling backtrack抽离说明
dev/2.0的 commit（GLTF_ROOT 容器 GLTFSceneParser: Always create container root node for consistent animation bone paths #2942/fix(loader): always create GLTF_ROOT container for consistent animation paths #2943、blendShape e2e 适配）Breaking changes (2.0)
Animator.findAnimatorState()返回AnimatorStatePlayData | null（旧返回AnimatorState）Animator.getCurrentAnimatorState()返回AnimatorState | null（旧实现 out-of-range layer 会抛错）AnimatorStatePlayData不再代理AnimatorState字段（name/clip/wrapMode/transitions/addStateMachineScript）—— 用playData.state.xxxAnimatorStatePlayData移出internal/，从@galacean/engine-core公开导出；stripInternal把 engine-managed runtime 字段从 d.ts 剥离，公开面只剩state/speedAnimator.findAnimatorState/play/crossFade对 out-of-range layerIndex 改为 safe no-op（旧实现会抛错）Test plan
tests/src/core/Animator.test.ts— 通过 53 个，含 per-state handle / per-instance speed / alias guard / out-of-range layer / zero-speed transition / state-machine self-transition / play interrupt crossFade clears dest / state identity 替换 / listener 不累积 / destroy detachtests/src/core/Entity.test.ts— 通过 39 个tests/src/loader/GLTFLoader.test.ts— 通过 7 个，含 GLTF wrapper / skin root（multi-root span / single-skeleton converge）tests/src/loader/SceneFormatV2.test.ts— 通过