fix(physics): gate contact buffering and correct collision normals - #3025
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% 80.03% +0.50%
===========================================
Files 904 906 +2
Lines 101168 101401 +233
Branches 11377 11619 +242
===========================================
+ Hits 80456 81154 +698
+ Misses 20528 20064 -464
+ Partials 184 183 -1
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:
|
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
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
第八轮增量审查覆盖 b9b56160c3...206b72acd6ad103d3b83e25fc3668cc29acbc4e1：compare 为 ahead 1 / behind 0，唯一新增 commit 是 206b72acd6ad chore: clean up PR changes。本轮没有新的 P0/P1，发现 1 条非阻塞 P2；运行时实现与测试未变，CI 的 lint、三平台 build、4 组 e2e 和 codecov 均通过。
阻塞级别：无 P0/P1，不阻塞；有 1 条 P2 建议修正。 本次实际 review 动作为 COMMENTED，目标 HEAD 为 206b72acd6ad103d3b83e25fc3668cc29acbc4e1。AutoCR 不执行 APPROVE，也不替代人工 Reviewer 的合入门禁。
已关闭问题清单
- [P1] contact-event demand 每个 fixed substep 全量重扫 — ✅ 已由
4e2894bcce的 dirty/reconcile 方案闭环；当前稳定路径只有一次布尔早退，生命周期变化时才扫描。 - [P1]
IPhysicsScene.setContactEventEnabled必选破坏第三方后端契约 — ✅ 已修为可选 capability，core 调用使用可选链。 - [P1] PR 标题含无关 “shaderlab” 且范围混装 — ✅ 已收窄为当前标题与 8 个物理/测试文件；
MeshColliderShape、PhysXDynamicCollider等全量栈问题不在本 PR，相关 mesh 重试问题归 #3066。 - [P1/P2] stale-pose、
applyForceAtPosition、tolerancesScale、中文注释及 fixedTimeStep 清理等旧全量栈反馈 — ✅ 最终窄 diff 不包含对应实现；其中 Transform getter 与 re-enter 时序的既有解释已验证成立，不重复提出。 - [P3]
PhysicsScene.ts反向扫描单行注释尾句号 — ✅ 已由b9b56160c3修复。
问题
-
[P2]
packages/core/src/physics/PhysicsScene.ts:32— 保留 contact-demand cache 的代码侧失效契约本轮删除了
8b3576a96d docs(physics): clarify contact demand lifecycle专门补入的唯一局部说明，但实现仍依赖这条非显然约束：_contactEventDemandDirty只由 collider/controller 增删和 Script 进入/离开 active scene 置脏；直接替换 active Script 实例或原型上的 collision callback 不会触发 reconcile。若后续 HMR 或动态 callback 接入者不知道这个边界，native contact buffering 可能继续保持 disabled，直到无关生命周期变化后才恢复，期间 collision callback 会静默漏报。PR 描述目前写明了该限制，但合入后不应让 correctness invariant 只存在于 PR 会话中。建议在字段或
_syncContactEventDemand旁恢复一段精简说明，明确“active 生命周期内 callback 视为稳定；live patch 必须触发统一 Script callback-registration refresh”。不要为此增加每帧扫描、collision-only setter、镜像状态或 compatibility branch。
架构、熵增与测试治理
- 上游 owner：active collider/controller 集合由
PhysicsScene._colliders持有，active Script 集合由各 collider entity 的_scripts持有；Script scene 生命周期与 collider/controller add/remove 是 demand 失效信号的唯一写入口。 - 派生状态 owner：
PhysicsScene通过_contactEventDemandDirty在 fixed step 前机械重算“是否存在 collision callback consumer”，并经可选IPhysicsScene.setContactEventEnabled下发。该 dirty flag 只表示“需要 reconcile”，不与 native enabled 状态重复 ownership。 - 下游 owner：
PhysXPhysicsScene._contactEventEnabled唯一决定是否跨 Embind 复制 contact points；trigger event 路径独立，不被 collision demand 错误合并。normal/impulse 的权威协议仍是ICollision的shape1 -> shape0，coreCollision只做 receiver-relative 符号转换，旧的 min-shape-ID 第二套排序已删除。 - 熵增净值：本轮没有增加 runtime owner、状态、转换、校验、fallback 或抽象；
.gitignore的/notes/临时规则被撤回后，最终 PR 也不再包含该无关文件。唯一净退化是上面 P2 所述的失效边界从代码迁到了 PR 描述。 - 测试治理：本轮无行为变化，不需要新增测试；既有 lifecycle/substep/trigger-only 测试继续覆盖 demand reconcile，static-dynamic 反向证伪测试继续锁定 normal 方向。未发现为旧测试保留的生产 compatibility branch、legacy fallback、wrapper、镜像状态或第二条转换/校验路径，也没有失效 fixture/mock/snapshot 需要清理。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
🫧 尘小沫
结论
第九轮增量审查覆盖 206b72acd6ad103d3b83e25fc3668cc29acbc4e1...fc2bcdae6952afd694b71c779b4e4f7b1b7743a0：compare 为 ahead 1 / behind 0，唯一新增 commit 是 fc2bcdae6952 perf(physics): gate native contact callbacks。把 gate 从 TypeScript buffering 入口下沉到 native simulation callback 的方向正确，本地 WASM 反向测试也证明 contact callback 能在 Embind 边界前停止、trigger 路径不受影响；但默认公开运行时仍加载不含新 capability 的旧 CDN 资产，导致默认用户静默退回无 gate 的热路径。
阻塞级别：存在 1 条 P1，阻塞；另保留上一轮 1 条 P2。 本次实际 review 动作为 REQUEST_CHANGES，目标 HEAD 为 fc2bcdae6952afd694b71c779b4e4f7b1b7743a0。AutoCR 不执行 APPROVE，也不替代人工 Reviewer 的合入门禁。
已关闭问题清单
- [P1] contact-event demand 每个 fixed substep 全量重扫 — ✅ 已由
4e2894bcce的 dirty/reconcile 方案闭环；当前稳定路径只有一次布尔早退，生命周期变化时才扫描。 - [P1]
IPhysicsScene.setContactEventEnabled必选破坏第三方后端契约 — ✅ 已修为可选 capability，core 调用使用可选链；本轮问题是 PhysX backend 与其自有 native runtime 的版本契约，不是重提第三方接口兼容。 - [P1] PR 标题含无关 “shaderlab” 且范围混装 — ✅ 已收窄为当前物理/测试范围；
MeshColliderShape、PhysXDynamicCollider等旧全量栈问题不在本 PR。 - [P1/P2] stale-pose、
applyForceAtPosition、tolerancesScale、中文注释及 fixedTimeStep 清理等旧全量栈反馈 — ✅ 当前窄 diff 不包含对应实现；其中 Transform getter 与 re-enter 时序的既有解释已验证成立，不重复提出。 - [P3]
PhysicsScene.ts反向扫描单行注释尾句号 — ✅ 已由b9b56160c3修复。
问题
-
[P1]
packages/physics-physx/src/PhysXPhysics.ts:68-73/PhysXPhysicsScene.ts:197-200— 默认 CDN runtime 没有随新的 native 协议发布新测试在
PhysicsScene.test.ts:126-134显式把 normal/SIMD URL 改成仓库内资产，因此覆盖到本 commit 新增了setContactEventEnabled的 WASM；但公开默认构造new PhysXPhysics()仍使用这里的固定 CDN URL。直接加载这两条默认 URL 后，normal runtime 的typeof PHYSX.setContactEventEnabled是"undefined"，normal/SIMD CDN WASM 也都不含该 Embind 名称，而目标 HEAD 的仓库内 WASM 包含它。这会让
PhysXPhysicsScene.setContactEventEnabled的可选链静默 no-op；与此同时本轮已删除_contactEventEnabled和_bufferContactEvent的 TypeScript guard。结果是默认用户的每个 contact 仍跨 Embind 调用并执行collision.getContacts()与逐点复制，正好退回本 PR 要消除的热路径成本，CI 只验证了显式本地 URL，未覆盖实际默认发布路径。应让
@galacean/engine-physics-physx统一拥有 TS + normal/SIMD JS/WASM 的版本契约：先把本 commit 对应的两套 runtime 发布到新的不可变 CDN 路径，再更新这两个默认 URL；同时在 PhysX 初始化边界把setContactEventEnabled作为该 backend 必需的 native capability 校验，避免内部可选链掩盖协议漂移。core 对第三方IPhysicsScene的可选 capability 继续保留，但不要恢复 TypeScript buffering fallback、legacy runtime 分支或第二套 gate。 -
[P2]
packages/core/src/physics/PhysicsScene.ts:33— 保留 contact-demand cache 的代码侧失效契约上一轮问题仍未关闭：
_contactEventDemandDirty只由 collider/controller 增删和 Script 进入/离开 active scene 置脏；直接替换 active Script 实例或原型上的 collision callback 不会触发 reconcile。当前 PR 描述写明了该边界，但代码侧唯一局部说明仍已被206b72acd6删除。若 HMR 或动态 callback 接入者不知道这一约束，native delivery 可能保持 disabled，collision callback 会静默漏报，直到无关生命周期变化触发重算。建议在字段或
_syncContactEventDemand旁恢复精简说明，明确“active 生命周期内 callback 视为稳定；live patch 必须触发统一 Script callback-registration refresh”。不要为此增加每帧扫描、collision-only setter、镜像状态或 compatibility branch。
架构、熵增与测试治理
- 上游 owner：active collider/controller 集合仍由
PhysicsScene._colliders持有，active Script 集合由 collider entity 的_scripts持有；Script scene 生命周期与 collider/controller add/remove 是 demand 的唯一失效入口，PhysicsScene在 fixed step 前机械派生布尔 demand。 - 下游 owner：本轮删除 TypeScript 的
_contactEventEnabled镜像字段与 callback 热路径分支，把 delivery 状态唯一地下沉到 native simulation callback；同版本资产下这是净减一个状态 owner 和一条转换路径。trigger state machine 仍独立，新增 trigger-only 链路测试证明没有被 collision gate 合并。 - 协议与发布 owner：当前仓库内 WASM 与默认 CDN WASM 形成两份并行协议，且 PhysX 内部可选链把版本错误变成静默降级。应保留“native callback 是唯一 gate owner”，删除 backend 内部的可选降级路径，并由 physics-physx 发布单元固定 TS/JS/WASM 与上游 native build commit；P1 修复后数据流应为
core demand -> PhysX backend required capability -> native callback gate。 - 熵增净值：代码形态本身从 JS mirror + branch 收敛为 native 单 owner，方向是负熵；但未同步默认资产把复杂度转移成部署期第三份真相，当前整体尚未完成收敛。normal/impulse 的权威协议仍是
ICollision的shape1 -> shape0，本轮未新增排序、转换或校验路径。 - 测试治理：新增 native-boundary spy 对“是否跨 Embind”这一不可由公开行为观察的性能契约提供了有效反向证伪，trigger-only 测试也锁住两条事件路径的分离；缺口是测试显式覆盖本地 runtime，却绕过默认 CDN 选择。未发现为旧测试保留的生产 compatibility branch、legacy fallback、wrapper、镜像状态或第二条 contact 转换路径，也没有失效 fixture/mock/snapshot 需要保留；默认资产契约应由发布/初始化 guard 补齐，而不是恢复旧生产 gate。
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.
🫧 尘小沫
结论
第十轮增量审查覆盖 fc2bcdae6952afd694b71c779b4e4f7b1b7743a0...0b39ba5d57bdeb169d91f2f3cc1aedfbdcb544bc：compare 为 ahead 3 / behind 0，新增 commit 依次为 a5b5c9d925 fix(physics): require native contact event gate、60ce99a440 refactor(physics): make contact demand event-driven、0b39ba5d57 refactor(physics): clarify contact event consumers。上一轮默认 CDN runtime 的协议漂移已闭环，但后两轮把精确的 collider+Script demand 改成了全场 Script 计数，同时把 PhysX 内部必需能力扩散成公开 backend 必选接口，分别造成热路径误开启和第三方接口破坏。
阻塞级别：存在 2 条 P1，阻塞。 本次实际 review 动作为 REQUEST_CHANGES，目标 HEAD 为 0b39ba5d57bdeb169d91f2f3cc1aedfbdcb544bc。AutoCR 不执行 APPROVE，也不替代人工 Reviewer 的合入门禁。
已关闭问题清单
- [P1] 默认 CDN runtime 没有随 native gate 协议发布 — ✅ 已由
a5b5c9d925闭环：两条新默认 JS 分别指向新的 normal/SIMD WASM；解压后的远端资产 SHA-256 与目标 HEAD 仓库资产逐字节一致（normal3768112a...、SIMD44c4f11f...），且都包含setContactEventEnabled。PhysX backend 内部也已删除可选降级调用。 - [P1] contact-event demand 每个 fixed substep 全量重扫 — ✅ 当前实现已移除 fixed-step 扫描；本轮新问题是计数丢失 collider 这一权威输入，不是重提扫描成本。
- [P1] PR 标题含无关 “shaderlab” 且范围混装 — ✅ 当前标题和最终文件范围均已收窄到 contact gate、normal direction、runtime 资产与对应测试。
- [P1/P2] stale-pose、
applyForceAtPosition、tolerancesScale、中文注释及 fixedTimeStep 清理等旧全量栈反馈 — ✅ 当前 PR 不包含对应实现；既有合理解释与拆分结论继续成立。 - [P3]
PhysicsScene.ts反向扫描单行注释尾句号 — ✅ 相关扫描现已整体删除，且目标 diff 未新增注释格式问题。
问题
-
[P1]
packages/design/src/physics/IPhysicsScene.ts:50/packages/core/src/physics/PhysicsScene.ts:70,716— 不要把 PhysX runtime 的必需能力上提为所有 backend 的必选契约a5b5c9d925为了解决默认 PhysX runtime 漂移，把setContactEventEnabled从可选重新改成必选，并让 core 构造路径直接调用它。这撤销了本 PR 前面已经闭环的第三方兼容修复：IPhysicsScene是公开 design 契约，仓库外 backend 即使保持原有“始终产生 contact event”的合法行为，也会立即编译失败；而默认 CDN 的根因只存在于@galacean/engine-physics-physx自己的 TS/JS/WASM 发布边界。应保留
PhysXPhysicsScene -> _physX.setContactEventEnabled的直接必需调用，让 PhysX 发布单元对自己的 native capability 负责；同时把公开IPhysicsScene.setContactEventEnabled恢复为可选，并在 core 的初始禁用与 0/1 边界调用处都使用可选链。这样数据流仍是core demand -> backend capability -> PhysX native gate，但不会为了修复一个 backend 的版本协议而扩大所有 backend 的契约，也不需要 fallback、wrapper 或第二套 gate。 -
[P1]
packages/core/src/Script.ts:231-260/PhysicsScene.ts:33,711-717/PhysicsScene.test.ts:164-208— 全场 Script 计数不是实际 contact consumer 的权威事实下游 dispatch 只会遍历发生接触的
shape.collider.entity._scripts（PhysicsScene.ts:755-790），因此 contact demand 的事实是“active collider/controller entity 上存在 collision callback Script”，不是“场景任意 active Script 覆写了 collision callback”。最新实现删除了 collider 增删这一输入，只按所有 Script 生命周期维护全场计数；新测试甚至明确用没有 collider 的script-onlyentity 断言 gate 必须打开，并删除了上一版“collider disabled 时关闭”的覆盖。结果是只要任意无 collider 的 Script 覆写 collision 方法，或 collider 被禁用但 Script 仍 active，整个 physics scene 的所有 contact 都会重新跨 native/Embind 边界复制 contact points，即使这个 Script 不可能收到事件；这直接击穿本 PR 的热路径目标。计数还在进入和离开时重新检查当前方法：active 期间 live patch 从无 callback 改为有 callback 后，disable 会先减成
-1，再次 enable 只回到0，上一轮指出的 callback-stability 边界由“暂不刷新”恶化为可持续污染计数。应保留
PhysicsScene._colliders与各 collider entity 的_scripts作为两层权威 owner，删除这个全场_contactEventConsumerCount及 script-only 正向 fixture；优先在已有低频生命周期失效边界机械派生精确布尔 demand，而不是再增加 per-Script 镜像状态或同步层。测试应恢复并锁住三条公开行为：script-only 不开启、collider disable 后关闭、active collider + collision Script 才开启；若仍保留“active 期间 callback 视为稳定”的约束，需把它留在代码侧统一 registration 契约旁，不能只留在 PR 描述。
架构、熵增与测试治理
- 上游 owner：实际 consumer 是 active collider/controller 集合与其 entity active Script 集合的交集；
PhysicsScene._colliders和Entity._scripts分别拥有这两份事实。最新全场 Script count 丢掉了 collider owner，因此不是该交集的等价索引。 - 下游 owner：native simulation callback 继续是唯一 contact delivery gate；TypeScript buffering mirror 与第二条热路径 branch 已删除，trigger state machine 保持独立。normal/impulse 的权威协议仍是
ICollision的shape1 -> shape0，core 只做 receiver-relative 符号转换。 - 协议与发布 owner：新的 normal/SIMD 默认 CDN 资产与仓库 WASM 已逐字节一致，上一轮部署期第三份真相已收敛。应继续由 physics-physx 发布单元强校验自身 native ABI，但删除对所有
IPhysicsScene实现的强制扩散。 - 熵增净值：本轮删除 dirty scan 和 24 行代码，局部行数下降；但用一个持久全场计数替代“collider × Script”机械派生事实，并通过测试重新定义 consumer，把复杂度转移成了错误 owner 和 native 热路径。修复方向应是删除该宽泛计数、回到权威集合的冷路径派生，不再添加第三份状态。
- 测试治理：目标 HEAD 的 lint、三平台 build、4 组 e2e、codecov project/patch 均通过；native-boundary 反向 spy、trigger-only 链路和 collision normal 方向测试仍有效。需要删除/重写的是本轮将 script-only 认作 consumer 的新 fixture，并恢复 collider disable 的负向断言；未发现需要为旧测试保留的 production compatibility branch、legacy fallback、wrapper、镜像 gate 或第二条 contact 转换/校验路径。
3 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.