fix(physics): gate contact event buffering#3025
Draft
luzhuang wants to merge 1 commit into
Draft
Conversation
WalkthroughThis PR refactors the physics system to improve pose synchronization, add kinematic transform control, implement deferred physics updates via hooks, gate contact event generation based on active handlers, and expand test coverage with comprehensive kinematic and force-at-position validation. ChangesPhysics Core Architecture and Contracts
DynamicCollider Kinematic and Force Enhancements
PhysX Backend: Tolerances, Kinematic CCD, and Contact Events
Test Engine Import Migration
Collision and Kinematic Behavior Tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
7 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
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/physics/shape/MeshColliderShape.ts (1)
201-204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop retrying unsupported non-convex dynamic mesh creation every tick.
Line [201]-Line [204] returns for a permanent unsupported state, but Line [249]-Line [250] keeps retrying while pending is true. After
meshassignment on non-kinematicDynamicCollider, this can spam errors and do needless per-frame work indefinitely.Suggested fix
override _onPhysicsUpdate(): void { - if (!this._pendingNativeShapeCreation || !this._mesh || !this._positions) return; + if (!this._pendingNativeShapeCreation || !this._mesh || !this._positions) return; + if (!this._isConvex && this._collider instanceof DynamicCollider && !this._collider.isKinematic) { + // Unsupported until collider becomes kinematic; avoid per-frame error spam. + return; + } this._createNativeShape(); }Also applies to: 249-250
🤖 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/physics/shape/MeshColliderShape.ts` around lines 201 - 204, The MeshColliderShape code currently logs an error for non-convex meshes on non-kinematic DynamicCollider but later keeps retrying while pending is true; change the logic so when you detect (!this._isConvex && this._collider instanceof DynamicCollider && !this._collider.isKinematic) you both log the error once and mark the creation as terminal by clearing/setting the pending flag (e.g., pending = false or this._pending = false) and avoid assigning mesh or scheduling further retries; update the same handling at the location that does the mesh assignment (the code that references mesh and pending) so it short-circuits on this unsupported state and prevents per-frame work.
🧹 Nitpick comments (3)
packages/physics-physx/src/PhysXPhysics.ts (2)
336-346: 💤 Low valuePartial
tolerancesScaleoverrides blend user options with PhysX native defaults.When a user provides only
lengthor onlyspeedintolerancesScale, the unprovided parameter falls back to the native PhysXtolerancesScalevalue (lines 337-338). This allows partial overrides but could surprise users who expect independent control.For example, if a user sets
{ tolerancesScale: { length: 2 } }, thespeedwill come from PhysX's native default (typically 10), and the computed_defaultSleepThresholdwill use that native speed value.This behavior appears intentional and enables flexible configuration, but consider documenting it in the
PhysXTolerancesScaleinterface JSDoc to clarify that partial overrides are supported and will blend with PhysX defaults.🤖 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/physics-physx/src/PhysXPhysics.ts` around lines 336 - 346, The _applyTolerancesScale function currently blends user-provided tolerances with PhysX native defaults when only one of length or speed is supplied; add clear JSDoc to the PhysXTolerancesScale interface explaining that partial overrides are supported and that unspecified fields fall back to the PhysX runtime defaults (which will affect computed values such as _defaultSleepThreshold via _updateScaledDefaults), and include a short example (e.g., { length: 2 } uses native speed) and mention that _assertPositiveFinite still validates provided values; update the interface JSDoc text adjacent to PhysXTolerancesScale to reflect this behavior so callers are not surprised.
348-351: ⚡ Quick winValidation of
tolerancesScaleoptions is deferred untilinitialize().The validation at lines 349-351 only runs during
initialize(), not in the constructor. If a user passes invalid options (e.g., negative or non-finite values), they won't receive an error until the asyncinitialize()promise rejects, which may be confusing.Consider validating
tolerancesScaleOptionsin the constructor so errors are raised synchronously at construction time:✅ Suggested early validation
In the constructor (after line 87):
this._tolerancesScaleOptions = resolvedOptions?.tolerancesScale; + if (this._tolerancesScaleOptions) { + if (this._tolerancesScaleOptions.length !== undefined) { + this._assertPositiveFinite(this._tolerancesScaleOptions.length, "tolerancesScale.length"); + } + if (this._tolerancesScaleOptions.speed !== undefined) { + this._assertPositiveFinite(this._tolerancesScaleOptions.speed, "tolerancesScale.speed"); + } + } this._updateScaledDefaults(this._tolerancesScaleOptions?.length ?? 1, this._tolerancesScaleOptions?.speed ?? 10);🤖 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/physics-physx/src/PhysXPhysics.ts` around lines 348 - 351, The constructor currently defers validation of tolerancesScale options until initialize(), causing async errors; validate them synchronously by calling the existing _assertPositiveFinite for each tolerancesScale option inside the class constructor (after setting tolerancesScaleOptions) so invalid values throw immediately. Locate the constructor where tolerancesScaleOptions (or tolerancesScale) is assigned and invoke _assertPositiveFinite for each numeric field (e.g., length, mass, speed or whatever keys are used) or add a small helper that iterates keys and calls _assertPositiveFinite, leaving initialize() validation as a fallback.packages/physics-lite/src/LitePhysicsMaterial.ts (1)
63-67: ⚡ Quick winUse
console.warnand fix grammar in the warning message.The warning message has a grammatical error and uses
console.loginstead ofconsole.warn. For a feature limitation that developers should notice,console.warnis more appropriate and will be more visible in console filters.📝 Suggested fix
private static _warnOnce(): void { if (LitePhysicsMaterial._warned) return; LitePhysicsMaterial._warned = true; - console.log("Physics-lite don't support physics material. Use Physics-PhysX instead!"); + console.warn("Physics-lite doesn't support physics material. Use Physics-PhysX instead!"); }🤖 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/physics-lite/src/LitePhysicsMaterial.ts` around lines 63 - 67, Update LitePhysicsMaterial._warnOnce to use console.warn instead of console.log and fix the message grammar; locate the static method _warnOnce and the _warned flag, keep the early return and flag set, then replace the string "Physics-lite don't support physics material. Use Physics-PhysX instead!" with a grammatically correct warning such as "physics-lite does not support physics materials; use physics-physx instead." (or similar), ensuring casing/branding matches project conventions.
🤖 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/physics/Collider.ts`:
- Around line 155-158: When re-enabling a collider the native object is re-added
to physics before its pose is synchronized, creating a stale-pose window; inside
_onEnableInScene() update the implementation to sync the entity transform to the
native collider immediately (call the existing native-pose sync helper or set
the native pose from this.entity.transform) before calling
this.scene.physics._addCollider(this), and clear/adjust
this._pendingReenterTeleport accordingly so there is no deferred pose update on
next tick. Ensure the same change is applied to both collider re-enable code
paths so the native object always starts with the current entity transform.
In `@packages/core/src/physics/DynamicCollider.ts`:
- Around line 404-414: The torque is being computed from this.entity.transform
but the native collider may have a different pose; update applyForceAtPosition
logic in DynamicCollider to fetch the native actor's current world pose (use the
native collider/actor API such as its getGlobalPose/getWorldTransform method)
and transform localCoM with that pose (replace transform.worldRotationQuaternion
and transform.worldPosition usage) before computing torque into
DynamicCollider._tempVector3_2, then call nativeCollider.addForce and
nativeCollider.addTorque using those values so the torque matches the native
actor's actual pose.
In `@packages/core/src/physics/shape/ColliderShape.ts`:
- Around line 57-59: The contactOffset getter on ColliderShape currently reads
Engine._nativePhysics.getDefaultContactOffset every time, coupling shape
behavior to a mutable global; fix by capturing the resolved default once when
the shape (or its native counterpart) is created and store it on the instance
(e.g., add a private _capturedDefaultContactOffset set during the constructor or
wherever the native shape is initialized), then change contactOffset to return
this._contactOffset ?? this._capturedDefaultContactOffset ?? 0.02; ensure the
capture uses Engine._nativePhysics?.getDefaultContactOffset?.() at creation time
so later changes to Engine._nativePhysics do not affect existing ColliderShape
instances.
In `@packages/design/src/physics/IPhysicsScene.ts`:
- Around line 46-50: The interface change made setContactEventEnabled required,
which breaks older IPhysicsScene implementations; make the hook optional in the
contract (declare setContactEventEnabled as an optional method on IPhysicsScene)
and update the call site in PhysicsScene (where
nativePhysicsManager.setContactEventEnabled is invoked) to use optional chaining
when calling it (i.e., call
setContactEventEnabled?.(this._hasCollisionEventConsumers())). This keeps the
API backward-compatible and avoids runtime undefined method errors.
In `@tests/src/core/physics/DynamicCollider.test.ts`:
- Around line 492-529: The test overrides scene.physics.fixedTimeStep and only
restores it after assertions, so a failed assertion leaks the modified timestep;
capture the originalFTS, run the probe/expect block inside a try, and restore
scene.physics.fixedTimeStep = originalFTS in a finally block. Locate the
originalFTS variable, the probe(...) calls that compute dv_1_60 and dv_1_480,
and the subsequent expects, then move the assignment that resets
scene.physics.fixedTimeStep into finally so the timestep is always restored even
if assertions fail.
In `@tests/src/core/physics/MeshColliderShape.test.ts`:
- Around line 207-212: The test assumes clone cooking completes synchronously by
asserting clonedGround.getComponent(StaticCollider).shapes[0]
(MeshColliderShape) has a non-null _nativeShape and a defined
_nativeShape._pxShape immediately; change this to a retry/wait-style assertion:
poll or await until clonedShape._nativeShape is non-null and its _pxShape is
defined (for example, use an existing test helper waitFor/poll utility or build
a short retry loop) instead of immediate expect() calls so the test tolerates
next-tick retries in the clone cooking flow.
---
Outside diff comments:
In `@packages/core/src/physics/shape/MeshColliderShape.ts`:
- Around line 201-204: The MeshColliderShape code currently logs an error for
non-convex meshes on non-kinematic DynamicCollider but later keeps retrying
while pending is true; change the logic so when you detect (!this._isConvex &&
this._collider instanceof DynamicCollider && !this._collider.isKinematic) you
both log the error once and mark the creation as terminal by clearing/setting
the pending flag (e.g., pending = false or this._pending = false) and avoid
assigning mesh or scheduling further retries; update the same handling at the
location that does the mesh assignment (the code that references mesh and
pending) so it short-circuits on this unsupported state and prevents per-frame
work.
---
Nitpick comments:
In `@packages/physics-lite/src/LitePhysicsMaterial.ts`:
- Around line 63-67: Update LitePhysicsMaterial._warnOnce to use console.warn
instead of console.log and fix the message grammar; locate the static method
_warnOnce and the _warned flag, keep the early return and flag set, then replace
the string "Physics-lite don't support physics material. Use Physics-PhysX
instead!" with a grammatically correct warning such as "physics-lite does not
support physics materials; use physics-physx instead." (or similar), ensuring
casing/branding matches project conventions.
In `@packages/physics-physx/src/PhysXPhysics.ts`:
- Around line 336-346: The _applyTolerancesScale function currently blends
user-provided tolerances with PhysX native defaults when only one of length or
speed is supplied; add clear JSDoc to the PhysXTolerancesScale interface
explaining that partial overrides are supported and that unspecified fields fall
back to the PhysX runtime defaults (which will affect computed values such as
_defaultSleepThreshold via _updateScaledDefaults), and include a short example
(e.g., { length: 2 } uses native speed) and mention that _assertPositiveFinite
still validates provided values; update the interface JSDoc text adjacent to
PhysXTolerancesScale to reflect this behavior so callers are not surprised.
- Around line 348-351: The constructor currently defers validation of
tolerancesScale options until initialize(), causing async errors; validate them
synchronously by calling the existing _assertPositiveFinite for each
tolerancesScale option inside the class constructor (after setting
tolerancesScaleOptions) so invalid values throw immediately. Locate the
constructor where tolerancesScaleOptions (or tolerancesScale) is assigned and
invoke _assertPositiveFinite for each numeric field (e.g., length, mass, speed
or whatever keys are used) or add a small helper that iterates keys and calls
_assertPositiveFinite, leaving initialize() validation as a fallback.
🪄 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: caa25bf2-3321-4a47-bfe4-7a8b0e79956d
📒 Files selected for processing (28)
packages/core/src/physics/CharacterController.tspackages/core/src/physics/Collider.tspackages/core/src/physics/Collision.tspackages/core/src/physics/DynamicCollider.tspackages/core/src/physics/PhysicsMaterial.tspackages/core/src/physics/PhysicsScene.tspackages/core/src/physics/index.tspackages/core/src/physics/shape/ColliderShape.tspackages/core/src/physics/shape/MeshColliderShape.tspackages/design/src/physics/IPhysics.tspackages/design/src/physics/IPhysicsScene.tspackages/physics-lite/src/LitePhysicsMaterial.tspackages/physics-lite/src/LitePhysicsScene.tspackages/physics-physx/src/PhysXDynamicCollider.tspackages/physics-physx/src/PhysXPhysics.tspackages/physics-physx/src/PhysXPhysicsScene.tspackages/physics-physx/src/shape/PhysXColliderShape.tstests/src/core/physics/CharacterController.test.tstests/src/core/physics/Collider.test.tstests/src/core/physics/ColliderShape.test.tstests/src/core/physics/Collision.test.tstests/src/core/physics/DynamicCollider.test.tstests/src/core/physics/HingeJoint.test.tstests/src/core/physics/Joint.test.tstests/src/core/physics/MeshColliderShape.test.tstests/src/core/physics/PhysicsMaterial.test.tstests/src/core/physics/PhysicsScene.test.tstests/src/core/physics/SpringJoint.test.ts
Comment on lines
155
to
158
| override _onEnableInScene(): void { | ||
| this.scene.physics._addCollider(this); | ||
| this._pendingReenterTeleport = true; | ||
| } |
There was a problem hiding this comment.
Enable-time native pose sync is still deferred in both collider paths.
Both re-enable flows add the native object back to physics before synchronizing it to the entity transform. That leaves a stale-pose window where immediate manual physics steps or scene queries can observe the pre-disable transform. The fix should happen inside _onEnableInScene() for both types rather than waiting for a later update tick.
🤖 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/physics/Collider.ts` around lines 155 - 158, When
re-enabling a collider the native object is re-added to physics before its pose
is synchronized, creating a stale-pose window; inside _onEnableInScene() update
the implementation to sync the entity transform to the native collider
immediately (call the existing native-pose sync helper or set the native pose
from this.entity.transform) before calling
this.scene.physics._addCollider(this), and clear/adjust
this._pendingReenterTeleport accordingly so there is no deferred pose update on
next tick. Ensure the same change is applied to both collider re-enable code
paths so the native object always starts with the current entity transform.
Comment on lines
+404
to
+414
| const transform = this.entity.transform; | ||
| const worldCoM = DynamicCollider._tempVector3_1; | ||
| Vector3.transformByQuat(localCoM, transform.worldRotationQuaternion, worldCoM); | ||
| worldCoM.add(transform.worldPosition); | ||
|
|
||
| const torque = DynamicCollider._tempVector3_2; | ||
| Vector3.subtract(position, worldCoM, torque); | ||
| Vector3.cross(torque, force, torque); | ||
|
|
||
| nativeCollider.addForce(force); | ||
| nativeCollider.addTorque(torque); |
There was a problem hiding this comment.
Compute applyForceAtPosition() from the native actor pose.
Line 404-Line 414 transforms the center of mass with this.entity.transform, but the rest of the method talks directly to the native body. Because entity→physics sync is deferred until the tick, a same-frame transform.setPosition()/rotate() followed by applyForceAtPosition() will derive torque from a pose the native actor does not yet have.
Suggested fix
- const transform = this.entity.transform;
- const worldCoM = DynamicCollider._tempVector3_1;
- Vector3.transformByQuat(localCoM, transform.worldRotationQuaternion, worldCoM);
- worldCoM.add(transform.worldPosition);
-
- const torque = DynamicCollider._tempVector3_2;
+ const actorPosition = DynamicCollider._tempVector3_1;
+ const actorRotation = DynamicCollider._tempQuat;
+ nativeCollider.getWorldTransform(actorPosition, actorRotation);
+
+ const worldCoM = DynamicCollider._tempVector3_2;
+ Vector3.transformByQuat(localCoM, actorRotation, worldCoM);
+ worldCoM.add(actorPosition);
+
+ const torque = actorPosition;
Vector3.subtract(position, worldCoM, torque);
Vector3.cross(torque, force, torque);📝 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.
Suggested change
| const transform = this.entity.transform; | |
| const worldCoM = DynamicCollider._tempVector3_1; | |
| Vector3.transformByQuat(localCoM, transform.worldRotationQuaternion, worldCoM); | |
| worldCoM.add(transform.worldPosition); | |
| const torque = DynamicCollider._tempVector3_2; | |
| Vector3.subtract(position, worldCoM, torque); | |
| Vector3.cross(torque, force, torque); | |
| nativeCollider.addForce(force); | |
| nativeCollider.addTorque(torque); | |
| const actorPosition = DynamicCollider._tempVector3_1; | |
| const actorRotation = DynamicCollider._tempQuat; | |
| nativeCollider.getWorldTransform(actorPosition, actorRotation); | |
| const worldCoM = DynamicCollider._tempVector3_2; | |
| Vector3.transformByQuat(localCoM, actorRotation, worldCoM); | |
| worldCoM.add(actorPosition); | |
| const torque = actorPosition; | |
| Vector3.subtract(position, worldCoM, torque); | |
| Vector3.cross(torque, force, torque); | |
| nativeCollider.addForce(force); | |
| nativeCollider.addTorque(torque); |
🤖 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/physics/DynamicCollider.ts` around lines 404 - 414, The
torque is being computed from this.entity.transform but the native collider may
have a different pose; update applyForceAtPosition logic in DynamicCollider to
fetch the native actor's current world pose (use the native collider/actor API
such as its getGlobalPose/getWorldTransform method) and transform localCoM with
that pose (replace transform.worldRotationQuaternion and transform.worldPosition
usage) before computing torque into DynamicCollider._tempVector3_2, then call
nativeCollider.addForce and nativeCollider.addTorque using those values so the
torque matches the native actor's actual pose.
Comment on lines
57
to
59
| get contactOffset(): number { | ||
| return this._contactOffset; | ||
| return this._contactOffset ?? Engine._nativePhysics?.getDefaultContactOffset?.() ?? 0.02; | ||
| } |
There was a problem hiding this comment.
contactOffset is now coupled to the global backend, not the shape.
For shapes with no explicit override, this getter now reads Engine._nativePhysics every time instead of returning the default that was actually captured by that shape/native object. If another engine or backend is created later, the same shape can start reporting a different contactOffset even though its native value never changed.
Please snapshot the resolved default per shape/native-shape creation path instead of resolving it through the mutable global on every read.
🤖 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/physics/shape/ColliderShape.ts` around lines 57 - 59, The
contactOffset getter on ColliderShape currently reads
Engine._nativePhysics.getDefaultContactOffset every time, coupling shape
behavior to a mutable global; fix by capturing the resolved default once when
the shape (or its native counterpart) is created and store it on the instance
(e.g., add a private _capturedDefaultContactOffset set during the constructor or
wherever the native shape is initialized), then change contactOffset to return
this._contactOffset ?? this._capturedDefaultContactOffset ?? 0.02; ensure the
capture uses Engine._nativePhysics?.getDefaultContactOffset?.() at creation time
so later changes to Engine._nativePhysics do not affect existing ColliderShape
instances.
Comment on lines
+46
to
+50
| /** | ||
| * Enable contact event buffering. | ||
| * @param enabled - Whether collision contact events should be buffered for dispatch. | ||
| */ | ||
| setContactEventEnabled(enabled: boolean): void; |
There was a problem hiding this comment.
Keep the new contact-demand hook backward-compatible.
Line 50 makes setContactEventEnabled a required public-contract member, but packages/core/src/physics/PhysicsScene.ts Line 649 only uses it as an optimization. That forces every out-of-tree IPhysicsScene implementation to update immediately and can still turn mixed-version JS integrations into undefined is not a function crashes during _update().
Compatible contract/call-site adjustment
- setContactEventEnabled(enabled: boolean): void;
+ setContactEventEnabled?(enabled: boolean): void;// packages/core/src/physics/PhysicsScene.ts
nativePhysicsManager.setContactEventEnabled?.(this._hasCollisionEventConsumers());📝 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.
Suggested change
| /** | |
| * Enable contact event buffering. | |
| * @param enabled - Whether collision contact events should be buffered for dispatch. | |
| */ | |
| setContactEventEnabled(enabled: boolean): void; | |
| /** | |
| * Enable contact event buffering. | |
| * `@param` enabled - Whether collision contact events should be buffered for dispatch. | |
| */ | |
| setContactEventEnabled?(enabled: boolean): void; |
🤖 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/design/src/physics/IPhysicsScene.ts` around lines 46 - 50, The
interface change made setContactEventEnabled required, which breaks older
IPhysicsScene implementations; make the hook optional in the contract (declare
setContactEventEnabled as an optional method on IPhysicsScene) and update the
call site in PhysicsScene (where nativePhysicsManager.setContactEventEnabled is
invoked) to use optional chaining when calling it (i.e., call
setContactEventEnabled?.(this._hasCollisionEventConsumers())). This keeps the
API backward-compatible and avoids runtime undefined method errors.
Comment on lines
+492
to
+529
| const scene = engine.sceneManager.activeScene; | ||
| const originalFTS = scene.physics.fixedTimeStep; | ||
|
|
||
| const probe = (fts: number) => { | ||
| rootEntity.clearChildren(); | ||
| scene.physics.fixedTimeStep = fts; | ||
| const box = addBox(new Vector3(2, 2, 2), DynamicCollider, new Vector3(0, 0, 0)); | ||
| const c = box.getComponent(DynamicCollider); | ||
| c.mass = 1; | ||
| c.useGravity = false; | ||
| c.linearDamping = 0; | ||
| c.angularDamping = 0; | ||
| c.applyForce(new Vector3(100, 0, 0)); | ||
| // Advance exactly one *frame* of wall time. Galacean's _update loops simulate | ||
| // until frame_dt accumulates: 1/60 → 1 substep; 1/480 → 8 substeps. | ||
| // @ts-ignore | ||
| scene.physics._update(1 / 60); | ||
| return c.linearVelocity.x; | ||
| }; | ||
|
|
||
| const dv_1_60 = probe(1 / 60); | ||
| const dv_1_480 = probe(1 / 480); | ||
|
|
||
| console.info( | ||
| `[fixedTimeStep probe] applyForce(F=100) over 1 frame (1/60s):\n` + | ||
| ` 1/60 step → dv = ${dv_1_60.toFixed(4)} m/s\n` + | ||
| ` 1/480 step → dv = ${dv_1_480.toFixed(4)} m/s\n` + | ||
| ` ratio (1/60 / 1/480) = ${(dv_1_60 / dv_1_480).toFixed(3)} (theory: 8)` | ||
| ); | ||
|
|
||
| scene.physics.fixedTimeStep = originalFTS; | ||
|
|
||
| // Theory: dv_1_60 = F·(1/60)/m = 100/60 ≈ 1.667 | ||
| expect(dv_1_60).toBeCloseTo(100 / 60, 2); | ||
| // Theory: dv_1_480 = F·(1/480)/m = 100/480 ≈ 0.208 | ||
| expect(dv_1_480).toBeCloseTo(100 / 480, 2); | ||
| // Ratio must be 8 (PhysX clears force per simulate) | ||
| expect(dv_1_60 / dv_1_480).toBeCloseTo(8, 1); |
There was a problem hiding this comment.
Restore fixedTimeStep in a finally block.
If any assertion in this probe fails, Line 522 never runs and later tests inherit the overridden timestep from this case.
Suggested fix
- const dv_1_60 = probe(1 / 60);
- const dv_1_480 = probe(1 / 480);
-
- console.info(
- `[fixedTimeStep probe] applyForce(F=100) over 1 frame (1/60s):\n` +
- ` 1/60 step → dv = ${dv_1_60.toFixed(4)} m/s\n` +
- ` 1/480 step → dv = ${dv_1_480.toFixed(4)} m/s\n` +
- ` ratio (1/60 / 1/480) = ${(dv_1_60 / dv_1_480).toFixed(3)} (theory: 8)`
- );
-
- scene.physics.fixedTimeStep = originalFTS;
-
- // Theory: dv_1_60 = F·(1/60)/m = 100/60 ≈ 1.667
- expect(dv_1_60).toBeCloseTo(100 / 60, 2);
- // Theory: dv_1_480 = F·(1/480)/m = 100/480 ≈ 0.208
- expect(dv_1_480).toBeCloseTo(100 / 480, 2);
- // Ratio must be 8 (PhysX clears force per simulate)
- expect(dv_1_60 / dv_1_480).toBeCloseTo(8, 1);
+ try {
+ const dv_1_60 = probe(1 / 60);
+ const dv_1_480 = probe(1 / 480);
+
+ console.info(
+ `[fixedTimeStep probe] applyForce(F=100) over 1 frame (1/60s):\n` +
+ ` 1/60 step → dv = ${dv_1_60.toFixed(4)} m/s\n` +
+ ` 1/480 step → dv = ${dv_1_480.toFixed(4)} m/s\n` +
+ ` ratio (1/60 / 1/480) = ${(dv_1_60 / dv_1_480).toFixed(3)} (theory: 8)`
+ );
+
+ // Theory: dv_1_60 = F·(1/60)/m = 100/60 ≈ 1.667
+ expect(dv_1_60).toBeCloseTo(100 / 60, 2);
+ // Theory: dv_1_480 = F·(1/480)/m = 100/480 ≈ 0.208
+ expect(dv_1_480).toBeCloseTo(100 / 480, 2);
+ // Ratio must be 8 (PhysX clears force per simulate)
+ expect(dv_1_60 / dv_1_480).toBeCloseTo(8, 1);
+ } finally {
+ scene.physics.fixedTimeStep = originalFTS;
+ }📝 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.
Suggested change
| const scene = engine.sceneManager.activeScene; | |
| const originalFTS = scene.physics.fixedTimeStep; | |
| const probe = (fts: number) => { | |
| rootEntity.clearChildren(); | |
| scene.physics.fixedTimeStep = fts; | |
| const box = addBox(new Vector3(2, 2, 2), DynamicCollider, new Vector3(0, 0, 0)); | |
| const c = box.getComponent(DynamicCollider); | |
| c.mass = 1; | |
| c.useGravity = false; | |
| c.linearDamping = 0; | |
| c.angularDamping = 0; | |
| c.applyForce(new Vector3(100, 0, 0)); | |
| // Advance exactly one *frame* of wall time. Galacean's _update loops simulate | |
| // until frame_dt accumulates: 1/60 → 1 substep; 1/480 → 8 substeps. | |
| // @ts-ignore | |
| scene.physics._update(1 / 60); | |
| return c.linearVelocity.x; | |
| }; | |
| const dv_1_60 = probe(1 / 60); | |
| const dv_1_480 = probe(1 / 480); | |
| console.info( | |
| `[fixedTimeStep probe] applyForce(F=100) over 1 frame (1/60s):\n` + | |
| ` 1/60 step → dv = ${dv_1_60.toFixed(4)} m/s\n` + | |
| ` 1/480 step → dv = ${dv_1_480.toFixed(4)} m/s\n` + | |
| ` ratio (1/60 / 1/480) = ${(dv_1_60 / dv_1_480).toFixed(3)} (theory: 8)` | |
| ); | |
| scene.physics.fixedTimeStep = originalFTS; | |
| // Theory: dv_1_60 = F·(1/60)/m = 100/60 ≈ 1.667 | |
| expect(dv_1_60).toBeCloseTo(100 / 60, 2); | |
| // Theory: dv_1_480 = F·(1/480)/m = 100/480 ≈ 0.208 | |
| expect(dv_1_480).toBeCloseTo(100 / 480, 2); | |
| // Ratio must be 8 (PhysX clears force per simulate) | |
| expect(dv_1_60 / dv_1_480).toBeCloseTo(8, 1); | |
| const scene = engine.sceneManager.activeScene; | |
| const originalFTS = scene.physics.fixedTimeStep; | |
| const probe = (fts: number) => { | |
| rootEntity.clearChildren(); | |
| scene.physics.fixedTimeStep = fts; | |
| const box = addBox(new Vector3(2, 2, 2), DynamicCollider, new Vector3(0, 0, 0)); | |
| const c = box.getComponent(DynamicCollider); | |
| c.mass = 1; | |
| c.useGravity = false; | |
| c.linearDamping = 0; | |
| c.angularDamping = 0; | |
| c.applyForce(new Vector3(100, 0, 0)); | |
| // Advance exactly one *frame* of wall time. Galacean's _update loops simulate | |
| // until frame_dt accumulates: 1/60 → 1 substep; 1/480 → 8 substeps. | |
| // `@ts-ignore` | |
| scene.physics._update(1 / 60); | |
| return c.linearVelocity.x; | |
| }; | |
| try { | |
| const dv_1_60 = probe(1 / 60); | |
| const dv_1_480 = probe(1 / 480); | |
| console.info( | |
| `[fixedTimeStep probe] applyForce(F=100) over 1 frame (1/60s):\n` + | |
| ` 1/60 step → dv = ${dv_1_60.toFixed(4)} m/s\n` + | |
| ` 1/480 step → dv = ${dv_1_480.toFixed(4)} m/s\n` + | |
| ` ratio (1/60 / 1/480) = ${(dv_1_60 / dv_1_480).toFixed(3)} (theory: 8)` | |
| ); | |
| // Theory: dv_1_60 = F·(1/60)/m = 100/60 ≈ 1.667 | |
| expect(dv_1_60).toBeCloseTo(100 / 60, 2); | |
| // Theory: dv_1_480 = F·(1/480)/m = 100/480 ≈ 0.208 | |
| expect(dv_1_480).toBeCloseTo(100 / 480, 2); | |
| // Ratio must be 8 (PhysX clears force per simulate) | |
| expect(dv_1_60 / dv_1_480).toBeCloseTo(8, 1); | |
| } finally { | |
| scene.physics.fixedTimeStep = originalFTS; | |
| } |
🤖 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/physics/DynamicCollider.test.ts` around lines 492 - 529, The
test overrides scene.physics.fixedTimeStep and only restores it after
assertions, so a failed assertion leaks the modified timestep; capture the
originalFTS, run the probe/expect block inside a try, and restore
scene.physics.fixedTimeStep = originalFTS in a finally block. Locate the
originalFTS variable, the probe(...) calls that compute dv_1_60 and dv_1_480,
and the subsequent expects, then move the assignment that resets
scene.physics.fixedTimeStep into finally so the timestep is always restored even
if assertions fail.
Comment on lines
+207
to
+212
| const clonedShape = clonedGround.getComponent(StaticCollider).shapes[0] as MeshColliderShape; | ||
| // @ts-ignore — inspect that the cloned shape actually has a usable native PhysX handle | ||
| expect(clonedShape._nativeShape).not.toBeNull(); | ||
| // @ts-ignore | ||
| expect(clonedShape._nativeShape._pxShape).toBeDefined(); | ||
|
|
There was a problem hiding this comment.
Avoid immediate native-handle assertions in a retry-based flow.
Line [209]-Line [212] assumes clone cooking completes synchronously, but the runtime now explicitly allows transient failures with next-tick retries. This can make the test flaky even when behavior is correct.
Suggested stabilization
- // `@ts-ignore` — inspect that the cloned shape actually has a usable native PhysX handle
- expect(clonedShape._nativeShape).not.toBeNull();
- // `@ts-ignore`
- expect(clonedShape._nativeShape._pxShape).toBeDefined();
+ // Allow retry path to complete before asserting internal native handle.
+ for (let i = 0; i < 10; i++) {
+ // `@ts-ignore`
+ if (clonedShape._nativeShape?._pxShape) break;
+ physicsScene._update(1 / 60);
+ }
+ // `@ts-ignore`
+ expect(clonedShape._nativeShape?._pxShape).toBeDefined();📝 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.
Suggested change
| const clonedShape = clonedGround.getComponent(StaticCollider).shapes[0] as MeshColliderShape; | |
| // @ts-ignore — inspect that the cloned shape actually has a usable native PhysX handle | |
| expect(clonedShape._nativeShape).not.toBeNull(); | |
| // @ts-ignore | |
| expect(clonedShape._nativeShape._pxShape).toBeDefined(); | |
| const clonedShape = clonedGround.getComponent(StaticCollider).shapes[0] as MeshColliderShape; | |
| // Allow retry path to complete before asserting internal native handle. | |
| for (let i = 0; i < 10; i++) { | |
| // `@ts-ignore` | |
| if (clonedShape._nativeShape?._pxShape) break; | |
| physicsScene._update(1 / 60); | |
| } | |
| // `@ts-ignore` | |
| expect(clonedShape._nativeShape?._pxShape).toBeDefined(); |
🤖 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/physics/MeshColliderShape.test.ts` around lines 207 - 212, The
test assumes clone cooking completes synchronously by asserting
clonedGround.getComponent(StaticCollider).shapes[0] (MeshColliderShape) has a
non-null _nativeShape and a defined _nativeShape._pxShape immediately; change
this to a retry/wait-style assertion: poll or await until
clonedShape._nativeShape is non-null and its _pxShape is defined (for example,
use an existing test helper waitFor/poll utility or build a short retry loop)
instead of immediate expect() calls so the test tolerates next-tick retries in
the clone cooking flow.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
requested changes
Jun 14, 2026
GuoLei1990
left a comment
GuoLei1990
left a comment
Member
There was a problem hiding this comment.
总结
把 #3014 / fix/shaderlab 的物理部分拆出来重提：collider 重入 teleport、kinematic transform 同步模式（Target/Teleport）、applyForceAtPosition、per-instance 物理材质 clone、contact event 按需开关、MeshColliderShape 异步 cook 重试、tolerancesScale 配置、kinematic 状态下 CCD flag 缓存。整体方向合理（contact event 按需 toggle、material per-instance clone、shape 延迟 cook 重试、kinematic CCD 缓存都对）。但有一个 per-frame 错误刷屏的真实 bug、一个热路径开销、design 接口破坏性变更，以及 PR 元信息/注释规范问题。
代码自上次审查（commit 7a07cc8）未变，以下保留仍未修复的问题并补充本轮新发现。
问题
[P1] MeshColliderShape.ts:185-188 — 永久不支持的 non-convex 动态网格会每帧 console.error 刷屏
_createNativeShape 在 "non-convex + 非 kinematic DynamicCollider" 这个永久不支持配置上 console.error 后直接 return，但没清掉 _pendingNativeShapeCreation。链路：
set mesh（mesh 可访问）→_extractMeshData成功 →_createNativeShape()命中 185 行早退、_nativeShape仍为 nullset mesh里_pendingNativeShapeCreation = !this._nativeShape=true- 此后每个物理 tick
_onPhysicsUpdate看到 pending=true + mesh/positions 都在 → 再调_createNativeShape→ 再console.error→ 永久每帧刷屏 + 每帧白跑
把 "transient cook 失败要重试" 和 "永久不支持要放弃" 两种语义压在同一个 pending flag 上了。修复落点在不支持分支，明确标记为永久失败：
if (!this._isConvex && this._collider instanceof DynamicCollider && !this._collider.isKinematic) {
Logger.error("MeshColliderShape: Non-convex mesh is not supported on non-kinematic DynamicCollider.");
this._pendingNativeShapeCreation = false; // permanent, not transient — don't retry
return;
}（顺带：项目有统一的 Logger，本文件这里及 _extractMeshData 几处都用原生 console.error/warn，建议统一走 Logger。）
[P1] PhysicsScene.ts:649 + 829-849 — _hasCollisionEventConsumers() 每个 substep 全量重扫，热路径开销
setContactEventEnabled(this._hasCollisionEventConsumers()) 放在 substep 循环内（for i < step），而 _hasCollisionEventConsumers 是 O(colliders × scriptsPerCollider) 的全量扫描，逐个对比 3 个 Script.prototype 方法引用。fixedTimeStep 越小、substep 越多（如 1/480 → 8 substep/帧），同一份消费者集合每帧被重扫 8 次。这是本 PR 新引入的每帧成本（改前 contact event 恒开、无扫描）。
两层改进：
- 最小修：消费者集合在单次
_update内不会变（substep 之间不会有 script enable/disable），把调用移到 substep 循环外，每帧只算一次。 - 本质解（失效信号挂 source-of-truth）：在 Script 的 collision callback enable/disable 时维护一个 scene 级消费者计数，
_hasCollisionEventConsumers退化为count > 0的 O(1) 判断，彻底消除每帧扫描。当前是消费者每帧自己 hash 整个集合的反模式。
[P1] IPhysicsScene.ts:50 区域 — setContactEventEnabled 设为必选，破坏 design 接口契约
新增 setContactEventEnabled(enabled: boolean): void; 无 ?，是必选方法。packages/design 是对外的物理后端契约，任何第三方自定义后端（实现 IPhysicsScene）不实现就编译失败。仓库内 physx/lite 都已更新，但接口本身的破坏性变更应兜底。建议设为可选 + 调用点可选链：
// IPhysicsScene
setContactEventEnabled?(enabled: boolean): void;
// PhysicsScene.ts:649
nativePhysicsManager.setContactEventEnabled?.(this._hasCollisionEventConsumers());[P1] PR 标题 / commit 前缀与内容不符
标题 fix(physics): split shaderlab physics fixes 里的 "shaderlab" 与实际内容（纯物理，不动任何 shaderlab 文件）无关。更重要的是本 PR 不只是 fix：kinematicTransformSyncMode 枚举、applyForceAtPosition 公开方法、per-instance material clone、PhysXPhysicsOptions/tolerancesScale 配置都是新功能 / 新 API，挂 fix: 前缀会影响 changelog 生成和回滚判断。建议改 feat(physics): 并去掉 "shaderlab" 字样。
[P2] PhysXDynamicCollider.ts:29-37 / 176-204 / 255-280 — 大段中文 JSDoc，偏离引擎英文注释惯例
新增了 16+ 行中文 JSDoc 解释（CCD 缓存、addForce/addTorque kinematic 提前 return、move normalize 等）。引擎是国际开源代码库，源码注释惯例为英文（base 分支各文件仅有零星 unicode 字符如 keyname 表，无成段中文 prose）。内容本身解释得很到位，建议译为英文以便国际贡献者维护。PhysXPhysicsScene.ts 同样有中文注释，一并处理。
关于 CodeRabbit 几条判断（给结论，部分不成立，不重复提出）
- CodeRabbit：
applyForceAtPosition（DynamicCollider.ts）应从 native actor pose 取 CoM 而非 entity transform。不成立：该方法用transform.worldPosition/worldRotationQuaternion（Transform getter 契约保证 dirty 时自动 flush 为最新），native actor 在_onUpdate同步到同一 entity pose 后才进物理 step，torque 与 entity pose 自洽。entity transform 作为 source-of-truth 是引擎一贯设计。 - CodeRabbit："Collider 重入有 stale-pose 窗口"。不成立：
_onEnableInScene标_pendingReenterTeleport，下一个_onUpdate在物理 step 前先 teleport 到 entity pose，step 看到的是正确 pose，窗口不暴露给模拟，注释已说明该时序。 - CodeRabbit 的 console.warn/grammar、tolerancesScale JSDoc、clone 断言改 retry 等均为 nitpick，可选采纳，不阻塞。
测试
- 回归覆盖扎实：R0（CCD kinematic 切换）、R6（clone 重建 native shape）、applyForceAtPosition 全分量 + clone 路径、contact-event 按需 enable/disable/trigger-only 三态、kine-kine/kine-dynamic 接触回调，多数是 red-verified 反向测试，方向对。
DynamicCollider.test.tsfixedTimeStep probe 若断言失败不还原 fixedTimeStep（CodeRabbit 也提了），建议 try/finally 包裹避免污染后续用例。非阻塞。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3025 +/- ##
========================================
Coverage 79.17% 79.17%
========================================
Files 914 914
Lines 101666 101763 +97
Branches 11209 11229 +20
========================================
+ Hits 80489 80573 +84
- Misses 20990 21003 +13
Partials 187 187
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
luzhuang
force-pushed
the
fix/physics-shaderlab-split
branch
from
e508107 to
d6b96b5
Compare
luzhuang
changed the title
This was referenced Jun 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
setContactEventEnabledimplementation.Collision.getContacts()normal orientation so contacts are reported relative to the receiver shape.Split Scope
This is PR 1/3 from the former broad physics split:
DynamicCollider.applyForceAtPositionis intentionally out of scope here; the long-term PhysX binding path is handled by #3039.Verification
pnpm -F @galacean/engine-core run b:typesgit diff --checkpnpm vitest run tests/src/core/physics/PhysicsScene.test.ts tests/src/core/physics/Collision.test.ts; this old worktree currently falls back to Node collection and fails onwindow is not definedfrom@galacean/enginepolyfill. Full GitHub Actions checks are the source of truth for browser/e2e on this split.