Support get contact points and get closest point on shape from a point #2458
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces a comprehensive enhancement to the physics collision handling system across multiple packages. The primary focus is on creating a more structured and detailed approach to collision events by introducing a new Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2458 +/- ##
===========================================
+ Coverage 67.12% 68.06% +0.94%
===========================================
Files 897 913 +16
Lines 93218 94778 +1560
Branches 7581 7975 +394
===========================================
+ Hits 62572 64513 +1941
+ Misses 30401 30015 -386
- Partials 245 250 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (11)
packages/design/src/physics/index.ts (1)
9-9: LGTM! Consider adding documentation.The addition of
ICollisionexport aligns with the PR's collision handling enhancements. However, since the PR objectives indicate missing documentation, consider adding JSDoc comments for the new interface.Add documentation above the export:
+/** Interface for collision data including contact points and collider IDs */ export type { ICollision } from "./ICollision";packages/core/src/physics/shape/PlaneColliderShape.ts (2)
14-17: Add tests for the unsupported method.The PR objectives indicate missing tests. Please add tests to verify:
- The method throws an appropriate error
- Error message is clear and helpful
- The behavior is consistent with other unsupported operations
Would you like me to help generate test cases for this method?
14-17: Document method behavior in JSDoc.Add documentation to clarify:
- Why the method is not supported for plane colliders
- What callers should do instead
- The behavior of the closestPoint parameter
Add documentation:
+ /** + * Not supported for plane colliders as planes are infinite. + * @throws {Error} Always throws an error as this operation is not supported + */ override getDistanceAndClosestPointFromPoint(point: Vector3, closestPoint: Vector3): number {packages/design/src/physics/ICollision.ts (1)
15-18: Consider decoupling from PhysX implementationThe interface name
PhysXVectorPxContactPairPointis tightly coupled to the PhysX physics engine. Consider using a more generic name likeIContactPointVectorto maintain implementation independence.packages/design/src/physics/shape/IColliderShape.ts (1)
44-51: LGTM! Consider adding parameter validation noteThe
pointDistancemethod signature and documentation look good. Consider adding a note about parameter validation, specifically whether null/undefined values are handled.* @param position - The position in world space * @param rotation - The rotation in world space * @param point - The point + * @throws {Error} When any parameter is null or undefined * @returns The distance information */packages/core/src/physics/Collision.ts (1)
38-52: Consider optimizing Vector3 object creationThe current implementation creates new Vector3 objects for each contact point. Consider reusing Vector3 objects to reduce garbage collection pressure in performance-critical scenarios.
getContacts(contacts: ContractPoint[]): ContractPoint[] { const nativeContractPoints = this._nativeCollision.getContacts(); + const tempPosition = new Vector3(); + const tempNormal = new Vector3(); + const tempImpulse = new Vector3(); for (let i = 0, n = nativeContractPoints.size(); i < n; i++) { const nativeContractPoint = nativeContractPoints.get(i); const { position, normal, impulse, separation } = nativeContractPoint; const contact: ContractPoint = { - position: new Vector3(position.x, position.y, position.z), - normal: new Vector3(normal.x, normal.y, normal.z), - impulse: new Vector3(impulse.x, impulse.y, impulse.z), + position: tempPosition.set(position.x, position.y, position.z).clone(), + normal: tempNormal.set(normal.x, normal.y, normal.z).clone(), + impulse: tempImpulse.set(impulse.x, impulse.y, impulse.z).clone(), separation: separation }; contacts.push(contact); } return contacts; }packages/physics-lite/src/LitePhysics.ts (1)
Line range hint
116-146: Standardize error messages and fix copy-paste errorsSeveral error messages have inconsistent formatting and some are incorrect due to copy-paste:
- createFixedJoint mentions "CapsuleColliderShape"
- createHingeJoint mentions "CapsuleColliderShape"
- createSpringJoint mentions "CapsuleColliderShape"
Apply these corrections:
createFixedJoint(collider: LiteCollider): IFixedJoint { - throw "Physics-lite don't support CapsuleColliderShape. Use Physics-PhysX instead!"; + throw "Physics-lite doesn't support FixedJoint. Use Physics-PhysX instead!"; } createHingeJoint(collider: LiteCollider): IHingeJoint { - throw "Physics-lite don't support CapsuleColliderShape. Use Physics-PhysX instead!"; + throw "Physics-lite doesn't support HingeJoint. Use Physics-PhysX instead!"; } createSpringJoint(collider: LiteCollider): ISpringJoint { - throw "Physics-lite don't support CapsuleColliderShape. Use Physics-PhysX instead!"; + throw "Physics-lite doesn't support SpringJoint. Use Physics-PhysX instead!"; }Also, consider standardizing all error messages in the class to use "doesn't" instead of "don't" for grammatical correctness.
packages/core/src/physics/shape/ColliderShape.ts (1)
138-157: Add parameter validation for input vectors.The implementation looks correct, but consider adding null/undefined checks for the input parameters to prevent runtime errors.
getDistanceAndClosestPointFromPoint(point: Vector3, closestPoint: Vector3): number { + if (!point || !closestPoint) { + throw new Error("Parameters 'point' and 'closestPoint' must not be null or undefined"); + } const tempQuat = ColliderShape._tempWorldRot; const tempPos = ColliderShape._tempWorldPos;packages/physics-physx/src/PhysXPhysics.ts (1)
95-95: Consider using environment variables for the script URL.Hardcoding URLs in the source code makes it difficult to change them in different environments (staging, production, etc.).
- script.src = `https://mdn.alipayobjects.com/rms/afts/file/A*eG1KRpOjfn0AAAAAAAAAAAAAARQnAQ/physx.release.js`; + script.src = process.env.PHYSX_WASM_URL || `https://mdn.alipayobjects.com/rms/afts/file/A*eG1KRpOjfn0AAAAAAAAAAAAAARQnAQ/physx.release.js`;packages/physics-physx/src/PhysXPhysicsScene.ts (1)
39-41: Consider unifying event handling patterns.The constructor parameters show a mixed pattern:
- Contact events use
ICollision- Trigger events use separate indices
Consider updating trigger events to use
ICollisionas well for consistency.packages/math/src/Quaternion.ts (1)
416-424: LGTM! Consider minor improvements for better maintainability.The implementation correctly converts angles from degrees to radians and creates the quaternion. A few suggestions to enhance the code:
- Extract the degree-to-radian conversion constant:
+ /** Conversion factor from degrees to radians */ + private static readonly DEG_TO_RAD = Math.PI / 180; static fromAngle(angle: Vector3, out: Quaternion): void { - const angleToRadian = Math.PI / 180; Quaternion.rotationYawPitchRoll( - angle.y * angleToRadian, - angle.x * angleToRadian, - angle.z * angleToRadian, + angle.y * Quaternion.DEG_TO_RAD, + angle.x * Quaternion.DEG_TO_RAD, + angle.z * Quaternion.DEG_TO_RAD, out ); }
- Add return type annotation for consistency with other methods:
- static fromAngle(angle: Vector3, out: Quaternion): void { + static fromAngle(angle: Vector3, out: Quaternion): void {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/core/src/physics/Collision.ts(1 hunks)packages/core/src/physics/PhysicsScene.ts(4 hunks)packages/core/src/physics/shape/ColliderShape.ts(3 hunks)packages/core/src/physics/shape/PlaneColliderShape.ts(2 hunks)packages/design/src/physics/ICollision.ts(1 hunks)packages/design/src/physics/IPhysics.ts(2 hunks)packages/design/src/physics/index.ts(1 hunks)packages/design/src/physics/shape/IColliderShape.ts(2 hunks)packages/design/src/physics/shape/index.ts(1 hunks)packages/math/src/Quaternion.ts(1 hunks)packages/physics-lite/src/LitePhysics.ts(2 hunks)packages/physics-lite/src/LitePhysicsScene.ts(3 hunks)packages/physics-lite/src/shape/LiteColliderShape.ts(2 hunks)packages/physics-physx/src/PhysXPhysics.ts(3 hunks)packages/physics-physx/src/PhysXPhysicsScene.ts(4 hunks)packages/physics-physx/src/shape/PhysXColliderShape.ts(2 hunks)
🔇 Additional comments (9)
packages/design/src/physics/shape/index.ts (1)
1-1: LGTM! Verify usage of the new exported type.
The addition of IPointDistanceInfo export is consistent with the PR's objective to support distance calculations.
✅ Verification successful
The new exported type IPointDistanceInfo is properly used across the physics implementations
The verification shows that IPointDistanceInfo is:
- Defined in
packages/design/src/physics/shape/IColliderShape.ts - Exported through the barrel file in
packages/design/src/physics/shape/index.ts - Correctly imported and implemented in both physics implementations:
packages/physics-lite/src/shape/LiteColliderShape.tspackages/physics-physx/src/shape/PhysXColliderShape.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the usage of IPointDistanceInfo across the codebase
rg "IPointDistanceInfo" --type ts
Length of output: 971
packages/core/src/physics/shape/PlaneColliderShape.ts (1)
14-17:
Critical: Improve unsupported method implementation.
Several issues with the current implementation:
- If the functionality is not supported, consider throwing an error instead of returning -1
- The method accepts a
closestPointparameter but doesn't document what happens to it - The error message has grammatical issues
Consider this implementation:
override getDistanceAndClosestPointFromPoint(point: Vector3, closestPoint: Vector3): number {
- console.error("PlaneColliderShape is not support getDistanceAndClosestPointFromPoint");
- return -1;
+ throw new Error("getDistanceAndClosestPointFromPoint is not supported by PlaneColliderShape");
}✅ Verification successful
Throw error for unsupported operation instead of returning -1
The method is defined in the base ColliderShape class and PlaneColliderShape overrides it. Since this is a deliberate override indicating non-support, throwing an error is more appropriate than returning -1 and logging to console, as it:
- Properly signals to callers that the operation is not supported
- Follows error handling best practices for unsupported operations
- Provides a clearer, grammatically correct message
override getDistanceAndClosestPointFromPoint(point: Vector3, closestPoint: Vector3): number {
- console.error("PlaneColliderShape is not support getDistanceAndClosestPointFromPoint");
- return -1;
+ throw new Error("getDistanceAndClosestPointFromPoint is not supported by PlaneColliderShape");
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other shapes properly implement this method
ast-grep --pattern 'getDistanceAndClosestPointFromPoint(point: Vector3, closestPoint: Vector3): number'
Length of output: 105
Script:
#!/bin/bash
# Search for class definitions that might implement this method
ast-grep --pattern 'class $_ extends $_ {
$$$
getDistanceAndClosestPointFromPoint($$$) {
$$$
}
$$$
}'
# Also search for interface or abstract class that might define this method
rg "getDistanceAndClosestPointFromPoint" -A 2 -B 2
Length of output: 1201
packages/design/src/physics/IPhysics.ts (1)
Line range hint 40-46: Verify the design decision for trigger callbacks
While contact callbacks have been updated to use the ICollision type, trigger callbacks still use the old number-based approach. This creates an inconsistency in the API design.
Let's check if this is intentional by looking for related discussions or implementations:
✅ Verification successful
The difference in trigger and contact callback signatures is intentional
The design decision is correct. From the codebase analysis:
- Contact callbacks use
ICollisionbecause they need detailed collision data (shape IDs, contact points, etc.) - Trigger callbacks use simple numeric IDs because:
- They are used internally by physics engines (PhysX, Lite) to map collider shapes
- The actual trigger events in
Scriptclass receiveColliderShapeobjects, not raw IDs - The IDs are transformed into proper
ColliderShapeobjects before reaching user code
This layered approach separates the low-level physics engine implementation from the high-level API exposed to users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for trigger-related implementations and discussions
echo "Searching for trigger-related implementations..."
rg -A 5 "onTrigger(Enter|Exit|Stay|Begin|End|Persist)" --type ts
echo "Searching for ICollision usage in trigger-related code..."
rg -A 5 "ICollision.*trigger" --type ts
Length of output: 43321
Script:
#!/bin/bash
# Let's check the implementation of Script class and its usage of ColliderShape vs ICollision
ast-grep --pattern 'class Script {
$$$
onTrigger$_($$$) {
$$$
}
$$$
}'
# Let's also check the implementation of ColliderShape to understand its relationship with ICollision
ast-grep --pattern 'class ColliderShape {
$$$
}'
# And check if there are any direct relationships between ColliderShape and ICollision
rg -A 5 "ColliderShape.*ICollision|ICollision.*ColliderShape" --type ts
Length of output: 211
Script:
#!/bin/bash
# Let's check how ColliderShape is used in the physics implementation
rg -A 5 "class.*ColliderShape" --type ts
# Also check for any ICollision interface definition
rg -A 5 "interface.*ICollision" --type ts
# And check the actual usage of trigger callbacks in the codebase
rg -A 5 "onTrigger(Enter|Exit|Stay)\(" --type ts
Length of output: 20318
packages/physics-lite/src/LitePhysics.ts (1)
47-49: LGTM: Contact callback signature updates
The contact callback signatures have been correctly updated to use the new ICollision type, improving type safety and providing more structured collision data.
packages/core/src/physics/shape/ColliderShape.ts (1)
14-15: LGTM! Good performance optimization.
Using static properties for temporary calculations avoids unnecessary object allocations during runtime.
packages/physics-physx/src/PhysXPhysics.ts (1)
143-145: LGTM! Improved type safety with ICollision interface.
The change from using two number parameters to using an ICollision object provides better type safety and a more structured approach to handling collision data.
packages/physics-physx/src/PhysXPhysicsScene.ts (2)
1-2: LGTM: Import changes align with the new collision interface.
The addition of ICollision to the imports is consistent with the refactoring to use the new collision interface.
25-27: Verify contact event handler implementations.
The contact event handlers have been updated to use the ICollision interface, but trigger events still use the old approach with separate indices. This creates an inconsistency in the event handling pattern.
Also applies to: 60-70, 88-94
packages/core/src/physics/PhysicsScene.ts (1)
33-35: Verify shape retrieval logic.
The shape retrieval from physicalObjectsMap using shape0Id and shape1Id needs validation:
- Are the IDs guaranteed to exist in the map?
- Is there proper error handling for missing shapes?
✅ Verification successful
Let me gather more context about the collision handling and shape management.
Let me check how the shapes are managed in the ColliderShape class to understand the lifecycle.
Shape retrieval logic is properly managed.
The shape retrieval implementation is safe because:
- Shapes are guaranteed to exist in the map as they are added in the ColliderShape constructor (
Engine._physicalObjectsMap[this._id] = this) - Shapes are properly cleaned up when destroyed (
delete Engine._physicalObjectsMap[this._id]) - The code includes null checks where needed (e.g., in raycast:
if (!shape) { return false; }) - The collision callbacks are only triggered by the native physics engine for actual collisions between existing shapes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for null checks and error handling when retrieving shapes
# Search for null checks on shape retrieval
rg -p "physicalObjectsMap\[.*\]" -A 2
Length of output: 1631
Script:
#!/bin/bash
# Check the class definition and initialization of physicalObjectsMap
ast-grep --pattern 'class PhysicsScene {
$$$
}'
# Check how collisions are handled
rg -p "class PhysicsScene" -A 10
# Check if there's any type definition for physicalObjectsMap
rg -p "physicalObjectsMap.*:" -A 2
Length of output: 33280
Script:
#!/bin/bash
# Check ColliderShape class implementation for shape management
ast-grep --pattern 'class ColliderShape {
$$$
}'
# Check where shapes are added/removed from the map
rg -p "Engine\._physicalObjectsMap" -A 2
Length of output: 1600
luzhuang
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/core/src/physics/shape/ColliderShape.ts (2)
138-143: Enhance method documentationConsider adding:
- Parameter validation requirements
- Return value details (is it always positive? units?)
- Example usage
- Performance considerations
138-157: Consider architectural improvements for the physics systemThe new distance calculation functionality is a good addition, but consider these architectural improvements:
Thread Safety:
- Document thread safety requirements
- Consider using a thread-local object pool for temporary objects
Error Handling:
- Implement a consistent error handling strategy
- Add debug assertions in development builds
Performance:
- Add caching mechanisms for frequently accessed transforms
- Consider SIMD optimizations for vector operations
Testing:
- Add unit tests for edge cases (null inputs, invalid states)
- Add performance benchmarks for the new functionality
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/physics/Collision.ts(1 hunks)packages/core/src/physics/shape/ColliderShape.ts(3 hunks)packages/design/src/physics/ICollision.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/design/src/physics/ICollision.ts
🔇 Additional comments (7)
packages/core/src/physics/shape/ColliderShape.ts (3)
3-3: LGTM: Import changes are appropriate
The addition of Quaternion to the import statement is necessary for the new functionality and is correctly grouped with related math imports.
144-157:
Add validation and safety checks
Several potential issues need to be addressed:
- Missing null checks for required properties:
if (!this._collider?.entity?.transform) {
throw new Error("ColliderShape is not properly initialized");
}- No validation for input parameters:
if (!point || !outClosestPoint) {
throw new Error("Input parameters cannot be null");
}- The distance calculation could be optimized:
- return Math.sqrt(distance);
+ return distance > 0 ? Math.sqrt(distance) : 0;- Consider cleanup of native resources:
try {
const res = this._nativeShape.pointDistance(tempPos, tempQuat, point);
// ... existing code ...
} finally {
// Add cleanup if needed
}Let's verify the native implementation:
14-15: Consider thread safety for shared temporary objects
While using static temporary objects is good for performance by reducing garbage collection, shared mutable state could cause issues if the shape calculations are performed concurrently. Consider:
- Adding documentation to warn about concurrent usage
- Making these instance-level properties if concurrent calculations are possible
Let's check if there are any concurrent usages:
✅ Verification successful
Let's examine the actual usage of these static properties to determine if there's a real concurrency risk.
Let's try a different approach to find the usage of these temporary variables:
Based on the evidence, I can now generate the final response:
Static temporary variables are safely used within synchronous context
The temporary variables _tempWorldPos and _tempWorldRot are only used within the synchronous getDistanceAndClosestPointFromPoint method. There's no evidence of concurrent access or usage within Workers/async contexts, making the current implementation safe. The performance optimization of reusing these objects is appropriate here.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any concurrent/async usage patterns
rg -l "async|Promise|Worker|concurrent|parallel" | xargs rg -l "ColliderShape"
Length of output: 722
Script:
#!/bin/bash
# Find where _tempWorldPos and _tempWorldRot are used
ast-grep --pattern 'ColliderShape._tempWorld$_'
# Also check for any Worker/async usage directly with ColliderShape
rg -B 3 -A 3 "ColliderShape.*Worker|async.*ColliderShape"
Length of output: 109
Script:
#!/bin/bash
# Search for usage of _tempWorldPos and _tempWorldRot in ColliderShape
rg -B 2 -A 2 "_tempWorld(Pos|Rot)" packages/core/src/physics/shape/ColliderShape.ts
# Check if physics calculations happen in workers
rg -B 3 -A 3 "new Worker.*physics|physics.*Worker"
# Check if ColliderShape methods are called from async contexts
ast-grep --pattern 'async $_ {
$$$
ColliderShape
$$$
}'
Length of output: 781
packages/core/src/physics/Collision.ts (4)
1-17: LGTM! Well-structured interface definition.
The ContactPoint interface is well-designed with:
- Clear and descriptive JSDoc comments
- Proper readonly modifiers to prevent accidental modifications
- Meaningful property names that reflect their purpose
29-31:
Add null safety check for _nativeCollision
The getter needs to handle the case where _nativeCollision is undefined.
get contactCount(): number {
+ if (!this._nativeCollision) {
+ return 0;
+ }
return this._nativeCollision.contactCount;
}38-52: 🛠️ Refactor suggestion
Add null safety and optimize Vector3 creation
Several improvements needed:
- Add null check for _nativeCollision
- Consider reusing Vector3 instances for better performance
- Add type checking for native contact point data
getContacts(outContacts: ContactPoint[]): ContactPoint[] {
+ if (!this._nativeCollision) {
+ return outContacts;
+ }
const nativeContactPoints = this._nativeCollision.getContacts();
+ if (!nativeContactPoints) {
+ return outContacts;
+ }
+
+ // Reuse these vectors to reduce allocations
+ const tempPosition = new Vector3();
+ const tempNormal = new Vector3();
+ const tempImpulse = new Vector3();
+
for (let i = 0, n = nativeContactPoints.size(); i < n; i++) {
const nativeContractPoint = nativeContactPoints.get(i);
+ if (!nativeContractPoint) continue;
+
const { position, normal, impulse, separation } = nativeContractPoint;
const contact: ContactPoint = {
- position: new Vector3(position.x, position.y, position.z),
- normal: new Vector3(normal.x, normal.y, normal.z),
- impulse: new Vector3(impulse.x, impulse.y, impulse.z),
+ position: tempPosition.set(position.x, position.y, position.z).clone(),
+ normal: tempNormal.set(normal.x, normal.y, normal.z).clone(),
+ impulse: tempImpulse.set(impulse.x, impulse.y, impulse.z).clone(),
separation: separation
};
outContacts.push(contact);
}
return outContacts;
}Likely invalid or redundant comment.
1-52: Verify integration with physics system events
Let's verify how this class integrates with the physics system's event handling.
✅ Verification successful
Let me gather more information about the physics system integration.
Based on the comprehensive results, I can now provide the final verification response:
Collision class is properly integrated with physics system
The verification shows that the Collision class is well-integrated into the physics system:
- A singleton instance is maintained in PhysicsScene for efficient collision event handling
- Proper event propagation through onCollisionEnter/Exit/Stay callbacks
- Correct transformation of native collision data to the engine's ContactPoint format
- Thread-safe collision data handling with proper cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of Collision class in event handlers
rg -t ts "new Collision|instanceof Collision" --glob "packages/**/*.ts"
# Search for ICollision interface usage
rg -t ts "implements ICollision|extends ICollision" --glob "packages/**/*.ts"
# Look for collision event handlers
ast-grep --pattern 'function $_(collision: Collision) {
$$$
}'
Length of output: 309
Script:
#!/bin/bash
# Search for collision-related event listeners and handlers
rg -t ts "onCollision|CollisionEvent" -A 5 --glob "packages/**/*.ts"
# Search for usage of ContactPoint interface
rg -t ts "ContactPoint" -A 3 --glob "packages/**/*.ts"
# Look for physics system initialization and collision setup
ast-grep --pattern 'class PhysicsScene {
$$$
}'
Length of output: 38267
|
support it also in lite? |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
packages/core/src/physics/shape/ColliderShape.ts (3)
14-15: LGTM! Consider adding documentation for temporary properties.The static properties for temporary calculations are a good optimization to avoid object allocation overhead.
Consider adding JSDoc comments to explain these are temporary calculation buffers to prevent accidental external usage:
+ /** @internal Temporary vector used for world position calculations. */ private static _tempWorldPos: Vector3 = new Vector3(); + /** @internal Temporary quaternion used for world rotation calculations. */ private static _tempWorldRot: Quaternion = new Quaternion();
144-164: Method implementation looks solid with good edge case handling.The implementation correctly handles the requirements including edge cases mentioned in previous reviews:
- Returns -1 for inactive colliders
- Uses input point when distance is 0 or negative (point inside shape)
- Efficiently uses temporary objects for calculations
Consider enhancing the warning message to be more informative:
- console.warn("The collider is not active in scene."); + console.warn("Cannot calculate distance: Collider is not enabled or not active in scene.");
138-143: Enhance method documentation with edge cases.While the documentation covers the basics, it would be helpful to document the edge cases.
Consider expanding the documentation:
/** * Get the distance and the closest point on the shape from a point. * @param point - The point * @param outClosestPoint - The result of the closest point on the shape * @returns The distance between the point and the shape + * @returns -1 if the collider is not enabled or not active in the scene + * @remarks If the point is inside the shape, outClosestPoint will be set to the input point + * and the distance will be 0 */tests/src/core/physics/ColliderShape.test.ts (1)
259-268: Fix variable name and add entity inactive test case.The test correctly verifies disabled collider behavior but could be improved.
- Fix the misleading variable name:
- const sphereShape = new BoxColliderShape(); + const boxShape = new BoxColliderShape();
- Add a test case for inactive entity:
it("getDistanceAndClosestPointFromPoint with inactive entity", () => { const boxShape = new BoxColliderShape(); dynamicCollider.addShape(boxShape); dynamicCollider.entity.isActive = false; const point = new Vector3(2, 0, 0); const closestPoint = new Vector3(); const distance = boxShape.getDistanceAndClosestPointFromPoint(point, closestPoint); expect(distance).to.eq(-1); });tests/src/core/physics/Collision.test.ts (3)
5-5: Consider using package import instead of relative pathThe import path
packages/core/types/physics/Collisionappears to be a relative path to the source code. Consider importing from the package instead (e.g.,@galacean/engine-core) to maintain consistency with other imports and avoid potential path resolution issues.
12-23: Document friction settings and add type annotationsThe function sets friction values to 0 without explaining why. Consider:
- Adding a JSDoc comment explaining the purpose and friction settings
- Making friction values configurable parameters
- Adding return type annotation
+/** + * Creates a box entity with a collider shape. + * @param cubeSize - The size of the box + * @param type - The type of collider (Dynamic or Static) + * @param pos - The position of the box + * @param friction - Optional friction settings (defaults to 0 for testing) + * @returns The created box entity + */ -function addBox(cubeSize: Vector3, type: typeof DynamicCollider | typeof StaticCollider, pos: Vector3) { +function addBox( + cubeSize: Vector3, + type: typeof DynamicCollider | typeof StaticCollider, + pos: Vector3, + friction = 0 +): Entity { const boxEntity = rootEntity.createChild("BoxEntity"); boxEntity.transform.setPosition(pos.x, pos.y, pos.z); const physicsBox = new BoxColliderShape(); - physicsBox.material.dynamicFriction = 0; - physicsBox.material.staticFriction = 0; + physicsBox.material.dynamicFriction = friction; + physicsBox.material.staticFriction = friction; physicsBox.size = cubeSize; const boxCollider = boxEntity.addComponent(type); boxCollider.addShape(physicsBox); return boxEntity; }
25-27: Document precision and consider using a named constantThe function rounds to 5 decimal places without explanation. Consider:
- Adding a JSDoc comment explaining the purpose and precision
- Extracting the precision value to a named constant
+const COLLISION_TEST_PRECISION = 5; + +/** + * Formats a number to a fixed precision for collision test comparisons. + * @param value - The number to format + * @returns The formatted number with 5 decimal places + */ function formatValue(value: number): number { - return Math.round(value * 100000) / 100000; + const factor = Math.pow(10, COLLISION_TEST_PRECISION); + return Math.round(value * factor) / factor; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/src/physics/Collision.ts(1 hunks)packages/core/src/physics/shape/ColliderShape.ts(3 hunks)packages/physics-lite/src/shape/LiteColliderShape.ts(2 hunks)tests/src/core/physics/ColliderShape.test.ts(1 hunks)tests/src/core/physics/Collision.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/physics-lite/src/shape/LiteColliderShape.ts
- packages/core/src/physics/Collision.ts
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
packages/physics-lite/src/LiteDynamicCollider.ts (1)
Line range hint
48-53: Standardize error handling across all methods.Currently, some methods use
Logger.errorwhile others throw string literals. This inconsistency makes error handling and debugging more difficult. Consider standardizing the approach across all methods.Example standardization:
addForce(force: Vector3): void { - throw "Physics-lite don't support addForce. Use Physics-PhysX instead!"; + Logger.error("Physics-lite don't support addForce. Use Physics-PhysX instead!"); } addTorque(torque: Vector3): void { - throw "Physics-lite don't support addTorque. Use Physics-PhysX instead!"; + Logger.error("Physics-lite don't support addTorque. Use Physics-PhysX instead!"); }Also applies to: 67-72, 77-82
🧹 Nitpick comments (6)
packages/core/src/physics/CharacterController.ts (1)
169-172: Consider documenting cloning behavior in class documentationSince this class has specific cloning requirements (with some properties using
@deepCloneand methods using@ignoreClone), it would be helpful to document the cloning behavior in the class-level JSDoc comments. This would help maintainers understand the cloning strategy.Add to the class documentation:
/** * The character controllers. + * @remarks + * When cloning this controller: + * - The upDirection vector is deep cloned + * - Bound callback methods are ignored during cloning */packages/physics-lite/src/LiteDynamicCollider.ts (2)
26-27: Improve error message grammar and product naming.The error messages have grammatical issues and inconsistent product naming:
- "don't" should be "doesn't" (grammar)
- "PhysX" should be consistently capitalized
Example improvement:
- Logger.error("Physics-lite don't support getInertiaTensor. Use Physics-PhysX instead!"); + Logger.error("Physics-lite doesn't support getInertiaTensor. Use PhysX instead!");Also applies to: 33-34, 41-42, 90-91, 97-98, 105-106, 112-113, 119-120, 126-127, 133-134, 140-141, 153-154, 160-161, 168-169, 175-176, 182-183, 196-197, 203-204, 210-211
Line range hint
7-8: Consider alternative design patterns for unsupported features.The current implementation raises several architectural concerns:
- The class implements
IDynamicColliderbut doesn't support any of its methods- This leads to runtime errors instead of compile-time safety
- The purpose of this lite implementation is unclear if all features are unsupported
Consider these alternatives:
- Create a reduced interface (e.g.,
ILiteDynamicCollider) with only supported features- Use the Null Object Pattern for unsupported features
- Add TypeScript compiler hints to mark unsupported methods as deprecated
Example implementation:
/** Reduced interface for lite version */ interface ILiteDynamicCollider { // Include only supported methods } /** * @deprecated Use PhysX implementation for full physics support */ export class LiteDynamicCollider implements ILiteDynamicCollider { // Implement only supported methods }packages/core/src/physics/shape/ColliderShape.ts (2)
147-150: Returning-1when collider is inactiveThe method
getDistanceAndClosestPointFromPointreturns-1when the collider is inactive, which can be ambiguous.Consider throwing an error or returning a more descriptive value to indicate the collider is inactive. Alternatively, update the documentation to specify that
-1indicates an inactive collider.
145-181: Potential issue with square root calculation affecting performanceThe method computes
Math.sqrt(distance)to return the distance. If the squared distance is sufficient for the use case, consider returning the squared distance to avoid the costly square root operation.However, ensure that all downstream code correctly handles squared distances.
packages/physics-lite/src/shape/LiteBoxColliderShape.ts (1)
89-89: Consider using a tolerance for floating-point comparisonsFloating-point precision errors can cause exact equality checks to fail even when points are very close. Instead of
Vector3.equals(p, point), consider using a small tolerance value to compare the distance.Apply this diff:
- if (Vector3.equals(p, point)) { + if (Vector3.distanceSquared(p, point) < 1e-6) {Where
1e-6is a suitable epsilon value for tolerance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/core/src/physics/CharacterController.ts(2 hunks)packages/core/src/physics/shape/ColliderShape.ts(3 hunks)packages/physics-lite/src/LiteDynamicCollider.ts(5 hunks)packages/physics-lite/src/shape/LiteBoxColliderShape.ts(3 hunks)packages/physics-lite/src/shape/LiteColliderShape.ts(5 hunks)packages/physics-lite/src/shape/LiteSphereColliderShape.ts(2 hunks)packages/physics-physx/src/shape/PhysXColliderShape.ts(2 hunks)tests/src/core/physics/ColliderShape.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/physics-physx/src/shape/PhysXColliderShape.ts
🔇 Additional comments (6)
packages/core/src/physics/CharacterController.ts (2)
8-8: LGTM: Import addition is appropriate
The addition of ignoreClone to the import statement is necessary for the new decorator usage and follows proper import practices.
169-172: Verify consistent usage of @ignoreClone across similar callback methods
The addition of @ignoreClone to _setUpDirection is appropriate as it's a bound callback method that shouldn't be cloned. However, let's verify this pattern is consistently applied.
#!/bin/bash
# Description: Check for similar bound callback methods to ensure consistent usage of @ignoreClone
# Look for similar patterns of bound methods in constructor
ast-grep --pattern 'constructor($$$) {
$$$
this.$_ = this.$_.bind(this);
$$$
}'
# Look for @ignoreClone usage on similar methods
rg "@ignoreClone\s+private\s+_\w+\s*\(\)" --type tspackages/physics-lite/src/LiteDynamicCollider.ts (1)
3-3: LGTM: Logger import enhances error handling consistency.
The addition of the Logger import from @galacean/engine standardizes error handling across the codebase.
tests/src/core/physics/ColliderShape.test.ts (1)
307-307: Misleading variable name: 'sphereShape' assigned to 'BoxColliderShape'
In the test cases for both PhysX and Lite physics, the variable sphereShape is assigned an instance of BoxColliderShape, which can cause confusion.
Consider renaming the variable to boxShape for clarity:
-const sphereShape = new BoxColliderShape();
+const boxShape = new BoxColliderShape();Also applies to: 523-523
packages/physics-lite/src/shape/LiteColliderShape.ts (1)
101-101: 🛠️ Refactor suggestion
Implement default pointDistance method or provide a fallback
The abstract method pointDistance in LiteColliderShape does not provide a default implementation. This may lead to unhandled cases if a subclass does not override it.
Consider providing a default implementation that returns an infinite distance and a zero vector:
-abstract pointDistance(position: Vector3, rotation: Quaternion, point: Vector3): IPointDistanceInfo;
+pointDistance(position: Vector3, rotation: Quaternion, point: Vector3): IPointDistanceInfo {
+ console.warn("Physics-lite doesn't support pointDistance. Use Physics-PhysX instead!");
+ return {
+ distance: Infinity,
+ closestPoint: new Vector3()
+ };
+}Likely invalid or redundant comment.
packages/physics-lite/src/shape/LiteBoxColliderShape.ts (1)
14-14: 🛠️ Refactor suggestion
Typo in method name _setBondingBox should be _setBoundingBox
The method name _setBondingBox appears to have a typographical error. It should likely be _setBoundingBox to accurately reflect its purpose of setting the bounding box.
Apply this diff to correct the method call:
- this._setBondingBox();
+ this._setBoundingBox();Please also update the method definition accordingly.
Likely invalid or redundant comment.
| * @param outClosestPoint - The result of the closest point on the shape | ||
| * @returns The distance between the point and the shape | ||
| */ | ||
| getDistanceAndClosestPointFromPoint(point: Vector3, outClosestPoint: Vector3): number { |
There was a problem hiding this comment.
#include "PxPhysicsAPI.h"
using namespace physx;
PxVec3 getClosestPointOnShape(PxShape* shape, const PxVec3& point) {
PxGeometryHolder geomHolder = shape->getGeometry(); // 获取 PxShape 的几何信息
if (!geomHolder.any().isValid()) {
throw std::runtime_error("Invalid geometry in shape.");
}
PxTransform pose = shape->getActor()->getGlobalPose() * shape->getLocalPose(); // 获取 PxShape 的全局变换
PxVec3 closestPoint;
PxReal distance = PxGeometryQuery::pointDistance(point, geomHolder.any(), pose, &closestPoint);
return closestPoint; // 返回最近点
}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
packages/physics-lite/src/shape/LiteBoxColliderShape.ts (2)
12-13:⚠️ Potential issueConsider thread safety implications of static mutable state
Using static mutable variables
_tempMatrixand_tempInvMatrixwithout synchronization can lead to data races in multithreaded environments.Replace static variables with local variables:
- private static _tempMatrix: Matrix = new Matrix(); - private static _tempInvMatrix: Matrix = new Matrix(); ... - const m = LiteBoxColliderShape._tempMatrix; - const invM = LiteBoxColliderShape._tempInvMatrix; + const m = new Matrix(); + const invM = new Matrix();
93-93:⚠️ Potential issueConsider returning actual distance instead of squared distance
The method returns squared distance which might be unexpected for API consumers. Consider using
Vector3.distance()instead ofVector3.distanceSquared()for more intuitive results.Apply this change:
- res.set(p.x, p.y, p.z, Vector3.distanceSquared(p, point)); + res.set(p.x, p.y, p.z, Vector3.distance(p, point));packages/physics-lite/src/shape/LiteSphereColliderShape.ts (1)
50-66:⚠️ Potential issueMultiple improvements needed for the pointDistance implementation
- The method is using squared distance instead of actual distance, which was previously flagged
- The current implementation performs unnecessary normalization
- Edge cases for points inside the sphere should be handled explicitly
Here's an optimized implementation:
override pointDistance(position: Vector3, rotation: Quaternion, point: Vector3): Vector4 { const p = LiteColliderShape._tempPoint; Vector3.subtract(point, position, p); - const direction = p.normalize(); - Vector3.scale(direction, this.worldRadius, p); + const distanceFromCenter = p.length(); + if (distanceFromCenter === 0) { + // Point is at sphere center, return arbitrary point on surface + p.set(this.worldRadius, 0, 0); + } else { + Vector3.scale(p, this.worldRadius / distanceFromCenter, p); + } p.add(position); const res = LiteColliderShape._tempVector4; - if (Vector3.equals(p, point)) { - res.set(point.x, point.y, point.z, 0); - } else { - res.set(p.x, p.y, p.z, Vector3.distanceSquared(p, point)); - } + res.set(p.x, p.y, p.z, Vector3.distance(p, point)); return res; }This implementation:
- Uses actual distance instead of squared distance
- Avoids unnecessary normalization
- Properly handles the case when point is at sphere center
- Is more efficient by reducing vector operations
packages/physics-physx/src/shape/PhysXColliderShape.ts (1)
132-138:⚠️ Potential issueAdd input validation and null checks.
The method needs additional error handling:
- Validate input parameters (translation, rotation, point)
- Add null checks for _pxGeometry and _pxShape
Apply this diff to add the necessary checks:
pointDistance(translation: Vector3, rotation: Quaternion, point: Vector3): Vector4 { + if (!translation || !rotation || !point) { + throw new Error("Invalid input parameters"); + } + if (!this._pxGeometry || !this._pxShape) { + throw new Error("PhysX geometry or shape not initialized"); + } const info = this._pxGeometry.pointDistance(this._pxShape.getGlobalPose(), point); const closestPoint = info.closestPoint; const res = PhysXColliderShape._tempVector4; res.set(closestPoint.x, closestPoint.y, closestPoint.z, info.distance); return res; }
🧹 Nitpick comments (7)
packages/physics-lite/src/shape/LiteBoxColliderShape.ts (1)
60-97: Consider documenting the return value formatThe method returns a Vector4 where (x,y,z) represents the closest point and w represents the distance. This should be documented in the method's JSDoc.
Add documentation:
/** * {@inheritDoc IColliderShape.pointDistance } + * @returns Vector4 where (x,y,z) is the closest point on the box to the input point, + * and w is the distance between the input point and the closest point. */packages/physics-lite/src/shape/LiteSphereColliderShape.ts (1)
47-49: Documentation needs improvementThe method documentation should include:
- Parameter descriptions
- Return value format explanation
- Edge case behavior description
/** * {@inheritDoc IColliderShape.pointDistance } + * @param position - The world position of the sphere center + * @param rotation - The world rotation of the sphere (unused for sphere shape) + * @param point - The point to calculate distance from + * @returns Vector4 where (x,y,z) is the closest point on sphere surface and w is the distance + * @remarks For points inside the sphere, the closest point will be on the surface */packages/physics-lite/src/shape/LiteColliderShape.ts (1)
95-98: Consider providing a default implementation for lite versionThe abstract pointDistance method forces all subclasses to implement it, but for the lite version, a default implementation returning infinity might be more appropriate.
-abstract pointDistance(position: Vector3, rotation: Quaternion, point: Vector3): IPointDistanceInfo; +pointDistance(position: Vector3, rotation: Quaternion, point: Vector3): IPointDistanceInfo { + console.warn("Physics-lite doesn't support pointDistance. Use Physics-PhysX instead!"); + return { + distance: Infinity, + closestPoint: new Vector3() + }; +}packages/core/src/physics/shape/ColliderShape.ts (1)
138-167: Consider enhancing method documentation.While the method documentation covers the basic parameters and return value, it would be helpful to add:
- A note about the -1 return value when the collider is inactive
- Examples of usage
- Edge cases and their handling
The implementation looks correct and follows the requirements from past review comments.
tests/src/core/physics/ColliderShape.test.ts (3)
254-287: Enhance test coverage for boxShape.getClosestPoint.While the basic functionality is tested, consider adding these test cases:
- Point exactly on box corners/edges
- Point inside the box
- Zero-size box
- Extreme scale values
- Different rotation angles (0, 90, 180 degrees)
Also applies to: 483-516
289-318: Enhance test coverage for sphereShape.getClosestPoint.Consider adding these test cases:
- Point exactly on sphere surface
- Point at sphere center
- Zero radius sphere
- Extreme scale values
Also applies to: 518-547
362-578: Consider extracting common test logic to reduce duplication.The test cases for PhysX and Lite are nearly identical. Consider:
- Creating shared test cases using a test factory pattern
- Only testing engine-specific behaviors separately
Example approach:
function createColliderShapeTests(engineType: 'PhysX' | 'Lite') { describe(`ColliderShape ${engineType}`, () => { // Common setup code // Shared test cases runBoxColliderTests(); runSphereColliderTests(); // Engine-specific tests if needed }); } createColliderShapeTests('PhysX'); createColliderShapeTests('Lite');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/core/src/physics/Collision.ts(1 hunks)packages/core/src/physics/shape/ColliderShape.ts(4 hunks)packages/core/src/physics/shape/PlaneColliderShape.ts(2 hunks)packages/design/src/physics/ICollision.ts(1 hunks)packages/design/src/physics/shape/IColliderShape.ts(2 hunks)packages/physics-lite/src/shape/LiteBoxColliderShape.ts(3 hunks)packages/physics-lite/src/shape/LiteColliderShape.ts(5 hunks)packages/physics-lite/src/shape/LiteSphereColliderShape.ts(2 hunks)packages/physics-physx/src/PhysXPhysics.ts(3 hunks)packages/physics-physx/src/shape/PhysXColliderShape.ts(4 hunks)tests/src/core/physics/ColliderShape.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/physics/shape/PlaneColliderShape.ts
- packages/design/src/physics/shape/IColliderShape.ts
- packages/physics-physx/src/PhysXPhysics.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/physics/Collision.ts
[error] 53-53: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (8)
packages/physics-lite/src/shape/LiteBoxColliderShape.ts (1)
79-88: LGTM: Correct implementation of point transformation and clamping
The affine transformation and point clamping logic is implemented correctly. The code properly:
- Transforms the point to local space
- Clamps it to the box bounds
- Transforms it back to world space
packages/physics-lite/src/shape/LiteSphereColliderShape.ts (2)
3-3: LGTM: Import changes are appropriate
The addition of Vector4 to the imports is necessary for the new pointDistance method's return type.
50-66: Verify collision detection system integration
The pointDistance method is crucial for collision detection. Let's verify its usage in the collision system.
✅ Verification successful
Implementation verified as correct and well-integrated
The pointDistance implementation in LiteSphereColliderShape is correctly integrated into the physics system:
- It follows the interface contract defined in
IColliderShape - The implementation is used properly in collision detection through
LitePhysicsScenewhich leveragesCollisionUtilfor sphere-box and sphere-sphere intersections - The method correctly handles both cases:
- When the point is on the sphere surface (distance = 0)
- When the point is away from the surface (returns closest point and squared distance)
- The implementation uses shared temporary vectors from the base class to avoid allocations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to pointDistance in collision detection code
rg -A 5 "pointDistance.*Vector4" --type typescript
# Look for test cases
rg -l "describe.*LiteSphereColliderShape" --type typescript
Length of output: 184
Script:
#!/bin/bash
# Search for references to pointDistance in collision detection code
rg -A 5 "pointDistance.*Vector4"
# Look for test cases
fd -e ts -e js "LiteSphereColliderShape.test"
# Search for usage of LiteSphereColliderShape
rg "LiteSphereColliderShape" -A 5
# Look for base class implementation
fd -e ts -e js "LiteColliderShape"
Length of output: 7684
Script:
#!/bin/bash
# Check the base class implementation and interface
cat packages/physics-lite/src/shape/LiteColliderShape.ts
cat packages/design/src/physics/shape/IColliderShape.ts
# Look for collision detection usage
rg "CollisionUtil" -A 5
Length of output: 37632
packages/core/src/physics/Collision.ts (1)
32-34:
Add null safety check for _nativeCollision
The contactCount getter should handle cases where _nativeCollision is undefined.
get contactCount(): number {
+ if (!this._nativeCollision) {
+ return 0;
+ }
return this._nativeCollision.contactCount;
}Likely invalid or redundant comment.
packages/physics-lite/src/shape/LiteColliderShape.ts (1)
43-46:
Use value equality check for rotation comparison
The current implementation uses reference equality (!==) which may miss updates when the values are equal but references are different.
setRotation(rotation: Vector3): void {
- if (rotation !== this._rotation) {
+ if (!Vector3.equals(rotation, this._rotation)) {
this._rotation.copyFrom(rotation);
Quaternion.rotationEuler(rotation.x, rotation.y, rotation.z, this._transform.rotationQuaternion);
}
}Likely invalid or redundant comment.
packages/core/src/physics/shape/ColliderShape.ts (1)
14-15: LGTM! Good use of static properties for temporary calculations.
Using static properties for temporary calculations helps avoid unnecessary memory allocations during runtime.
packages/physics-physx/src/shape/PhysXColliderShape.ts (2)
1-1: LGTM!
The Vector4 import is correctly added and necessary for the new functionality.
23-23: Consider thread safety implications of shared temporary vector.
While using a static temporary vector is a common optimization pattern, it could lead to race conditions if pointDistance is called concurrently from multiple threads. Consider documenting this limitation or making the vector instance-specific if thread safety is required.
✅ Verification successful
Static temporary vector usage appears safe in current context
The investigation shows that:
_tempVector4is only used within thepointDistancemethod to return results- While the codebase does use workers, they are confined to texture/model loading operations
- The physics module (
PhysXColliderShape) is not used within any worker contexts
The static temporary vector optimization is appropriate here as the physics calculations appear to run on the main thread only. The protected access modifier correctly prevents external modification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any concurrent/async usages of pointDistance
rg -l "async.*pointDistance|Promise.*pointDistance|Worker.*pointDistance|Thread.*pointDistance"
Length of output: 97
Script:
#!/bin/bash
# Let's first check where pointDistance is used and how _tempVector4 is accessed
rg -A 5 "_tempVector4" packages/physics-physx/src/shape/PhysXColliderShape.ts
# Also check for any worker/thread related imports or configurations in the project
rg -l "Worker|Thread|concurrent|parallel" --type ts
Length of output: 1023
Script:
#!/bin/bash
# Let's check if this physics module is used in any worker/thread context
rg -l "PhysXColliderShape" packages/loader/src/ktx2/WorkerPool.ts packages/loader/src/GLTFLoader.ts
# Also check how pointDistance is used
ast-grep --pattern 'pointDistance($$$)'
Length of output: 141
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/core/src/physics/Collision.ts (2)
8-17: Initialize the separation propertyThe
separationproperty should be initialized to prevent undefined behavior. Consider adding a default value or initializing it in the constructor.export class ContactPoint { readonly position = new Vector3(); readonly normal = new Vector3(); readonly impulse = new Vector3(); - separation: number; + separation: number = 0; }
26-27: Add documentation for shape propertyThe
shapeproperty lacks JSDoc documentation explaining its purpose and usage.+/** The ColliderShape instance involved in the collision. */ shape: ColliderShape;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/physics/Collision.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/physics/Collision.ts
[error] 52-52: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (3)
packages/core/src/physics/Collision.ts (3)
23-24: Add null safety for _nativeCollision property
The _nativeCollision property could be accessed before initialization.
/** @internal */
-_nativeCollision: ICollision;
+_nativeCollision?: ICollision;32-34: Add null safety check for contactCount getter
The getter should handle cases where _nativeCollision is undefined.
get contactCount(): number {
+ if (!this._nativeCollision) {
+ return 0;
+ }
return this._nativeCollision.contactCount;
}41-59:
Improve robustness and performance of getContacts method
Several improvements are needed:
- Add null safety check
- Clear the output array before use
- Avoid assignment in expression (static analysis warning)
- Consider performance optimizations
getContacts(outContacts: ContactPoint[]): ContactPoint[] {
+ if (!this._nativeCollision) {
+ outContacts.length = 0;
+ return outContacts;
+ }
const { shape0Id, shape1Id } = this._nativeCollision;
+ outContacts.length = 0;
const nativeContactPoints = this._nativeCollision.getContacts();
for (let i = 0, n = nativeContactPoints.size(); i < n; i++) {
const nativeContractPoint = nativeContactPoints.get(i);
const { position, normal, impulse, separation } = nativeContractPoint;
let factor = shape0Id > shape1Id ? -1 : 1;
- const contact = (outContacts[i] ||= new ContactPoint());
+ let contact = outContacts[i];
+ if (!contact) {
+ contact = new ContactPoint();
+ outContacts[i] = contact;
+ }
contact.position.set(position.x, position.y, position.z);
contact.normal.set(normal.x, normal.y, normal.z).scale(factor);
contact.impulse.set(impulse.x, impulse.y, impulse.z).scale(factor);
contact.separation = separation;
}
return outContacts;
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/core/src/physics/Collision.ts (2)
9-10:⚠️ Potential issueAdd null safety for _nativeCollision
The
_nativeCollisionproperty should be properly initialized and type-guarded.- _nativeCollision: ICollision; + private _nativeCollision?: ICollision; + + constructor(nativeCollision: ICollision) { + this._nativeCollision = nativeCollision; + }
18-20:⚠️ Potential issueAdd null check in contactCount getter
The getter should handle cases where
_nativeCollisionis undefined.get contactCount(): number { + if (!this._nativeCollision) { + return 0; + } return this._nativeCollision.contactCount; }
🧹 Nitpick comments (6)
packages/physics-physx/src/shape/PhysXColliderShape.ts (2)
1-1: Consider thread-safety implications of static temporary vector.While using static temporary vectors is a common optimization pattern to avoid allocations, it can cause issues in concurrent scenarios if multiple operations use it simultaneously. Consider documenting this limitation or using a non-static approach if concurrent access is possible.
Also applies to: 23-23
129-138: Consider making the API more explicit.While using Vector4 to combine the closest point (xyz) and distance (w) is space-efficient, it makes the API less explicit and could be confusing. Consider:
- Using a dedicated return type that clearly separates point and distance
- Adding JSDoc to clarify that w component represents distance
Example improvement:
/** * Calculates the closest point on the shape to the given point. * @param point The point to check against * @returns Vector4 where (x,y,z) is the closest point and w is the distance */ pointDistance(point: Vector3): Vector4 { const info = this._pxGeometry.pointDistance(this._pxShape.getGlobalPose(), point); const closestPoint = info.closestPoint; const res = PhysXColliderShape._tempVector4; res.set(closestPoint.x, closestPoint.y, closestPoint.z, info.distance); return res; }Or better yet:
interface PointDistanceResult { closestPoint: Vector3; distance: number; } pointDistance(point: Vector3): PointDistanceResult { const info = this._pxGeometry.pointDistance(this._pxShape.getGlobalPose(), point); return { closestPoint: new Vector3(info.closestPoint.x, info.closestPoint.y, info.closestPoint.z), distance: info.distance }; }packages/physics-lite/src/shape/LiteBoxColliderShape.ts (2)
84-92: Consider optimizing matrix transformationsThe current implementation performs two matrix transformations:
- Transform point to local space
- Transform point back to world space
Consider combining these transformations into a single matrix operation to improve performance.
- Matrix.affineTransformation(scale, rotation, position, m); - Matrix.invert(m, invM); - Vector3.transformCoordinate(point, invM, p); + // Compute combined transformation matrix once + Matrix.affineTransformation(scale, rotation, position, m); + Matrix.invert(m, invM); + + // Transform point to local space + Vector3.transformCoordinate(point, invM, p); + const min = boundingBox.min; const max = boundingBox.max; p.x = Math.max(min.x, Math.min(p.x, max.x)); p.y = Math.max(min.y, Math.min(p.y, max.y)); p.z = Math.max(min.z, Math.min(p.z, max.z)); - Vector3.transformCoordinate(p, m, p); + + // Transform back using original matrix + Vector3.transformCoordinate(p, m, p);
60-62: Consider API naming consistencyAs noted in the PR comments, there's a discrepancy between this API name (
pointDistance) and the physics API (getDistanceAndClosestPointFromPoint). While the current name follows PhysX conventions, consider:
- Adding a documentation note explaining the naming choice
- Providing an alias method for API consistency
packages/core/src/physics/shape/ColliderShape.ts (1)
3-3: Remove unused Matrix importThe
Matrixtype is imported but not used in the changes. Consider removing it to keep the imports clean.-import { MathUtil, Matrix, Quaternion, Vector3 } from "@galacean/engine-math"; +import { MathUtil, Quaternion, Vector3 } from "@galacean/engine-math";Also applies to: 14-15
packages/core/src/physics/ContactPoint.ts (1)
6-14: Consider adding constructor and validationConsider these improvements:
- Add a constructor to initialize vectors and separation
- Add validation for the separation value
export class ContactPoint { + constructor() { + this.separation = 0; + } + readonly position = new Vector3(); readonly normal = new Vector3(); readonly impulse = new Vector3(); - separation: number; + private _separation = 0; + + get separation(): number { + return this._separation; + } + + set separation(value: number) { + if (!Number.isFinite(value)) { + throw new Error("Separation must be a finite number"); + } + this._separation = value; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/core/src/physics/Collision.ts(1 hunks)packages/core/src/physics/ContactPoint.ts(1 hunks)packages/core/src/physics/index.ts(1 hunks)packages/core/src/physics/shape/ColliderShape.ts(4 hunks)packages/design/src/physics/ICollision.ts(1 hunks)packages/design/src/physics/shape/IColliderShape.ts(2 hunks)packages/physics-lite/src/shape/LiteBoxColliderShape.ts(3 hunks)packages/physics-lite/src/shape/LiteColliderShape.ts(5 hunks)packages/physics-lite/src/shape/LiteSphereColliderShape.ts(2 hunks)packages/physics-physx/src/shape/PhysXColliderShape.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/design/src/physics/shape/IColliderShape.ts
- packages/physics-lite/src/shape/LiteSphereColliderShape.ts
- packages/design/src/physics/ICollision.ts
- packages/physics-lite/src/shape/LiteColliderShape.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/physics/Collision.ts
[error] 38-38: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (9)
packages/physics-physx/src/shape/PhysXColliderShape.ts (2)
62-62: Verify rotation order consistency across codebase.
The parameter order change in rotationYawPitchRoll from (x,y,z) to (y,x,z) aligns with the core math implementation, but requires verification across the codebase.
132-138: Add error handling for PhysX objects.
The method should check if _pxGeometry and _pxShape are initialized before use.
packages/physics-lite/src/shape/LiteBoxColliderShape.ts (1)
12-13: Consider thread safety implications of static variables
Using static mutable variables _tempMatrix and _tempInvMatrix without synchronization can lead to data races in multithreaded environments.
packages/core/src/physics/shape/ColliderShape.ts (2)
85-85: LGTM! Good documentation improvement
The clarification about rotation being in radians is helpful for API users.
144-160: 🛠️ Refactor suggestion
Improve distance handling and error messaging
A few suggestions to enhance the implementation:
- The warning message should be more specific about why the operation failed
- The distance handling needs improvement for negative cases
getClosestPoint(point: Vector3, outClosestPoint: Vector3): number {
const collider = this._collider;
if (collider.enabled === false || collider.entity._isActiveInHierarchy === false) {
- console.warn("The collider is not active in scene.");
+ console.warn("Cannot get closest point: collider is disabled or entity is inactive in hierarchy.");
return -1;
}
const res = this._nativeShape.pointDistance(point);
const distance = res.w;
if (distance > 0) {
outClosestPoint.set(res.x, res.y, res.z);
} else {
outClosestPoint.copyFrom(point);
}
- return Math.sqrt(distance);
+ return distance > 0 ? Math.sqrt(distance) : distance;
}Let's verify the usage of this method across test files:
packages/core/src/physics/index.ts (1)
8-9: LGTM! New exports are properly organized
The new exports for Collision and ContactPoint are correctly placed and align with the physics module structure.
packages/core/src/physics/ContactPoint.ts (1)
6-14: LGTM! Well-designed contact point structure
The ContactPoint class is well-structured with proper readonly protection for vector properties and clear documentation.
packages/core/src/physics/Collision.ts (2)
29-45:
Improve robustness and performance of getContacts
Several improvements needed for the getContacts method:
- Add null safety check
- Avoid assignment in expression (flagged by static analysis)
- Consider pre-allocating contact points
getContacts(outContacts: ContactPoint[]): number {
+ if (!this._nativeCollision) {
+ return 0;
+ }
+
const { shape0Id, shape1Id } = this._nativeCollision;
const factor = shape0Id < shape1Id ? 1 : -1;
const nativeContactPoints = this._nativeCollision.getContacts();
const length = nativeContactPoints.size();
for (let i = 0; i < length; i++) {
const nativeContractPoint = nativeContactPoints.get(i);
- const contact = (outContacts[i] ||= new ContactPoint());
+ let contact = outContacts[i];
+ if (!contact) {
+ contact = new ContactPoint();
+ outContacts[i] = contact;
+ }
contact.position.copyFrom(nativeContractPoint.position);
contact.normal.copyFrom(nativeContractPoint.normal).scale(factor);
contact.impulse.copyFrom(nativeContractPoint.impulse).scale(factor);
contact.separation = nativeContractPoint.separation;
}
return length;
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
29-45: Verify contact point calculations
The scaling of normal and impulse vectors based on shape IDs needs verification to ensure correct physics behavior.
✅ Verification successful
Let me gather more context about the physics collision handling to better understand the vector scaling pattern.
Let me check the test files to verify the correctness of this scaling behavior.
Contact point scaling implementation is correct
The scaling implementation is verified to be correct based on the following evidence:
- The documentation in
ContactPoint.tsexplicitly states: "The normal direction points from the second shape to the first shape" - The test case in
Collision.test.tsverifies the expected behavior of contact points, including normal and impulse values - The factor scaling (1 or -1) based on shape IDs ensures consistent directionality of contact normals regardless of the order of the colliding shapes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for similar vector scaling patterns in physics calculations
# Look for similar vector scaling patterns in physics calculations
rg -A 5 'scale\(.*factor\)' --type ts
# Look for shape ID comparisons influencing calculations
ast-grep --pattern 'if ($shape0Id $_comparison $shape1Id) {
$$$
}'
Length of output: 621
Script:
#!/bin/bash
# Search for other collision-related files and implementations
fd collision -t f -i
# Look for contact point handling in physics engine
rg -i "contact.*point" --type ts -A 5
# Search for shape ID usage patterns
rg "shape.*Id" --type ts -A 3
# Look for vector scaling in physics calculations
ast-grep --pattern 'scale($factor)'
Length of output: 8181
Script:
#!/bin/bash
# Look for collision tests
cat tests/src/core/physics/Collision.test.ts
# Look for any documentation about collision handling
cat packages/core/src/physics/README.md 2>/dev/null
cat packages/core/README.md 2>/dev/null
# Search for comments about contact normal direction
rg -i "normal.*direction" --type ts
Length of output: 3681
🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/core/src/physics/Collision.ts (1)
18-20:⚠️ Potential issueAdd null safety check for contactCount getter
The getter should check for null/undefined _nativeCollision to prevent runtime errors.
get contactCount(): number { + if (!this._nativeCollision) { + return 0; + } return this._nativeCollision.contactCount; }packages/physics-lite/src/shape/LiteColliderShape.ts (1)
46-49:⚠️ Potential issueFix reference comparison in setRotation method
The current implementation uses reference equality (
!==) instead of value equality, which can miss updates when the values are equal but references differ.Additionally, consider caching the quaternion calculation to avoid unnecessary computations when the rotation hasn't actually changed:
setRotation(rotation: Vector3): void { - if (rotation !== this._rotation) { + if (!this._rotation.equals(rotation)) { this._rotation.copyFrom(rotation); Quaternion.rotationEuler(rotation.x, rotation.y, rotation.z, this._transform.rotationQuaternion); } }
🧹 Nitpick comments (1)
packages/physics-lite/src/shape/LiteColliderShape.ts (1)
35-35: Add documentation for the rotation propertyPlease add JSDoc comments to clarify:
- The purpose of this rotation property
- The units used (radians or degrees)
- Any constraints or valid ranges for the values
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/physics/Collision.ts(1 hunks)packages/core/src/physics/shape/ColliderShape.ts(2 hunks)packages/physics-lite/src/shape/LiteColliderShape.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/physics/shape/ColliderShape.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/physics/Collision.ts
[error] 39-39: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (2)
packages/core/src/physics/Collision.ts (1)
9-10:
Add proper initialization for _nativeCollision
The _nativeCollision property could be accessed before initialization, potentially causing runtime errors.
Consider one of these approaches:
- Add a constructor:
/** @internal */
-_nativeCollision: ICollision;
+_nativeCollision: ICollision;
+
+constructor(nativeCollision: ICollision) {
+ this._nativeCollision = nativeCollision;
+}- Or make it optional with runtime checks:
/** @internal */
-_nativeCollision: ICollision;
+_nativeCollision?: ICollision;Likely invalid or redundant comment.
packages/physics-lite/src/shape/LiteColliderShape.ts (1)
1-1: LGTM! Well-structured temporary calculation properties
The addition of static properties for temporary calculations is a good optimization to avoid unnecessary object creation during physics calculations.
Also applies to: 12-16
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
ContactPointclass for structured collision data.getClosestPointmethod toColliderShapeand its implementations for distance calculations.ICollisioninterface to encapsulate collision event data.ICollisionparameter.pointDistancemethod toLiteBoxColliderShapeandLiteSphereColliderShapefor distance calculations.pointDistancemethod inLiteDynamicColliderfor improved distance calculations.CharacterControllerto includeignoreClonefunctionality.Bug Fixes
LiteDynamicColliderby replacingconsole.errorwithLogger.error.Tests
ColliderShapein bothLitePhysicsandPhysXcontexts.