Revert "feat(physics): 添加 MeshColliderShape 支持"#2890
Revert "feat(physics): 添加 MeshColliderShape 支持"#2890GuoLei1990 wants to merge 1 commit intodev/2.0from
Conversation
This reverts commit 9f51994.
WalkthroughThis PR comprehensively removes the MeshColliderShape feature from the physics system, including implementation classes, interfaces, E2E tests, and adjusts related collision detection handling across core, design, and physics provider packages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Augment PR SummarySummary: This PR reverts the recently added PhysX Changes:
Technical Notes: After this revert, the public API no longer exposes mesh-based colliders; any downstream code relying on MeshCollider tests/E2E coverage will need to be updated accordingly. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| protected _addNativeShape(shape: ColliderShape): void { | ||
| shape._collider = this; | ||
| shape._nativeShape?.setWorldScale(this.entity.transform.lossyWorldScale); | ||
| this._nativeCollider.addShape(shape._nativeShape); |
There was a problem hiding this comment.
_addNativeShape() no longer applies setWorldScale(this.entity.transform.lossyWorldScale) before addShape(), so shapes added after the transform update flag is cleared may keep a default scale until the next transform change (incorrect collision sizes on scaled entities). Is scale being applied anywhere else on shape-add now?
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| break; | ||
| case CollisionDetectionMode.ContinuousDynamic: | ||
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, true); | ||
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, false); |
There was a problem hiding this comment.
In ContinuousDynamic, eENABLE_CCD_FRICTION is enabled while eENABLE_CCD is explicitly disabled; PhysX documents that eENABLE_CCD_FRICTION requires eENABLE_CCD, so this mode may end up with CCD effectively off.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| @@ -165,17 +165,13 @@ export class PhysXDynamicCollider extends PhysXCollider implements IDynamicColli | |||
| switch (value) { | |||
| case CollisionDetectionMode.Continuous: | |||
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, true); | |||
There was a problem hiding this comment.
In setCollisionDetectionMode(), the Continuous/ContinuousSpeculative branches no longer clear the other CCD-related flags, so switching modes can leave stale flags set from a previous mode and change the intended semantics.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #2890 +/- ##
===========================================
- Coverage 83.03% 78.57% -4.47%
===========================================
Files 796 870 +74
Lines 90449 95101 +4652
Branches 9503 9494 -9
===========================================
- Hits 75107 74728 -379
- Misses 15260 20213 +4953
- Partials 82 160 +78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/physics-physx/src/PhysXDynamicCollider.ts`:
- Around line 169-172: In CollisionDetectionMode.ContinuousDynamic inside
PhysXDynamicCollider, enable the core CCD flag instead of disabling it: when
handling CollisionDetectionMode.ContinuousDynamic call
this._pxActor.setRigidBodyFlag with physX.PxRigidBodyFlag.eENABLE_CCD set to
true (in addition to keeping eENABLE_CCD_FRICTION = true) so that
eENABLE_CCD_FRICTION has effect and continuous collision detection is actually
enabled.
In `@packages/physics-physx/src/shape/PhysXColliderShape.ts`:
- Line 153: The destroy() method currently calls this._pxGeometry.delete() and
this._pxShape.release() without guarding against undefined, which creates a
fragile contract because protected _pxGeometry: any has no default; update the
class to either initialize _pxGeometry and _pxShape to null/undefined-safe
defaults at declaration (e.g., protected _pxGeometry: any = null; protected
_pxShape: any = null) or add null/undefined guards in destroy() to only call
delete()/release() when the properties are set (check this._pxGeometry and
this._pxShape before invoking their methods) so future subclasses won't trigger
a TypeError.
| case CollisionDetectionMode.ContinuousDynamic: | ||
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, true); | ||
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, false); | ||
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD_FRICTION, true); | ||
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_SPECULATIVE_CCD, false); | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant section
fd "PhysXDynamicCollider.ts" --type fRepository: galacean/engine
Length of output: 109
🏁 Script executed:
# Read the file around the specified lines
cat -n packages/physics-physx/src/PhysXDynamicCollider.ts | sed -n '160,180p'Repository: galacean/engine
Length of output: 1332
🏁 Script executed:
# Get more context around the function containing these lines
cat -n packages/physics-physx/src/PhysXDynamicCollider.ts | sed -n '140,200p'Repository: galacean/engine
Length of output: 2704
🏁 Script executed:
# Search for test files related to PhysXDynamicCollider or collision detection mode
fd -t f "\.test\.|\.spec\." | xargs grep -l "ContinuousDynamic\|PhysXDynamicCollider" 2>/dev/null | head -20Repository: galacean/engine
Length of output: 105
🏁 Script executed:
# Look for test files in the physics-physx package
fd -t f "\.(test|spec)\." packages/physics-physx/ 2>/dev/nullRepository: galacean/engine
Length of output: 41
🏁 Script executed:
# Search for any documentation or comments about eENABLE_CCD_FRICTION
rg "eENABLE_CCD_FRICTION" -B 3 -A 3Repository: galacean/engine
Length of output: 2176
🏁 Script executed:
# Check git log for PR `#2880` or related commits
git log --oneline --all 2>/dev/null | grep -i "2880\|ContinuousDynamic\|eENABLE_CCD" | head -20Repository: galacean/engine
Length of output: 41
🏁 Script executed:
# Read the test file around the ContinuousDynamic test
cat -n tests/src/core/physics/DynamicCollider.test.ts | grep -B 10 -A 10 "ContinuousDynamic"Repository: galacean/engine
Length of output: 2365
🏁 Script executed:
# Check if there's any git history available (look for recent commits related to this file)
git log --oneline packages/physics-physx/src/PhysXDynamicCollider.ts 2>/dev/null | head -20Repository: galacean/engine
Length of output: 117
🏁 Script executed:
# Look for any documentation or enum definitions for CollisionDetectionMode
rg "CollisionDetectionMode" --type ts -A 5 | head -50Repository: galacean/engine
Length of output: 4284
🏁 Script executed:
# Check PhysX wrapper or type definitions for flag meanings
fd -t f -e ts -e js | xargs grep -l "PxRigidBodyFlag\|eENABLE_CCD" | head -10Repository: galacean/engine
Length of output: 156
🏁 Script executed:
# Let's check the full CollisionDetectionMode enum definition
cat -n packages/physics-physx/src/PhysXDynamicCollider.ts | sed -n '1,50p'Repository: galacean/engine
Length of output: 2143
🏁 Script executed:
# Search for the enum definition
rg "enum CollisionDetectionMode" -A 10Repository: galacean/engine
Length of output: 2033
🌐 Web query:
PhysX PxRigidBodyFlag eENABLE_CCD_FRICTION eENABLE_CCD dependency documentation
💡 Result:
PxRigidBodyFlag::eENABLE_CCD_FRICTION depends on PxRigidBodyFlag::eENABLE_CCD: the API reference explicitly states that CCD friction “requires PxRigidBodyFlag::eENABLE_CCD to be raised to have any effect”. It also notes CCD friction is off by default because it can introduce artifacts, and enabling it restores behavior closer to older PhysX versions. [1]
For eENABLE_CCD_FRICTION to actually do anything in a simulation, you also need the normal CCD prerequisites enabled:
- Scene-level CCD:
PxSceneDesc::flags |= PxSceneFlag::eENABLE_CCD;[2] - Pairwise CCD contact generation in filtering:
PxPairFlag::eDETECT_CCD_CONTACT(and typicallyeSOLVE_CONTACT) for the interacting shape pair(s) [2][3] - Per-body CCD:
body->setRigidBodyFlag(PxRigidBodyFlag::eENABLE_CCD, true);[2] - Then you can enable CCD friction:
body->setRigidBodyFlag(PxRigidBodyFlag::eENABLE_CCD_FRICTION, true);[2]
Sources: NVIDIA PhysX SDK API reference for PxRigidBodyFlag [1], and PhysX “Advanced Collision Detection / CCD” documentation + related PxPairFlag reference [2][3].
[1] (docs.nvidia.com)
[2] (docs.nvidia.com)
[3] (docs.nvidia.com)
Citations:
- 1: https://docs.nvidia.com/gameworks/content/gameworkslibrary/physx/apireference/3.3.4/files/structPxRigidBodyFlag.html?utm_source=openai
- 2: https://docs.nvidia.com/gameworks/content/gameworkslibrary/physx/guide/3.3.4/Manual/AdvancedCollisionDetection.html?utm_source=openai
- 3: https://docs.nvidia.com/gameworks/content/gameworkslibrary/physx/apireference/files/structPxPairFlag.html?utm_source=openai
🌐 Web query:
PhysX PxRigidBodyFlag eENABLE_SPECULATIVE_CCD eENABLE_CCD requirement dependency
💡 Result:
PxRigidBodyFlag::eENABLE_SPECULATIVE_CCD does not depend on PxSceneFlag::eENABLE_CCD, PxPairFlag::eDETECT_CCD_CONTACT, or PxRigidBodyFlag::eENABLE_CCD (the “sweep-based” CCD pipeline). You can enable speculative CCD just by setting eENABLE_SPECULATIVE_CCD on the body. [1]
By contrast, “sweep-based” CCD has these dependencies:
- Scene must enable CCD:
PxSceneFlag::eENABLE_CCD(set at scene creation). [2] - Per-body enable:
PxRigidBodyFlag::eENABLE_CCDon bodies that should use sweep-based CCD. [2] - Per-pair enable:
PxPairFlag::eDETECT_CCD_CONTACTin your filter shader for the interacting pair(s). [2]
Notes:
- Speculative CCD is allowed on kinematic actors and works best with PCM. [1]
- If you enable speculative CCD on kinematics while also using sweep-based CCD for dynamics, avoid also enabling sweep-based CCD for the kinematic–dynamic interactions, because sweep-based CCD can “overrule” the speculative behavior. [1]
Sources: Advanced Collision Detection (Speculative CCD) [1]; PxSceneFlag::eENABLE_CCD requirements (pair flag + per-body flag) [2].
ContinuousDynamic sets eENABLE_CCD = false while enabling eENABLE_CCD_FRICTION, rendering CCD non-functional for this mode.
Per official PhysX documentation, eENABLE_CCD_FRICTION "requires PxRigidBodyFlag::eENABLE_CCD to be raised to have any effect." With eENABLE_CCD explicitly set to false on line 170, the eENABLE_CCD_FRICTION = true call on line 171 has no effect, leaving the body with no continuous collision detection—indistinguishable from Discrete mode. This directly contradicts the documented intent: "continuous collision detection is on for colliding with static and dynamic geometry."
The fix is straightforward:
Fix
case CollisionDetectionMode.ContinuousDynamic:
- this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, false);
+ this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, true);
this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD_FRICTION, true);
break;📝 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.
| case CollisionDetectionMode.ContinuousDynamic: | |
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, true); | |
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, false); | |
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD_FRICTION, true); | |
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_SPECULATIVE_CCD, false); | |
| break; | |
| case CollisionDetectionMode.ContinuousDynamic: | |
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, true); | |
| this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD_FRICTION, true); | |
| break; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/physics-physx/src/PhysXDynamicCollider.ts` around lines 169 - 172,
In CollisionDetectionMode.ContinuousDynamic inside PhysXDynamicCollider, enable
the core CCD flag instead of disabling it: when handling
CollisionDetectionMode.ContinuousDynamic call this._pxActor.setRigidBodyFlag
with physX.PxRigidBodyFlag.eENABLE_CCD set to true (in addition to keeping
eENABLE_CCD_FRICTION = true) so that eENABLE_CCD_FRICTION has effect and
continuous collision detection is actually enabled.
| destroy(): void { | ||
| this._pxShape.release(); | ||
| this._pxGeometry?.delete(); | ||
| this._pxGeometry.delete(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the PhysXColliderShape base class and understand its structure
find . -type f -name "*.ts" | xargs grep -l "class PhysXColliderShape"Repository: galacean/engine
Length of output: 115
🏁 Script executed:
# Now run the verification script to find all subclasses and their _pxGeometry assignments
rg -n --type=ts "extends PhysXColliderShape" -A 60 | grep -E "(extends PhysXColliderShape|_pxGeometry\s*=)"Repository: galacean/engine
Length of output: 1299
🏁 Script executed:
# Also check the base class definition to understand the property declaration
rg -n --type=ts "class PhysXColliderShape" -A 30Repository: galacean/engine
Length of output: 2710
🏁 Script executed:
# Check the destroy() method and initialization flow in PhysXColliderShape
rg -n --type=ts "destroy\(\)" packages/physics-physx/src/shape/PhysXColliderShape.ts -B 5 -A 10Repository: galacean/engine
Length of output: 447
🏁 Script executed:
# Check one subclass constructor to see when _pxGeometry is initialized relative to super()
rg -n --type=ts "constructor" packages/physics-physx/src/shape/PhysXSphereColliderShape.ts -A 20Repository: galacean/engine
Length of output: 740
🏁 Script executed:
# Verify the base class has all necessary initialization guards or patterns
rg -n --type=ts "protected _pxGeometry" packages/physics-physx/src/shape/PhysXColliderShape.ts -B 5 -A 5Repository: galacean/engine
Length of output: 427
🏁 Script executed:
# Check the _initialize() method to understand the full initialization sequence
rg -n --type=ts "_initialize\(" packages/physics-physx/src/shape/PhysXColliderShape.ts -A 30Repository: galacean/engine
Length of output: 669
🏁 Script executed:
# Check for any event handlers or lifecycle patterns that might call destroy during construction
rg -n --type=ts "destroy\(\)" packages/physics-physx/src -lRepository: galacean/engine
Length of output: 401
🏁 Script executed:
# Check PhysXColliderShape for super class or initialization patterns
head -100 packages/physics-physx/src/shape/PhysXColliderShape.tsRepository: galacean/engine
Length of output: 3274
Removal of optional chaining is correct given all concrete subclasses initialize _pxGeometry before destroy() is called.
All four concrete subclasses (PhysXSphereColliderShape, PhysXBoxColliderShape, PhysXCapsuleColliderShape, PhysXPlaneColliderShape) consistently initialize _pxGeometry in their constructors before calling _initialize(), which sets up _pxShape. MeshColliderShape is confirmed removed. However, since protected _pxGeometry: any has no default value (implicitly undefined), the direct calls to _pxGeometry.delete() and _pxShape.release() in destroy() create an implicit contract that future subclasses must also initialize these properties in their constructors—there is no guard to prevent a TypeError if this contract is broken.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/physics-physx/src/shape/PhysXColliderShape.ts` at line 153, The
destroy() method currently calls this._pxGeometry.delete() and
this._pxShape.release() without guarding against undefined, which creates a
fragile contract because protected _pxGeometry: any has no default; update the
class to either initialize _pxGeometry and _pxShape to null/undefined-safe
defaults at declaration (e.g., protected _pxGeometry: any = null; protected
_pxShape: any = null) or add null/undefined guards in destroy() to only call
delete()/release() when the properties are set (check this._pxGeometry and
this._pxShape before invoking their methods) so future subclasses won't trigger
a TypeError.
1 participant
Reverts #2880
Summary by CodeRabbit
Breaking Changes
Improvements
Chores