fix(physics): gate contact buffering and correct collision normals#3025
fix(physics): gate contact buffering and correct collision normals#3025luzhuang wants to merge 10 commits into
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:
WalkthroughThe PR adds demand-based PhysX contact-event buffering, tracks collision callback consumers, corrects contact orientation scaling, and adds regression tests for normals and event-demand transitions. ChangesPhysics contact events and collision reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Script
participant PhysicsScene
participant PhysXPhysicsScene
participant CollisionCallback
Script->>PhysicsScene: mark collision consumer state dirty
PhysicsScene->>PhysicsScene: scan collider scripts before fixed-step update
PhysicsScene->>PhysXPhysicsScene: setContactEventEnabled(true or false)
PhysXPhysicsScene->>PhysXPhysicsScene: buffer or discard native contact events
PhysXPhysicsScene->>CollisionCallback: dispatch buffered collision contacts
Possibly related issues
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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
| 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.
| 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.
| 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.
| 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.
| 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.
| 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.
| 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.
| 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
left a comment
GuoLei1990
left a comment
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.52% 79.63% +0.10%
===========================================
Files 904 904
Lines 101168 101278 +110
Branches 11377 11425 +48
===========================================
+ Hits 80456 80653 +197
+ Misses 20528 20441 -87
Partials 184 184
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
luzhuang
changed the title
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.
总结
第四轮审查（87e2e84a1c → tip 8b3576a96d，线性追加两个 docs(physics): commit，无 force-push，无 behavior change）。上轮我已 --approve 解除门控，本轮 delta 是 纯文档 6 添加 + 1 改行，无 P0/P1/P2。方向正确，收尾干净。
本轮 delta 验证
对照 87e2e84a1c...8b3576a96d（compare 报 status: ahead by 2，纯线性、无 force-push），只有两处改动，均为文档：
-
PhysicsScene.ts:32-37—_collisionEventConsumersDirty私有字段补多行 JSDoc +@remarks，把 PR 描述里的"callback 在 script active 期间视为稳定、runtime 方法替换不失效缓存"契约固化到字段旁。逐点核实非 aspirational：@remarks声称"collider 或 script 生命周期变更才 rescan"，与代码一致——_markCollisionEventConsumersDirty()仅在 collider add/remove（:672/:700）、character-controller add/remove（:686/:713）及 Script 生命周期路径调用，_syncContactEventDemand首行if (!this._collisionEventConsumersDirty) return（:848）早退，从不每帧轮询、不在 runtime 方法赋值时置脏。两句@remarks均句号收尾，字段多行 JSDoc 合规。 -
ICollision.ts:11—getContacts()JSDoc 从Get contact points.扩为Get contact points. Contact normals and impulses must point from shape1 to shape0.，即上轮我已核对并肯定的shape1 → shape0契约的最终固化。与本文件既有惯例一致（interface 成员单行/** */、句号结尾），与Collision.ts:32的 sign-fix（factor = shape.id === shape1Id ? 1 : -1）语义闭环，无矛盾。Git 历史透镜：该行原文只有Get contact points.，本次是新增契约句，不推翻任何前序记录理由。
CI 全绿（build×3 / e2e×4 / codecov patch+project+coverage / lint / labeler）。改动行无 console.*、无注释规范问题。
结论
LGTM。 纯文档收尾，门控上轮已解除（reviewDecision=REVIEW_REQUIRED 是 repo 要求他人 owner 审，非我门控），无需再动 review 状态。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
第五轮审查（8b3576a96d → tip 5fbf0742cd，线性追加一个 docs(physics): commit，无 force-push，无 behavior change）。上轮我已 --approve 解除门控，本轮 delta 是纯文档 1 行新增，无 P0/P1/P2，仅 1 条 P3 风格。方向正确。
本轮 delta 验证
对照 8b3576a96d...5fbf0742cd（compare 报 status: ahead by 1 / behind by 0，纯线性、无 force-push），唯一改动是 PhysicsScene.ts:852 在 _syncContactEventDemand 的反向扫描循环前补一行 // 注释：
// Scan backward so newly appended colliders and scripts are checked first, increasing the chance of an early exit.
非 aspirational，逐点核实与代码一致：外层 for (let i = this._colliders.length - 1; i >= 0; --i) 反向遍历 collider、内层 for (let j = scripts.length - 1; j >= 0; --j) 反向遍历 script，命中 _hasCollisionEventCallbacks() 即 setContactEventEnabled?.(true); return 早退——三点全对。「newest-first 抬高早退概率」的理由自洽：置脏（_markCollisionEventConsumersDirty）的诱因往往是刚追加的 collision-event 消费者，位于数组尾部，先查尾部更可能在扫完全集前命中并早退。
问题
-
[P3] PhysicsScene.ts:852 — 单行
//注释末尾多了句号本文件所有既有单行
//注释均无尾句号（// Parse parameters based on new overload patterns、// Dispatch contact events、// entity.destroy() is deferred...等），新增这行以...early exit.收尾是唯一 outlier。建议去掉句号与全文件惯例对齐。锦上添花，不阻塞。
CI 全绿（build×3 / e2e×4 / codecov patch+project+coverage / lint / labeler）。
结论
LGTM。 纯文档收尾，仅一处 // 尾句号可选修正。门控上轮已解除（reviewDecision=REVIEW_REQUIRED 是 repo 要求他人 owner 审，非我门控），无需再动 review 状态。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
第六轮审查（5fbf0742cd → tip 6fe08652f4，线性追加一个 refactor(physics): commit，无 force-push，无 behavior change）。上轮我已 --approve 解除门控，本轮 delta 是纯术语重命名 + JSDoc 措辞 + 测试 import 等价替换，无 P0/P1/P2/新 P3。方向正确。
本轮 delta 验证
对照 5fbf0742cd...6fe08652f4（compare 报 status: ahead by 1 / behind by 0，纯线性、无 force-push），唯一 commit 是 refactor(physics): align contact demand terminology，跨 5 文件 +18/-16，分三组，逐组核实：
① 术语重命名（Script.ts + PhysicsScene.ts） — _markCollisionEventConsumersDirty → _markContactEventDemandDirty、_collisionEventConsumersDirty → _contactEventDemandDirty。纯改名，零行为变化：GitHub 代码搜索全仓两个旧名 total_count=0（无残留）；PhysicsScene.ts 5 个 mark 调用点 + 方法定义 + 字段 + _syncContactEventDemand 早退读取全部同步改名。方向正确——新名向早已存在的 _syncContactEventDemand（source: PR #2458 前即有）的 "contact event demand" 概念收敛，消除了 mark/field 用 "collisionEventConsumers" 而 sync 用 "contactEventDemand" 的双词根不一致。字段的 @remarks 多行 JSDoc（"callback 在 script active 期间视为稳定、runtime 方法替换不失效缓存"）随改名原样保留、仍准确。反向扫描循环体 + // Scan backward... 注释字节级不变。
② ICollision.ts JSDoc（两处）：
shape0Id/shape1Id：The unique id of the first/second **collider**→The unique ID of the first/second **shape** in the contact pair。语义修正：该 ID 经PxShape.userData传入、是 shape ID 非 collider ID（一个 collider 可含多个 shape），与Collision.ts:32比对nativeCollision.shape1Id（shape 而非 collider）一致。单行/** */+ 句号，符合本文件 interface 成员惯例。getContacts()：单行 → 多行/** */+@remarks，point from shape1 to shape0→point from the second shape in the pair to the first。契约语义保留非反转：Git 历史透镜——66011dd9cf(#2458) 原文只有Get contact points.，上轮efa90b7520新增must point from shape1 to shape0，本 commit 只是把它重排为@remarks多行 + 改用序数英文措辞。「second shape → first」经紧邻字段文档（shape0Id=first /shape1Id=second）无歧义映射回shape1 → shape0，与Collision.ts:32sign-fix 语义闭环，不推翻efa90b7520的契约。多行 JSDoc 句号收尾合规。
③ 测试 import 等价替换（Collision.test.ts + PhysicsScene.test.ts）— WebGLEngine 从 @galacean/engine-rhi-webgl 改为 @galacean/engine。等价：packages/galacean/src/index.ts:12 export * from "@galacean/engine-rhi-webgl" 透出同一符号，import 正常解析，与仓库测试走顶层 facade 的趋势对齐。Collision.test.ts 的方向断言（normal.x === -1）未动，回归网仍在。
CI 全绿（build×3 / e2e×4 / codecov patch+project+coverage / lint / labeler）。
结论
LGTM。 纯术语/文档收尾 + 测试 import 等价替换，无新问题。既有 [P3] PhysicsScene.ts // 扫描注释尾句号在未改动行上、上轮已提，不重复。门控上轮已解除（reviewDecision=REVIEW_REQUIRED 是 repo 要求他人 owner 审，非我门控），无需再动 review 状态。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
第七轮审查（6fe08652f4 → tip b9b56160c3，线性追加一个 style(physics): commit，无 force-push，无 behavior change）。上轮我唯一开放的那条 [P3] 本轮已精确闭环，无新 P0/P1/P2/P3。方向正确，收尾干净。
本轮 delta 验证
对照 6fe08652f4...b9b56160c3（compare 报 status: ahead by 1 / behind by 0，纯线性、无 force-push），唯一 commit 是 style(physics): align inline comment punctuation，单文件 +1/-1：
-
PhysicsScene.ts:852—_syncContactEventDemand反向扫描循环前那行//注释去掉尾句号：// Scan backward so newly appended colliders and scripts are checked first, increasing the chance of an early exit这正是我第五轮（
5fbf0742cd）提的唯一开放 [P3]——该行是全文件唯一以句号结尾的单行//注释，本轮去句号后与文件既有惯例（// Dispatch contact events、// entity.destroy() is deferred...等均无尾句号）对齐。零行为变化，注释文本仍准确：外层for (let i = this._colliders.length - 1; i >= 0; --i)反向遍历 collider、内层for (let j = scripts.length - 1; j >= 0; --j)反向遍历 script，命中_hasCollisionEventCallbacks()即setContactEventEnabled?.(true); return早退——三点全对，「newest-first 抬高早退概率」的理由仍自洽。
已关闭问题清单（对照前六轮）
- [P1]
setContactEventEnabled必选破坏接口契约 — ✅ 已修（第三轮闭环，现为可选 + 可选链）。 - [P1] PR 标题含无关 "shaderlab" + fix/feat 混装 — ✅ 已修（现为
fix(physics): gate contact buffering and correct collision normals）。 - [P1] MeshColliderShape 每帧
console.error刷屏 — 不在本 PR 范围（已随 MeshColliderShape 迁到 #3066）。 - [P3]
PhysicsScene.ts扫描注释尾句号 — ✅ 本轮精确闭环。
CI 全绿（build×3 / e2e×4 / codecov patch+project+coverage / lint / labeler）。
结论
LGTM。 纯 style 收尾，上轮唯一 P3 已闭环，无新问题。门控我在第三轮已解除（reviewDecision=REVIEW_REQUIRED 是 repo 要求他人 owner 审，非我门控），无需再动 review 状态。
GuoLei1990
requested review from
Copilot and
cptbtptpbcptdtptp
and removed request for
Copilot
2 participants
Problem
Unused contact buffering
PhysX contact points were copied across the Embind boundary whenever a contact was reported, even when no active collision callback could consume them.
Incorrect collision normal orientation
Collision.getContacts()oriented contact normals and impulses by comparing the numeric IDs of the two shapes:This ordering was intentional, but relied on a correlation rather than the PhysX contact-pair contract:
PxShape.userData.PxContactPair::shapes[0]andshapes[1]unchanged; it does not sort them by Galacean ID.shape1to pairshape0.RigidID— not the Galacean shape ID.For the existing dynamic-dynamic tests, PhysX's internal
RigidIDordering commonly placed the earlier-created actor inshape1. Because Galacean shape IDs also followed creation order,shape1Idhappened to equal the smaller Galacean shape ID, so the old heuristic passed even when creation order was reversed.Static-dynamic pairs break that correlation. PhysX places the static actor in
shape1regardless of the Galacean ID. When the dynamic shape is created first and the static shape second,shape1has the larger Galacean ID. The old heuristic then reverses an already-correct normal.The core layer dispatches
Collision.shapeas the other shape. Therefore the receiver-relative direction follows directly from the native pair:shape1: the nativeshape1 -> shape0direction already points from other to self; keep it.shape0: reverse it.The native
ICollisioncontract now explicitly requires contact normals and impulses to point fromshape1toshape0, so other physics backends can implement the same semantics without relying on PhysX-specific assumptions.Solution
Script.PhysicsSceneinvalidate contact-event demand when script, collider, or character-controller lifecycle state changes.setContactEventEnabledcapability.shape1 -> shape0pair contract instead of numeric shape ID ordering.Script callback lifecycle boundary
Contact-event demand follows the engine's existing lifecycle-based Script callback registration model:
onCollisionEnter,onCollisionExit, oronCollisionStay.Callback methods are treated as stable while a Script remains active in the scene. Assigning a collision callback directly to an active instance, or replacing its prototype method in place, does not itself invalidate contact-event demand. This matches the existing registration behavior of
onUpdate,onLateUpdate, andonPhysicsUpdate, which are also inspected when a Script enters the active scene rather than polled every frame.A source reload that recreates Script instances naturally follows the lifecycle path above. If live HMR needs to patch active instances without recreation, the HMR integration must trigger a unified Script callback-registration refresh covering update and collision callbacks. Adding per-frame scans or collision-only method setters would create a separate callback model and add permanent hot-path cost.
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.Regression Proof
With the old shape-ID-based orientation restored:
HEADLESS=true pnpm vitest run tests/src/core/physics/Collision.test.ts --reporter=verbose-11The failing case creates the dynamic shape first and the static shape second. PhysX places the static shape in pair
shape1, makingshape1Idthe larger Galacean ID and disproving the old ordering assumption.With this PR: 5 passed.
Verification
pnpm run b:modulepnpm run b:typesHEADLESS=true pnpm vitest run tests/src/core/physics/PhysicsScene.test.ts tests/src/core/physics/Collision.test.ts— 55 passedgit diff --checkSummary by CodeRabbit
setContactEventEnabled(enabled)to control native contact event buffering/dispatch behavior.