Fix: allow anyState transitions to interrupt crossFade & fix transition bugs #2805
Conversation
WalkthroughAdds cross-fade transition interrupt handling to the Animator, allowing noExitTime transitions to preempt active cross-fades. Introduces a new private helper method to check and apply interrupt transitions. Updates AnimatorStateTransitionCollection to use a noExitTimeCount boundary for separating exit-time-dependent logic. Expands test coverage significantly. Changes
Sequence DiagramsequenceDiagram
participant Animator
participant AnimatorStateTransitionCollection
participant AnimatorState as AnimatorState/LayerData
participant NewState as Target State
Animator->>Animator: Start cross-fade update pass
Animator->>AnimatorStateTransitionCollection: _checkCrossFadeInterrupt(layer, transitions, ...)
AnimatorStateTransitionCollection->>AnimatorStateTransitionCollection: Iterate noExitTime transitions (0 to noExitTimeCount)
AnimatorStateTransitionCollection-->>Animator: Return matched interrupt transition (or null)
alt Interrupt Found
Animator->>AnimatorState: Update current state with interrupt transition
Animator->>NewState: Apply transition, consume deltaTime
Animator->>Animator: Return early (bypass cross-fade logic)
else No Interrupt
Animator->>Animator: Continue existing cross-fade progression
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2805 +/- ##
===========================================
+ Coverage 83.03% 83.21% +0.18%
===========================================
Files 796 796
Lines 90383 90436 +53
Branches 9488 9496 +8
===========================================
+ Hits 75046 75256 +210
+ Misses 15255 15098 -157
Partials 82 82
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/core/src/animation/Animator.ts (2)
1571-1574: Skip onUpdate for zero advance; consider tolerating sub-epsilon deltas.The zero-delta guard aligns script callbacks with evaluation (no time advanced => no onUpdate). To avoid spurious onUpdate from floating drift (e.g., tiny ±1e-15 residuals), consider a tolerance:
- if (deltaTime === 0) { + if (Math.abs(deltaTime) <= MathUtil.zeroTolerance) { return; }This still allows meaningful reverse playback (negative deltas) to reach onUpdate.
1548-1578: Clarify parameter semantics for maintainers (deltaTime here is “clip-time delta”, not engine time).deltaTime in this helper is the per-state “played” delta (may be 0 or negative), not engine Time.deltaTime. A short doc block will prevent future regressions.
TypeScript snippet to add above the method (no code movement required):
/** * Fire animation events and invoke state machine scripts for this slice. * Note: `deltaTime` is the state's clip-time delta used for evaluation in this call. * It is 0 when no time advanced (e.g., first frame after play/crossFade, zero speed, zero-duration transition), * and may be negative during reverse playback. */tests/src/core/Animator.test.ts (3)
711-755: Great coverage for “finish this frame” semantics; make cleanup robust.The assertions precisely capture the intended callback order. To avoid leaving scripts attached if an assertion throws, wrap spy assertions in try/finally and always remove scripts:
- const walkExitSpy = vi.spyOn(walkScript, "onStateExit"); + const walkExitSpy = vi.spyOn(walkScript, "onStateExit"); // Play source state, then in the same frame crossFade to dest with duration 0 animator.play("Walk"); animator.crossFade("Run", 0); animator.update(0.1); - expect(walkExitSpy).toHaveBeenCalledTimes(1); - expect(walkUpdateSpy).toHaveBeenCalledTimes(0); - expect(runEnterSpy).toHaveBeenCalledTimes(1); - expect(runUpdateSpy).toHaveBeenCalledTimes(0); - expect(runExitSpy).toHaveBeenCalledTimes(0); - - // cleanup: remove scripts - // @ts-ignore - walkState._removeStateMachineScript(walkScript); - // @ts-ignore - runState._removeStateMachineScript(runScript); + try { + expect(walkExitSpy).toHaveBeenCalledTimes(1); + expect(walkUpdateSpy).toHaveBeenCalledTimes(0); + expect(runEnterSpy).toHaveBeenCalledTimes(1); + expect(runUpdateSpy).toHaveBeenCalledTimes(0); + expect(runExitSpy).toHaveBeenCalledTimes(0); + } finally { + // cleanup: remove scripts + // @ts-ignore + walkState._removeStateMachineScript(walkScript); + // @ts-ignore + runState._removeStateMachineScript(runScript); + }
757-787: Remove unused variable to keep tests clean.walkState is declared but never used.
- const walkState = animator.findAnimatorState("Walk"); - const runState = animator.findAnimatorState("Run"); + const runState = animator.findAnimatorState("Run");
789-813: Reverse playback callback check looks good; optionally assert no extra calls.Current check ensures at least one onStateUpdate during reverse playback. To tighten it slightly without brittleness, assert exactly one call (mirrors your positive-delta tests).
- expect(runUpdateSpy).toHaveBeenCalledTimes(1); + expect(runUpdateSpy).toHaveBeenCalledTimes(1);If you later add multiple updates in this test, keep the exact count aligned.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/core/src/animation/Animator.ts(1 hunks)tests/src/core/Animator.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/animation/Animator.ts (1)
packages/core/src/base/Time.ts (1)
deltaTime(40-42)
tests/src/core/Animator.test.ts (1)
packages/core/src/animation/index.ts (1)
StateMachineScript(23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: codecov
- GitHub Check: e2e (22.x)
🔇 Additional comments (4)
packages/core/src/animation/Animator.ts (1)
1562-1569: Correctly suppress onUpdate when state finishes in this slice; onExit fires once.The early-return on Finished prevents same-frame onUpdate after onExit and guards against duplicate exits across re-entrance. This matches the stated intent and the tests.
tests/src/core/Animator.test.ts (3)
326-327: Formatting-only change; OK.
872-874: Formatting-only change; OK.
1125-1125: Style-only change to closing brace; OK.
|
This PR is a partial fix. |
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
This PR is a partial fix.
However, I found that the animation state machine's events didn't follow the animation state machine, which is a system design error. This should be fixed from this point forward.
|
First, it's important to understand that this is a Callback to the state machine, so it's independent of the Clip's behavior. The state machine strives for state continuity. Therefore:
|
- 新增 MeshColliderShape 类，支持凸包和三角网格碰撞体 - 新增 IMeshColliderShape 接口定义 - PhysXPhysics 中实现 createMeshColliderShape 方法 - 初始化 PxCooking 用于网格碰撞体的烘焙 - 更新 PhysX WASM 文件以支持网格碰撞体 - 修复 Collider 添加 shape 时设置世界缩放 - 添加 MeshColliderShape 单元测试和 E2E 测试
- 重写 physx-mesh-collider 测试，使用 glTF 模型展示网格碰撞 - 新增 physx-mesh-collider-data 测试，展示 setMeshData API - 添加 CCD 支持防止高速物体穿透薄网格 - 更新 PhysX WASM 支持 setCookingMeshPreprocessParams - e2e 测试使用本地 physx 文件而非 CDN - 删除 engine-mcp 目录
- 删除 C++ 源码中的调试日志 - 重新编译并上传 physx wasm/js 到 CDN - 更新 PhysXPhysics.ts 中的默认 CDN URL - e2e case 改回使用默认 CDN
- ColliderShape: setter 使用可选链处理 null - MeshColliderShape: 删除冗余 override 和回调重绑定 - PhysXMeshColliderShape: 提取内存分配公共方法 减少 122 行重复代码
- Collider._addNativeShape 中 setWorldScale 添加可选链保护 - 补充运行时动态添加 shape 缩放的单元测试
- move() 内部调用 PhysX 的 setKinematicTarget()，专为 kinematic 刚体设计 - 在非 kinematic 刚体上调用会导致 PhysX 隐式转换为 kinematic 模式 - 添加 isKinematic 检查，非 kinematic 时打印警告并返回 Closes galacean#2879
- 修复 MeshColliderShape.ts prettier 格式问题 - 调大 MeshCollider e2e 测试的 diffPercentage 阈值 (0 -> 0.02)
MeshColliderShape: - Fix: indices missing didn't return false - Fix: setMesh after addShape didn't add native shape to collider - Add super._syncNative() to sync base class properties - Extract _extractIndices method for code reuse - Add comment for PhysX Uint16/Uint32 indices - Add isConvex doc noting setMesh must be called after changing PhysXPhysics: - Remove unused buildTriangleAdjacencies setting
- Add try/finally to ensure WASM memory (_malloc) is freed on failure - Add null check for _pxMesh after cooking (returns null on failure) - Make setMeshData atomic: create new mesh before releasing old one - Release JS memory (_vertices/_indices) after copying to WASM
When switching from kinematic to dynamic, resync the collision detection mode to ensure CCD flags are properly restored to the user's setting.
- Block adding triangle mesh to non-kinematic DynamicCollider - Block switching isKinematic to false when triangle mesh is attached - Skip mass/inertia calculation for kinematic bodies (not used per PhysX doc) - Remove unused buildTriangleAdjacencies from setCookingParams
…to dynamic - Add mass/inertia recalculation when isKinematic changes from true to false - Improve _pxCookingParams documentation
- Change error message from "Triangle mesh" to "triangle mesh" for consistency - Update test to match the error message
- Remove _kinematicTargetSet flag - Remove auto move() call in _onUpdate - Fix PhysicsScene collision tests
Remove e2e test files without baseline images: - physx-mesh-collider-kinematic-bug.ts - physx-mesh-collider-sphere-container.backup.ts - physx-mesh-collider-sphere-container.ts
- Add _checkCrossFadeInterrupt method - Check anyState noExitTime transitions at beginning of crossFade update - Call _updateState after interrupt to consume full deltaTime - Add test case for anyState interrupting crossFade
luzhuang
force-pushed
the
fix/animator
branch
from
88bd9e5 to
516a23a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@e2e/case/physx-mesh-collider-data.ts`:
- Around line 232-236: onStart currently assumes getComponent(MeshRenderer)
returns non-null and calls getMaterial() on this._renderer; add a null-safety
check after retrieving the renderer (this._renderer =
this.entity.getComponent(MeshRenderer)) and bail out or throw a clear error if
it's null, e.g. guard before calling this._renderer.getMaterial(); ensure
subsequent lines that access this._material and call
this._originalColor.copyFrom(this._material.baseColor) only run when
this._material is non-null (perform a similar null check for getMaterial()) to
avoid runtime errors.
In `@e2e/case/physx-mesh-collider.ts`:
- Around line 204-209: The offset calculation and spawn positions use potWidth
directly which can be invalid; change them to use actualPotWidth (the variable
that applies the fallback) so offset = actualPotWidth * 0.15 and the Vector3
entries that reference offset remain consistent; update the lines constructing
offset and spherePositions (where offset, dropHeight, and spherePositions are
defined) to reference actualPotWidth instead of potWidth.
In `@packages/core/src/physics/Collider.ts`:
- Around line 183-187: The call in Collider._onUpdate must match the null-safety
used in _addNativeShape: change the direct call to
shape._nativeShape.setWorldScale(...) to use optional chaining
(shape._nativeShape?.setWorldScale(...)) so it safely handles null/undefined
_nativeShape; update the invocation in the _onUpdate method of the Collider
class to use _nativeShape?.setWorldScale with the same
entity.transform.lossyWorldScale argument.
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts`:
- Around line 96-102: The setWorldScale override in PhysXMeshColliderShape calls
this._updateGeometry() unconditionally which can throw if mesh data (_pxMesh)
isn't set; update PhysXMeshColliderShape.override setWorldScale to keep the
super.setWorldScale(scale) call but guard the call to this._updateGeometry()
with the same null check used in setDoubleSided/setTightBounds (e.g., if
(this._pxMesh) this._updateGeometry()), ensuring _pxMesh is present before
updating geometry.
- Around line 46-74: In setMeshData of PhysXMeshColliderShape, make mesh
creation exception-safe by capturing oldMesh and oldGeometry, then wrapping the
update/_createMesh() and setGeometry calls in a try block and ensuring
oldMesh.release() and oldGeometry.delete() run in a finally (or by catching
exceptions, cleaning up the newly created partial resources and releasing the
old ones, then rethrowing) so that if _createMesh() or _pxShape.setGeometry
throws, the previous resources are still released and no leak occurs; reference
the setMeshData method, the _createMesh call, the _pxShape.setGeometry call, and
the oldMesh/oldGeometry variables when implementing the fix.
🧹 Nitpick comments (6)
packages/core/src/animation/Animator.ts (1)
1200-1220: Consider consolidating with_checkNoExitTimeTransition.This method is similar to
_checkNoExitTimeTransition(lines 1181-1198) but with two differences:
- Iterates over
noExitTimeCountinstead ofcount- Adds a check to skip transitions targeting
currentDestStateThe iteration difference is significant:
_checkNoExitTimeTransitioniterates over all transitions despite its name, while this method only iterates over noExitTime transitions. If both behaviors are intentional, consider:
- Renaming
_checkNoExitTimeTransitionfor clarity, or- Parameterizing the common logic to reduce duplication
♻️ Potential refactor to consolidate logic
+ private _checkTransitionsForMatch( + layerData: AnimatorLayerData, + transitionCollection: AnimatorStateTransitionCollection, + aniUpdate: boolean, + onlyNoExitTime: boolean = false, + skipDestState?: AnimatorState + ): AnimatorStateTransition { + const limit = onlyNoExitTime ? transitionCollection.noExitTimeCount : transitionCollection.count; + for (let i = 0; i < limit; ++i) { + const transition = transitionCollection.get(i); + if (skipDestState && transition.destinationState === skipDestState) continue; + if ( + transition.mute || + (transitionCollection.isSoloMode && !transition.solo) || + !this._checkConditions(transition) + ) + continue; + + return this._applyTransition(layerData, transition, aniUpdate); + } + return null; + }e2e/case/physx-mesh-collider-data.ts (1)
68-95: Consider extracting shared vertex generation logic.The vertex generation in
createWavyTerrain(lines 79-94) duplicates the same calculation increateTerrainMesh(lines 32-56). Consider extracting this into a shared helper to reduce duplication.♻️ Suggested refactor
// Shared helper for generating terrain vertex and index data function generateTerrainData(gridSize: number, scale: number) { const positions: number[] = []; const indices: number[] = []; for (let z = 0; z <= gridSize; z++) { for (let x = 0; x <= gridSize; x++) { const px = (x - gridSize / 2) * scale; const pz = (z - gridSize / 2) * scale; const py = Math.sin(x * 0.5) * Math.cos(z * 0.5) * 1.5; positions.push(px, py, pz); } } for (let z = 0; z < gridSize; z++) { for (let x = 0; x < gridSize; x++) { const i = z * (gridSize + 1) + x; indices.push(i, i + gridSize + 1, i + 1); indices.push(i + 1, i + gridSize + 1, i + gridSize + 2); } } return { positions, indices }; }packages/core/src/physics/shape/ColliderShape.ts (1)
181-189: Consider adding null guards to_syncNative()for consistency.The
_syncNative()method still accessesthis._nativeShapedirectly without optional chaining, while the setters now use?.. This could cause issues if_syncNative()is called before the native shape is created (e.g., inMeshColliderShapewhich has lazy initialization).Note that
MeshColliderShape._syncNative()does add a guard (if (this._nativeShape)), but if subclasses callsuper._syncNative()before their native shape exists, this will throw.🔧 Suggested fix for consistency
protected _syncNative(): void { + if (!this._nativeShape) { + return; + } this._nativeShape.setPosition(this._position); this._nativeShape.setRotation(this._rotation); this._nativeShape.setContactOffset(this._contactOffset); this._nativeShape.setIsTrigger(this._isTrigger); this._nativeShape.setMaterial(this._material._nativeMaterial); this._collider?._handleShapesChanged(ColliderShapeChangeFlag.Property); }packages/physics-physx/src/PhysXPhysics.ts (2)
158-175: Consider adding validation for cooking parameters.The
setCookingParamsmethod directly sets values without validation. NegativemeshWeldTolerancevalues could cause unexpected behavior.🔧 Optional: Add parameter validation
setCookingParams(params: { /** Mesh weld tolerance. If mesh welding is enabled, this controls the distance at which vertices are welded. */ meshWeldTolerance?: number; /** Mesh preprocessing flags (bitwise OR of MeshPreprocessingFlag values). */ meshPreprocessParams?: number; }): void { const cp = this._pxCookingParams; if (params.meshWeldTolerance !== undefined) { + if (params.meshWeldTolerance < 0) { + console.warn("PhysXPhysics: meshWeldTolerance should be >= 0"); + } cp.meshWeldTolerance = params.meshWeldTolerance; } if (params.meshPreprocessParams !== undefined) { this._physX.setCookingMeshPreprocessParams(cp, params.meshPreprocessParams); } }
334-346: Consider using a named constant for the WeldVertices flag.The magic number
1forWeldVerticescould be more readable with a named constant, especially sinceMeshPreprocessingFlagis mentioned in thesetCookingParamsdocumentation.🔧 Optional: Use named constant
+/** MeshPreprocessingFlag.WeldVertices */ +const WELD_VERTICES_FLAG = 1; + private _init(physX: any): void { // ... existing code ... // Initialize cooking for mesh colliders const cookingParams = new physX.PxCookingParams(tolerancesScale); // Set default cooking params (WeldVertices + small tolerance) - physX.setCookingMeshPreprocessParams(cookingParams, 1); // WeldVertices = 1 + physX.setCookingMeshPreprocessParams(cookingParams, WELD_VERTICES_FLAG); cookingParams.meshWeldTolerance = 0.001;packages/core/src/physics/shape/MeshColliderShape.ts (1)
217-257: Duplicate registration in_physicalObjectsMap.Line 243 registers
thisinEngine._physicalObjectsMap[this._id], but the baseColliderShapeconstructor already does this (line 138 ofColliderShape.ts). While this is harmless since the ID is the same, it's redundant.🔧 Remove redundant registration
// Create new shape this._nativeShape = Engine._nativePhysics.createMeshColliderShape( this._id, this._vertices, vertexCount, this._indices, this._isConvex, this._material._nativeMaterial ); - Engine._physicalObjectsMap[this._id] = this; // Sync doubleSided and tightBounds to newly created native shape
| onStart(): void { | ||
| this._renderer = this.entity.getComponent(MeshRenderer); | ||
| this._material = this._renderer.getMaterial() as PBRMaterial; | ||
| this._originalColor.copyFrom(this._material.baseColor); | ||
| } |
There was a problem hiding this comment.
Add null-safety check for component retrieval.
getComponent(MeshRenderer) may return null if the component is not found, which would cause a runtime error when accessing getMaterial().
🐛 Proposed fix
onStart(): void {
- this._renderer = this.entity.getComponent(MeshRenderer);
- this._material = this._renderer.getMaterial() as PBRMaterial;
- this._originalColor.copyFrom(this._material.baseColor);
+ this._renderer = this.entity.getComponent(MeshRenderer);
+ if (this._renderer) {
+ this._material = this._renderer.getMaterial() as PBRMaterial;
+ if (this._material) {
+ this._originalColor.copyFrom(this._material.baseColor);
+ }
+ }
}📝 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.
| onStart(): void { | |
| this._renderer = this.entity.getComponent(MeshRenderer); | |
| this._material = this._renderer.getMaterial() as PBRMaterial; | |
| this._originalColor.copyFrom(this._material.baseColor); | |
| } | |
| onStart(): void { | |
| this._renderer = this.entity.getComponent(MeshRenderer); | |
| if (this._renderer) { | |
| this._material = this._renderer.getMaterial() as PBRMaterial; | |
| if (this._material) { | |
| this._originalColor.copyFrom(this._material.baseColor); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@e2e/case/physx-mesh-collider-data.ts` around lines 232 - 236, onStart
currently assumes getComponent(MeshRenderer) returns non-null and calls
getMaterial() on this._renderer; add a null-safety check after retrieving the
renderer (this._renderer = this.entity.getComponent(MeshRenderer)) and bail out
or throw a clear error if it's null, e.g. guard before calling
this._renderer.getMaterial(); ensure subsequent lines that access this._material
and call this._originalColor.copyFrom(this._material.baseColor) only run when
this._material is non-null (perform a similar null check for getMaterial()) to
avoid runtime errors.
| const offset = potWidth * 0.15; | ||
| const spherePositions = [ | ||
| new Vector3(0, dropHeight, 0), // Center | ||
| new Vector3(-offset, dropHeight + 0.1, offset), | ||
| new Vector3(offset, dropHeight + 0.2, -offset) | ||
| ]; |
There was a problem hiding this comment.
Use actualPotWidth for consistency with fallback logic.
Line 204 uses potWidth directly, but actualPotWidth (which has the fallback value) should be used instead. If bounds are invalid, potWidth could be Infinity or negative, causing invalid spawn positions.
🐛 Proposed fix
- const offset = potWidth * 0.15;
+ const offset = actualPotWidth * 0.15;📝 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.
| const offset = potWidth * 0.15; | |
| const spherePositions = [ | |
| new Vector3(0, dropHeight, 0), // Center | |
| new Vector3(-offset, dropHeight + 0.1, offset), | |
| new Vector3(offset, dropHeight + 0.2, -offset) | |
| ]; | |
| const offset = actualPotWidth * 0.15; | |
| const spherePositions = [ | |
| new Vector3(0, dropHeight, 0), // Center | |
| new Vector3(-offset, dropHeight + 0.1, offset), | |
| new Vector3(offset, dropHeight + 0.2, -offset) | |
| ]; |
🤖 Prompt for AI Agents
In `@e2e/case/physx-mesh-collider.ts` around lines 204 - 209, The offset
calculation and spawn positions use potWidth directly which can be invalid;
change them to use actualPotWidth (the variable that applies the fallback) so
offset = actualPotWidth * 0.15 and the Vector3 entries that reference offset
remain consistent; update the lines constructing offset and spherePositions
(where offset, dropHeight, and spherePositions are defined) to reference
actualPotWidth instead of potWidth.
| protected _addNativeShape(shape: ColliderShape): void { | ||
| shape._collider = this; | ||
| shape._nativeShape?.setWorldScale(this.entity.transform.lossyWorldScale); | ||
| this._nativeCollider.addShape(shape._nativeShape); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/core/src/physics/Collider.ts | sed -n '110,195p'Repository: galacean/engine
Length of output: 2716
🏁 Script executed:
rg -n '_nativeShape' --type=ts -g '!*.d.ts' packages/core/src/physics/Collider.tsRepository: galacean/engine
Length of output: 328
🏁 Script executed:
rg -n '_nativeShape\.' --type=ts -g '!*.d.ts' | grep -v '_nativeShape?\.'Repository: galacean/engine
Length of output: 1256
Inconsistent null-safety between _addNativeShape and _onUpdate.
Line 185 correctly uses optional chaining (_nativeShape?.setWorldScale), but line 120 in _onUpdate calls the same method without the null check. Both lines invoke setWorldScale on the same property, so they should handle null-safety consistently. Line 120 should also use optional chaining.
🐛 Proposed fix for consistency
_onUpdate(): void {
if (this._updateFlag.flag) {
const { transform } = this.entity;
(<IStaticCollider>this._nativeCollider).setWorldTransform(
transform.worldPosition,
transform.worldRotationQuaternion
);
const worldScale = transform.lossyWorldScale;
for (let i = 0, n = this.shapes.length; i < n; i++) {
- this.shapes[i]._nativeShape.setWorldScale(worldScale);
+ this.shapes[i]._nativeShape?.setWorldScale(worldScale);
}
this._updateFlag.flag = false;
}
}🤖 Prompt for AI Agents
In `@packages/core/src/physics/Collider.ts` around lines 183 - 187, The call in
Collider._onUpdate must match the null-safety used in _addNativeShape: change
the direct call to shape._nativeShape.setWorldScale(...) to use optional
chaining (shape._nativeShape?.setWorldScale(...)) so it safely handles
null/undefined _nativeShape; update the invocation in the _onUpdate method of
the Collider class to use _nativeShape?.setWorldScale with the same
entity.transform.lossyWorldScale argument.
| setMeshData( | ||
| vertices: Float32Array, | ||
| vertexCount: number, | ||
| indices: Uint16Array | Uint32Array | null, | ||
| isConvex: boolean | ||
| ): void { | ||
| // Save old resources | ||
| const oldMesh = this._pxMesh; | ||
| const oldGeometry = this._pxGeometry; | ||
|
|
||
| // Update data and create new mesh (may throw on failure) | ||
| this._pxMesh = null; | ||
| this._pxGeometry = null; | ||
| this._isConvex = isConvex; | ||
| this._vertices = vertices; | ||
| this._vertexCount = vertexCount; | ||
| this._indices = indices; | ||
|
|
||
| this._createMesh(); | ||
| this._pxShape.setGeometry(this._pxGeometry); | ||
|
|
||
| // Release old resources only after successful creation | ||
| if (oldMesh) { | ||
| oldMesh.release(); | ||
| } | ||
| if (oldGeometry) { | ||
| oldGeometry.delete(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential resource leak on mesh creation failure.
If _createMesh() (line 64) throws an error, the old resources (oldMesh, oldGeometry) are never released because the cleanup code (lines 68-73) only executes after successful geometry update.
🐛 Suggested fix for exception safety
setMeshData(
vertices: Float32Array,
vertexCount: number,
indices: Uint16Array | Uint32Array | null,
isConvex: boolean
): void {
// Save old resources
const oldMesh = this._pxMesh;
const oldGeometry = this._pxGeometry;
// Update data and create new mesh (may throw on failure)
this._pxMesh = null;
this._pxGeometry = null;
this._isConvex = isConvex;
this._vertices = vertices;
this._vertexCount = vertexCount;
this._indices = indices;
- this._createMesh();
- this._pxShape.setGeometry(this._pxGeometry);
-
- // Release old resources only after successful creation
- if (oldMesh) {
- oldMesh.release();
- }
- if (oldGeometry) {
- oldGeometry.delete();
- }
+ try {
+ this._createMesh();
+ this._pxShape.setGeometry(this._pxGeometry);
+ } catch (e) {
+ // Restore old resources on failure
+ this._pxMesh = oldMesh;
+ this._pxGeometry = oldGeometry;
+ throw e;
+ }
+
+ // Release old resources only after successful creation
+ if (oldMesh) {
+ oldMesh.release();
+ }
+ if (oldGeometry) {
+ oldGeometry.delete();
+ }
}📝 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.
| setMeshData( | |
| vertices: Float32Array, | |
| vertexCount: number, | |
| indices: Uint16Array | Uint32Array | null, | |
| isConvex: boolean | |
| ): void { | |
| // Save old resources | |
| const oldMesh = this._pxMesh; | |
| const oldGeometry = this._pxGeometry; | |
| // Update data and create new mesh (may throw on failure) | |
| this._pxMesh = null; | |
| this._pxGeometry = null; | |
| this._isConvex = isConvex; | |
| this._vertices = vertices; | |
| this._vertexCount = vertexCount; | |
| this._indices = indices; | |
| this._createMesh(); | |
| this._pxShape.setGeometry(this._pxGeometry); | |
| // Release old resources only after successful creation | |
| if (oldMesh) { | |
| oldMesh.release(); | |
| } | |
| if (oldGeometry) { | |
| oldGeometry.delete(); | |
| } | |
| } | |
| setMeshData( | |
| vertices: Float32Array, | |
| vertexCount: number, | |
| indices: Uint16Array | Uint32Array | null, | |
| isConvex: boolean | |
| ): void { | |
| // Save old resources | |
| const oldMesh = this._pxMesh; | |
| const oldGeometry = this._pxGeometry; | |
| // Update data and create new mesh (may throw on failure) | |
| this._pxMesh = null; | |
| this._pxGeometry = null; | |
| this._isConvex = isConvex; | |
| this._vertices = vertices; | |
| this._vertexCount = vertexCount; | |
| this._indices = indices; | |
| try { | |
| this._createMesh(); | |
| this._pxShape.setGeometry(this._pxGeometry); | |
| } catch (e) { | |
| // Restore old resources on failure | |
| this._pxMesh = oldMesh; | |
| this._pxGeometry = oldGeometry; | |
| throw e; | |
| } | |
| // Release old resources only after successful creation | |
| if (oldMesh) { | |
| oldMesh.release(); | |
| } | |
| if (oldGeometry) { | |
| oldGeometry.delete(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts` around lines 46 -
74, In setMeshData of PhysXMeshColliderShape, make mesh creation exception-safe
by capturing oldMesh and oldGeometry, then wrapping the update/_createMesh() and
setGeometry calls in a try block and ensuring oldMesh.release() and
oldGeometry.delete() run in a finally (or by catching exceptions, cleaning up
the newly created partial resources and releasing the old ones, then rethrowing)
so that if _createMesh() or _pxShape.setGeometry throws, the previous resources
are still released and no leak occurs; reference the setMeshData method, the
_createMesh call, the _pxShape.setGeometry call, and the oldMesh/oldGeometry
variables when implementing the fix.
| /** | ||
| * {@inheritDoc IColliderShape.setWorldScale } | ||
| */ | ||
| override setWorldScale(scale: Vector3): void { | ||
| super.setWorldScale(scale); | ||
| this._updateGeometry(); | ||
| } |
There was a problem hiding this comment.
Missing null check in setWorldScale override.
Unlike setDoubleSided and setTightBounds which guard _updateGeometry() with this._pxMesh checks, setWorldScale calls _updateGeometry() unconditionally. This will fail if called before mesh data is set.
🐛 Add null guard
override setWorldScale(scale: Vector3): void {
super.setWorldScale(scale);
- this._updateGeometry();
+ if (this._pxMesh) {
+ this._updateGeometry();
+ }
}📝 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.
| /** | |
| * {@inheritDoc IColliderShape.setWorldScale } | |
| */ | |
| override setWorldScale(scale: Vector3): void { | |
| super.setWorldScale(scale); | |
| this._updateGeometry(); | |
| } | |
| /** | |
| * {`@inheritDoc` IColliderShape.setWorldScale } | |
| */ | |
| override setWorldScale(scale: Vector3): void { | |
| super.setWorldScale(scale); | |
| if (this._pxMesh) { | |
| this._updateGeometry(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/physics-physx/src/shape/PhysXMeshColliderShape.ts` around lines 96 -
102, The setWorldScale override in PhysXMeshColliderShape calls
this._updateGeometry() unconditionally which can throw if mesh data (_pxMesh)
isn't set; update PhysXMeshColliderShape.override setWorldScale to keep the
super.setWorldScale(scale) call but guard the call to this._updateGeometry()
with the same null check used in setDoubleSided/setTightBounds (e.g., if
(this._pxMesh) this._updateGeometry()), ensuring _pxMesh is present before
updating geometry.
516a23a to
476f91b
Compare
luzhuang
changed the title
luzhuang
force-pushed
the
fix/animator
branch
from
476f91b to
516a23a
Compare
2 participants
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix
Summary
Changes
新功能：CrossFade 打断
在
_updateCrossFadeState和_updateCrossFadeFromPoseState开头添加 anyState noExitTime transition 检查，允许在 crossFade 过程中被打断并切换到新状态。_checkCrossFadeInterrupt方法，检查时跳过目标状态与当前 crossFade 目标相同的 transition（避免无限循环）_updateState让新状态消费当前帧的 deltaTimeBug 修复
Bug 1:
_addTransition排序边界错误当添加
hasExitTime=true的 transition 时，排序逻辑会与所有 transitions 比较（包括 noExitTime transitions），可能导致新 transition 被插入到 noExitTime transitions 前面，破坏数组结构[noExitTime..., hasExitTime...]。Bug 2:
_checkNoExitTimeTransition循环范围错误使用
transitionCollection.count作为循环上限，会错误地检查 hasExitTime transitions，可能使它们在 exitTime 到达前提前触发。Files Changed
Animator.ts: 添加 crossFade interrupt 逻辑和_checkCrossFadeInterrupt方法，修复_checkNoExitTimeTransition循环范围AnimatorStateTransitionCollection.ts: 修复_addTransition排序边界Animator.test.ts: 增强 afterEach 清理，新增 crossFade interrupt 和 noExitTime scan 测试Test Plan
HEADLESS=true npx vitest run tests/src/core/Animator.test.ts- 29 tests passed