Opt physics experience and opt physics unit test #2420
Conversation
WalkthroughThe pull request introduces extensive modifications across various classes and interfaces within the physics engine. Key changes include the addition of new properties and methods, updates to existing property management, and a shift from cloning to synchronization of properties with native counterparts. Several methods related to collider shapes have been removed or refactored, enhancing encapsulation and simplifying the management of shapes within the physics scene. The test suites have been significantly expanded and refined to cover new functionalities and ensure robust testing of the physics components. Changes
Suggested reviewers
Poem
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 #2420 +/- ##
===========================================
- Coverage 67.22% 67.10% -0.13%
===========================================
Files 895 897 +2
Lines 92094 93218 +1124
Branches 7309 7576 +267
===========================================
+ Hits 61912 62550 +638
- Misses 29937 30423 +486
Partials 245 245
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: 23
🧹 Outside diff range and nitpick comments (53)
packages/core/src/physics/joint/FixedJoint.ts (1)
9-12: Add error handling for missing collider component.The refactoring to
_createJointimproves the class structure and aligns with other joint implementations. However, consider adding error handling for cases where the collider component is missing.Consider this safer implementation:
protected _createJoint(): void { const colliderInfo = this._colliderInfo; colliderInfo.collider = this.entity.getComponent(Collider); + if (!colliderInfo.collider) { + throw new Error("FixedJoint requires a Collider component on the entity"); + } this._nativeJoint = PhysicsScene._nativePhysics.createFixedJoint(colliderInfo.collider._nativeCollider); + if (!this._nativeJoint) { + throw new Error("Failed to create native fixed joint"); + } }packages/core/src/physics/joint/JointLimits.ts (2)
9-11: Enhance contactDistance documentation with validation rulesWhile the default value calculation is now documented, the documentation could be improved by:
- Explaining why these specific default values were chosen
- Documenting any validation rules for custom values
- Providing example calculations
Consider expanding the documentation:
/** Distance inside the limit value at which the limit will be considered to be active by the solver. * Default is the lesser of 0.1 radians, and 0.49 * (upperLimit - lowerLimit) + * @example + * // For limits between 0 and π/2 radians: + * // contactDistance = min(0.1, 0.49 * (π/2 - 0)) ≈ 0.1 + * @remarks + * The 0.49 factor ensures stable constraint solving by preventing the contact + * distance from being too close to the total range. */
Line range hint
1-18: Consider adding validation in the constructor/settersThe class could benefit from runtime validation to ensure proper usage of the new radian-based limits.
Consider adding validation:
export class JointLimits { + private static readonly MAX_CONTACT_DISTANCE = Math.PI * 2; + /** The upper angular limit (in radians) of the joint. */ - max: number = 0; + private _max: number = 0; + get max(): number { return this._max; } + set max(value: number) { + if (value < -Math.PI * 2 || value > Math.PI * 2) { + console.warn('Joint limit outside expected radian range [-2π, 2π]'); + } + this._max = value; + } /** The lower angular limit (in radians) of the joint. */ - min: number = 0; + private _min: number = 0; + get min(): number { return this._min; } + set min(value: number) { + if (value < -Math.PI * 2 || value > Math.PI * 2) { + console.warn('Joint limit outside expected radian range [-2π, 2π]'); + } + if (value > this._max) { + throw new Error('Lower limit cannot be greater than upper limit'); + } + this._min = value; + } /** Distance inside the limit value at which the limit will be considered to be active by the solver. * Default is the lesser of 0.1 radians, and 0.49 * (upperLimit - lowerLimit) */ - contactDistance: number = -1; + private _contactDistance: number = -1; + get contactDistance(): number { + if (this._contactDistance === -1) { + return Math.min(0.1, 0.49 * (this._max - this._min)); + } + return this._contactDistance; + } + set contactDistance(value: number) { + if (value < 0) { + throw new Error('Contact distance must be non-negative'); + } + if (value > JointLimits.MAX_CONTACT_DISTANCE) { + console.warn('Large contact distance may impact performance'); + } + this._contactDistance = value; + }packages/physics-physx/src/joint/PhysXFixedJoint.ts (1)
Line range hint
8-22: Documentation and test coverage needed.Given this is a significant behavioral change in joint initialization:
- The class documentation should be updated to clarify the joint's attachment behavior (whether it's between two colliders or to the world)
- Unit tests should be added to verify both attachment scenarios
Would you like me to:
- Generate improved documentation that clarifies the joint behavior?
- Create unit tests covering both attachment scenarios?
packages/core/src/physics/shape/BoxColliderShape.ts (1)
36-36: Consider using class fields with arrow functions for auto-binding.While the current binding approach works, using class fields with arrow functions could provide a cleaner and more maintainable solution that automatically preserves the 'this' context.
Consider refactoring to:
-private _setSize(): void { +private _setSize = (): void => { (<IBoxColliderShape>this._nativeShape).setSize(this._size); } constructor() { super(); this._nativeShape = PhysicsScene._nativePhysics.createBoxColliderShape( this._id, this._size, this._material._nativeMaterial ); - this._size._onValueChanged = this._setSize.bind(this); + this._size._onValueChanged = this._setSize; }This approach:
- Eliminates the need for explicit binding
- Ensures the method is always bound correctly
- Reduces the chance of binding-related bugs
packages/design/src/physics/joints/IJoint.ts (4)
19-20: Add JSDoc documentation for the setAnchor method.The method lacks documentation to explain its purpose and behavior. Please add JSDoc comments similar to
setConnectedAnchorto clarify:
- The purpose of this anchor point
- Whether it's in world or local space
- How it relates to
connectedAnchor+ /** + * The anchor position. + * @remarks Specify whether this is in world space or local space, and how it differs from connectedAnchor. + */ setAnchor(value: Vector3): void;
27-29: Consider clarifying the naming pattern for mass scale methods.The current naming pattern might be confusing:
setConnectedMassScaleaffects collider 0setMassScaleaffects collider 1This seems counterintuitive as "connected" typically implies the secondary entity. Consider either:
- Swapping the collider numbers to match the naming
- Renaming the methods to better reflect which collider they affect
50-51: Add comprehensive documentation for the destroy method.This cleanup method requires documentation to specify:
- When it should be called
- What resources it cleans up
- Whether it's safe to call multiple times
- Any side effects or interactions with other joint properties
+ /** + * Cleans up resources associated with this joint. + * @remarks Document when this should be called, what it cleans up, and whether it's safe for multiple calls. + */ destroy(): void;
Line range hint
1-51: Consider adding corresponding getter methods.The interface currently only provides setter methods. Consider adding corresponding getters for properties like:
getAnchor(): Vector3getConnectedAnchor(): Vector3getMassScale(): number- etc.
This would make it easier to:
- Read and validate joint configurations
- Debug joint behavior
- Implement features that need to query joint state
packages/design/src/physics/ICharacterController.ts (1)
52-55: Enhance documentation with more details about contact offset.The JSDoc should explain:
- The purpose of contact offset in character controller physics
- The valid range of values (if any)
- The impact on physics simulation
- Default value (if any)
Example enhancement:
/** * Sets the contact offset. + * Contact offset determines the distance at which the character controller starts + * detecting potential collisions. A larger value provides earlier collision detection + * but may impact performance. * @param offset The new contact offset for the controller. + * @remarks Typical values range from 0.01 to 0.1. Default is 0.02. */packages/core/src/physics/PhysicsMaterial.ts (1)
30-32: Enhance property documentationWhile specifying the range is helpful, the documentation could be more comprehensive to help users choose appropriate values.
Consider expanding the documentation:
/** - * The coefficient of bounciness, ranging from 0 to 1. + * The coefficient of bounciness, ranging from 0 to 1. + * @default 0 + * @remarks + * - 0: No bounce (perfectly inelastic collision) + * - 1: Perfect bounce (perfectly elastic collision) + * - Common values: 0.3-0.5 for rubber, 0.7-0.9 for bouncy materials */packages/core/src/physics/joint/SpringJoint.ts (1)
93-99: Optimize type casting and add null check.The repeated type casting to
ISpringJointcan be optimized, and a null check should be added for the native joint.Consider this optimization:
protected override _syncBackends(): void { super._syncBackends(); + if (!this._nativeJoint) { + return; + } + const nativeJoint = <ISpringJoint>this._nativeJoint; - (<ISpringJoint>this._nativeJoint).setMinDistance(this._minDistance); - (<ISpringJoint>this._nativeJoint).setMaxDistance(this._maxDistance); - (<ISpringJoint>this._nativeJoint).setTolerance(this._tolerance); - (<ISpringJoint>this._nativeJoint).setStiffness(this._stiffness); - (<ISpringJoint>this._nativeJoint).setDamping(this._damping); + nativeJoint.setMinDistance(this._minDistance); + nativeJoint.setMaxDistance(this._maxDistance); + nativeJoint.setTolerance(this._tolerance); + nativeJoint.setStiffness(this._stiffness); + nativeJoint.setDamping(this._damping); }packages/physics-physx/src/joint/PhysXJoint.ts (2)
34-42: Enhance documentation for anchor setting methods.While the implementation is correct, the documentation could be more descriptive about:
- The coordinate space used (local vs. world)
- The effect of setting anchors on joint behavior
- The relationship between the two anchor points
/** - * {@inheritDoc IJoint.setConnectedAnchor } + * Sets the anchor point for the first body in its local coordinate space. + * This point defines where the joint is attached to the first body. + * @param value The anchor point in the first body's local space */
89-91: Enhance resource cleanup in destroy method.To prevent memory leaks and dangling references, consider clearing all internal references after releasing the joint.
destroy(): void { this._pxJoint.release(); + this._pxJoint = null; + this._collider = null; }packages/physics-physx/src/joint/PhysXHingeJoint.ts (3)
11-14: LGTM! Consider making _xAxis readonly for safety.The new properties improve encapsulation by maintaining local state. The static _xAxis is appropriately shared across instances.
Consider making _xAxis readonly to prevent accidental modifications:
- protected static _xAxis = new Vector3(1, 0, 0); + protected static readonly _xAxis = new Vector3(1, 0, 0);
34-43: LGTM! Consider caching intermediate calculations.The axis calculation and anchor updates are correct. However, the cross product and angle calculations could be cached if setAxis is called frequently.
Consider caching intermediate calculations:
setAxis(value: Vector3): void { const xAxis = PhysXHingeJoint._xAxis; const axisRotationQuaternion = this._axisRotationQuaternion; + const normalizedValue = value.clone().normalize(); - xAxis.set(1, 0, 0); - value.normalize(); - const angle = Math.acos(Vector3.dot(xAxis, value)); - Vector3.cross(xAxis, value, xAxis); + const angle = Math.acos(Vector3.dot(xAxis, normalizedValue)); + const crossResult = Vector3.cross(xAxis, normalizedValue, new Vector3()); - Quaternion.rotationAxisAngle(xAxis, angle, axisRotationQuaternion); + Quaternion.rotationAxisAngle(crossResult, angle, axisRotationQuaternion); this._setLocalPose(0, this._anchor, axisRotationQuaternion); this._setLocalPose(1, this._connectedAnchor, axisRotationQuaternion); }
45-52: LGTM! Add JSDoc comments for overridden methods.The anchor methods correctly maintain local state and update poses. Consider adding documentation for clarity.
Add JSDoc comments to document the overridden behavior:
+ /** + * Sets the anchor point relative to the first actor. + * @param value The anchor point in world space. + */ override setAnchor(value: Vector3): void { this._setLocalPose(0, value, this._axisRotationQuaternion); this._anchor.copyFrom(value); } + /** + * Sets the anchor point relative to the second actor. + * @param value The anchor point in world space. + */ override setConnectedAnchor(value: Vector3): void { this._setLocalPose(1, value, this._axisRotationQuaternion); this._connectedAnchor.copyFrom(value); }packages/design/src/physics/IDynamicCollider.ts (3)
22-23: Add JSDoc comments for damping getter methods.The getter methods
getLinearDamping()andgetAngularDamping()should have JSDoc comments for consistency with their corresponding setter methods.Apply these additions:
+ /** + * Gets the linear damping coefficient. + * @returns Linear damping coefficient. + */ getLinearDamping(): number; + /** + * Gets the angular damping coefficient. + * @returns Angular damping coefficient. + */ getAngularDamping(): number;Also applies to: 30-30
49-50: Add JSDoc comment for angular velocity getter method.The
getAngularVelocity()method should have JSDoc comments for consistency withgetLinearVelocity().Apply this addition:
+ /** + * Get the angular velocity of the actor. + * @param out - The angular velocity of the actor. + */ getAngularVelocity(out: Vector3): Vector3;
98-99: Add JSDoc comments for sleep-related getter methods.The sleep-related getter methods should have documentation for consistency and clarity.
Apply these additions:
+ /** + * Gets the mass-normalized kinetic energy threshold below which an actor may go to sleep. + * @returns Energy threshold for sleeping. + */ getSleepThreshold(): number; + /** + * Checks if the dynamic collider is currently sleeping. + * @returns True if the collider is sleeping, false otherwise. + */ isSleeping(): boolean;Also applies to: 148-149
packages/core/src/physics/Collider.ts (1)
145-148: Consider improving method name and adding error handling.While centralizing shape synchronization is good, consider these improvements:
- The method name
_syncBackendsis vague. Consider_syncShapesToPhysicsBackendto better describe its purpose.- Add error handling for invalid shapes or failed synchronization attempts.
- protected _syncBackends(): void { + protected _syncShapesToPhysicsBackend(): void { for (let i = 0, n = this.shapes.length; i < n; i++) { + if (!this.shapes[i]) { + console.warn(`Invalid shape at index ${i}`); + continue; + } this._addPhysicsShape(this.shapes[i]); } }packages/core/src/physics/CharacterController.ts (3)
19-33: Add validation for contact offset value.The contact offset implementation looks good, but consider adding validation to prevent negative or zero values which could cause physics simulation issues.
set contactOffset(value: number) { + if (value <= 0) { + throw new Error("Contact offset must be greater than 0"); + } if (this._contactOffset !== value) { this._contactOffset = value; (<ICharacterController>this._nativeCollider).setContactOffset(value); } }
17-18: Use Math.SQRT1_2 for better precision.Instead of the approximate value 0.707, consider using
Math.SQRT1_2which provides the exact value for cos(45°).- private _slopeLimit: number = 0.707; + private _slopeLimit: number = Math.SQRT1_2;🧰 Tools
🪛 Biome
[error] 18-18: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
Line range hint
13-194: Consider performance optimization opportunities.While the code structure is solid, there might be optimization opportunities:
- Cache frequently accessed values like
this._nativeCollider- Consider using object pooling for Vector3 instances in frequently called methods
- Profile the impact of constant physics updates, especially in
_onUpdateand_onLateUpdateWould you like me to elaborate on any of these optimization strategies?
🧰 Tools
🪛 Biome
[error] 18-18: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
packages/physics-lite/src/LiteDynamicCollider.ts (1)
Line range hint
21-189: Add unit tests for new methods.Static analysis indicates that many new methods lack test coverage. Even though these are stub implementations, it's important to verify their behavior, especially:
- Methods that return default values
- Error handling in getter methods
- Console error logging in setter methods
Would you like me to help generate unit tests for these methods? Here's an example of what they might look like:
describe('LiteDynamicCollider', () => { it('should return input vector for getInertiaTensor', () => { const collider = new LiteDynamicCollider(new Vector3(), new Quaternion()); const input = new Vector3(1, 2, 3); expect(collider.getInertiaTensor(input)).toBe(input); }); it('should throw error for getAngularDamping', () => { const collider = new LiteDynamicCollider(new Vector3(), new Quaternion()); expect(() => collider.getAngularDamping()).toThrow(); }); });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 65-66: packages/physics-lite/src/LiteDynamicCollider.ts#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 69-70: packages/physics-lite/src/LiteDynamicCollider.ts#L69-L70
Added lines #L69 - L70 were not covered by tests
[warning] 80-81: packages/physics-lite/src/LiteDynamicCollider.ts#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 125-126: packages/physics-lite/src/LiteDynamicCollider.ts#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 135-135: packages/physics-lite/src/LiteDynamicCollider.ts#L135
Added line #L135 was not covered by tests
[warning] 153-153: packages/physics-lite/src/LiteDynamicCollider.ts#L153
Added line #L153 was not covered by tests
[warning] 164-164: packages/physics-lite/src/LiteDynamicCollider.ts#L164
Added line #L164 was not covered by tests
[warning] 182-182: packages/physics-lite/src/LiteDynamicCollider.ts#L182
Added line #L182 was not covered by testspackages/physics-physx/src/PhysXDynamicCollider.ts (1)
Line range hint
160-179: Consider optimizing the collision detection mode implementation.The current implementation has some inconsistencies and potential improvements:
- Inconsistent usage of
physXvsthis._physXPhysics._physX- Repeated access to flag properties
Consider this refactor for better maintainability:
setCollisionDetectionMode(value: number): void { const physX = this._physXPhysics._physX; + const flags = physX.PxRigidBodyFlag; switch (value) { case CollisionDetectionMode.Continuous: - this._pxActor.setRigidBodyFlag(this._physXPhysics._physX.PxRigidBodyFlag.eENABLE_CCD, true); + this._pxActor.setRigidBodyFlag(flags.eENABLE_CCD, true); break; case CollisionDetectionMode.ContinuousDynamic: - this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, false); - this._pxActor.setRigidBodyFlag(this._physXPhysics._physX.PxRigidBodyFlag.eENABLE_CCD_FRICTION, true); + this._pxActor.setRigidBodyFlag(flags.eENABLE_CCD, false); + this._pxActor.setRigidBodyFlag(flags.eENABLE_CCD_FRICTION, true); break; case CollisionDetectionMode.ContinuousSpeculative: - this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, false); - this._pxActor.setRigidBodyFlag(this._physXPhysics._physX.PxRigidBodyFlag.eENABLE_SPECULATIVE_CCD, true); + this._pxActor.setRigidBodyFlag(flags.eENABLE_CCD, false); + this._pxActor.setRigidBodyFlag(flags.eENABLE_SPECULATIVE_CCD, true); break; case CollisionDetectionMode.Discrete: - this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD, false); - this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_CCD_FRICTION, false); - this._pxActor.setRigidBodyFlag(physX.PxRigidBodyFlag.eENABLE_SPECULATIVE_CCD, false); + this._pxActor.setRigidBodyFlag(flags.eENABLE_CCD, false); + this._pxActor.setRigidBodyFlag(flags.eENABLE_CCD_FRICTION, false); + this._pxActor.setRigidBodyFlag(flags.eENABLE_SPECULATIVE_CCD, false); break; } }tests/src/core/physics/SpringJoint.test.ts (3)
11-22: Enhance helper function type safety and maintainabilityConsider these improvements to the
addBoxhelper function:
- Use a more specific type for
cubeSizeinstead ofVector3- Extract friction values to named constants
- Add parameter validation
+ const ZERO_FRICTION = 0; + + interface BoxDimensions { + width: number; + height: number; + depth: number; + } + - function addBox(cubeSize: Vector3, type: typeof DynamicCollider | typeof StaticCollider, pos: Vector3) { + function addBox(dimensions: BoxDimensions, type: typeof DynamicCollider | typeof StaticCollider, pos: Vector3) { + if (dimensions.width <= 0 || dimensions.height <= 0 || dimensions.depth <= 0) { + throw new Error('Box dimensions must be positive numbers'); + } + 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 = ZERO_FRICTION; + physicsBox.material.staticFriction = ZERO_FRICTION; - physicsBox.size = cubeSize; + physicsBox.size = new Vector3(dimensions.width, dimensions.height, dimensions.depth); const boxCollider = boxEntity.addComponent(type); boxCollider.addShape(physicsBox); return boxEntity; }
35-192: Reduce code duplication in test setupEach test case contains similar setup code. Consider extracting common setup logic to beforeEach or helper methods.
interface SpringJointTestSetup { boxEntity1: Entity; boxEntity2: Entity; joint: SpringJoint; collider1: DynamicCollider; collider2: DynamicCollider; } function setupSpringJointTest(config: Partial<{ stiffness: number; damping: number; minDistance: number; maxDistance: number; tolerance: number; }> = {}): SpringJointTestSetup { const boxEntity1 = addBox({ width: 1, height: 1, depth: 1 }, DynamicCollider, new Vector3(0, 5, 0)); const boxEntity2 = addBox({ width: 1, height: 1, depth: 1 }, DynamicCollider, new Vector3(2, 5, 0)); const joint = boxEntity2.addComponent(SpringJoint); const collider1 = boxEntity1.getComponent(DynamicCollider); const collider2 = boxEntity2.getComponent(DynamicCollider); joint.connectedCollider = collider1; collider1.isKinematic = true; joint.autoConnectedAnchor = false; joint.anchor = new Vector3(-0.5, 0, 0); joint.connectedAnchor = new Vector3(0.5, 0, 0); // Apply defaults or custom config joint.stiffness = config.stiffness ?? 10000; joint.damping = config.damping ?? 1000; joint.minDistance = config.minDistance ?? 1; joint.maxDistance = config.maxDistance ?? 5; joint.tolerance = config.tolerance ?? 0; return { boxEntity1, boxEntity2, joint, collider1, collider2 }; }🧰 Tools
🪛 Biome
[error] 41-41: 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)
[error] 71-71: 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)
[error] 94-94: 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)
[error] 124-124: 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)
[error] 147-147: 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)
[error] 169-169: 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)
35-192: Enhance test coverage with edge cases and error scenariosWhile the current tests cover the basic functionality, consider adding:
- Error scenarios (invalid property values)
- Edge cases (zero/negative values)
- More descriptive assertion messages
Example additional test cases:
it("should throw error for invalid property values", () => { const { joint } = setupSpringJointTest(); expect(() => { joint.stiffness = -1; }).toThrow(); expect(() => { joint.damping = -1; }).toThrow(); expect(() => { joint.minDistance = -1; }).toThrow(); }); it("should handle zero stiffness", () => { const { joint, boxEntity2, collider2 } = setupSpringJointTest({ stiffness: 0 }); collider2.applyForce(new Vector3(100, 0, 0)); updatePhysics(engine.sceneManager.activeScene, 1); // Expect no constraint forces expect(boxEntity2.transform.position.x).toBeGreaterThan(2); });Also consider adding more descriptive messages to existing assertions:
- expect(boxEntity2.transform.position.x).eq(2); + expect(boxEntity2.transform.position.x).eq(2, "Box should maintain its position when within tolerance");🧰 Tools
🪛 Biome
[error] 41-41: 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)
[error] 71-71: 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)
[error] 94-94: 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)
[error] 124-124: 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)
[error] 147-147: 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)
[error] 169-169: 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)
tests/src/core/physics/HingeJoint.test.ts (6)
21-32: Document the purpose of zero friction settings.The friction values are set to 0 without explanation. Consider adding a comment explaining why this is necessary for the tests.
const physicsBox = new BoxColliderShape(); +// Set frictions to 0 to eliminate friction forces from affecting test results physicsBox.material.dynamicFriction = 0; physicsBox.material.staticFriction = 0;
34-36: Consider using a named constant for precision value.The magic number 100000 should be extracted into a named constant to explain its purpose.
+const PRECISION_FACTOR = 100000; // 5 decimal places precision for floating-point comparisons + function formatValue(value: number) { - return Math.round(value * 100000) / 100000; + return Math.round(value * PRECISION_FACTOR) / PRECISION_FACTOR; }
38-47: Add cleanup in afterAll hook.The test suite should clean up resources in an afterAll hook to prevent memory leaks.
beforeEach(function () { rootEntity.clearChildren(); engine.sceneManager.activeScene.physics.gravity = new Vector3(0, -9.81, 0); }); + +afterAll(async function () { + await engine.destroy(); +});
173-194: Fix typo in test name "gearRation".The test name contains a typo - "gearRation" should be "gearRatio".
-it("gearRation", function () { +it("gearRatio", function () {
49-231: Consider refactoring common test setup into shared fixtures.Each test has similar setup code for creating boxes and configuring the joint. Consider extracting this into a shared fixture or helper function to reduce duplication.
interface TestFixture { boxEntity1: Entity; boxEntity2: Entity; joint: HingeJoint; } function createBasicJointSetup(pos2: Vector3): TestFixture { const boxEntity1 = addBox(new Vector3(1, 1, 1), DynamicCollider, new Vector3(0, 5, 0)); const boxEntity2 = addBox(new Vector3(1, 1, 1), DynamicCollider, pos2); const collider1 = boxEntity1.getComponent(DynamicCollider); collider1.isKinematic = true; const joint = boxEntity1.addComponent(HingeJoint); joint.connectedCollider = boxEntity2.getComponent(DynamicCollider); joint.anchor = new Vector3(0.5, 0, 0); joint.axis = new Vector3(0, 1, 0); return { boxEntity1, boxEntity2, joint }; }
49-231: Consider using constants for common test values.Various magic numbers are used throughout the tests (e.g., torque values, motor velocities). Consider extracting these into named constants to improve maintainability and document their purpose.
const TEST_CONSTANTS = { TORQUE_FORCE: 1000, MOTOR_VELOCITY: 30, DEFAULT_STIFFNESS: 100, DEFAULT_DAMPING: 30, // ... other constants } as const;tests/src/core/physics/Collider.test.ts (2)
73-73: Consider documenting the reason for the bounciness value.The addition of
bounciness = 0.1aligns with the new_contactOffsetinCharacterController, but this relationship isn't documented. Consider adding a comment explaining why this specific value was chosen and its relationship to other physics properties.physicsBox.size = new Vector3(cubeSize, cubeSize, cubeSize); + // Set bounciness to 0.1 to match CharacterController._contactOffset for consistent collision behavior physicsBox.material.bounciness = 0.1;
Line range hint
1-270: Consider enhancing test coverage for material properties.While the test suite comprehensively covers collision scenarios, it could benefit from explicit tests for material properties like bounciness. Consider adding test cases that verify:
- Different bounciness values and their impact on collision response
- Interaction between bounciness and other material properties
- Edge cases (e.g., bounciness = 0, bounciness = 1)
Here's a suggested test case structure:
it("should handle different bounciness values correctly", function () { const testValues = [0, 0.1, 0.5, 1]; for (const value of testValues) { // Reset entities rootEntity.clearChildren(); // ... setup code ... physicsBox.material.bounciness = value; const boxCollider = boxEntity.addComponent(DynamicCollider); boxCollider.addShape(physicsBox); const sphereCollider = sphereEntity.addComponent(DynamicCollider); sphereCollider.addShape(physicsSphere); // Apply force and update physics sphereCollider.applyForce(new Vector3(500, 0, 0)); for (let i = 0; i < 5; i++) { engine.physicsManager._update(16); } // Verify position changes based on bounciness // Higher bounciness should result in greater rebound distance const finalPosition = sphereEntity.transform.position.x; // Add appropriate assertions based on expected behavior } });tests/src/core/physics/DynamicCollider.test.ts (5)
21-32: Consider enhancing theaddPlanehelper function.The function could benefit from the following improvements:
- Add type annotations for parameters
- Extract magic numbers (20, 1, 20) into named constants
- Add parameter validation
+ const PLANE_SIZE = 20; + const PLANE_HEIGHT = 1; + - function addPlane(x: number, y: number, z: number) { + function addPlane(x: number, y: number, z: number): Entity { + if (!Number.isFinite(x) || !Number.isFinite(y) || !Number.isFinite(z)) { + throw new Error('Invalid coordinates provided to addPlane'); + } const planeEntity = rootEntity.createChild("PlaneEntity"); planeEntity.transform.setPosition(x, y, z); - planeEntity.transform.setScale(20, 1, 20); + planeEntity.transform.setScale(PLANE_SIZE, PLANE_HEIGHT, PLANE_SIZE);
61-89: Consider adding edge cases to shape manipulation tests.The shape manipulation tests could be more comprehensive by including:
- Attempting to add null/undefined shapes
- Testing shape removal when the shape doesn't exist
- Verifying error cases
it("addShape and removeShape", function () { // ... existing tests ... // Add tests for edge cases expect(() => collider.addShape(null)).to.throw(); expect(() => collider.addShape(undefined)).to.throw(); const nonExistentShape = new BoxColliderShape(); collider.removeShape(nonExistentShape); expect(collider.shapes.length).eq(0); });
158-175: Consider extracting test constants and improving mass test assertions.The mass test uses magic numbers and could benefit from:
- Named constants for test values
- More precise assertions with acceptable margins
- Documentation of the expected behavior
+ const TEST_MASS = 1; + const SMALL_MASS = 0.01; + const TEST_FORCE = new Vector3(1, 0, 0); + const VELOCITY_EPSILON = 0.0001; it("mass", function () { const box = addBox(new Vector3(2, 2, 2), DynamicCollider, new Vector3(0, 0, 0)); const boxCollider = box.getComponent(DynamicCollider); - boxCollider.mass = 1; + boxCollider.mass = TEST_MASS; - expect(boxCollider.mass).eq(1); + expect(boxCollider.mass).eq(TEST_MASS); - boxCollider.applyForce(new Vector3(1, 0, 0)); + boxCollider.applyForce(TEST_FORCE); updatePhysics(1); - expect(formatValue(boxCollider.linearVelocity.x)).eq(0.01667); + expect(Math.abs(boxCollider.linearVelocity.x - 0.01667)).lessThan(VELOCITY_EPSILON);
372-393: Enhance force and torque tests with more scenarios.The force and torque tests could be more comprehensive by:
- Testing combined forces and torques
- Validating edge cases (zero force/torque)
- Testing force application at different points
it("combined forces and torques", function () { const box = addBox(new Vector3(2, 2, 2), DynamicCollider, new Vector3(0, 0, 0)); const boxCollider = box.getComponent(DynamicCollider); boxCollider.applyForce(new Vector3(10, 0, 0)); boxCollider.applyTorque(new Vector3(0, 10, 0)); updatePhysics(1); // Verify combined motion expect(box.transform.position.x).greaterThan(0); expect(box.transform.rotation.y).not.eq(0); }); it("zero force and torque", function () { const box = addBox(new Vector3(2, 2, 2), DynamicCollider, new Vector3(0, 0, 0)); const boxCollider = box.getComponent(DynamicCollider); boxCollider.applyForce(new Vector3(0, 0, 0)); boxCollider.applyTorque(new Vector3(0, 0, 0)); updatePhysics(1); // Verify no motion expect(box.transform.position.x).eq(0); expect(box.transform.rotation.y).eq(0); });
17-421: Consider adding additional test categories.The test suite would benefit from:
- Error handling tests (e.g., invalid parameters, component initialization failures)
- Performance tests (e.g., multiple colliders, complex scenarios)
- Memory management tests (e.g., proper cleanup of physics resources)
Example test structure:
describe("DynamicCollider", () => { describe("Error Handling", () => { it("should handle invalid parameters gracefully", () => { // Test invalid mass, forces, etc. }); it("should cleanup resources on initialization failure", () => { // Test cleanup }); }); describe("Performance", () => { it("should handle multiple simultaneous collisions", () => { // Create multiple colliding objects }); }); });tests/src/core/physics/CharacterController.test.ts (5)
103-103: UseMath.SQRT1_2Instead of Approximate Numeric ConstantTo enhance precision and readability, replace the approximate value
0.707with the standard library constantMath.SQRT1_2.Apply this diff:
- expect(controller.slopeLimit).eq(0.707); + expect(controller.slopeLimit).eq(Math.SQRT1_2);🧰 Tools
🪛 Biome
[error] 103-103: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
151-151: Remove Redundant Self-AssignmentAssigning
controller.upDirectionto itself is redundant and can be safely removed to clean up the code.Apply this diff:
- controller.upDirection = controller.upDirection;🧰 Tools
🪛 Biome
[error] 151-151: upDirection is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
182-182: UseMath.SQRT1_2Instead of Approximate Numeric ConstantReplace
0.707withMath.SQRT1_2to utilize the precise value provided by the standard library.Apply this diff:
- expect(controller.slopeLimit).eq(0.707); + expect(controller.slopeLimit).eq(Math.SQRT1_2);🧰 Tools
🪛 Biome
[error] 182-182: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
197-197: UseMath.SQRT1_2Instead of Approximate Numeric ConstantFor better precision and code clarity, consider using
Math.SQRT1_2instead of the hardcoded value0.707.Apply this diff:
- expect(controller.slopeLimit).eq(0.707); + expect(controller.slopeLimit).eq(Math.SQRT1_2);🧰 Tools
🪛 Biome
[error] 197-197: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
64-64: Set Gravity Once During InitializationSetting
physicsManager.gravityinsideonPhysicsUpdatecauses it to be reassigned every physics update, which is unnecessary. To improve performance, set the gravity once during initialization.In the
beforeEachfunction, add:+ engine.sceneManager.activeScene.physics.gravity = new Vector3(0, -9.8, 0);And remove the gravity assignment from
onPhysicsUpdate:- physicsManager.gravity = new Vector3(0, -9.8, 0);packages/core/src/physics/DynamicCollider.ts (5)
3-3: Consider using a more specific import.Instead of importing the entire
CloneManagermodule, consider importing only the specific functions that are being used in this file, such asdeepCloneandignoreClone. This can help improve the clarity and maintainability of the code.-import { deepClone, ignoreClone } from "../clone/CloneManager"; +import { deepClone } from "../clone/CloneManager";
31-32: Ensure the automatic properties are thoroughly tested.The new properties
_automaticCenterOfMassand_automaticInertiaTensorhave been introduced to allow for automatic updates of the center of mass and inertia tensor when mass changes or shapes are added or removed. To maintain the reliability and correctness of the physics simulation, it's crucial to thoroughly test these properties and their impact on theDynamicCollider's behavior.
108-117: Ensure the new automatic properties are properly documented.The new
automaticCenterOfMassandautomaticInertiaTensorproperties have been added to control the automatic updating of the center of mass and inertia tensor. To improve the usability and maintainability of theDynamicColliderclass, ensure that these properties are properly documented with clear descriptions of their purpose, behavior, and any relevant considerations for users.Also applies to: 135-146
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 109-110: packages/core/src/physics/DynamicCollider.ts#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 116-117: packages/core/src/physics/DynamicCollider.ts#L116-L117
Added lines #L116 - L117 were not covered by tests
357-359: Consider renaming theisSleepingmethod for clarity.The
isSleepingmethod has been added to check whether the collider is currently sleeping. To improve the clarity and consistency of the method name, consider renaming it toisSleeping()(with parentheses) to match the naming convention of other methods in the class.- isSleeping(): boolean { + isSleeping(): boolean {
396-401: Ensure the_setMassAndUpdateInertiamethod is thoroughly tested.The new
_setMassAndUpdateInertiamethod has been introduced to set the mass and update the inertia tensor of the collider when the collider is active in the scene. Given the critical role of this method in maintaining the accuracy of the physics simulation, it's important to ensure that it is thoroughly tested with different scenarios and edge cases.tests/src/core/physics/PhysicsMaterial.test.ts (1)
149-152: Use rotation constraints instead of setting large inertia tensor values.To prevent the boxes from rotating, large inertia tensor values are assigned after disabling automatic inertia calculation. Setting such large values can lead to numerical instability. Instead, consider freezing the rotation axes using rigid body constraints provided by the physics engine. This approach is more stable and intention-revealing.
Here's how you might adjust the code:
- collider.automaticInertiaTensor = false; - collider.inertiaTensor.set(10000, 10000, 10000); + collider.freezeRotation = true;Also applies to: 172-177, 196-197, 224-226, 258-260, 292-294, 326-328, 364-366, 402-404, 440-442, 458-459, 382-384
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (29)
packages/core/src/physics/CharacterController.ts(4 hunks)packages/core/src/physics/Collider.ts(2 hunks)packages/core/src/physics/DynamicCollider.ts(14 hunks)packages/core/src/physics/PhysicsMaterial.ts(2 hunks)packages/core/src/physics/joint/FixedJoint.ts(1 hunks)packages/core/src/physics/joint/HingeJoint.ts(2 hunks)packages/core/src/physics/joint/Joint.ts(5 hunks)packages/core/src/physics/joint/JointLimits.ts(1 hunks)packages/core/src/physics/joint/SpringJoint.ts(1 hunks)packages/core/src/physics/shape/BoxColliderShape.ts(1 hunks)packages/design/src/physics/ICharacterController.ts(1 hunks)packages/design/src/physics/IDynamicCollider.ts(4 hunks)packages/design/src/physics/joints/IHingeJoint.ts(0 hunks)packages/design/src/physics/joints/IJoint.ts(2 hunks)packages/physics-lite/src/LiteDynamicCollider.ts(2 hunks)packages/physics-physx/src/PhysXCharacterController.ts(1 hunks)packages/physics-physx/src/PhysXDynamicCollider.ts(6 hunks)packages/physics-physx/src/PhysXPhysics.ts(1 hunks)packages/physics-physx/src/joint/PhysXFixedJoint.ts(1 hunks)packages/physics-physx/src/joint/PhysXHingeJoint.ts(2 hunks)packages/physics-physx/src/joint/PhysXJoint.ts(2 hunks)packages/physics-physx/src/shape/PhysXColliderShape.ts(0 hunks)tests/src/core/physics/CharacterController.test.ts(2 hunks)tests/src/core/physics/Collider.test.ts(1 hunks)tests/src/core/physics/DynamicCollider.test.ts(1 hunks)tests/src/core/physics/HingeJoint.test.ts(1 hunks)tests/src/core/physics/Joint.test.ts(1 hunks)tests/src/core/physics/PhysicsMaterial.test.ts(1 hunks)tests/src/core/physics/SpringJoint.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/design/src/physics/joints/IHingeJoint.ts
- packages/physics-physx/src/shape/PhysXColliderShape.ts
✅ Files skipped from review due to trivial changes (1)
- packages/physics-physx/src/PhysXPhysics.ts
🧰 Additional context used
🪛 Biome
packages/core/src/physics/CharacterController.ts
[error] 18-18: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
tests/src/core/physics/CharacterController.test.ts
[error] 103-103: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
[error] 151-151: upDirection is assigned to itself.
This is where is assigned.
(lint/correctness/noSelfAssign)
[error] 182-182: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
[error] 197-197: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
tests/src/core/physics/Joint.test.ts
[error] 185-185: 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)
[error] 231-231: 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)
tests/src/core/physics/SpringJoint.test.ts
[error] 41-41: 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)
[error] 71-71: 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)
[error] 94-94: 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)
[error] 124-124: 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)
[error] 147-147: 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)
[error] 169-169: 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)
🪛 GitHub Check: codecov/patch
packages/core/src/physics/CharacterController.ts
[warning] 172-172: packages/core/src/physics/CharacterController.ts#L172
Added line #L172 was not covered by tests
packages/core/src/physics/Collider.ts
[warning] 142-143: packages/core/src/physics/Collider.ts#L142-L143
Added lines #L142 - L143 were not covered by tests
packages/core/src/physics/DynamicCollider.ts
[warning] 103-104: packages/core/src/physics/DynamicCollider.ts#L103-L104
Added lines #L103 - L104 were not covered by tests
[warning] 109-110: packages/core/src/physics/DynamicCollider.ts#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 116-117: packages/core/src/physics/DynamicCollider.ts#L116-L117
Added lines #L116 - L117 were not covered by tests
[warning] 125-125: packages/core/src/physics/DynamicCollider.ts#L125
Added line #L125 was not covered by tests
[warning] 136-137: packages/core/src/physics/DynamicCollider.ts#L136-L137
Added lines #L136 - L137 were not covered by tests
[warning] 143-144: packages/core/src/physics/DynamicCollider.ts#L143-L144
Added lines #L143 - L144 were not covered by tests
[warning] 172-172: packages/core/src/physics/DynamicCollider.ts#L172
Added line #L172 was not covered by tests
[warning] 413-414: packages/core/src/physics/DynamicCollider.ts#L413-L414
Added lines #L413 - L414 were not covered by tests
[warning] 419-420: packages/core/src/physics/DynamicCollider.ts#L419-L420
Added lines #L419 - L420 were not covered by tests
packages/core/src/physics/joint/HingeJoint.ts
[warning] 145-149: packages/core/src/physics/joint/HingeJoint.ts#L145-L149
Added lines #L145 - L149 were not covered by tests
packages/core/src/physics/joint/Joint.ts
[warning] 39-39: packages/core/src/physics/joint/Joint.ts#L39
Added line #L39 was not covered by tests
[warning] 85-86: packages/core/src/physics/joint/Joint.ts#L85-L86
Added lines #L85 - L86 were not covered by tests
[warning] 125-126: packages/core/src/physics/joint/Joint.ts#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 209-210: packages/core/src/physics/joint/Joint.ts#L209-L210
Added lines #L209 - L210 were not covered by tests
[warning] 218-219: packages/core/src/physics/joint/Joint.ts#L218-L219
Added lines #L218 - L219 were not covered by tests
[warning] 252-253: packages/core/src/physics/joint/Joint.ts#L252-L253
Added lines #L252 - L253 were not covered by tests
[warning] 257-260: packages/core/src/physics/joint/Joint.ts#L257-L260
Added lines #L257 - L260 were not covered by tests
packages/physics-lite/src/LiteDynamicCollider.ts
[warning] 22-23: packages/physics-lite/src/LiteDynamicCollider.ts#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 28-29: packages/physics-lite/src/LiteDynamicCollider.ts#L28-L29
Added lines #L28 - L29 were not covered by tests
[warning] 65-66: packages/physics-lite/src/LiteDynamicCollider.ts#L65-L66
Added lines #L65 - L66 were not covered by tests
[warning] 69-70: packages/physics-lite/src/LiteDynamicCollider.ts#L69-L70
Added lines #L69 - L70 were not covered by tests
[warning] 80-81: packages/physics-lite/src/LiteDynamicCollider.ts#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 125-126: packages/physics-lite/src/LiteDynamicCollider.ts#L125-L126
Added lines #L125 - L126 were not covered by tests
[warning] 135-135: packages/physics-lite/src/LiteDynamicCollider.ts#L135
Added line #L135 was not covered by tests
[warning] 153-153: packages/physics-lite/src/LiteDynamicCollider.ts#L153
Added line #L153 was not covered by tests
[warning] 164-164: packages/physics-lite/src/LiteDynamicCollider.ts#L164
Added line #L164 was not covered by tests
[warning] 182-182: packages/physics-lite/src/LiteDynamicCollider.ts#L182
Added line #L182 was not covered by tests
packages/physics-physx/src/PhysXDynamicCollider.ts
[warning] 34-35: packages/physics-physx/src/PhysXDynamicCollider.ts#L34-L35
Added lines #L34 - L35 were not covered by tests
🔇 Additional comments (31)
packages/core/src/physics/joint/JointLimits.ts (1)
5-8:
Breaking Change: Angular units changed from degrees to radians
The documentation update indicates a change from degrees to radians for both max and min properties. This is a breaking change that requires careful migration handling.
Let's verify the usage of these properties across the codebase:
Consider:
- Adding migration guide documentation
- Including runtime validation to detect values likely provided in degrees
- Adding deprecation warnings for the old degree-based API if maintaining backward compatibility
packages/physics-physx/src/joint/PhysXFixedJoint.ts (1)
14-20:
Verify the actor parameter order change.
The modification in parameter order (swapping null and collider._pxActor) could significantly affect joint behavior. This change appears to modify the joint from being fixed to a collider to being fixed to the world/ground instead.
Please ensure:
- This change aligns with PhysX API requirements
- The new behavior is intentional and matches the physics requirements
- Existing simulations won't break with this change
packages/core/src/physics/shape/BoxColliderShape.ts (1)
36-36: Verify the timing of size updates in the physics engine.
The size change callback directly updates the native physics shape. Ensure this doesn't cause performance issues if the size is updated frequently during physics simulations.
Let's check for other size update patterns in the codebase:
✅ Verification successful
Let me gather more information about the value change handling patterns and physics update cycles.
Let me gather more information about the physics update cycle and size change handling.
Based on the analysis of the code, I can now provide a final response:
Size updates in physics are optimized and safe for frequent changes
The physics system is designed to handle size updates efficiently:
- Size changes are batched and only applied during physics updates via the fixed timestep system
- The physics engine uses a half-size optimization internally to reduce calculations
- Updates are synchronized with the physics simulation cycle rather than being applied immediately
- The implementation in both PhysX and Lite backends shows proper handling of scale changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for patterns of physics shape size updates
# to understand the update frequency and potential performance impact
# Check for direct size updates
rg -A 2 "setSize|_setSize" --type ts
# Look for Vector3 value change handlers in physics components
ast-grep --pattern 'class $_ extends ColliderShape {
$$$
_onValueChanged
$$$
}'
Length of output: 1748
Script:
#!/bin/bash
# Check for other Vector3 value change handlers and update patterns
rg -A 5 "_onValueChanged|onUpdate" --type ts
# Look for physics update/simulation related code
rg -A 3 "updatePhysics|simulatePhysics|physicsUpdate" --type ts
# Check for any performance-related comments or documentation
rg -B 2 "performance|throttle|frequency" --type ts
Length of output: 127075
Script:
#!/bin/bash
# Check physics update implementation and frequency
rg -A 5 "_update|fixedUpdate" packages/core/src/physics/ --type ts
# Look for size-related physics shape implementations
rg -A 3 "setSize|_setSize" packages/physics-physx/src/ packages/physics-lite/src/ --type ts
# Check for any performance-related comments in physics code
rg -B 2 "performance|update" packages/core/src/physics/ packages/physics-physx/src/ packages/physics-lite/src/ --type ts
Length of output: 28768
packages/core/src/physics/PhysicsMaterial.ts (1)
9-11:
Breaking Change: Default physics material properties changed to zero
Changing the default values from 0.1 to 0 for bounciness and friction properties is a breaking change that could significantly affect existing physics simulations. While starting from zero might be more predictable, it represents perfectly slippery surfaces with no bounce by default.
Let's verify the impact and usage of these properties:
Consider:
- Adding a migration guide for users
- Documenting the rationale for this change
- Potentially introducing a factory method for creating materials with the old default values
packages/physics-physx/src/joint/PhysXJoint.ts (1)
24-28: Verify the actor parameter order change across the codebase.
The parameter order in setActors has been reversed. While this might be intentional, we should verify that all derived joint classes and test cases have been updated accordingly to prevent incorrect physical connections.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to understand the parameter order change better.
I need to see the actual implementations to verify if the parameter order change in setActors is correct and consistent with the interface design and base implementation.
After analyzing the code, I can now generate the final response:
The parameter order in setActors is correct and consistent with the design.
The implementation is verified to be correct because:
- The
IJointinterface definessetConnectedCollider(value: ICollider)which is correctly implemented inPhysXJoint. - The parameter order in
setActors(this._collider?._pxActor || null, value?._pxActor || null)matches the expected behavior where:- First parameter is the actor from the joint's own collider
- Second parameter is the actor from the connected collider being set
- This is consistent with the base
Jointclass implementation which manages the collider relationships in the same order.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all joint implementations and their setActors usage
ast-grep --pattern 'class $_ extends PhysXJoint {
$$$
}'
# Search for test cases involving setConnectedCollider
rg -l "setConnectedCollider.*" --type ts
Length of output: 14988
Script:
#!/bin/bash
# Check the base interface definition
cat packages/design/src/physics/joints/IJoint.ts
# Check the implementation in Joint.ts
cat packages/core/src/physics/joint/Joint.ts
# Check the full implementation of PhysXJoint
cat packages/physics-physx/src/joint/PhysXJoint.ts
Length of output: 13687
packages/physics-physx/src/joint/PhysXHingeJoint.ts (1)
21-24: Verify the reversed actor parameters in createRevoluteJoint.
The order of actors in createRevoluteJoint has been swapped. Please confirm this is intentional and doesn't affect joint behavior.
✅ Verification successful
The actor parameter order change in createRevoluteJoint is correct and intentional
Based on the code analysis:
- The change aligns with the PhysX joint creation pattern where the first actor is the primary body and the second is the connected body
- The test cases in
HingeJoint.test.tsconfirm this pattern, where the first collider is typically the kinematic/fixed body and the second is the dynamic/connected body - The example in
physx-joint-basic.tsalso follows this pattern for hinge joint creation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other joint creation patterns to verify consistency
ast-grep --pattern 'createRevoluteJoint($_actor1, $_, $_, $_actor2, $_, $_)'
Length of output: 78
Script:
#!/bin/bash
# Let's try a broader search for revolute joint creation and usage patterns
rg -l "createRevoluteJoint"
# Then look for the actual implementation and usage
rg "createRevoluteJoint" -A 5 -B 5
# Also search for any documentation or comments about joint creation
rg -l "RevoluteJoint|HingeJoint"
# Look for test files that might show the expected usage
fd "test|spec" -e ts | xargs rg "RevoluteJoint|HingeJoint"
Length of output: 2529
Script:
#!/bin/bash
# Let's look at the PhysX implementation and interface definitions
cat packages/physics-physx/src/PhysXPhysics.ts
# Check the interface definition for hinge joint
cat packages/design/src/physics/joints/IHingeJoint.ts
# Look at example usage
cat examples/physx-joint-basic.ts
# Check the actual test implementation for proper usage
cat tests/src/core/physics/HingeJoint.test.ts
Length of output: 26624
packages/design/src/physics/IDynamicCollider.ts (1)
Line range hint 22-149: Overall changes look good!
The additions to IDynamicCollider interface:
- Provide a more complete API surface for accessing physical properties
- Maintain consistency with existing patterns
- Follow TypeScript conventions
- Don't introduce breaking changes
The interface is well-structured and the new methods enhance its functionality.
packages/core/src/physics/Collider.ts (2)
142-143: Add test coverage for cloning functionality.
The changes to _cloneTo lack test coverage. Since this is part of the ICustomClone interface and handles critical shape synchronization during cloning, it should be thoroughly tested.
Would you like me to help create test cases for the cloning functionality? The tests should verify:
- Shapes are properly synchronized after cloning
- No duplicate shapes are created
- Physics backend is properly updated
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 142-143: packages/core/src/physics/Collider.ts#L142-L143
Added lines #L142 - L143 were not covered by tests
123-123:
Verify potential duplicate shape registration.
The _onEnableInScene method already iterates over shapes to register them with scene physics. The addition of _syncBackends() might cause shapes to be registered twice since _addPhysicsShape also registers shapes with scene physics when _phasedActiveInScene is true.
packages/physics-physx/src/PhysXCharacterController.ts (1)
57-62: LGTM! Implementation follows established patterns.
The implementation is consistent with other setter methods in the class, properly uses optional chaining for null safety, and is well-documented.
Let's verify the interface implementation is complete:
✅ Verification successful
Implementation correctly matches the interface and follows consistent usage pattern
The verification confirms:
- The
setContactOffsetmethod signature inPhysXCharacterControllerexactly matches theICharacterControllerinterface definition - The implementation follows the same pattern used across the codebase in both PhysX and Lite physics implementations
- The method is properly used by the core physics module through the interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all ICharacterController interface methods are implemented
# and that the setContactOffset signature matches the interface definition
# Search for the interface definition and implementation
ast-grep --pattern 'interface ICharacterController {
$$$
setContactOffset(offset: number): void;
$$$
}'
# Verify no other signatures exist that might conflict
rg -A 1 'setContactOffset'
Length of output: 2151
packages/core/src/physics/joint/HingeJoint.ts (2)
37-37: LGTM! Type assertion is safe in this context.
The type assertion to IHingeJoint is safe here since the native joint is guaranteed to be initialized before this setter is called.
144-149: Add test coverage for motor synchronization.
The motor synchronization block lacks test coverage. Please add tests to verify that motor properties are correctly synchronized with the native backend.
Would you like me to help generate test cases for the motor synchronization logic?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 145-149: packages/core/src/physics/joint/HingeJoint.ts#L145-L149
Added lines #L145 - L149 were not covered by tests
packages/physics-physx/src/PhysXDynamicCollider.ts (4)
Line range hint 84-113: LGTM! Mass and inertia tensor handling is implemented correctly.
The methods for managing center of mass and inertia tensor properties are well-implemented with proper Vector3 conversions and clear method names.
Line range hint 115-147: LGTM! Physics property getters are properly implemented.
The new getter methods for maximum angular velocity, depenetration velocity, and sleep threshold are correctly implemented as straightforward pass-through methods to the PhysX actor.
240-242: LGTM! Sleep state checker is properly implemented.
The isSleeping method is correctly implemented as a pass-through to the PhysX actor and complements the existing sleep and wakeUp methods.
33-69: LGTM! Consider adding unit tests for the new getter methods.
The implementation of the getter methods for linear/angular damping and velocities is correct and maintains good symmetry with their corresponding setters. The Vector3 conversions are handled properly.
Let's verify the test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-35: packages/physics-physx/src/PhysXDynamicCollider.ts#L34-L35
Added lines #L34 - L35 were not covered by tests
tests/src/core/physics/HingeJoint.test.ts (2)
1-15: LGTM! Imports are well-organized and complete.
All necessary dependencies are properly imported and logically grouped.
1-231: Overall, this is a well-structured and comprehensive test suite.
The test coverage is thorough, testing various aspects of the HingeJoint component including axis behavior, limits, motor controls, and free spin functionality. While there are some opportunities for improvement in code organization and documentation, the tests themselves are well-designed and provide good validation of the component's behavior.
tests/src/core/physics/Collider.test.ts (1)
Line range hint 89-270: Verify test stability with the new bounciness value.
The addition of bounciness could affect collision response timing. While the current tests use flexible assertions (toBeGreaterThan), we should verify that the tests remain stable across different environments and runs.
✅ Verification successful
Let me analyze the collision test cases and their assertions to determine if we can verify the stability through static analysis.
Based on the analysis of the test file, I can now generate a final response:
The test suite is resilient to the bounciness value change
The collision tests are well-designed to be stable with the new bounciness value (0.1) because:
- The collision event assertions (
onCollisionEnter,onCollisionStay,onCollisionExit) use flexibletoBeGreaterThan(1)checks that accommodate variations in physics simulation steps - Position checks are binary (either
equal(5)ornot.to.be.equal(5)), not checking exact positions that could be affected by bounce physics - The test cases properly separate different collision scenarios (Dynamic/Static/Kinematic) and their assertions match the expected behavior for each type
- The trigger-based tests are independent of bounciness as they only check for overlap events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test stability by running tests multiple times
# Test: Run the tests multiple times to check for consistent results
for i in {1..5}; do
echo "Run #$i"
# Using vitest to run specific test file
npx vitest run tests/src/core/physics/Collider.test.ts
done
# Look for any test failures or inconsistencies in the results
Length of output: 844
Script:
#!/bin/bash
# Let's examine the test file and related physics components to understand the collision behavior
# Check for any bounciness-related changes in the test setup
rg -B 5 -A 5 "bounciness|restitution" tests/src/core/physics/Collider.test.ts
# Look for the physics box and sphere setup
rg -B 10 "physicsBox|physicsSphere" tests/src/core/physics/Collider.test.ts
# Check the CollisionScript implementation
fd -e ts -e js CollisionScript --exec cat {}
Length of output: 7783
packages/core/src/physics/joint/Joint.ts (2)
16-16: Appropriate change to abstract class
Converting Joint to an abstract class enhances the architecture by indicating that Joint is not meant to be instantiated directly but to be subclassed. This change improves code clarity and enforces proper usage.
19-22: Confirm necessity of deep cloning collider info
Replacing @ignoreClone with @deepClone for _colliderInfo and _connectedColliderInfo means these properties will be deeply cloned when cloning Joint instances. Ensure that deep cloning is required and does not introduce unintended side effects, such as unnecessary memory usage or performance implications.
packages/core/src/physics/DynamicCollider.ts (8)
66-67: LGTM!
The changes to the linearVelocity, angularVelocity, centerOfMass, inertiaTensor, and their respective getters now retrieve the values directly from the native collider. This ensures that the properties always reflect the current state of the collider, improving the accuracy and consistency of the physics simulation.
Also applies to: 80-81, 125-126, 136-137, 152-153
172-173: LGTM!
The changes to the applyForce, applyTorque, move, sleep, wakeUp, and other methods now include a check for the _phasedActiveInScene flag before executing their logic. This ensures that these operations are only performed when the collider is active in the scene, preventing unnecessary computations and potential errors.
Also applies to: 186-187, 200-201, 214-215, 228-229, 242-243, 256-257, 312-313, 320-321, 343-344, 350-351, 358-359, 365-366
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 172-172: packages/core/src/physics/DynamicCollider.ts#L172
Added line #L172 was not covered by tests
286-291: LGTM!
The addShape, removeShape, and clearShapes methods have been overridden to include calls to _setMassAndUpdateInertia() when either _automaticCenterOfMass or _automaticInertiaTensor is enabled. This ensures that the mass and inertia tensor are automatically updated whenever shapes are added, removed, or cleared, maintaining the accuracy and consistency of the physics simulation.
Also applies to: 293-298, 300-305
378-394: LGTM!
The _syncBackends method has been updated to synchronize various properties with the native collider, ensuring that the native collider always reflects the current state of the DynamicCollider instance. This improves the consistency and accuracy of the physics simulation across different backends.
405-406: LGTM!
The private methods _setLinearVelocity, _setAngularVelocity, _setCenterOfMass, and _setInertiaTensor have been updated to include checks for the _phasedActiveInScene flag and the respective automatic properties before updating the native collider. This ensures that the native collider is only updated when necessary, improving performance and maintaining consistency with the automatic updating behavior.
Also applies to: 408-409, 412-414, 418-420
25-26: Verify the impact of changing _maxDepenetrationVelocity to Infinity.
The _maxDepenetrationVelocity property has been updated from a fixed value of 1000 to Infinity. While this change may be intended to allow for unlimited depenetration velocity, it's important to assess the potential impact on the physics simulation and collision resolution.
#!/bin/bash
# Description: Verify the usage and impact of `_maxDepenetrationVelocity` property.
# Test: Search for usage of `_maxDepenetrationVelocity` property.
# Expect: Only accessed via getter/setter, no direct modifications.
rg -A 10 $'_maxDepenetrationVelocity'
# Test: Search for usage of `setMaxDepenetrationVelocity` method in other files.
# Expect: No manual setting of `maxDepenetrationVelocity` to a finite value.
rg -A 5 $'setMaxDepenetrationVelocity'100-106: Verify the conditional logic for updating mass and inertia.
The mass setter now includes additional logic to conditionally call _setMassAndUpdateInertia() based on the values of _automaticInertiaTensor and _automaticCenterOfMass properties. While this change aims to improve the automatic updating of mass and inertia, it's important to verify that the conditional logic is correct and covers all necessary scenarios.
✅ Verification successful
Based on the search results, I can now generate the final response as I have enough information to verify the conditional logic for updating mass and inertia:
The conditional logic for mass and inertia updates is correctly implemented
The implementation follows a consistent pattern where:
_setMassAndUpdateInertia()is called when either_automaticInertiaTensoror_automaticCenterOfMassis true, which updates both mass and inertia togethersetMass()is only called directly when both automatic properties are false, which updates just the mass value- This pattern is consistently applied across the codebase in various scenarios like shape removal, property changes, etc.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the conditional logic for updating mass and inertia.
# Test: Search for usage of `_setMassAndUpdateInertia` method.
# Expect: Called only when `_automaticInertiaTensor` or `_automaticCenterOfMass` is true.
rg -A 10 $'_setMassAndUpdateInertia'
# Test: Search for usage of `setMass` method.
# Expect: Called only when both `_automaticInertiaTensor` and `_automaticCenterOfMass` are false.
rg -A 10 $'setMass'
Length of output: 21713
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 103-104: packages/core/src/physics/DynamicCollider.ts#L103-L104
Added lines #L103 - L104 were not covered by tests
15-24: Verify the impact of changing @ignoreClone to @deepClone.
The @ignoreClone decorator has been replaced with @deepClone for several private properties. This change indicates a shift in how these properties are cloned. Ensure that this change aligns with the intended behavior and does not introduce any unintended side effects or performance implications.
✅ Verification successful
Let me generate additional scripts to properly verify the impact of changing @ignoreClone to @deepClone.
This will help us understand:
- How cloning is implemented in the DynamicCollider class
- The implementation differences between @deepClone and @ignoreClone decorators
- The cloning behavior of Vector3 objects which are the types being decorated
Let me generate additional scripts to understand the cloning behavior and its impact.
This will help us understand:
- How Vector3 implements cloning
- How these properties are used in DynamicCollider
- The implementation difference between @deepClone and @ignoreClone decorators
Let me generate one final script to check the clone decorator implementations.
This will help us understand:
- The implementation details of @deepClone and @ignoreClone decorators
- The Vector3 class implementation and its cloning behavior
Based on the code analysis, I can now provide a final response about the impact of changing from @ignoreClone to @deepClone.
The change from @ignoreClone to @deepClone for physics properties is appropriate and beneficial.
The change affects how _linearVelocity, _angularVelocity, _centerOfMass, and _inertiaTensor Vector3 properties are handled during cloning:
- With
@ignoreClone, these properties were completely skipped during cloning, which could lead to shared references between cloned instances - With
@deepClone, each cloned DynamicCollider gets its own independent copies of these Vector3 properties, preventing unwanted shared state
The implementation shows these properties are used for critical physics calculations and are properly synchronized with the native physics engine through setter methods. Having independent copies for each collider instance is important because:
- Each property has its own
_onValueChangedcallback that syncs with the native physics engine - The values are frequently updated through physics simulation
- Independent instances prevent one collider's physics state from accidentally affecting another
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage and impact of `@deepClone` decorator.
# Test: Search for usage of `@deepClone` decorator.
# Expect: Only used on intended properties.
rg -A 5 $'@deepClone'
# Test: Search for usage of `_linearVelocity`, `_angularVelocity`, `_centerOfMass`, `_inertiaTensor` properties.
# Expect: No direct modifications, only accessed via getters/setters.
rg -A 10 $'_linearVelocity|_angularVelocity|_centerOfMass|_inertiaTensor'
Length of output: 6797
Script:
#!/bin/bash
# Search for clone-related functionality and usage
rg -A 5 "clone\(\)" packages/core/src/physics/DynamicCollider.ts
# Search for decorator definitions and their implementations
rg -A 5 "@deepClone|@ignoreClone" packages/core/src/
# Search for Vector3 cloning behavior
rg -A 5 "Vector3.*clone" packages/core/src/
Length of output: 67754
Script:
#!/bin/bash
# Search for clone-related functionality in Vector3 and related classes
rg -A 5 "clone\(\)" packages/core/src/math/Vector3.ts
# Search for DynamicCollider's usage of these properties
rg -A 5 "(_linearVelocity|_angularVelocity|_centerOfMass|_inertiaTensor)" packages/core/src/physics/DynamicCollider.ts
# Search for clone decorator implementation
rg -A 10 "@deepClone|@ignoreClone" packages/core/src/clone/
Length of output: 4285
Script:
#!/bin/bash
# Search for clone decorator implementations and their usage
fd -e ts -x cat {} \; -p packages/core/src/clone/
# Search for Vector3 implementation
fd Vector3.ts -x cat {} \; -p packages/core/src/math/
Length of output: 8660
tests/src/core/physics/PhysicsMaterial.test.ts (2)
20-32: Well-structured helper function addPlane.
The addPlane function cleanly encapsulates the creation and setup of a plane entity with its physics properties. This enhances code readability and reuse across tests.
34-45: Efficient addBox helper function for entity creation.
The addBox function effectively abstracts the setup of box entities, allowing flexibility in specifying size, type, and position. This promotes clean and maintainable test code.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (17)
packages/core/src/physics/joint/SpringJoint.ts (1)
26-26: Consider extracting native joint casting to improve maintainability.The repeated type casting pattern
(<ISpringJoint>this._nativeJoint)could be simplified by creating a protected getter.+ protected get _nativeSpringJoint(): ISpringJoint { + return this._nativeJoint as ISpringJoint; + } set minDistance(value: number) { if (this._minDistance !== value) { this._minDistance = value; - (<ISpringJoint>this._nativeJoint)?.setMinDistance(value); + this._nativeSpringJoint?.setMinDistance(value); } }Also applies to: 40-40, 54-54, 68-68, 82-82
packages/core/src/physics/shape/ColliderShape.ts (1)
136-136: Consider adding error handling in _cloneTo.The simplified
_cloneTomethod could benefit from basic error handling to ensuretargetis valid and initialized before calling_syncBackends()._cloneTo(target: ColliderShape) { + if (!target) { + throw new Error("Clone target cannot be null or undefined"); + } target._syncBackends(); }packages/core/src/physics/joint/Joint.ts (1)
224-242: Add documentation for anchor calculationsThe anchor calculation logic is complex and would benefit from detailed documentation explaining the mathematical transformations and coordinate space conversions.
Add JSDoc comments explaining:
- The coordinate spaces involved (local vs world)
- The mathematical relationship between anchors
- Why different calculations are needed for connected vs unconnected cases
Example:
/** * Calculates the connected anchor position based on the self anchor and transforms. * For connected joints: Transforms self anchor to connected body's local space * For unconnected joints: Transforms self anchor to world space * @private */ private _calculateConnectedAnchor(): void {tests/src/core/physics/SpringJoint.test.ts (1)
35-231: Consider improving test structure and reducing duplicationThe test cases share a lot of common setup code which could be extracted to improve maintainability and readability.
Consider these improvements:
- Extract common setup to a beforeEach hook:
describe("SpringJoint", () => { let boxEntity1: Entity; let boxEntity2: Entity; let joint: SpringJoint; beforeEach(() => { // Common setup engine.sceneManager.activeScene.physics.gravity = new Vector3(0, 0, 0); boxEntity1 = addBox(new Vector3(1, 1, 1), DynamicCollider, new Vector3(0, 5, 0)); boxEntity2 = addBox(new Vector3(1, 1, 1), DynamicCollider, new Vector3(2, 5, 0)); joint = boxEntity2.addComponent(SpringJoint); // Common joint configuration const collider = boxEntity1.getComponent(DynamicCollider); joint.connectedCollider = collider; collider.isKinematic = true; joint.autoConnectedAnchor = false; joint.anchor = new Vector3(-0.5, 0, 0); joint.connectedAnchor = new Vector3(0.5, 0, 0); }); it("should respect minDistance", () => { // Test-specific configuration joint.stiffness = 10000; joint.damping = 1000; // ... rest of the test }); });
- Create helper functions for common operations:
function updatePhysics(deltaTime: number = 1) { // @ts-ignore engine.sceneManager.activeScene.physics._update(deltaTime); } function configureJoint(joint: SpringJoint, config: Partial<{ stiffness: number; damping: number; minDistance: number; maxDistance: number; tolerance: number; }>) { Object.assign(joint, config); }These changes would:
- Reduce code duplication
- Improve test maintainability
- Make test cases more focused on what they're actually testing
- Make it easier to add new test cases
🧰 Tools
🪛 Biome
[error] 41-41: 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)
[error] 71-71: 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)
[error] 94-94: 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)
[error] 124-124: 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)
[error] 147-147: 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)
[error] 169-169: 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)
[error] 200-200: 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)
tests/src/core/physics/Collider.test.ts (1)
295-346: Consider refactoring duplicate code in clone tests.The
clone StaticColliderandclone DynamicCollidertests share similar setup and verification logic. Consider extracting common test setup into a helper function to improve maintainability.+ function setupColliderTest(box: Entity, collider: DynamicCollider) { + // Avoid the box rotating + collider.automaticInertiaTensor = false; + collider.inertiaTensor.set(10000000, 10000000, 10000000); + collider.applyForce(new Vector3(1000, 0, 0)); + collider.shapes[0].material.dynamicFriction = 1; + } it("clone StaticCollider", function () { const box = addBox(new Vector3(1, 1, 1), DynamicCollider, new Vector3(0, 0.5, 0)); const collider = box.getComponent(DynamicCollider); - // Avoid the box rotating - collider.automaticInertiaTensor = false; - collider.inertiaTensor.set(10000000, 10000000, 10000000); - collider.applyForce(new Vector3(1000, 0, 0)); - collider.shapes[0].material.dynamicFriction = 1; + setupColliderTest(box, collider); // ... rest of the test }); it("clone DynamicCollider", function () { const box = addBox(new Vector3(1, 1, 1), DynamicCollider, new Vector3(0, 0.5, 0)); const collider = box.getComponent(DynamicCollider); - // Avoid the box rotating - collider.automaticInertiaTensor = false; - collider.inertiaTensor.set(10000000, 10000000, 10000000); - collider.applyForce(new Vector3(1000, 0, 0)); - collider.shapes[0].material.dynamicFriction = 1; + setupColliderTest(box, collider); // ... rest of the test });packages/core/src/physics/DynamicCollider.ts (4)
108-146: Document performance implications of automatic propertiesThe automatic update of mass and inertia when shapes change could impact performance, especially with frequent shape modifications. Consider adding JSDoc comments explaining:
- Performance implications
- When to use automatic vs. manual updates
- Best practices for shape management
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 109-110: packages/core/src/physics/DynamicCollider.ts#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 116-117: packages/core/src/physics/DynamicCollider.ts#L116-L117
Added lines #L116 - L117 were not covered by tests
[warning] 136-137: packages/core/src/physics/DynamicCollider.ts#L136-L137
Added lines #L136 - L137 were not covered by tests
[warning] 143-144: packages/core/src/physics/DynamicCollider.ts#L143-L144
Added lines #L143 - L144 were not covered by tests
286-305: Consider batching shape operationsWhen multiple shapes are added or removed in succession, each operation triggers a separate mass and inertia update. Consider adding batch operations to optimize performance.
+ /** + * Adds multiple shapes at once, updating mass and inertia only once. + * @param shapes - Array of shapes to add + */ + addShapes(shapes: ColliderShape[]): void { + shapes.forEach(shape => super.addShape(shape)); + if (this._automaticCenterOfMass || this._automaticInertiaTensor) { + this._setMassAndUpdateInertia(); + } + }
Line range hint
312-365: Add tests for phase-dependent operationsThe new phase checks (
this._phasedActiveInScene) protect against invalid operations, but need test coverage for both active and inactive phases. This affects:
- Force application
- Movement
- Sleep/wake operations
Would you like me to help generate comprehensive test cases?
389-405: Optimize backend synchronizationThe
_syncBackendsmethod synchronizes all properties individually. Consider:
- Batching updates where possible
- Only syncing changed properties
- Adding debug logging for troubleshooting
tests/src/core/physics/Joint.test.ts (2)
11-26: Add JSDoc documentation to helper functions.The helper functions
addBoxandformatValuewould benefit from JSDoc documentation explaining their purpose, parameters, and return values.Add documentation like this:
+/** + * Creates a box entity with specified size, collider type, and position + * @param cubeSize - The size of the box in 3D space + * @param type - The type of collider (DynamicCollider or StaticCollider) + * @param pos - The position of the box in 3D space + * @returns The created box entity + */ function addBox(cubeSize: Vector3, type: typeof DynamicCollider | typeof StaticCollider, pos: Vector3) { +/** + * Rounds a number to 5 decimal places + * @param value - The number to format + * @returns The formatted number + */ function formatValue(value: number) {
120-142: Improve test case names for better clarity.The test cases would benefit from more descriptive names that indicate the specific behavior being tested.
Consider renaming tests to better describe their purpose:
- it("massScale", function () { + it("should maintain joint stability until mass scale threshold is exceeded", function () { - it("connectedMassScale", function () { + it("should maintain joint stability until connected mass scale threshold is exceeded", function () { - it("inertiaScale", function () { + it("should correctly scale angular motion based on inertia scale", function () { - it("breakForce", function () { + it("should break joint when applied force exceeds break force threshold", function () { - it("breakTorque", function () { + it("should break joint when applied torque exceeds break torque threshold", function () {Also applies to: 144-166, 168-212, 214-258, 260-280, 282-301
tests/src/core/physics/DynamicCollider.test.ts (3)
21-32: Add return type annotation and extract magic numbers.Consider these improvements to the
addPlanehelper function:
- Add return type annotation
- Extract magic numbers into named constants
+ const DEFAULT_PLANE_SCALE = 20; + - function addPlane(x: number, y: number, z: number) { + function addPlane(x: number, y: number, z: number): Entity { const planeEntity = rootEntity.createChild("PlaneEntity"); planeEntity.transform.setPosition(x, y, z); - planeEntity.transform.setScale(20, 1, 20); + planeEntity.transform.setScale(DEFAULT_PLANE_SCALE, 1, DEFAULT_PLANE_SCALE);
34-45: Extract common physics material setup into a helper function.The friction setup is duplicated between
addPlaneandaddBox. Consider extracting it into a helper function.+ function setupFrictionlessMaterial(shape: BoxColliderShape | PlaneColliderShape): void { + shape.material.dynamicFriction = 0; + shape.material.staticFriction = 0; + } function addBox(cubeSize: Vector3, type: typeof DynamicCollider | typeof StaticCollider, pos: Vector3): Entity { // ... const physicsBox = new BoxColliderShape(); - physicsBox.material.dynamicFriction = 0; - physicsBox.material.staticFriction = 0; + setupFrictionlessMaterial(physicsBox); // ... }Also applies to: 47-49
423-446: Add assertions to verify state modifications.The "inActive modification" test sets multiple properties but doesn't verify their effects. Consider adding assertions to validate the modifications.
it("inActive modification", function () { const box = addBox(new Vector3(2, 2, 2), DynamicCollider, new Vector3(0, 0, 0)); const boxCollider = box.getComponent(DynamicCollider); box.isActive = false; boxCollider.linearDamping = 0.1; + expect(boxCollider.linearDamping).eq(0.1); boxCollider.mass = 1; + expect(boxCollider.mass).eq(1); // Add more assertions for other property modifications });tests/src/core/physics/PhysicsMaterial.test.ts (2)
20-32: Helper functions are well-structured but could be more reusable.The helper functions
addPlane,addBox, andformatValueare well-implemented, but they could be moved to a shared test utilities file for reuse across other physics test files.Consider moving these helper functions to a shared test utilities file (e.g.,
tests/src/core/physics/test-utils.ts).Also applies to: 34-45, 47-49
61-469: Test cases are comprehensive but could benefit from better organization.The test suite thoroughly covers different physics material properties and their combinations. However, the organization could be improved to make it more maintainable.
Consider:
- Using nested describe blocks to group related tests
- Adding more descriptive test names
- Using beforeEach to set up common test scenarios
Example structure:
describe('PhysicsMaterial', () => { describe('Bounciness', () => { describe('Combine Modes', () => { it('should average bounce values when mode is Average', () => { // ... }); // ... }); }); describe('Friction', () => { describe('Static Friction', () => { // ... }); describe('Dynamic Friction', () => { // ... }); }); });tests/src/core/physics/HingeJoint.test.ts (1)
21-36: Extract 'addBox' and 'formatValue' into a shared utility module for reuse.The functions
addBoxandformatValueare utility functions that may be reused across multiple test files. By moving them into a shared module, you can reduce code duplication and enhance maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
packages/core/src/physics/DynamicCollider.ts(14 hunks)packages/core/src/physics/joint/HingeJoint.ts(6 hunks)packages/core/src/physics/joint/Joint.ts(7 hunks)packages/core/src/physics/joint/SpringJoint.ts(5 hunks)packages/core/src/physics/shape/ColliderShape.ts(4 hunks)tests/src/core/physics/Collider.test.ts(3 hunks)tests/src/core/physics/DynamicCollider.test.ts(1 hunks)tests/src/core/physics/HingeJoint.test.ts(1 hunks)tests/src/core/physics/Joint.test.ts(1 hunks)tests/src/core/physics/PhysicsMaterial.test.ts(1 hunks)tests/src/core/physics/SpringJoint.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/physics/DynamicCollider.ts
[warning] 103-104: packages/core/src/physics/DynamicCollider.ts#L103-L104
Added lines #L103 - L104 were not covered by tests
[warning] 109-110: packages/core/src/physics/DynamicCollider.ts#L109-L110
Added lines #L109 - L110 were not covered by tests
[warning] 116-117: packages/core/src/physics/DynamicCollider.ts#L116-L117
Added lines #L116 - L117 were not covered by tests
[warning] 136-137: packages/core/src/physics/DynamicCollider.ts#L136-L137
Added lines #L136 - L137 were not covered by tests
[warning] 143-144: packages/core/src/physics/DynamicCollider.ts#L143-L144
Added lines #L143 - L144 were not covered by tests
[warning] 172-172: packages/core/src/physics/DynamicCollider.ts#L172
Added line #L172 was not covered by tests
[warning] 424-425: packages/core/src/physics/DynamicCollider.ts#L424-L425
Added lines #L424 - L425 were not covered by tests
[warning] 430-431: packages/core/src/physics/DynamicCollider.ts#L430-L431
Added lines #L430 - L431 were not covered by tests
packages/core/src/physics/joint/HingeJoint.ts
[warning] 153-157: packages/core/src/physics/joint/HingeJoint.ts#L153-L157
Added lines #L153 - L157 were not covered by tests
packages/core/src/physics/joint/Joint.ts
[warning] 38-38: packages/core/src/physics/joint/Joint.ts#L38
Added line #L38 was not covered by tests
[warning] 84-85: packages/core/src/physics/joint/Joint.ts#L84-L85
Added lines #L84 - L85 were not covered by tests
[warning] 110-110: packages/core/src/physics/joint/Joint.ts#L110
Added line #L110 was not covered by tests
[warning] 124-124: packages/core/src/physics/joint/Joint.ts#L124
Added line #L124 was not covered by tests
[warning] 246-247: packages/core/src/physics/joint/Joint.ts#L246-L247
Added lines #L246 - L247 were not covered by tests
[warning] 251-254: packages/core/src/physics/joint/Joint.ts#L251-L254
Added lines #L251 - L254 were not covered by tests
🪛 Biome
tests/src/core/physics/Joint.test.ts
[error] 174-174: 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)
[error] 220-220: 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)
tests/src/core/physics/SpringJoint.test.ts
[error] 41-41: 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)
[error] 71-71: 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)
[error] 94-94: 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)
[error] 124-124: 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)
[error] 147-147: 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)
[error] 169-169: 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)
[error] 200-200: 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 (21)
packages/core/src/physics/joint/SpringJoint.ts (2)
86-89: Past review comment about error handling is still applicable.
92-98: 🛠️ Refactor suggestion
Add error handling and verify native joint initialization.
The _syncBackends method should verify that the native joint is properly initialized before attempting to sync properties.
protected override _syncBackends(): void {
super._syncBackends();
+ if (!this._nativeJoint) {
+ throw new Error("Failed to sync SpringJoint: Native joint not initialized");
+ }
(<ISpringJoint>this._nativeJoint).setMinDistance(this._minDistance);
(<ISpringJoint>this._nativeJoint).setMaxDistance(this._maxDistance);
(<ISpringJoint>this._nativeJoint).setTolerance(this._tolerance);
(<ISpringJoint>this._nativeJoint).setStiffness(this._stiffness);
(<ISpringJoint>this._nativeJoint).setDamping(this._damping);
}packages/core/src/physics/shape/ColliderShape.ts (2)
147-153: Verify test coverage for backend synchronization.
While the new _syncBackends method improves code organization, we should ensure it's properly covered by unit tests, especially for error cases and the order of operations.
#!/bin/bash
# Check for existing tests related to backend synchronization
rg -A 5 "describe.*ColliderShape.*sync|test.*sync.*backend" --type ts5-5: Verify the impact of switching to @deepClone.
The change from @ignoreClone to @deepClone for _rotation and _position properties will result in deep cloning of these Vector3 objects. While this ensures complete isolation between cloned instances, it might impact performance during cloning operations.
Also applies to: 25-28
✅ Verification successful
These scripts will help us understand:
- How ColliderShape cloning is tested
- How Vector3 cloning is handled in similar contexts
- If there's a pattern of using deepClone with Vector3 properties
This will give us better context about whether the switch to @deepClone is justified and follows established patterns in the codebase.
These scripts will help us understand:
- The relationship between Vector3 properties and cloning decorators
- Current patterns of decorating Vector3 properties
- The context of ColliderShape class where these changes are being made
@deepClone is appropriate for position and rotation properties
The change from @ignoreClone to @deepClone for _position and _rotation Vector3 properties is justified:
- These properties are instance-specific and represent the shape's spatial configuration
- Other internal references like
_collider,_nativeShape, and_idcorrectly use@ignoreClone - Vector3 instances need deep cloning to prevent shared references between cloned shapes, which could lead to unintended modifications
The performance impact of deep cloning these two Vector3 properties is minimal compared to the potential bugs from shared references.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these properties are modified after cloning in any test files
rg -A 5 "clone.*ColliderShape.*rotation|position" --type ts
Length of output: 71
Script:
#!/bin/bash
# Let's try a broader search for ColliderShape cloning tests
rg -A 10 "ColliderShape.*clone" --type ts
# Also search for Vector3 cloning tests since position and rotation are Vector3
rg -A 10 "Vector3.*clone" --type ts
# Check for any usage of deepClone decorator on Vector3 properties
ast-grep --pattern '@deepClone
$_: Vector3'
Length of output: 127
Script:
#!/bin/bash
# Let's try a different approach to understand the cloning behavior
# Check for any files that import both Vector3 and cloning decorators
rg -l "import.*Vector3.*import.*(@ignoreClone|@deepClone)" --type ts
# Look for existing patterns of Vector3 property decorators
ast-grep --pattern '@$_
$_: Vector3'
# Check the ColliderShape class implementation
rg -A 20 "class ColliderShape" --type ts
Length of output: 1696
packages/core/src/physics/joint/HingeJoint.ts (2)
146-157: 🛠️ Refactor suggestion
Add test coverage for motor synchronization.
The _syncBackends method needs test coverage for the motor synchronization logic. Also, note that the gearRation typo needs to be fixed here as well.
Let's verify the missing test coverage:
Apply this diff to fix the typo:
if (motor) {
(<IHingeJoint>this._nativeJoint).setDriveVelocity(motor.targetVelocity);
(<IHingeJoint>this._nativeJoint).setDriveForceLimit(motor.forceLimit);
- (<IHingeJoint>this._nativeJoint).setDriveGearRatio(motor.gearRation);
+ (<IHingeJoint>this._nativeJoint).setDriveGearRatio(motor.gearRatio);
(<IHingeJoint>this._nativeJoint).setHingeJointFlag(HingeJointFlag.DriveFreeSpin, motor.freeSpin);
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 153-157: packages/core/src/physics/joint/HingeJoint.ts#L153-L157
Added lines #L153 - L157 were not covered by tests
15-24: LGTM! Properties are well-structured.
The properties are well-organized with appropriate types and decorators. The caching mechanism for _angle and _velocity is a good optimization.
Let's verify test coverage for these properties:
✅ Verification successful
Properties are well-tested with comprehensive coverage
The test file shows thorough coverage of all properties:
_axisis tested in "axis" test case with proper verification of angle and velocity_hingeFlagsfunctionality is verified throughuseLimits,useMotor, anduseSpringtests_useSpringis tested in "softLimit" test case_jointMonitor(motor) is extensively tested in "motor", "forceLimit", "gearRation", and "freeSpin" test cases_limitsis verified in both "hardLimit" and "softLimit" test cases_angleand_velocitycaching is validated across multiple test cases with precise value checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for HingeJoint properties
rg -A 5 "describe.*HingeJoint" "tests/"
Length of output: 52
Script:
#!/bin/bash
# Let's try a broader search for test files related to joints and hinges
fd "test|spec" -e ts | xargs rg -l "HingeJoint|hinge.*joint" -i
# Also search for the test directory structure
fd "test|spec" -t d
Length of output: 212
Script:
#!/bin/bash
# Let's also check the implementation details to understand the test requirements
rg "class HingeJoint" -A 5
Length of output: 485
Script:
#!/bin/bash
# Search for any usage patterns to determine test coverage needs
rg "new HingeJoint" -A 3
Length of output: 26
Script:
#!/bin/bash
# Let's examine the test file content to verify coverage of the properties
cat "tests/src/core/physics/HingeJoint.test.ts"
Length of output: 10394
packages/core/src/physics/joint/Joint.ts (2)
15-26: LGTM! Good architectural improvements
The changes improve the class design by:
- Making it abstract to enforce proper inheritance
- Adding static temp vector to reduce GC pressure
- Using deepClone decorators for proper object copying
191-222: LGTM! Well-structured lifecycle management
The lifecycle methods and sync logic are well implemented with proper separation of concerns. However, we should verify that all properties are properly synced when the joint is re-enabled.
Let's verify the sync behavior:
✅ Verification successful
Let me gather more information to verify if there are any property setters that modify these values and if they properly call _syncBackends.
Let me try a different approach to find property setters.
Property setters properly handle native joint updates
The verification shows that all property setters correctly update both the internal state and the native joint when it exists. The _syncBackends method in _onEnableInScene ensures a complete sync of all properties when the joint is re-enabled, while individual setters maintain consistency during runtime. This is a robust implementation that maintains proper synchronization between the high-level joint properties and the native joint instance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other places where these properties are modified without calling _syncBackends
rg -A 2 "this\._nativeJoint\.(set|get)" --type ts
Length of output: 1139
Script:
#!/bin/bash
# Search for property setters that might modify joint properties
ast-grep --pattern 'set $_($$$) {
$$$
}' packages/core/src/physics/joint/Joint.ts
Length of output: 85
Script:
#!/bin/bash
# Search for property setters using ripgrep
rg "set \w+\(" packages/core/src/physics/joint/Joint.ts -A 5
Length of output: 2189
tests/src/core/physics/SpringJoint.test.ts (3)
1-33: LGTM! Well-structured test setup
The test setup follows best practices with proper initialization, cleanup, and a reusable helper function.
41-41: Refactor assignments in expressions for better readability
The assignments within expressions make the code harder to read and maintain, as flagged by the static analysis tool. This pattern is repeated across multiple test cases.
Also applies to: 71-71, 94-94, 124-124, 147-147, 169-169, 200-200
🧰 Tools
🪛 Biome
[error] 41-41: 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)
35-231: LGTM! Comprehensive test coverage
The test suite thoroughly covers:
- Distance constraints (min/max)
- Tolerance handling
- Stiffness variations (low/high)
- Damping effects
- Component cloning
- Inactive state modifications
The assertions effectively verify the expected behaviors in each scenario.
🧰 Tools
🪛 Biome
[error] 41-41: 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)
[error] 71-71: 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)
[error] 94-94: 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)
[error] 124-124: 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)
[error] 147-147: 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)
[error] 169-169: 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)
[error] 200-200: 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)
tests/src/core/physics/Collider.test.ts (3)
49-64: Well-structured helper functions!
The addition of addBox and formatValue helper functions improves test maintainability and readability. addBox nicely encapsulates common entity creation logic, while formatValue properly handles floating-point precision comparisons.
Line range hint 68-90: Test setup properly configured!
The test setup is well-structured with proper initialization in beforeAll and beforeEach hooks. The physicsBox.material.bounciness value of 0.1 correctly aligns with the default value in PhysicsMaterial.ts.
348-357: Good coverage of inactive collider modifications!
The test case thoroughly verifies that shape management operations (add, remove, clear) and collider destruction work correctly on inactive colliders.
packages/core/src/physics/DynamicCollider.ts (2)
25-25: Verify the impact of maxDepenetrationVelocity change
Changing maxDepenetrationVelocity from a finite value (1000) to Infinity could affect physics behavior, particularly in scenarios with deep penetration between colliders. This might lead to unexpected object behavior or performance issues.
#!/bin/bash
# Search for tests or usage patterns of maxDepenetrationVelocity
rg -l "maxDepenetrationVelocity" --type tsAlso applies to: 31-32
407-431:
Enhance error handling and test coverage
The private utility methods need:
- Test coverage for error conditions
- Error handling for invalid physics states
- Validation of input parameters
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 424-425: packages/core/src/physics/DynamicCollider.ts#L424-L425
Added lines #L424 - L425 were not covered by tests
[warning] 430-431: packages/core/src/physics/DynamicCollider.ts#L430-L431
Added lines #L430 - L431 were not covered by tests
tests/src/core/physics/Joint.test.ts (1)
174-174: **** The assignments within expressions have already been addressed in previous review comments.
Also applies to: 220-220
🧰 Tools
🪛 Biome
[error] 174-174: 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)
tests/src/core/physics/DynamicCollider.test.ts (1)
50-59: LGTM!
The test setup follows best practices by:
- Creating fresh test environment in beforeAll
- Cleaning up between tests in beforeEach
tests/src/core/physics/PhysicsMaterial.test.ts (1)
246-249: Fix incorrect force application in tests.
The force is being applied to the deactivated collider instead of the new one in several test cases.
This issue was already identified in a previous review. The force should be applied to collider2 instead of collider.
Also applies to: 280-283, 314-317
tests/src/core/physics/HingeJoint.test.ts (2)
64-65: Consider encapsulating the physics update call and documenting @ts-ignore usage.
This comment addresses the repeated use of @ts-ignore for physics._update calls. It's recommended to create a helper function that encapsulates this internal API call and includes documentation explaining the necessity of @ts-ignore.
Also applies to: 90-91, 118-119, 140-141, 164-165, 168-169, 192-193, 214-215, 222-223, 227-228, 252-253
257-269: Ensure modifications to inactive entities behave as expected.
Modifying components on inactive entities may not have the intended effect until the entity is activated. Verify that this behavior is as expected for your test case.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (11)
packages/core/src/physics/joint/JointMotor.ts (5)
4-8: Enhance class documentationWhile the class has basic documentation, it would be more helpful to include:
- Example usage
- Common use cases
- Relationship with other joint types
Consider expanding the documentation like this:
/** * The JointMotor is used to motorize a joint. + * + * Used to control joint motion by applying forces to reach a target velocity. + * Common use cases include: + * - Motorized hinges + * - Powered wheels + * - Robotic joint control + * + * @example + * ```ts + * const motor = new JointMotor(); + * motor.targetVelocity = 5; // rad/s + * motor.forceLimit = 1000; // N + * ``` */
15-25: Specify units in targetVelocity documentationThe documentation should specify the units for velocity and force to prevent confusion.
Update the documentation:
/** - * The motor will apply a force up to force to achieve targetVelocity. + * The motor will apply a force up to forceLimit to achieve the target velocity. + * @param value Target velocity in radians per second (rad/s) */
27-37: Improve forceLimit documentation and consider a more reasonable defaultThe current implementation uses Number.MAX_VALUE as default, which might be excessive. Also, the documentation should specify units and valid ranges.
Consider these improvements:
/** * The force limit. + * @param value Maximum force in Newtons (N) + * @min 0 + * @default 1000 */ get forceLimit(): number { return this._forceLimit; } private _forceLimit: number = Number.MAX_VALUE; + private _forceLimit: number = 1000; // Default to 1000N
39-49: Enhance gearRatio documentationThe current documentation doesn't explain the purpose or effect of the gear ratio.
Improve the documentation:
/** - * Gear ration for the motor + * Gear ratio for the motor. Multiplies the motor's effective force and divides + * its velocity. A ratio > 1 increases torque but reduces speed, while a ratio < 1 + * does the opposite. + * @param value The gear ratio multiplier + * @min 0 + * @default 1.0 */
51-61: Add usage example to freeSpin documentationWhile the current documentation explains the behavior, an example would make it clearer.
Enhance the documentation:
/** * If freeSpin is enabled the motor will only accelerate but never slow down. + * Useful for implementing one-way motors or ratchet mechanisms. + * @example + * ```ts + * // Create a motor that only accelerates in one direction + * motor.freeSpin = true; + * motor.targetVelocity = 5; // Motor will accelerate to 5 rad/s + * motor.targetVelocity = 2; // Motor will maintain current speed instead of slowing down + * ``` */packages/core/src/physics/joint/JointLimits.ts (2)
10-14: Consider adding value validation for physics properties.While the private properties are well-encapsulated, some properties might benefit from validation:
_contactDistancestarts at -1, but the documentation suggests it should be positive_maxand_minmight need relative validation (max should be greater than min)Consider adding validation in the setters:
set max(value: number) { + if (value < this._min) { + throw new Error("Max value cannot be less than min value"); + } if (this._max !== value) { this._max = value; this._updateFlagManager.dispatch(); } }
59-85: Enhance validation and documentation for spring properties.The stiffness and damping properties need additional validation and more detailed documentation:
- Both properties should likely be non-negative
- Documentation should specify units and typical value ranges
Add validation and enhance documentation:
/** * The spring forces used to reach the target position. + * @param value Non-negative value representing the spring constant (N/m) + * @throws {Error} If value is negative */ set stiffness(value: number) { + if (value < 0) { + throw new Error("Stiffness must be non-negative"); + } if (this._stiffness !== value) { this._stiffness = value; this._updateFlagManager.dispatch(); } } /** * The damper force uses to dampen the spring. + * @param value Non-negative value representing the damping coefficient (Ns/m) + * @throws {Error} If value is negative */ set damping(value: number) { + if (value < 0) { + throw new Error("Damping must be non-negative"); + } if (this._damping !== value) { this._damping = value; this._updateFlagManager.dispatch(); } }packages/core/src/physics/joint/HingeJoint.ts (2)
45-60: Document caching behavior for angle and velocity properties.The getters now cache values in
_angleand_velocity. Consider adding JSDoc comments to document this caching behavior and its implications./** * The current angle in degrees of the joint relative to its rest position. + * @remarks The value is cached after reading from the native joint. */ get angle(): number { /** * The angular velocity of the joint in degrees per second. + * @remarks The value is cached after reading from the native joint. */ get velocity(): Readonly<number> {
Line range hint
113-141: Use optional chaining for event listener registration.Improve the code's robustness by using optional chaining when registering event listeners.
set motor(value: JointMotor) { if (this._jointMotor !== value) { - if (this._jointMotor) { - this._jointMotor._updateFlagManager.removeListener(this._onMotorChanged); - } + this._jointMotor?._updateFlagManager.removeListener(this._onMotorChanged); this._jointMotor = value; - value && value._updateFlagManager.addListener(this._onMotorChanged); + value?._updateFlagManager.addListener(this._onMotorChanged); this._onMotorChanged(); } } set limits(value: JointLimits) { if (this._limits !== value) { - if (this._limits) { - this._limits._updateFlagManager.removeListener(this._onLimitsChanged); - } + this._limits?._updateFlagManager.removeListener(this._onLimitsChanged); this._limits = value; - value && value._updateFlagManager.addListener(this._onLimitsChanged); + value?._updateFlagManager.addListener(this._onLimitsChanged); this._onLimitsChanged(); } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 109-109: packages/core/src/physics/joint/HingeJoint.ts#L109
Added line #L109 was not covered by tests
[warning] 115-116: packages/core/src/physics/joint/HingeJoint.ts#L115-L116
Added lines #L115 - L116 were not covered by tests🪛 Biome
[error] 119-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
tests/src/core/physics/SpringJoint.test.ts (2)
11-22: Consider extracting addBox helper to a shared test utilities fileThe
addBoxhelper function is a great addition to streamline the creation of box entities in the tests. However, since it's a generic utility that could be useful across multiple test files, consider extracting it to a shared test utilities file.This would promote reusability and maintainability of the test codebase.
221-231: Test inActive modification edge casesThe "inActive modification" test case covers the scenario where a
SpringJointcomponent is added to an inactive entity and then destroyed.Consider adding more edge cases to this test to ensure the robustness of the
SpringJointcomponent when dealing with inactive entities, such as:
- Modifying joint properties while the entity is inactive and then activating the entity.
- Deactivating an entity with an active joint and verifying that the joint is properly deactivated.
- Destroying the joint component while the entity is inactive and then activating the entity.
I can provide examples of how to implement these additional test cases if you'd like. Do you want me to go ahead and propose the code changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/core/src/physics/joint/HingeJoint.ts(6 hunks)packages/core/src/physics/joint/JointLimits.ts(1 hunks)packages/core/src/physics/joint/JointMotor.ts(1 hunks)tests/src/core/physics/HingeJoint.test.ts(1 hunks)tests/src/core/physics/Joint.test.ts(1 hunks)tests/src/core/physics/SpringJoint.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/src/core/physics/HingeJoint.test.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/physics/joint/HingeJoint.ts
[warning] 109-109: packages/core/src/physics/joint/HingeJoint.ts#L109
Added line #L109 was not covered by tests
[warning] 115-116: packages/core/src/physics/joint/HingeJoint.ts#L115-L116
Added lines #L115 - L116 were not covered by tests
[warning] 135-135: packages/core/src/physics/joint/HingeJoint.ts#L135
Added line #L135 was not covered by tests
[warning] 158-162: packages/core/src/physics/joint/HingeJoint.ts#L158-L162
Added lines #L158 - L162 were not covered by tests
🪛 Biome
packages/core/src/physics/joint/HingeJoint.ts
[error] 119-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 139-139: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
tests/src/core/physics/Joint.test.ts
[error] 174-174: 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)
[error] 220-220: 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)
tests/src/core/physics/SpringJoint.test.ts
[error] 41-41: 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)
[error] 71-71: 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)
[error] 94-94: 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)
[error] 124-124: 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)
[error] 147-147: 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)
[error] 169-169: 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)
[error] 200-200: 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 (12)
packages/core/src/physics/joint/JointMotor.ts (1)
8-8: Verify UpdateFlagManager usage across the physics system
The class consistently uses UpdateFlagManager to notify about property changes. Let's verify this pattern is used consistently across related classes.
Also applies to: 24-24, 36-36, 48-48, 60-60
✅ Verification successful
Based on the results, I can now generate the final response as we have sufficient information about the UpdateFlagManager usage pattern.
UpdateFlagManager is consistently implemented in physics joint components
The UpdateFlagManager is properly used in both JointMotor and JointLimits classes, with consistent dispatch patterns after property changes. The implementation follows the same pattern across all properties:
- All property setters call
dispatch()after value updates - Both classes maintain proper encapsulation with private fields and public accessors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other physics-related files using UpdateFlagManager
echo "Checking UpdateFlagManager usage pattern in physics-related files:"
rg -l "UpdateFlagManager" "packages/core/src/physics"
echo "\nChecking dispatch pattern in physics-related files:"
rg "dispatch\(\)" "packages/core/src/physics" -A 1 -B 1
Length of output: 2426
packages/core/src/physics/joint/JointLimits.ts (1)
1-8: LGTM! Verify UpdateFlagManager integration.
The addition of UpdateFlagManager for change tracking is a good practice. The class documentation and internal flag setup look correct.
Let's verify the UpdateFlagManager usage pattern across other joint-related files:
✅ Verification successful
UpdateFlagManager integration is correctly implemented
The verification shows that:
- Both JointLimits and JointMotor consistently use UpdateFlagManager with the same pattern
- They are properly integrated into HingeJoint which manages them as private properties with getters/setters
- Both classes are correctly exported through the joint module's index
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if UpdateFlagManager is consistently used across joint files
# Expected: Similar usage pattern in other joint files
# Search for UpdateFlagManager usage in joint files
rg -l "UpdateFlagManager" "packages/core/src/physics/joint/"
Length of output: 151
Script:
#!/bin/bash
# Let's check the implementation pattern in JointMotor.ts for comparison
rg "UpdateFlagManager" "packages/core/src/physics/joint/JointMotor.ts" -B 2 -A 2
# Also check how these classes are used together
rg "JointLimits|JointMotor" "packages/core/src/physics/joint/" -l
Length of output: 526
Script:
#!/bin/bash
# Let's check how HingeJoint uses these classes
rg "JointLimits|JointMotor" "packages/core/src/physics/joint/HingeJoint.ts" -B 1 -A 1
# Also verify the exports
cat "packages/core/src/physics/joint/index.ts"
Length of output: 956
packages/core/src/physics/joint/HingeJoint.ts (3)
15-24: LGTM: Improved property declarations with appropriate decorators.
The switch to @deepClone for complex objects (_axis, _jointMotor, _limits) ensures proper cloning behavior, which is essential for maintaining the integrity of joint instances when cloned.
165-183: LGTM: Well-structured event handlers.
The event handlers are well-implemented with:
- Proper null checks before accessing the native joint
- Clear separation between soft and hard limit handling
- Appropriate synchronization of all relevant properties
151-162: Add test coverage for _syncBackends method.
The _syncBackends method lacks test coverage, particularly for the motor synchronization logic. This is critical functionality that should be thoroughly tested.
Would you like me to help generate comprehensive test cases for the _syncBackends method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 158-162: packages/core/src/physics/joint/HingeJoint.ts#L158-L162
Added lines #L158 - L162 were not covered by tests
tests/src/core/physics/Joint.test.ts (3)
11-26: Well-structured helper functions!
The helper functions improve test maintainability:
addBox: Encapsulates box entity creation logicformatValue: Handles floating-point precision consistently
39-338: Comprehensive test coverage!
The test suite thoroughly covers various joint behaviors:
- Connection scenarios (null and inter-collider)
- Mass and inertia scaling
- Break force and torque
- Cloning and inactive state modifications
🧰 Tools
🪛 Biome
[error] 174-174: 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)
[error] 220-220: 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)
39-338: Well-structured test cases!
The tests follow best practices:
- Clear AAA (Arrange-Act-Assert) pattern
- Focused test cases
- Appropriate assertion matchers
🧰 Tools
🪛 Biome
[error] 174-174: 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)
[error] 220-220: 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)
tests/src/core/physics/SpringJoint.test.ts (4)
1-232: Comprehensive test suite for SpringJoint component
The test file provides a thorough set of unit tests for the SpringJoint component, covering various scenarios and configurations. The tests validate the behavior of the joint under different property settings such as minDistance, maxDistance, tolerance, stiffness, and damping. The use of the addBox helper function promotes code reuse and improves readability.
Great job on implementing a well-structured and comprehensive test suite!
🧰 Tools
🪛 Biome
[error] 41-41: 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)
[error] 71-71: 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)
[error] 94-94: 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)
[error] 124-124: 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)
[error] 147-147: 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)
[error] 169-169: 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)
[error] 200-200: 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)
52-53: Remove @ts-ignore by properly typing physics update method
The repeated use of @ts-ignore for physics._update suggests a typing issue that should be addressed properly instead of being ignored.
Consider either:
- Adding proper type definitions for the internal _update method
- Creating a test utility method that properly exposes this functionality
// Add to a test utilities file
export function updatePhysics(scene: Scene, deltaTime: number): void {
(scene.physics as any)._update(deltaTime);
}Also applies to: 82-83, 105-106, 135-136, 158-159, 180-181
41-41: Refactor assignments in expressions
The assignments within expressions make the code harder to read and maintain. They've also been flagged by the static analysis tool.
Refactor these assignments to be more explicit:
- const collider = (joint.connectedCollider = boxEntity.getComponent(DynamicCollider));
+ const collider = boxEntity.getComponent(DynamicCollider);
+ joint.connectedCollider = collider;Also applies to: 71-71, 94-94, 124-124, 147-147, 169-169, 200-200
🧰 Tools
🪛 Biome
[error] 41-41: 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)
194-219: Verify cloned entity behavior
The "clone" test case creates a clone of boxEntity2 and deactivates the original entity. It then adds the cloned entity to the rootEntity and performs an update.
Verify that the cloned entity behaves independently of the original deactivated entity and that the joint configuration is correctly cloned.
To verify the cloned entity's behavior, you can add assertions to check the position and velocity of the cloned entity after the update and compare them with the original deactivated entity.
// ...
// @ts-ignore
engine.sceneManager.activeScene.physics._update(1);
expect(newBox.transform.position.x).eq(6);
expect(boxEntity2.transform.position.x).eq(2); // Original entity should remain at its initial position
const clonedCollider = newBox.getComponent(DynamicCollider);
expect(clonedCollider.linearVelocity.x).not.eq(0); // Cloned entity should have non-zero velocity
expect(collider2.linearVelocity.x).eq(0); // Original entity should have zero velocity🧰 Tools
🪛 Biome
[error] 200-200: 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
🧹 Outside diff range and nitpick comments (2)
packages/core/src/physics/DynamicCollider.ts (2)
66-70: Consider refactoring the velocity getter implementationsThe current implementation temporarily nullifies and reassigns _onValueChanged, which is a code smell. Consider:
- Creating a utility method to handle this pattern
- Finding an alternative approach that doesn't require ts-ignore
+ private _withoutValueChangeCallback<T>(vector: Vector3, callback: () => void): void { + const originalCallback = (vector as any)._onValueChanged; + (vector as any)._onValueChanged = null; + callback(); + (vector as any)._onValueChanged = originalCallback; + } get linearVelocity(): Vector3 { - //@ts-ignore - this._linearVelocity._onValueChanged = null; - (<IDynamicCollider>this._nativeCollider).getLinearVelocity(this._linearVelocity); - //@ts-ignore - this._linearVelocity._onValueChanged = this._setLinearVelocity; + this._withoutValueChangeCallback(this._linearVelocity, () => { + (<IDynamicCollider>this._nativeCollider).getLinearVelocity(this._linearVelocity); + }); return this._linearVelocity; }Also applies to: 84-88
435-440: Consider breaking down _setMassAndUpdateInertia methodThe method handles multiple responsibilities. Consider splitting it into smaller, focused methods for better maintainability.
+ private _updateCenterOfMass(): void { + !this._automaticCenterOfMass && + (<IDynamicCollider>this._nativeCollider).setCenterOfMass(this._centerOfMass); + } + + private _updateInertiaTensor(): void { + !this._automaticInertiaTensor && + (<IDynamicCollider>this._nativeCollider).setInertiaTensor(this._inertiaTensor); + } private _setMassAndUpdateInertia(): void { (<IDynamicCollider>this._nativeCollider).setMassAndUpdateInertia(this._mass); - !this._automaticCenterOfMass && (<IDynamicCollider>this._nativeCollider).setCenterOfMass(this._centerOfMass); - !this._automaticInertiaTensor && (<IDynamicCollider>this._nativeCollider).setInertiaTensor(this._inertiaTensor); + this._updateCenterOfMass(); + this._updateInertiaTensor(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/physics/DynamicCollider.ts(9 hunks)
🔇 Additional comments (4)
packages/core/src/physics/DynamicCollider.ts (4)
310-328: Add test coverage for shape management methods
These methods handle critical physics calculations when shapes are modified. Ensure comprehensive test coverage for:
- Shape operations with automatic calculations enabled/disabled
- Mass and inertia updates after shape changes
25-25: Verify the specific maxDepenetrationVelocity value
The value 1.0000000331813535e32 seems unusually specific. Please verify if this is the intended value and document the reasoning behind this specific number.
✅ Verification successful
Based on the search results, I can now provide a final response:
The maxDepenetrationVelocity value is intentionally set to a very large number
The value 1.0000000331813535e32 appears to be an intentionally set very large default value that effectively acts as "infinity" for the maximum velocity allowed when resolving penetration between colliding objects. This is evidenced by:
- The tests show that this value can be overridden to smaller values like 0 or 10 when specific penetration velocity limits are needed
- The PhysX implementation fully supports this parameter while the Physics-lite implementation explicitly does not support it
- The documentation indicates this is used as "Maximum velocity of a collider when moving out of penetrating state"
- Having a very large default allows objects to separate quickly by default when they penetrate each other, unless explicitly limited
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any documentation or comments about this value
rg -i "maxdepenetrationvelocity.*1\.0+e32"
Length of output: 44
Script:
#!/bin/bash
# Let's search for any occurrences of this specific number in the codebase
rg "1\.0000000331813535e32"
# Also search for any comments or documentation containing "depenetration"
rg -i "depenetration" -A 2 -B 2
# Let's also check if this value is used in any physics-related constants or configurations
rg "maxDepenetrationVelocity" -A 2 -B 2
Length of output: 39618
Line range hint 336-367: Verify inactive state handling
The addition of _phasedActiveInScene checks is good for preventing invalid operations, but needs testing to ensure proper behavior in all states.
✅ Verification successful
Let's also check the implementation of _phasedActiveInScene to understand its usage:
And let's look for any existing test patterns for similar physics operations:
Let's also check for any enable/disable testing patterns in the Component base class since _phasedActiveInScene is defined there:
Based on the examination of the DynamicCollider test file and the Component implementation, I can now provide a final response:
Inactive state handling is properly tested and verified
The DynamicCollider test file includes comprehensive testing for inactive state handling in the "inActive modification" test case, which verifies that:
- All physics operations are properly gated by the _phasedActiveInScene check
- Operations during inactive state don't affect the physics state
- Operations resume correctly when the entity becomes active again
Key test evidence:
- Test case "inActive modification" specifically tests all physics operations while inactive
- The test verifies that position remains unchanged during inactive state
- The test confirms operations resume working after reactivation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests covering inactive state handling
rg -l "test.*inactive.*collider|test.*collider.*inactive"
Length of output: 59
Script:
#!/bin/bash
# Let's search for test files related to DynamicCollider
fd "DynamicCollider.*test" --type f
# Also search for any test files containing "collider" and "active" or "inactive"
rg -l "collider.*(?:active|inactive)|(?:active|inactive).*collider" --type-add 'test:*.{test,spec}.*' -t test
Length of output: 196
Script:
#!/bin/bash
# Search for _phasedActiveInScene definition and usage
rg "_phasedActiveInScene" -A 2 -B 2
Length of output: 3592
Script:
#!/bin/bash
# Search for test cases related to physics operations
rg "(?:describe|it).*(?:force|torque|move)" --type-add 'test:*.{test,spec}.*' -t test -A 5
Length of output: 6728
Script:
#!/bin/bash
# Let's examine the DynamicCollider test file specifically
cat tests/src/core/physics/DynamicCollider.test.ts
Length of output: 21139
Script:
#!/bin/bash
# Search for enable/disable testing in Component tests
rg "(?:describe|it).*(?:enable|disable|active)" tests/src/core/Component.test.ts -A 5
Length of output: 160
13-14: Verify the impact of removing @ignoreClone decorators
The removal of @ignoreClone decorators from multiple properties will change their cloning behavior. This could impact how DynamicCollider instances are duplicated.
Also applies to: 19-19, 24-27, 30-32
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/core/src/physics/Collider.ts (1)
127-130: Add JSDoc comment for the hook methodThe
_handleShapesChangedappears to be a hook method for derived classes. Please add documentation explaining its purpose and when it gets called.packages/core/src/physics/CharacterController.ts (1)
64-65: Update documentation to reference Math.SQRT1_2For consistency with the code change, update the documentation to reference Math.SQRT1_2 instead of the magic number.
/** * The slope limit for the controller, the value is the cosine value of the maximum slope angle. - * @defaultValue 0.707(the cosine value of 45 degrees) + * @defaultValue Math.SQRT1_2 (the cosine value of 45 degrees) */tests/src/core/physics/Joint.test.ts (2)
11-22: Add JSDoc documentation for theaddBoxhelper functionThe helper function would benefit from documentation describing its parameters and return value.
+/** + * Creates a box entity with physics properties + * @param cubeSize - The size of the box + * @param type - The type of collider to add + * @param pos - The position of the box + * @returns The created box entity + */ function addBox(cubeSize: Vector3, type: typeof DynamicCollider | typeof StaticCollider, pos: Vector3) {
344-362: Split "inActive modification" test into multiple test casesThe test case "inActive modification" tests multiple behaviors in a single test. Consider splitting it into separate test cases for better clarity and maintainability:
- Test autoConnectedAnchor modifications
- Test collider connections
- Test scale modifications
- Test break force/torque modifications
it("should handle autoConnectedAnchor modifications when inactive", function () { // Test autoConnectedAnchor behavior }); it("should handle collider connections when inactive", function () { // Test collider connection behavior }); it("should handle scale modifications when inactive", function () { // Test scale modification behavior }); it("should handle break force/torque modifications when inactive", function () { // Test break force/torque behavior });packages/core/src/physics/DynamicCollider.ts (2)
25-25: Consider documenting the magic numberThe value
1.0000000331813535e32seems to be a very specific constant. Consider extracting it to a named constant with documentation explaining its significance and how it was derived.+ /** Maximum velocity constant derived from... */ + private static readonly MAX_DEPENETRATION_VELOCITY = 1.0000000331813535e32; - private _maxDepenetrationVelocity = 1.0000000331813535e32; + private _maxDepenetrationVelocity = DynamicCollider.MAX_DEPENETRATION_VELOCITY;
458-475: Consider using a custom error type for automatic mode warningsThe warning messages are helpful, but consider creating a custom error type for better error handling and consistency.
+ class AutomaticModeError extends Error { + constructor(property: string) { + super(`${property} is automatically calculated. Set automatic${property} to false for manual control.`); + this.name = 'AutomaticModeError'; + } + } private _handleCenterOfMassChanged(): void { if (this._automaticCenterOfMass) { - console.warn( - "The center of mass is automatically calculated, please set automaticCenterOfMass to false if you want to set it manually." - ); + console.warn(new AutomaticModeError('CenterOfMass').message); } else { (<IDynamicCollider>this._nativeCollider).setCenterOfMass(this._centerOfMass); } }tests/src/core/physics/DynamicCollider.test.ts (1)
21-53: Consider improving helper functions for better maintainability.The helper functions could be enhanced in several ways:
- Extract magic numbers into named constants
- Make friction values configurable parameters
- Use a more specific type for the collider parameter in addBox
+ const PLANE_SIZE = 20; + const DEFAULT_FRICTION = 0; + + interface ColliderType { + new (...args: any[]): DynamicCollider | StaticCollider; + } + function addPlane(x: number, y: number, z: number) { const planeEntity = rootEntity.createChild("PlaneEntity"); planeEntity.transform.setPosition(x, y, z); - planeEntity.transform.setScale(20, 1, 20); + planeEntity.transform.setScale(PLANE_SIZE, 1, PLANE_SIZE); const physicsPlane = new PlaneColliderShape(); - physicsPlane.material.dynamicFriction = 0; - physicsPlane.material.staticFriction = 0; + physicsPlane.material.dynamicFriction = DEFAULT_FRICTION; + physicsPlane.material.staticFriction = DEFAULT_FRICTION; const planeCollider = planeEntity.addComponent(StaticCollider); planeCollider.addShape(physicsPlane); return planeEntity; } - function addBox(cubeSize: Vector3, type: typeof DynamicCollider | typeof StaticCollider, pos: Vector3) { + function addBox(cubeSize: Vector3, type: ColliderType, pos: Vector3) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/core/src/physics/CharacterController.ts(3 hunks)packages/core/src/physics/Collider.ts(5 hunks)packages/core/src/physics/DynamicCollider.ts(9 hunks)packages/core/src/physics/shape/ColliderShape.ts(4 hunks)tests/src/core/physics/DynamicCollider.test.ts(1 hunks)tests/src/core/physics/Joint.test.ts(1 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/CharacterController.ts
[error] 18-18: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
tests/src/core/physics/Joint.test.ts
[error] 191-191: 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)
[error] 238-238: 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 (21)
packages/core/src/physics/Collider.ts (6)
54-54: LGTM! Method name change improves clarity
The rename from _addPhysicsShape to _addNativeShape better reflects its purpose of synchronizing with the native collider implementation.
66-66: LGTM! Consistent method naming
The rename to _removeNativeShape maintains consistency with the _addNativeShape pattern.
110-110: LGTM! Improved scene integration
The simplified scene integration provides better separation of concerns by delegating collider management to the physics scene.
Also applies to: 117-117
132-136: LGTM! Well-structured native sync method
The new _syncNative method provides a clean way to ensure all shapes are properly synchronized with the native collider.
143-149: LGTM! Proper cleanup sequence
The destroy logic now ensures proper cleanup by removing and destroying shapes before clearing the array.
153-161: LGTM! Clean native shape management
The new _addNativeShape and _removeNativeShape methods provide a clear and focused API for managing shapes in the native collider.
packages/core/src/physics/CharacterController.ts (4)
18-18: Use Math.SQRT1_2 instead of magic number 0.707
For better precision and maintainability, replace the approximate value with the standard library constant.
- private _slopeLimit = 0.707;
+ private _slopeLimit = Math.SQRT1_2;🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
21-32: LGTM! Good improvements to stepOffset handling
The added validation and improved documentation make the behavior clearer and more robust.
147-147: LGTM! Clean simplification of scene enable logic
The streamlined approach of directly calling physics scene methods improves code clarity.
154-155: LGTM! Consistent with enable logic changes
The simplified disable logic maintains symmetry with the enable method changes.
tests/src/core/physics/Joint.test.ts (2)
51-52: Create a test utility for physics updates
Multiple occurrences of @ts-ignore are used to access the internal _update method.
Also applies to: 76-77, 99-100, 147-148, 173-174, 198-199, 219-220, 245-246, 291-292, 310-311, 337-338
191-191: Avoid assignments within expressions
For better readability, separate the assignments from the expressions.
Also applies to: 238-238
🧰 Tools
🪛 Biome (1.9.4)
[error] 191-191: 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)
packages/core/src/physics/DynamicCollider.ts (5)
108-114: LGTM! Good handling of automatic vs manual modes
The mass setter properly handles both automatic and manual modes, ensuring that inertia and center of mass are updated when necessary.
310-328: LGTM! Consistent shape management implementation
The shape management methods properly handle mass and inertia updates when shapes are modified. The implementation is consistent across all methods.
Line range hint 336-367: LGTM! Good safety check for inactive colliders
The addition of the _phasedActiveInScene check prevents operations on inactive colliders, which is a good safety measure.
422-441: LGTM! Comprehensive synchronization implementation
The _syncNative method properly synchronizes all properties with the native collider, handling both automatic and manual modes correctly.
444-449: Add test coverage for mass and inertia updates
The _setMassAndUpdateInertia method handles critical physics properties. Please ensure comprehensive test coverage.
✅ Verification successful
Test coverage for mass and inertia updates exists and is comprehensive
The test file contains thorough coverage for mass and inertia-related functionality:
- Mass updates and effects are tested in the "mass" test case
- Center of mass is tested in "centerOfMass" and "automaticCenterOfMass" test cases
- Inertia tensor is tested in "inertiaTensor" and "automaticInertiaTensor" test cases
The tests verify:
- Mass property updates and their effects on physics behavior
- Manual and automatic center of mass calculations
- Manual and automatic inertia tensor calculations
- The impact of these properties on object behavior
- Warning messages for invalid property modifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for mass and inertia updates
rg -l "describe.*DynamicCollider" | xargs rg "test.*mass.*update.*inertia"
Length of output: 76
Script:
#!/bin/bash
# First, let's find test files related to DynamicCollider
fd "DynamicCollider.*test|test.*DynamicCollider" -t f
# Then, let's look for any test files in physics directory
fd "test" -t f packages/core/src/physics/
# Let's also check for any mass or inertia related tests with broader search
rg -i "test.*mass|test.*inertia" packages/core/src/physics/
Length of output: 206
Script:
#!/bin/bash
# Let's examine the content of the DynamicCollider test file
cat tests/src/core/physics/DynamicCollider.test.ts
Length of output: 21192
tests/src/core/physics/DynamicCollider.test.ts (4)
103-110: 🛠️ Refactor suggestion
Create a test utility to encapsulate physics updates.
The test suite has excessive use of @ts-ignore to access internal physics update method. This makes the tests fragile and harder to maintain.
// PhysicsTestUtils.ts
export class PhysicsTestUtils {
static updatePhysics(scene: Scene, deltaTime: number): void {
(scene.physics as any)._update(deltaTime);
}
}
// Usage in tests
PhysicsTestUtils.updatePhysics(engine.sceneManager.activeScene, 1);Also applies to: 118-119, 133-134, 149-150, 169-170, 177-178, 189-190, 195-196, 237-238, 245-246
54-63:
Add cleanup in afterEach/afterAll hooks.
The test setup should clean up resources after tests to prevent memory leaks and ensure test isolation.
beforeEach(function () {
rootEntity.clearChildren();
});
+
+ afterEach(function () {
+ // Clean up any remaining physics objects
+ engine.sceneManager.activeScene.physics._update(0);
+ });
+
+ afterAll(function () {
+ // Dispose engine resources
+ engine.destroy();
+ });Likely invalid or redundant comment.
395-420: 🛠️ Refactor suggestion
Decouple tests from PhysX implementation details.
The collisionDetectionMode test is tightly coupled with PhysX internals. Consider testing the behavior rather than implementation details.
it("collisionDetectionMode", function () {
const box = addBox(new Vector3(2, 2, 2), DynamicCollider, new Vector3(0, 0, 0));
+ const target = addBox(new Vector3(2, 2, 2), StaticCollider, new Vector3(10, 0, 0));
const boxCollider = box.getComponent(DynamicCollider);
- // @ts-ignore
- const physX = boxCollider._nativeCollider._physXPhysics._physX;
boxCollider.collisionDetectionMode = CollisionDetectionMode.Discrete;
- // @ts-ignore
- expect(boxCollider._nativeCollider._pxActor.getRigidBodyFlags(physX.PxRigidBodyFlag.eENABLE_CCD)).toBeFalsy();
+ // Test high-speed collision with Discrete mode
+ boxCollider.linearVelocity = new Vector3(1000, 0, 0);
+ engine.sceneManager.activeScene.physics._update(1/60);
+ // Expect tunneling through the target due to discrete mode
+ expect(box.transform.position.x).toBeGreaterThan(10);
boxCollider.collisionDetectionMode = CollisionDetectionMode.Continuous;
- // @ts-ignore
- expect(boxCollider._nativeCollider._pxActor.getRigidBodyFlags(physX.PxRigidBodyFlag.eENABLE_CCD)).toBeTruthy();
+ box.transform.setPosition(0, 0, 0);
+ boxCollider.linearVelocity = new Vector3(1000, 0, 0);
+ engine.sceneManager.activeScene.physics._update(1/60);
+ // Expect collision detection despite high speed
+ expect(box.transform.position.x).toBeLessThan(10);Likely invalid or redundant comment.
494-530: 🛠️ Refactor suggestion
Break down large test case into focused scenarios.
The "inActive modification" test case is too large and tests multiple behaviors. This makes it harder to understand and maintain.
- it("inActive modification", function () {
+ describe("inactive entity modifications", () => {
+ let box: Entity;
+ let boxCollider: DynamicCollider;
+
+ beforeEach(() => {
+ box = addBox(new Vector3(2, 2, 2), DynamicCollider, new Vector3(0, 0, 0));
+ boxCollider = box.getComponent(DynamicCollider);
+ box.isActive = false;
+ });
+
+ it("should preserve properties when entity becomes active", () => {
+ boxCollider.linearDamping = 0.1;
+ boxCollider.mass = 1;
+ box.isActive = true;
+ expect(boxCollider.linearDamping).eq(0.1);
+ expect(boxCollider.mass).eq(1);
+ });
+
+ it("should apply kinematic movement only when active", () => {
+ boxCollider.isKinematic = true;
+ boxCollider.move(new Vector3(1, 0, 0));
+ box.isActive = true;
+ engine.sceneManager.activeScene.physics._update(1/60);
+ expect(box.transform.position.x).eq(0);
+
+ boxCollider.move(new Vector3(1, 0, 0));
+ engine.sceneManager.activeScene.physics._update(1/60);
+ expect(box.transform.position.x).eq(1);
+ });
+ });Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
packages/physics-physx/src/PhysXCollider.ts (1)
19-19: Consider adding proper typing for _pxActorThe
_pxActorproperty is typed asany, which bypasses TypeScript's type checking. Consider adding proper typing to maintain type safety and improve code maintainability.packages/core/src/physics/Collider.ts (1)
78-81: Consider optimizing shape removal loopWhile the implementation is correct, consider using Array.forEach or for...of for better readability:
- for (let i = 0, n = shapes.length; i < n; i++) { - this._removeNativeShape(shapes[i]); - } + shapes.forEach(shape => this._removeNativeShape(shape));packages/core/src/physics/joint/HingeJoint.ts (3)
38-38: Improve type safety in axis setter.While the optional chaining improves null safety, the type casting can be optimized.
-(<IHingeJoint>this._nativeJoint)?.setAxis(axis); +const nativeJoint = this._nativeJoint as IHingeJoint; +nativeJoint?.setAxis(axis);
74-74: Reduce type casting duplication in flag setters.Extract the native joint casting to a private getter to reduce code duplication.
+private get _nativeHingeJoint(): IHingeJoint | null { + return this._nativeJoint as IHingeJoint; +} -(<IHingeJoint>this._nativeJoint)?.setHingeJointFlag(HingeJointFlag.LimitEnabled, value); +this._nativeHingeJoint?.setHingeJointFlag(HingeJointFlag.LimitEnabled, value);Also applies to: 88-88, 102-102
154-166: Improve code organization in _syncNative method.The method has multiple type casts and could benefit from using the suggested
_nativeHingeJointgetter.protected override _syncNative(): void { super._syncNative(); const motor = this._jointMotor; + const nativeJoint = this._nativeHingeJoint; - (<IHingeJoint>this._nativeJoint).setAxis(this._axis); + nativeJoint?.setAxis(this._axis); // Apply similar changes to other lines }tests/src/core/physics/Collider.test.ts (2)
49-60: Add parameter validation to addBox utility function.While the function effectively encapsulates box creation logic, it should validate input parameters to prevent runtime errors.
function addBox(cubeSize: Vector3, type: typeof DynamicCollider | typeof StaticCollider, pos: Vector3) { + if (!cubeSize || !type || !pos) { + throw new Error("Required parameters cannot be null or undefined"); + } const boxEntity = rootEntity.createChild("BoxEntity"); boxEntity.transform.setPosition(pos.x, pos.y, pos.z);
68-69: Consider extracting physics material properties into constants.The bounciness value of 0.1 should be defined as a named constant for better maintainability and clarity.
+const PHYSICS_MATERIAL_PROPERTIES = { + BOUNCINESS: 0.1, + DYNAMIC_FRICTION: 0, + STATIC_FRICTION: 0 +}; beforeEach(function () { // ... - physicsBox.material.bounciness = 0.1; + physicsBox.material.bounciness = PHYSICS_MATERIAL_PROPERTIES.BOUNCINESS;Also applies to: 90-90
packages/core/src/physics/DynamicCollider.ts (3)
24-24: Consider documenting the maxDepenetrationVelocity constantThe value 1.0000000331813535e32 seems unusually specific. Consider adding a comment explaining the significance of this precise value.
314-314: Consider adding debug logging for inactive collidersWhen operations are skipped due to inactive colliders, consider adding debug logging to help with troubleshooting.
- applyForce(force: Vector3): void { - this._phasedActiveInScene && (<IDynamicCollider>this._nativeCollider).addForce(force); + applyForce(force: Vector3): void { + if (!this._phasedActiveInScene) { + console.debug('Skipping force application on inactive collider'); + return; + } + (<IDynamicCollider>this._nativeCollider).addForce(force); }Also applies to: 322-322, 345-345
439-458: Consider using a shared utility for warning messagesThe warning messages in _handleCenterOfMassChanged and _handleInertiaTensorChanged are similar and could be refactored to use a shared utility function.
+ private _warnAutoCalculated(property: string): void { + console.warn( + `The ${property} is automatically calculated, please set automatic${property} to false if you want to set it manually.` + ); + } private _handleCenterOfMassChanged(): void { if (this._automaticCenterOfMass) { - console.warn( - "The center of mass is automatically calculated, please set automaticCenterOfMass to false if you want to set it manually." - ); + this._warnAutoCalculated('center of mass'); } else { (<IDynamicCollider>this._nativeCollider).setCenterOfMass(this._centerOfMass); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
packages/core/src/physics/Collider.ts(5 hunks)packages/core/src/physics/DynamicCollider.ts(9 hunks)packages/core/src/physics/joint/HingeJoint.ts(6 hunks)packages/core/src/physics/shape/ColliderShape.ts(4 hunks)packages/physics-physx/src/PhysXCharacterController.ts(3 hunks)packages/physics-physx/src/PhysXCollider.ts(3 hunks)packages/physics-physx/src/PhysXPhysicsScene.ts(4 hunks)packages/physics-physx/src/shape/PhysXColliderShape.ts(2 hunks)tests/src/core/physics/Collider.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/physics-physx/src/PhysXCharacterController.ts
- packages/physics-physx/src/shape/PhysXColliderShape.ts
🔇 Additional comments (30)
packages/physics-physx/src/PhysXCollider.ts (3)
32-35:
Add parameter validation in addShape
The shape parameter should be validated before performing operations. Also consider adding error handling for duplicate shapes.
addShape(shape: PhysXColliderShape): void {
+ if (!shape) {
+ throw new Error("Shape cannot be null or undefined");
+ }
+ if (this._shapes.includes(shape)) {
+ throw new Error("Shape is already attached to this collider");
+ }
this._pxActor.attachShape(shape._pxShape);
this._shapes.push(shape);
this._scene?._addColliderShape(shape._id);
}43-45:
Add index validation in removeShape
The current implementation directly splices the array without checking if the shape exists, which could throw an error if the shape isn't found.
removeShape(shape: PhysXColliderShape): void {
+ if (!shape) {
+ throw new Error("Shape cannot be null or undefined");
+ }
this._pxActor.detachShape(shape._pxShape, true);
const shapes = this._shapes;
- shapes.splice(shapes.indexOf(shape), 1);
+ const index = shapes.indexOf(shape);
+ if (index === -1) {
+ throw new Error("Shape is not attached to this collider");
+ }
+ shapes.splice(index, 1);
this._scene?._removeColliderShape(shape._id);
}Likely invalid or redundant comment.
35-35: Verify scene shape management methods
The collider now delegates shape management to the scene through _addColliderShape and _removeColliderShape. Let's verify these methods exist and handle shapes correctly.
Also applies to: 45-45
✅ Verification successful
Shape management methods are properly implemented
The verification confirms that _addColliderShape and _removeColliderShape methods are correctly implemented in PhysXPhysicsScene:
_addColliderShapeinitializes event mapping for the shape_removeColliderShapeproperly cleans up events associated with the shape- Both methods are consistently used across PhysX and Lite physics implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for shape management methods in PhysXPhysicsScene
ast-grep --pattern 'class PhysXPhysicsScene {
$$$
_addColliderShape($_) {
$$$
}
$$$
_removeColliderShape($_) {
$$$
}
$$$
}'
Length of output: 147
Script:
#!/bin/bash
# Let's try a broader search for the scene class and its methods
rg -A 5 "class.*PhysXPhysicsScene"
# Also search for method definitions directly
rg "_addColliderShape|_removeColliderShape" --type ts
Length of output: 2118
Script:
#!/bin/bash
# Let's check the implementation of these methods in PhysXPhysicsScene
rg -A 5 "_addColliderShape|_removeColliderShape" packages/physics-physx/src/PhysXPhysicsScene.ts
Length of output: 952
packages/core/src/physics/shape/ColliderShape.ts (6)
5-5: LGTM: Improved cloning strategy for position and rotation
The switch from @ignoreClone to @deepClone for position and rotation properties ensures proper deep cloning of these essential transform properties.
Also applies to: 27-29
132-133: Add test coverage for object tracking lifecycle
The object tracking implementation needs test coverage to verify proper lifecycle management.
61-61: LGTM: Added validation for contactOffset
The use of Math.max(0, value) properly enforces the documented constraint that contactOffset must be greater than or equal to 0.
140-140: LGTM: Improved cleanup and cloning implementation
The changes properly handle object lifecycle:
- Cloning now uses centralized synchronization
- Destroy method properly cleans up native references and removes tracking
Also applies to: 148-150
152-159: Add error handling to _syncNative method
The _syncNative method should include error handling for native shape operations.
164-164: Verify the impact of multiple shape change notifications
The addition of _handleShapesChanged calls in both _setPosition and _setRotation could lead to redundant notifications when both properties change simultaneously.
Also applies to: 169-169
✅ Verification successful
Multiple shape change notifications are handled appropriately
The implementation shows that _handleShapesChanged() serves different purposes:
- In
Colliderclass, it's an empty method that gets overridden - In
DynamicCollider, it updates mass and inertia only when automatic calculations are enabled - The notifications in
ColliderShapeare necessary as each property change (position,rotation,material) can independently affect the physics calculations
The redundant notifications when both position and rotation change are not problematic because:
- The base implementation is a no-op
- The dynamic collider implementation has proper guards (
if (this._automaticCenterOfMass || this._automaticInertiaTensor)) - Each property change needs to trigger recalculation independently for correctness
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for patterns where both position and rotation are set together
rg -A 5 '_setPosition.*_setRotation|_setRotation.*_setPosition'
# Check the implementation of _handleShapesChanged
ast-grep --pattern '_handleShapesChanged() {
$$$
}'
Length of output: 121
Script:
#!/bin/bash
# Let's try a different approach to find the implementation and usage patterns
# Find the implementation of _handleShapesChanged
ast-grep --pattern 'class $_ {
$$$
_handleShapesChanged() {
$$$
}
$$$
}'
# Look for patterns where shape properties are modified
rg -B 2 -A 2 "_handleShapesChanged"
# Check for position and rotation setters
ast-grep --pattern 'class $_ {
$$$
_setPosition($$$) {
$$$
}
$$$
}'
ast-grep --pattern 'class $_ {
$$$
_setRotation($$$) {
$$$
}
$$$
}'
Length of output: 2967
packages/core/src/physics/Collider.ts (6)
67-68: LGTM! Consistent shape management
The changes maintain symmetry with addShape implementation and properly notify shape changes.
126-132: LGTM! Clean implementation of cloning and change notifications
The implementation simplifies cloning through native synchronization and provides a proper hook for derived classes to handle shape changes.
134-137: LGTM! Well-structured native synchronization
The method provides a clean centralized way to synchronize shapes with the native physics backend.
145-162: LGTM! Proper resource cleanup and encapsulation
The implementation ensures proper cleanup of shapes during destruction and provides clean encapsulation of native shape operations.
112-112: LGTM! Verify physics system integration
The simplified implementation provides better separation of concerns by delegating shape management to the physics system.
Let's verify the physics system properly handles colliders:
Also applies to: 119-119
✅ Verification successful
Physics system properly manages colliders - Implementation verified
The physics system (PhysicsScene) has robust collider management:
- Maintains collider collection with proper indexing
- Handles native physics scene integration
- Implements both
_addColliderand_removeColliderwith complete functionality
The simplified implementation in Collider.ts correctly delegates to these well-implemented methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for physics system implementation of collider management
ast-grep --pattern 'class $_ {
$$$
_addCollider($_) {
$$$
}
_removeCollider($_) {
$$$
}
$$$
}'
Length of output: 116
Script:
#!/bin/bash
# Let's try a different approach to find physics system implementation
# First, find the physics-related files
fd "Physics" packages/core/src
# Then search for collider management methods
rg "_addCollider|_removeCollider" packages/core/src -A 5
Length of output: 1887
54-55: LGTM! Verify shape change notifications
The changes improve naming consistency and add proper change notifications. The implementation correctly handles shape ownership transfer.
Let's verify if all collider implementations properly handle shape changes:
✅ Verification successful
Shape change notifications are properly implemented
The verification shows that the _handleShapesChanged hook is correctly implemented across the codebase:
- Base
Colliderclass provides the default empty implementation DynamicColliderproperly overrides it for dynamic physics updatesColliderShapecorrectly triggers the notification on its parent collider- The hook is called at appropriate points: shape addition, removal, and property changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for classes extending Collider and verify they override _handleShapesChanged
ast-grep --pattern 'class $_ extends Collider {
$$$
_handleShapesChanged() {
$$$
}
$$$
}'
Length of output: 103
Script:
#!/bin/bash
# Let's first find all files that might contain Collider implementations
fd -e ts -e js "Collider"
# Then let's search for classes extending Collider
rg "class.*extends.*Collider" -A 10
# Also search for _handleShapesChanged usage
rg "_handleShapesChanged"
Length of output: 18258
packages/core/src/physics/joint/HingeJoint.ts (4)
Line range hint 110-138: LGTM! Null safety improvements implemented correctly.
The motor and limits setters now properly handle null cases and manage listeners safely.
148-152: Add error handling for missing Collider component.
The method assumes the Collider component exists on the entity.
168-188: Improve null safety and reduce type casting in change handlers.
The handlers would benefit from the same improvements suggested for other methods.
9-25: Verify the impact of switching from @ignoreClone to @deepClone.
The change from @ignoreClone to @deepClone for _axis, _jointMotor, and _limits properties affects how these properties are handled during object cloning. Ensure that deep cloning these properties doesn't create unintended side effects, especially for the motor and limits that maintain their own update listeners.
✅ Verification successful
The switch from @ignoreClone to @deepClone is correct and necessary
Based on the test file and implementation details:
- The test case "clone" explicitly verifies that cloned joints have independent
_updateFlagManagerinstances for both limits and motor properties - The test confirms that modifying properties of the cloned joint (limits.min and motor.targetVelocity) doesn't affect the original joint
- Both
JointMotorandJointLimitsclasses use@deepClonefor their_updateFlagManagerto ensure independent update tracking - The base
Jointclass also uses@deepClonefor collider info properties that need independent copies
The change from @ignoreClone to @deepClone for _axis, _jointMotor, and _limits is correct as it ensures proper isolation between original and cloned instances, which is verified by the existing test cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for clone-related tests and usages
rg -A 5 "clone.*HingeJoint" --type ts
Length of output: 39
Script:
#!/bin/bash
# Let's try a broader search for clone-related tests and implementations
rg -l "CloneManager" --type ts
# Then search for HingeJoint usage patterns
rg -l "HingeJoint" --type ts
# Look for clone decorators usage in joint classes
ast-grep --pattern '@$_Clone
class $_ extends Joint'
Length of output: 2866
Script:
#!/bin/bash
# Let's examine the test file for cloning behavior
cat tests/src/core/physics/HingeJoint.test.ts
# Check JointMotor and JointLimits implementation for clone-related code
cat packages/core/src/physics/joint/JointMotor.ts
cat packages/core/src/physics/joint/JointLimits.ts
# Look for clone decorator usage in the base Joint class
cat packages/core/src/physics/joint/Joint.ts
Length of output: 27852
packages/physics-physx/src/PhysXPhysicsScene.ts (6)
121-126: LGTM! Clean implementation of collider removal.
The implementation properly handles cleanup of scene references, actor removal, and shape management.
Line range hint 133-144: LGTM! Proper character controller initialization.
The implementation correctly handles controller initialization and shape management.
227-229: Add input validation for shape ID.
The method should validate the shape ID before initializing the event map.
_addColliderShape(id: number) {
+ if (typeof id !== 'number' || id < 0) {
+ throw new Error("Invalid shape ID");
+ }
this._physXManager._eventMap[id] = {};
}234-249: LGTM! Thorough event cleanup implementation.
The implementation properly handles:
- Cleanup of current events
- Event pool management
- Proper deletion of event map entries
154-158: 🛠️ Refactor suggestion
Remove redundant optional chaining operator.
The shape?._id has unnecessary optional chaining since there's already a truthy check with shape &&.
- shape && this._removeColliderShape(shape?._id);
+ shape && this._removeColliderShape(shape._id);Likely invalid or redundant comment.
109-114: LGTM! Verify shape management consistency.
The implementation correctly manages collider shapes and scene references. The iteration through shapes is properly handled.
Let's verify the shape management consistency:
✅ Verification successful
Shape management implementation is consistent and correct
The verification confirms that the shape management implementation is consistent across the codebase:
PhysXPhysicsScenecorrectly manages shape IDs through_addColliderShapeand_removeColliderShapePhysXColliderandPhysXCharacterControllerproperly invoke these methods when shapes are added or removed- The shape ID management is consistently used for event handling and controller management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct shape management calls
# and verify the consistency of shape handling across the codebase
# Check for any remaining direct shape management calls
rg -l "addColliderShape|removeColliderShape" --type ts
# Look for shape management patterns
ast-grep --pattern 'shape._id' --type ts
Length of output: 512
Script:
#!/bin/bash
# Let's verify shape management consistency with corrected commands
# Check shape management patterns in PhysXPhysicsScene.ts
rg "_addColliderShape|_removeColliderShape" packages/physics-physx/src/PhysXPhysicsScene.ts -A 3
# Check how shapes are managed in related files
rg "shapes\." packages/physics-physx/src/PhysXCollider.ts -A 2
# Check for shape ID usage patterns
rg "shape\._id" packages/physics-physx/src/ -A 2
Length of output: 2219
tests/src/core/physics/Collider.test.ts (3)
62-64: LGTM! Clean and focused utility function.
The formatValue function effectively handles floating-point precision issues for test comparisons.
349-362: LGTM! Comprehensive inactive modification test.
The test effectively verifies shape management operations with appropriate assertions.
364-371: LGTM! Thorough destroy test.
The test comprehensively verifies all aspects of collider destruction.
packages/core/src/physics/DynamicCollider.ts (2)
422-427: Add test coverage for mass and inertia updates
The _setMassAndUpdateInertia method lacks test coverage, particularly for the interaction between automatic and manual modes.
12-31: Verify the impact of removing @ignoreClone decorators
The removal of @ignoreClone decorators from several properties might affect the cloning behavior of DynamicCollider instances.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
packages/physics-lite/src/LiteDynamicCollider.ts (1)
Line range hint
1-224: Consider improving the architecture documentation.The class implements
IDynamicColliderbut most operations direct users to use Physics-PhysX instead. This suggests a need for:
- Clear documentation about when to use LiteDynamicCollider vs PhysX
- Consider adding a feature flag or configuration option to prevent instantiation of lite versions when PhysX features are required
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-77: packages/physics-lite/src/LiteDynamicCollider.ts#L76-L77
Added lines #L76 - L77 were not covered by tests
[warning] 83-84: packages/physics-lite/src/LiteDynamicCollider.ts#L83-L84
Added lines #L83 - L84 were not covered by tests
[warning] 90-91: packages/physics-lite/src/LiteDynamicCollider.ts#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 97-98: packages/physics-lite/src/LiteDynamicCollider.ts#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 105-105: packages/physics-lite/src/LiteDynamicCollider.ts#L105
Added line #L105 was not covered by tests
[warning] 112-112: packages/physics-lite/src/LiteDynamicCollider.ts#L112
Added line #L112 was not covered by tests
[warning] 119-119: packages/physics-lite/src/LiteDynamicCollider.ts#L119
Added line #L119 was not covered by tests
[warning] 126-126: packages/physics-lite/src/LiteDynamicCollider.ts#L126
Added line #L126 was not covered by tests
[warning] 133-133: packages/physics-lite/src/LiteDynamicCollider.ts#L133
Added line #L133 was not covered by tests
[warning] 140-140: packages/physics-lite/src/LiteDynamicCollider.ts#L140
Added line #L140 was not covered by tests
[warning] 147-148: packages/physics-lite/src/LiteDynamicCollider.ts#L147-L148
Added lines #L147 - L148 were not covered by tests
[warning] 153-154: packages/physics-lite/src/LiteDynamicCollider.ts#L153-L154
Added lines #L153 - L154 were not covered by tests
[warning] 160-161: packages/physics-lite/src/LiteDynamicCollider.ts#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 168-168: packages/physics-lite/src/LiteDynamicCollider.ts#L168
Added line #L168 was not covered by tests
[warning] 175-175: packages/physics-lite/src/LiteDynamicCollider.ts#L175
Added line #L175 was not covered by tests
[warning] 182-183: packages/physics-lite/src/LiteDynamicCollider.ts#L182-L183
Added lines #L182 - L183 were not covered by tests
[warning] 189-189: packages/physics-lite/src/LiteDynamicCollider.ts#L189
Added line #L189 was not covered by tests
[warning] 196-196: packages/physics-lite/src/LiteDynamicCollider.ts#L196
Added line #L196 was not covered by tests
[warning] 203-203: packages/physics-lite/src/LiteDynamicCollider.ts#L203
Added line #L203 was not covered by tests
[warning] 210-210: packages/physics-lite/src/LiteDynamicCollider.ts#L210
Added line #L210 was not covered by testspackages/physics-physx/src/PhysXPhysicsScene.ts (2)
Line range hint
133-144: Add null check before accessing shape._idWhile the code handles shape existence check, it should also ensure shape._id is defined before using it.
if (shape) { const lastPXManager = characterController._pxManager; if (lastPXManager !== this) { lastPXManager && characterController._destroyPXController(); characterController._createPXController(this, shape); } - this._addColliderShape(shape._id); + if (shape._id !== undefined) { + this._addColliderShape(shape._id); + } }
234-249: LGTM with a suggestion for type safetyThe event cleanup logic is thorough and handles all edge cases. Consider adding type safety for the id parameter.
- _removeColliderShape(id: number) { + _removeColliderShape(id: number & { __brand: 'ShapeId' }) { const { _eventPool: eventPool, _currentEvents: currentEvents } = this; const { _eventMap: eventMap } = this._physXManager;packages/core/src/physics/joint/Joint.ts (3)
15-16: Consider thread safety implications of static _tempVector3The static
_tempVector3used for temporary calculations could lead to race conditions if the joint calculations happen concurrently.Consider making this an instance variable or using a temporary vector pool for thread safety.
204-207: Enhance cleanup in _onDisableInSceneThe cleanup of native joint could be more robust by checking for null and handling potential errors.
override _onDisableInScene(): void { + if (this._nativeJoint) { + try { this._nativeJoint.destroy(); + } catch (error) { + console.error("Failed to destroy native joint:", error); + } this._nativeJoint = null; + } }
77-80: Enhance warning message for connectedAnchor setterThe warning message could be more informative by suggesting the correct approach.
- console.warn("Cannot set connectedAnchor when autoConnectedAnchor is true."); + console.warn("Cannot set connectedAnchor when autoConnectedAnchor is true. Set autoConnectedAnchor to false first if you need manual control.");tests/src/core/physics/Collider.test.ts (1)
114-115: Consider improving type declarations for physics._updateMultiple
@ts-ignorecomments are used to access the internal_updatemethod. While this is acceptable in test code, consider:
- Adding proper type declarations for testing purposes
- Creating a test utility function to wrap the physics update call
// Add to test utilities function updatePhysics(engine: WebGLEngine, deltaTime: number) { (engine.sceneManager.activeScene.physics as any)._update(deltaTime); }Also applies to: 132-133, 151-152, 212-214, 254-256
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/core/src/physics/PhysicsMaterial.ts(2 hunks)packages/core/src/physics/joint/Joint.ts(7 hunks)packages/physics-lite/src/LiteDynamicCollider.ts(2 hunks)packages/physics-physx/src/PhysXDynamicCollider.ts(5 hunks)packages/physics-physx/src/PhysXPhysicsScene.ts(4 hunks)tests/src/core/physics/Collider.test.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/physics/PhysicsMaterial.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/physics/joint/Joint.ts
[warning] 218-219: packages/core/src/physics/joint/Joint.ts#L218-L219
Added lines #L218 - L219 were not covered by tests
[warning] 251-252: packages/core/src/physics/joint/Joint.ts#L251-L252
Added lines #L251 - L252 were not covered by tests
[warning] 257-258: packages/core/src/physics/joint/Joint.ts#L257-L258
Added lines #L257 - L258 were not covered by tests
packages/physics-lite/src/LiteDynamicCollider.ts
[warning] 26-28: packages/physics-lite/src/LiteDynamicCollider.ts#L26-L28
Added lines #L26 - L28 were not covered by tests
[warning] 33-35: packages/physics-lite/src/LiteDynamicCollider.ts#L33-L35
Added lines #L33 - L35 were not covered by tests
[warning] 76-77: packages/physics-lite/src/LiteDynamicCollider.ts#L76-L77
Added lines #L76 - L77 were not covered by tests
[warning] 83-84: packages/physics-lite/src/LiteDynamicCollider.ts#L83-L84
Added lines #L83 - L84 were not covered by tests
[warning] 90-91: packages/physics-lite/src/LiteDynamicCollider.ts#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 97-98: packages/physics-lite/src/LiteDynamicCollider.ts#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 105-105: packages/physics-lite/src/LiteDynamicCollider.ts#L105
Added line #L105 was not covered by tests
[warning] 112-112: packages/physics-lite/src/LiteDynamicCollider.ts#L112
Added line #L112 was not covered by tests
[warning] 119-119: packages/physics-lite/src/LiteDynamicCollider.ts#L119
Added line #L119 was not covered by tests
[warning] 126-126: packages/physics-lite/src/LiteDynamicCollider.ts#L126
Added line #L126 was not covered by tests
[warning] 133-133: packages/physics-lite/src/LiteDynamicCollider.ts#L133
Added line #L133 was not covered by tests
[warning] 140-140: packages/physics-lite/src/LiteDynamicCollider.ts#L140
Added line #L140 was not covered by tests
[warning] 147-148: packages/physics-lite/src/LiteDynamicCollider.ts#L147-L148
Added lines #L147 - L148 were not covered by tests
[warning] 153-154: packages/physics-lite/src/LiteDynamicCollider.ts#L153-L154
Added lines #L153 - L154 were not covered by tests
[warning] 160-161: packages/physics-lite/src/LiteDynamicCollider.ts#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 168-168: packages/physics-lite/src/LiteDynamicCollider.ts#L168
Added line #L168 was not covered by tests
[warning] 175-175: packages/physics-lite/src/LiteDynamicCollider.ts#L175
Added line #L175 was not covered by tests
[warning] 182-183: packages/physics-lite/src/LiteDynamicCollider.ts#L182-L183
Added lines #L182 - L183 were not covered by tests
[warning] 189-189: packages/physics-lite/src/LiteDynamicCollider.ts#L189
Added line #L189 was not covered by tests
[warning] 196-196: packages/physics-lite/src/LiteDynamicCollider.ts#L196
Added line #L196 was not covered by tests
[warning] 203-203: packages/physics-lite/src/LiteDynamicCollider.ts#L203
Added line #L203 was not covered by tests
[warning] 210-210: packages/physics-lite/src/LiteDynamicCollider.ts#L210
Added line #L210 was not covered by tests
🔇 Additional comments (17)
packages/physics-lite/src/LiteDynamicCollider.ts (4)
25-28: Avoid misleading return values in getter methods.
These getter methods return the input parameter unmodified after logging an error, which could be misleading as it suggests the method performed some calculation.
Also applies to: 32-35, 96-98, 159-161
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 26-28: packages/physics-lite/src/LiteDynamicCollider.ts#L26-L28
Added lines #L26 - L28 were not covered by tests
75-77: Standardize error handling across methods.
Methods inconsistently handle errors - some throw strings while others use console.error. This makes error handling unpredictable for consumers.
Also applies to: 82-84, 146-148, 188-189
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-77: packages/physics-lite/src/LiteDynamicCollider.ts#L76-L77
Added lines #L76 - L77 were not covered by tests
Line range hint 22-210: Add test coverage for new methods.
All new methods lack test coverage. Even though these methods only log errors or return default values, it's important to verify their behavior.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-77: packages/physics-lite/src/LiteDynamicCollider.ts#L76-L77
Added lines #L76 - L77 were not covered by tests
[warning] 83-84: packages/physics-lite/src/LiteDynamicCollider.ts#L83-L84
Added lines #L83 - L84 were not covered by tests
[warning] 90-91: packages/physics-lite/src/LiteDynamicCollider.ts#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 97-98: packages/physics-lite/src/LiteDynamicCollider.ts#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 105-105: packages/physics-lite/src/LiteDynamicCollider.ts#L105
Added line #L105 was not covered by tests
[warning] 112-112: packages/physics-lite/src/LiteDynamicCollider.ts#L112
Added line #L112 was not covered by tests
[warning] 119-119: packages/physics-lite/src/LiteDynamicCollider.ts#L119
Added line #L119 was not covered by tests
[warning] 126-126: packages/physics-lite/src/LiteDynamicCollider.ts#L126
Added line #L126 was not covered by tests
[warning] 133-133: packages/physics-lite/src/LiteDynamicCollider.ts#L133
Added line #L133 was not covered by tests
[warning] 140-140: packages/physics-lite/src/LiteDynamicCollider.ts#L140
Added line #L140 was not covered by tests
[warning] 147-148: packages/physics-lite/src/LiteDynamicCollider.ts#L147-L148
Added lines #L147 - L148 were not covered by tests
[warning] 153-154: packages/physics-lite/src/LiteDynamicCollider.ts#L153-L154
Added lines #L153 - L154 were not covered by tests
[warning] 160-161: packages/physics-lite/src/LiteDynamicCollider.ts#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 168-168: packages/physics-lite/src/LiteDynamicCollider.ts#L168
Added line #L168 was not covered by tests
[warning] 175-175: packages/physics-lite/src/LiteDynamicCollider.ts#L175
Added line #L175 was not covered by tests
[warning] 182-183: packages/physics-lite/src/LiteDynamicCollider.ts#L182-L183
Added lines #L182 - L183 were not covered by tests
[warning] 189-189: packages/physics-lite/src/LiteDynamicCollider.ts#L189
Added line #L189 was not covered by tests
[warning] 196-196: packages/physics-lite/src/LiteDynamicCollider.ts#L196
Added line #L196 was not covered by tests
[warning] 203-203: packages/physics-lite/src/LiteDynamicCollider.ts#L203
Added line #L203 was not covered by tests
[warning] 210-210: packages/physics-lite/src/LiteDynamicCollider.ts#L210
Added line #L210 was not covered by tests
40-42: 🛠️ Refactor suggestion
Maintain consistency in setter method implementations.
The method should follow the same pattern as other setter methods in the class.
setMassAndUpdateInertia(mass: number): void {
- console.error("Physics-lite don't support setMassAndUpdateInertia. Use Physics-PhysX instead!");
+ console.error(`Physics-lite don't support setMassAndUpdateInertia. Use Physics-PhysX instead!`);
}Likely invalid or redundant comment.
packages/physics-physx/src/PhysXDynamicCollider.ts (3)
Line range hint 33-147: LGTM! Well-implemented getter methods.
The new getter methods are well-structured with:
- Consistent use of out parameters for vector operations to minimize allocations
- Clear documentation referencing the interface
- Proper implementation of the PhysX API
255-261: LGTM! Clean implementation of isSleeping.
The method is well-documented and properly implements the interface requirement, completing the sleep-related functionality alongside the existing sleep() and wakeUp() methods.
182-183: Review the flag setting sequence in ContinuousDynamic mode.
There's a potential issue in the order of operations:
- Line 182 disables CCD
- Line 183 enables CCD friction
This sequence might create a brief window where neither flag is set. Consider setting both flags in a more atomic way or document why this specific order is necessary.
Let's check if this is a common pattern in other implementations:
packages/physics-physx/src/PhysXPhysicsScene.ts (2)
121-126: 🛠️ Refactor suggestion
Add null checks for defensive programming
Similar to addCollider, this method should validate its inputs.
removeCollider(collider: PhysXCollider): void {
+ if (!collider) {
+ throw new Error("Collider cannot be null");
+ }
collider._scene = null;
this._pxScene.removeActor(collider._pxActor, true);
const shapes = collider._shapes;
for (let i = 0, n = shapes.length; i < n; i++) {
this._removeColliderShape(shapes[i]._id);
}
}227-229: 🛠️ Refactor suggestion
Add input validation for shape ID
The method should validate the shape ID before initializing the event map.
_addColliderShape(id: number) {
+ if (typeof id !== 'number' || id < 0) {
+ throw new Error("Invalid shape ID");
+ }
this._physXManager._eventMap[id] = {};
}packages/core/src/physics/joint/Joint.ts (4)
183-186:
Remove @ts-ignore by implementing proper type definitions
The @ts-ignore comments mask potential type safety issues. This was previously flagged but hasn't been addressed.
Create proper interfaces for the event handlers:
interface ValueChangeHandler {
_onValueChanged: (owner: AnchorOwner) => void;
}
interface Vector3WithChangeHandler extends Vector3, ValueChangeHandler {}
// Then update the property types
protected _colliderInfo = new JointColliderInfo<Vector3WithChangeHandler>();211-227:
Add error handling for native operations
The _syncNative method performs multiple native operations without error handling.
Wrap the native operations in try-catch blocks:
protected _syncNative(): void {
if (this._nativeJoint) {
+ try {
this._nativeJoint.setConnectedCollider(this._connectedColliderInfo.collider?._nativeCollider || null);
// ... other operations ...
this._nativeJoint.setBreakTorque(this._torque);
+ } catch (error) {
+ console.error("Failed to sync joint properties:", error);
+ }
}
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 218-219: packages/core/src/physics/joint/Joint.ts#L218-L219
Added lines #L218 - L219 were not covered by tests
249-259:
Add test coverage for transform change handlers
The transform change handlers are critical for joint behavior but lack test coverage.
Add test cases for transform change handlers to verify the correct behavior when scale changes occur.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 251-252: packages/core/src/physics/joint/Joint.ts#L251-L252
Added lines #L251 - L252 were not covered by tests
[warning] 257-258: packages/core/src/physics/joint/Joint.ts#L257-L258
Added lines #L257 - L258 were not covered by tests
229-247: 🛠️ Refactor suggestion
Add validation for vector calculations
The _calculateConnectedAnchor method performs vector operations without validating for edge cases.
Add validation for zero scales and NaN values to prevent potential issues.
tests/src/core/physics/Collider.test.ts (4)
49-64: Well-structured utility functions!
The addition of addBox and formatValue functions improves code maintainability by:
- Reducing code duplication in test cases
- Providing consistent box entity creation
- Handling floating-point precision comparison properly
68-69: Clean setup improvements!
The changes enhance test setup clarity by:
- Directly assigning rootEntity from the active scene
- Explicitly setting the bounciness value for consistent physics behavior
Also applies to: 90-90
341-354: Well-structured shape management test!
The test case thoroughly verifies shape management operations with proper assertions:
- Shape addition verification
- Shape removal verification
- Shape clearing verification
- Collider destruction verification
356-363: Comprehensive destruction test!
The test case properly verifies:
- Collider destruction state
- Component removal
- Shape cleanup
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
tests/src/core/physics/SpringJoint.test.ts (1)
11-22: Add documentation for the helper functionThe
addBoxhelper function should include JSDoc comments explaining:
- The purpose of the function
- Why friction is set to 0
- Parameter descriptions and return type
+/** + * Creates a box entity with a collider for physics testing. + * Friction is set to 0 to isolate joint behavior from friction effects. + * @param cubeSize - The size of the box collider + * @param type - The type of collider (dynamic or static) + * @param pos - The initial position of the box + * @returns The created box entity + */ function addBox(cubeSize: Vector3, type: typeof DynamicCollider | typeof StaticCollider, pos: Vector3) {tests/src/core/physics/Joint.test.ts (2)
39-362: Consider improving test organization and descriptionsWhile the test coverage is comprehensive, consider these improvements:
- Add descriptive comments before each test case explaining the specific behavior being tested
- Group related test cases using
describeblocks (e.g., group all mass-related tests, all break-related tests)- Consider adding edge cases for error conditions
Example structure:
describe('Joint Mass Properties', () => { it('should apply mass scale correctly', () => { // existing massScale test }); it('should apply connected mass scale correctly', () => { // existing connectedMassScale test }); });🧰 Tools
🪛 Biome (1.9.4)
[error] 191-191: 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)
[error] 238-238: 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)
132-157: Consider consolidating similar test casesThe
massScaleandconnectedMassScaletest cases share very similar setup and verification logic. Consider extracting common setup into a shared helper function to reduce duplication:function setupMassScaleTest(useConnected: boolean) { engine.sceneManager.activeScene.physics.gravity = new Vector3(0, -1, 0); const box1 = addBox(new Vector3(1, 1, 1), DynamicCollider, new Vector3(0, 5, 0)); const box2 = addBox(new Vector3(1, 1, 1), DynamicCollider, new Vector3(0, 1, 0)); const box1Collider = box1.getComponent(DynamicCollider); const box2Collider = box2.getComponent(DynamicCollider); box1Collider.isKinematic = true; const fixedJoint = useConnected ? box1.addComponent(FixedJoint) : box2.addComponent(FixedJoint); fixedJoint.connectedCollider = useConnected ? box2.getComponent(DynamicCollider) : box1.getComponent(DynamicCollider); fixedJoint.automaticConnectedAnchor = true; fixedJoint.breakForce = 10; return { box1Collider, box2Collider, fixedJoint }; }Also applies to: 159-183
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/core/src/physics/joint/Joint.ts(7 hunks)tests/src/core/physics/HingeJoint.test.ts(1 hunks)tests/src/core/physics/Joint.test.ts(1 hunks)tests/src/core/physics/SpringJoint.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/src/core/physics/HingeJoint.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
tests/src/core/physics/Joint.test.ts
[error] 191-191: 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)
[error] 238-238: 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)
tests/src/core/physics/SpringJoint.test.ts
[error] 41-41: 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)
[error] 73-73: 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)
[error] 96-96: 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)
[error] 128-128: 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)
[error] 152-152: 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)
[error] 175-175: 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)
[error] 208-208: 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 (13)
packages/core/src/physics/joint/Joint.ts (5)
15-17: LGTM! Good architectural decisions
Making the class abstract and using a static Vector3 for calculations are good practices for performance and proper inheritance hierarchy.
288-304: LGTM! Well-structured supporting types
The AnchorOwner enum and JointColliderInfo class are well-designed with appropriate use of decorators.
186-194:
Fix TypeScript type safety issues in constructor
The constructor still contains multiple @ts-ignore comments which bypass type checking. This was previously flagged but hasn't been addressed.
Create proper interfaces for the event handlers:
interface ValueChangeHandler {
_onValueChanged: (owner: AnchorOwner) => void;
}
interface Vector3WithChangeHandler extends Vector3, ValueChangeHandler {}
interface TransformUpdateManager {
addListener(listener: (type: TransformModifyFlags) => void): void;
removeListener(listener: (type: TransformModifyFlags) => void): void;
}Then update the property types:
protected _colliderInfo = new JointColliderInfo<Vector3WithChangeHandler>();232-250: 🛠️ Refactor suggestion
Add validation for vector calculations
The _calculateConnectedAnchor method performs vector operations without validating for edge cases.
Add validation for zero scales and NaN values:
private _calculateConnectedAnchor(): void {
const colliderInfo = this._colliderInfo;
const connectedColliderInfo = this._connectedColliderInfo;
const { worldPosition: selfPos } = this.entity.transform;
+ if (!selfPos || !this._isValidVector(selfPos)) {
+ console.warn("Invalid self position in joint calculation");
+ return;
+ }
// ... rest of the method
}
+ private _isValidVector(v: Vector3): boolean {
+ return !isNaN(v.x) && !isNaN(v.y) && !isNaN(v.z);
+ }252-261: 🛠️ Refactor suggestion
Add test coverage for transform change handlers
The transform change handlers are critical for joint behavior but lack test coverage.
Add test cases for transform scale changes:
describe('Joint transform changes', () => {
it('should update anchor when transform scale changes', () => {
const joint = new TestJoint(entity);
const spy = jest.spyOn(joint as any, '_updateActualAnchor');
entity.transform.setWorldScale(2, 2, 2);
expect(spy).toHaveBeenCalledWith(AnchorOwner.Self);
});
});tests/src/core/physics/SpringJoint.test.ts (4)
24-28: Add cleanup logic in afterAll hook
The engine resources should be properly disposed of after all tests are complete.
30-33: Remove redundant gravity reset
The gravity reset in beforeEach is unnecessary as each test explicitly sets gravity to (0, 0, 0).
53-54: Remove @ts-ignore by properly typing physics update method
The repeated use of @ts-ignore for physics._update should be addressed.
Also applies to: 82-83, 108-109, 140-141, 164-165, 187-188, 223-224
41-41: Refactor assignments in expressions
The assignments within expressions make the code harder to read and maintain.
Also applies to: 73-73, 96-96, 128-128, 152-152, 175-175, 208-208
🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: 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)
tests/src/core/physics/Joint.test.ts (4)
11-22: LGTM! Well-structured helper function.
The addBox helper function effectively encapsulates the common setup logic, improving test maintainability and reducing code duplication.
24-26: LGTM! Precise number formatting.
The formatValue helper function appropriately handles floating-point precision issues in comparisons.
51-52: Create a test utility for physics updates
Multiple occurrences of @ts-ignore are used to access the internal _update method. Consider creating a test utility to encapsulate this functionality.
Also applies to: 76-77, 99-100, 147-148, 173-174, 198-199, 219-220, 245-246, 291-292, 310-311
191-191: Avoid assignments within expressions
For better readability, separate the assignments from the expressions.
Also applies to: 238-238
🧰 Tools
🪛 Biome (1.9.4)
[error] 191-191: 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: 2
🧹 Outside diff range and nitpick comments (11)
packages/physics-physx/src/shape/PhysXBoxColliderShape.ts (1)
Line range hint
31-48: Good optimization of the scaling system.The removal of
_sizeScalein favor of direct_worldScaleusage is a good architectural decision that:
- Reduces complexity by eliminating redundant scaling state
- Makes the scaling behavior more intuitive
- Maintains proper physics update sequence
packages/physics-physx/src/shape/PhysXCapsuleColliderShape.ts (3)
Line range hint
67-82: Consider optimizing controller updatesWhile the height scaling logic is correct, consider caching the controllers length to avoid repeated length property access in the loop.
- const controllers = this._controllers; - for (let i = 0, n = controllers.length; i < n; i++) { + const controllers = this._controllers; + const len = controllers.length; + for (let i = 0; i < len; i++) {
Line range hint
120-144: Consider adding null check for controllers arrayWhile the implementation is correct, consider adding a null/empty check for the controllers array to avoid unnecessary loop initialization.
- const controllers = this._controllers; - for (let i = 0, n = controllers.length; i < n; i++) { + const controllers = this._controllers; + if (!controllers?.length) return; + for (let i = 0, n = controllers.length; i < n; i++) {
Line range hint
41-144: Consider extracting common scaling logicThe scaling logic is duplicated across methods. Consider extracting it into private helper methods to improve maintainability and reduce duplication.
private _getScaledRadius(): number { const sizeScale = this._worldScale; switch (this._upAxis) { case ColliderShapeUpAxis.X: return this._radius * Math.max(sizeScale.y, sizeScale.z); case ColliderShapeUpAxis.Y: return this._radius * Math.max(sizeScale.x, sizeScale.z); case ColliderShapeUpAxis.Z: return this._radius * Math.max(sizeScale.x, sizeScale.y); } } private _getScaledHeight(): number { const sizeScale = this._worldScale; switch (this._upAxis) { case ColliderShapeUpAxis.X: return this._halfHeight * sizeScale.x; case ColliderShapeUpAxis.Y: return this._halfHeight * sizeScale.y; case ColliderShapeUpAxis.Z: return this._halfHeight * sizeScale.z; } } private _updateControllers(radius: number, height: number): void { const controllers = this._controllers; if (!controllers?.length) return; for (let i = 0, n = controllers.length; i < n; i++) { const pxController = controllers.get(i)._pxController; if (pxController) { pxController.setRadius(radius); pxController.setHeight(height); } } }packages/core/src/physics/joint/HingeJoint.ts (4)
38-38: Optimize type casting in axis setter.Consider storing the type-cast native joint in a local variable to avoid repeated casting.
-(<IHingeJoint>this._nativeJoint)?.setAxis(axis); +const nativeJoint = this._nativeJoint as IHingeJoint; +nativeJoint?.setAxis(axis);
74-74: Consolidate flag operations.Both
useLimitsanduseMotorsetters follow the same pattern. Consider creating a helper method to handle flag operations.+private _setJointFlag(flag: HingeJointFlag, value: boolean): void { + value ? (this._hingeFlags |= flag) : (this._hingeFlags &= ~flag); + const nativeJoint = this._nativeJoint as IHingeJoint; + nativeJoint?.setHingeJointFlag(flag, value); +} set useLimits(value: boolean) { if (value !== this.useLimits) { - value ? (this._hingeFlags |= HingeJointFlag.LimitEnabled) : (this._hingeFlags &= ~HingeJointFlag.LimitEnabled); - (<IHingeJoint>this._nativeJoint)?.setHingeJointFlag(HingeJointFlag.LimitEnabled, value); + this._setJointFlag(HingeJointFlag.LimitEnabled, value); } }Also applies to: 88-88
164-176: Optimize type casting in _syncNative.Multiple type casts to
IHingeJointcan be consolidated.protected override _syncNative(): void { super._syncNative(); + const nativeJoint = this._nativeJoint as IHingeJoint; const motor = this._jointMotor; - (<IHingeJoint>this._nativeJoint).setAxis(this._axis); - (<IHingeJoint>this._nativeJoint).setHingeJointFlag(HingeJointFlag.LimitEnabled, this.useLimits); - (<IHingeJoint>this._nativeJoint).setHingeJointFlag(HingeJointFlag.DriveEnabled, this.useMotor); + nativeJoint.setAxis(this._axis); + nativeJoint.setHingeJointFlag(HingeJointFlag.LimitEnabled, this.useLimits); + nativeJoint.setHingeJointFlag(HingeJointFlag.DriveEnabled, this.useMotor); if (motor) { - (<IHingeJoint>this._nativeJoint).setDriveVelocity(motor.targetVelocity); - (<IHingeJoint>this._nativeJoint).setDriveForceLimit(motor.forceLimit); - (<IHingeJoint>this._nativeJoint).setDriveGearRatio(motor.gearRatio); - (<IHingeJoint>this._nativeJoint).setHingeJointFlag(HingeJointFlag.DriveFreeSpin, motor.freeSpin); + nativeJoint.setDriveVelocity(motor.targetVelocity); + nativeJoint.setDriveForceLimit(motor.forceLimit); + nativeJoint.setDriveGearRatio(motor.gearRatio); + nativeJoint.setHingeJointFlag(HingeJointFlag.DriveFreeSpin, motor.freeSpin); } }
178-198: Apply consistent type optimization pattern.The event handlers should follow the same type optimization pattern suggested for _syncNative.
private _onMotorChanged() { const motor = this._jointMotor; if (this._nativeJoint) { + const nativeJoint = this._nativeJoint as IHingeJoint; - (<IHingeJoint>this._nativeJoint).setDriveVelocity(motor.targetVelocity); - (<IHingeJoint>this._nativeJoint).setDriveForceLimit(motor.forceLimit); - (<IHingeJoint>this._nativeJoint).setDriveGearRatio(motor.gearRatio); - (<IHingeJoint>this._nativeJoint).setHingeJointFlag(HingeJointFlag.DriveFreeSpin, motor.freeSpin); + nativeJoint.setDriveVelocity(motor.targetVelocity); + nativeJoint.setDriveForceLimit(motor.forceLimit); + nativeJoint.setDriveGearRatio(motor.gearRatio); + nativeJoint.setHingeJointFlag(HingeJointFlag.DriveFreeSpin, motor.freeSpin); } }packages/core/src/physics/joint/Joint.ts (3)
15-17: Consider thread safety implications of static temporary vectorThe static
_tempVector3could lead to race conditions if multiple joint calculations occur concurrently. Consider making this an instance variable or using a vector pool.
252-264: Improve documentation for transform change handlersThe transform change handlers would benefit from more detailed documentation explaining:
- When these handlers are triggered
- The impact of different transform flags
- The relationship between transform changes and anchor updates
+/** + * Handles changes in the entity's transform. + * @param type - The flags indicating which transform properties changed + * @remarks Only responds to world scale changes to update the actual anchor position + */ @ignoreClone private _onSelfTransformChanged(type: TransformModifyFlags): void {
291-294: Add documentation for AnchorOwner enum valuesThe AnchorOwner enum values would benefit from detailed documentation explaining their purpose and usage.
enum AnchorOwner { + /** Indicates the anchor belongs to the joint's entity */ Self = 0x1, + /** Indicates the anchor belongs to the connected entity */ Connected = 0x2, + /** Indicates both anchors are involved */ Both = 0x3 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
packages/core/src/physics/joint/HingeJoint.ts(6 hunks)packages/core/src/physics/joint/Joint.ts(7 hunks)packages/core/src/physics/shape/BoxColliderShape.ts(2 hunks)packages/core/src/physics/shape/ColliderShape.ts(4 hunks)packages/design/src/physics/joints/ISpringJoint.ts(0 hunks)packages/physics-physx/src/joint/PhysXHingeJoint.ts(2 hunks)packages/physics-physx/src/joint/PhysXSpringJoint.ts(0 hunks)packages/physics-physx/src/shape/PhysXBoxColliderShape.ts(2 hunks)packages/physics-physx/src/shape/PhysXCapsuleColliderShape.ts(3 hunks)packages/physics-physx/src/shape/PhysXColliderShape.ts(3 hunks)
💤 Files with no reviewable changes (2)
- packages/design/src/physics/joints/ISpringJoint.ts
- packages/physics-physx/src/joint/PhysXSpringJoint.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/physics/shape/BoxColliderShape.ts
- packages/physics-physx/src/joint/PhysXHingeJoint.ts
🔇 Additional comments (17)
packages/physics-physx/src/shape/PhysXBoxColliderShape.ts (2)
31-34: LGTM! Simplified scaling logic looks correct.
The change to use _worldScale directly instead of _sizeScale simplifies the scaling logic while maintaining correct physics behavior. The geometry update sequence is properly maintained.
Let's verify that this change doesn't affect existing physics calculations:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Verified: Scale simplification is safe and correctly implemented
The change to use _worldScale directly instead of _sizeScale is correct because:
- The base
PhysXColliderShapeclass properly manages_worldScaleand handles scale updates for both physics shapes and controllers - No other physics components were depending on
_sizeScale(no references found in the codebase) - The implementation follows the
IBoxColliderShapeinterface contract - The scaling logic maintains the proper update sequence: updating geometry, shape, and notifying controllers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests that verify box collider scaling behavior
rg -l "BoxColliderShape.*scale" --type ts
Length of output: 43
Script:
#!/bin/bash
# Let's try a broader search for box collider tests and scaling-related code
rg -l "BoxCollider" --type ts
# Also search for any direct usage of PhysXBoxColliderShape
rg -l "PhysXBoxColliderShape" --type ts
# Look for test files specifically
fd "test.*\.ts$|spec.*\.ts$"
Length of output: 4753
Script:
#!/bin/bash
# Let's examine the ColliderShape.test.ts and related test files for scaling tests
rg -A 5 "BoxCollider.*scale|setWorldScale" tests/src/core/physics/ColliderShape.test.ts tests/src/core/physics/CharacterController.test.ts
# Also check the interface definition to understand the expected behavior
cat packages/design/src/physics/shape/IBoxColliderShape.ts
# Look for any other usage of setWorldScale in physics components
rg "setWorldScale" packages/physics-physx/src/
Length of output: 1695
Script:
#!/bin/bash
# Let's check the base class implementation and interface
cat packages/physics-physx/src/shape/PhysXColliderShape.ts
# Also check if there are any direct references to _sizeScale in other files
rg "_sizeScale" packages/physics-physx/src/
Length of output: 5091
Line range hint 44-48: Consider handling negative scale values.
While the scaling logic is correct, it might be worth validating or documenting behavior with negative scale values, as they could affect physics calculations in unexpected ways.
Let's check if there are any existing scale value validations:
packages/core/src/physics/shape/ColliderShape.ts (7)
5-5: LGTM: Improved cloning strategy for position and rotation.
The switch from @ignoreClone to @deepClone for position and rotation properties ensures proper deep cloning of these essential transform properties.
Also applies to: 27-29
61-61: LGTM: Proper validation for contact offset.
The use of Math.max(0, value) ensures the contact offset remains non-negative, which is essential for physics calculations.
132-133: Add test coverage for physics object tracking.
While the implementation of object tracking in Engine._physicalObjectsMap is correct, test coverage is needed to verify the lifecycle management.
140-140: LGTM: Improved cloning implementation.
The use of _syncNative centralizes the synchronization logic, making the code more maintainable and less prone to errors.
148-149: LGTM: Proper cleanup implementation.
The destroy method now properly handles cleanup by:
- Nullifying the native shape reference
- Removing the instance from the physics object tracking map
162-171: LGTM: Proper change notification implementation.
The addition of _handleShapesChanged notifications in position and rotation setters ensures proper physics updates when transforms change.
152-160: 🛠️ Refactor suggestion
Add error handling and remove redundant notification.
Two issues to address:
- Add null checks and error handling for native shape operations
- The
_handleShapesChangedcall might be redundant as it's already called in_setPositionand_setRotation
packages/physics-physx/src/shape/PhysXColliderShape.ts (2)
86-86: LGTM! Good improvement in scale handling
The use of Math.abs ensures that scale values are always positive, which is crucial for proper physics simulation.
100-107:
Enhance error handling and parameter validation
The method needs several improvements:
- Fix the property name usage from
_contractOffsetto_contactOffset - Add parameter validation
- Add null checks for
_pxShape
setContactOffset(offset: number): void {
+ if (typeof offset !== 'number' || isNaN(offset) || offset < 0) {
+ throw new Error('Contact offset must be a non-negative number');
+ }
- this._contractOffset = offset;
+ this._contactOffset = offset;
const controllers = this._controllers;
if (controllers.length) {
for (let i = 0, n = controllers.length; i < n; i++) {
controllers.get(i)._pxController?.setContactOffset(offset);
}
} else {
+ if (!this._pxShape) {
+ throw new Error('PhysX shape is not initialized');
+ }
this._pxShape.setContactOffset(offset);
}
}packages/physics-physx/src/shape/PhysXCapsuleColliderShape.ts (1)
Line range hint 41-57: LGTM: Correct scaling logic for radius calculation
The implementation correctly handles radius scaling based on the up axis orientation, using the maximum scale of the appropriate dimensions to maintain the capsule's circular cross-section.
packages/core/src/physics/joint/HingeJoint.ts (2)
151-156: Verify thread safety in _onDisableInScene.
The method reads values from the native joint before disabling. Ensure there are no race conditions if the joint is modified concurrently.
9-25: Verify deep cloning behavior for joint properties.
The switch from @ignoreClone to @deepClone for _axis, _jointMotor, and _limits properties implies these objects will be deeply cloned. Ensure that deep cloning these properties doesn't break any references or cause unexpected behavior in the physics simulation.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
The switch to @deepClone decorator is appropriate and consistent with the codebase patterns
The change from @ignoreClone to @deepClone for _axis, _jointMotor, and _limits properties is correct because:
- The
HingeJointclass needs to maintain independent copies of these properties for each instance - These properties are mutable objects (Vector3, JointMotor, JointLimits) that affect the physics simulation state
- Other joint classes in the codebase (
SpringJoint,FixedJoint) follow similar patterns for their properties - The codebase's test suite includes extensive clone-related tests for various components, showing that deep cloning is a well-established pattern for component state management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other joint classes using these decorators
rg "@(ignore|deep)Clone.*_(axis|jointMotor|limits)" --type ts
# Check for any clone-related tests
rg "clone.*HingeJoint" --type test
Length of output: 129
Script:
#!/bin/bash
# Search for other joint classes using these decorators with broader context
rg "@(ignore|deep)Clone.*_" --type ts -B 1 -A 1
# Check for clone-related tests with correct file extension
rg "clone.*HingeJoint" -g "*.ts" -g "*.js"
# Look for clone manager usage patterns
ast-grep --pattern 'class $_ extends Joint {
$$$
@deepClone
$_
$$$
}'
Length of output: 14920
Script:
#!/bin/bash
# Search for other joint classes to understand the decorator pattern
rg "class \w+Joint extends Joint" --type ts -B 1 -A 5
# Look for JointMotor and JointLimits class definitions
ast-grep --pattern 'class $_ {
$$$
_updateFlagManager
$$$
}'
# Search for clone-related tests in the test directory
fd -e test.ts -e spec.ts -x rg "clone" {}
Length of output: 21748
packages/core/src/physics/joint/Joint.ts (3)
186-193: Remove @ts-ignore by properly typing event handlers
The constructor contains multiple @ts-ignore comments which bypass TypeScript's type checking.
199-210: Add error handling for native joint lifecycle
The lifecycle methods should include error handling for native operations to prevent crashes.
232-250: Add validation for vector calculations
The vector calculations should include validation for edge cases.
| /** @internal */ | ||
| _contractOffset: number = 0.02; |
There was a problem hiding this comment.
Fix property name typo: _contractOffset → _contactOffset
The property name appears to be misspelled. It should be _contactOffset to maintain consistency with the setContactOffset method and accurately represent its purpose.
- _contractOffset: number = 0.02;
+ _contactOffset: number = 0.02;📝 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.
| /** @internal */ | |
| _contractOffset: number = 0.02; | |
| /** @internal */ | |
| _contactOffset: number = 0.02; |
| get automaticConnectedAnchor(): boolean { | ||
| return this._automaticConnectedAnchor; | ||
| } | ||
|
|
||
| set automaticConnectedAnchor(value: boolean) { | ||
| this._automaticConnectedAnchor = value; | ||
| value && this._calculateConnectedAnchor(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for automaticConnectedAnchor changes
The setter for automaticConnectedAnchor should validate the current state before applying changes. Consider checking if:
- The joint is properly initialized
- The connected collider exists (if required)
set automaticConnectedAnchor(value: boolean) {
+ if (this._automaticConnectedAnchor === value) {
+ return;
+ }
this._automaticConnectedAnchor = value;
- value && this._calculateConnectedAnchor();
+ if (value) {
+ try {
+ this._calculateConnectedAnchor();
+ } catch (error) {
+ console.error("Failed to calculate connected anchor:", error);
+ this._automaticConnectedAnchor = false;
+ }
+ }
}📝 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.
| get automaticConnectedAnchor(): boolean { | |
| return this._automaticConnectedAnchor; | |
| } | |
| set automaticConnectedAnchor(value: boolean) { | |
| this._automaticConnectedAnchor = value; | |
| value && this._calculateConnectedAnchor(); | |
| get automaticConnectedAnchor(): boolean { | |
| return this._automaticConnectedAnchor; | |
| } | |
| set automaticConnectedAnchor(value: boolean) { | |
| if (this._automaticConnectedAnchor === value) { | |
| return; | |
| } | |
| this._automaticConnectedAnchor = value; | |
| if (value) { | |
| try { | |
| this._calculateConnectedAnchor(); | |
| } catch (error) { | |
| console.error("Failed to calculate connected anchor:", error); | |
| this._automaticConnectedAnchor = false; | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/core/src/physics/DynamicCollider.ts (4)
24-24: Document the rationale for the specific maxDepenetrationVelocity valueThe value
1.0000000331813535e32seems unusually specific. Please add a comment explaining why this particular value was chosen.
107-113: Document performance implications of automatic calculationsThe automatic calculation features for mass, center of mass, and inertia tensor may have performance implications when shapes are frequently modified. Consider adding documentation about:
- Performance characteristics
- Best practices for when to use automatic vs. manual calculations
- Impact on physics simulation stability
Also applies to: 115-128, 151-166
316-316: Document the _phasedActiveInScene checkThe addition of the
_phasedActiveInScenecheck is good for performance, but consider adding a comment explaining:
- When a collider is considered "phased active"
- The implications of skipping physics operations when inactive
Also applies to: 324-324, 347-347
357-363: Enhance isSleeping documentationThe
isSleepingmethod's documentation should explain:
- Its relationship with
sleepThreshold- When and why a collider might go to sleep
- Impact on performance and physics simulation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/physics/CharacterController.ts(3 hunks)packages/core/src/physics/DynamicCollider.ts(9 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/physics/CharacterController.ts
[error] 18-18: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
🔇 Additional comments (8)
packages/core/src/physics/CharacterController.ts (4)
18-18: Use Math.SQRT1_2 instead of magic number
Replace the approximate value 0.707 with Math.SQRT1_2 (≈0.7071067811865476) from the standard library for better precision and maintainability.
- private _slopeLimit = 0.707;
+ private _slopeLimit = Math.SQRT1_2;🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
Line range hint 21-33: LGTM! Improved property validation and documentation
Good improvements:
- Added validation to ensure stepOffset is non-negative
- Clear documentation explaining the relationship between stepOffset and obstacle height
- Proper synchronization with native collider
🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
Line range hint 108-116: LGTM! Proper shape constraint enforcement
Good implementation of the single shape constraint with clear error messaging.
🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Prefer constants from the standard library.
Unsafe fix: Use Math.SQRT1_2 instead.
(lint/suspicious/noApproximativeNumericConstant)
157-162:
Add missing contactOffset synchronization
The _syncNative method should also synchronize the contactOffset property with the native collider.
protected override _syncNative(): void {
super._syncNative();
(<ICharacterController>this._nativeCollider).setStepOffset(this._stepOffset);
(<ICharacterController>this._nativeCollider).setNonWalkableMode(this._nonWalkableMode);
(<ICharacterController>this._nativeCollider).setUpDirection(this._upDirection);
(<ICharacterController>this._nativeCollider).setSlopeLimit(this._slopeLimit);
+ (<ICharacterController>this._nativeCollider).setContactOffset(this._contactOffset);
}Likely invalid or redundant comment.
packages/core/src/physics/DynamicCollider.ts (4)
65-69: Add error handling for native getter calls
This is a duplicate of a previous review comment regarding the need for error handling in native getter calls.
Also applies to: 83-87
402-422: Add error handling in _syncNative
This is a duplicate of a previous review comment regarding the need for error handling in the _syncNative method.
424-429:
Add @ignoreClone to _setMassAndUpdateInertia
The _setMassAndUpdateInertia method interacts with native code and should be decorated with @ignoreClone to prevent issues during cloning.
+ @ignoreClone
private _setMassAndUpdateInertia(): void {Likely invalid or redundant comment.
12-13: Verify clone behavior for physics properties
The removal of @ignoreClone from several properties might lead to unexpected behavior during object cloning, as physics properties often need special handling.
Also applies to: 18-18, 23-26, 29-31
✅ Verification successful
Let's examine the test files to understand how cloning is handled.
Let's check the actual clone implementation in DynamicCollider to see how these properties are handled.
Let's try a different approach to find the clone implementation.
Based on the evidence gathered, I can now provide the final response:
Removal of @ignoreClone is intentional and safe
The removal of @ignoreClone from the physics properties is correct because:
- DynamicCollider implements proper clone behavior through
_cloneTo()method which explicitly copies all physics properties (linearVelocity, angularVelocity, centerOfMass, inertiaTensor) - The test suite (
Collider.test.tsandColliderShape.test.ts) verifies correct cloning behavior for various collider configurations - Only internal implementation details (like
_index) retain the@ignoreClonedecorator in the parent class
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for clone-related tests to verify proper handling
rg -l "clone.*DynamicCollider" --type ts
Length of output: 172
Script:
#!/bin/bash
# Check clone-related tests for DynamicCollider
rg "clone.*DynamicCollider" -A 10 -B 2 tests/src/core/physics/Collider.test.ts tests/src/core/physics/ColliderShape.test.ts
Length of output: 3641
Script:
#!/bin/bash
# Check clone implementation in DynamicCollider
ast-grep --pattern 'clone() {
$$$
}' packages/core/src/physics/DynamicCollider.ts
Length of output: 85
Script:
#!/bin/bash
# Search for clone-related code in DynamicCollider
rg "clone" -A 10 -B 5 packages/core/src/physics/DynamicCollider.ts
# Also check the parent class implementation
rg "clone" -A 10 -B 5 packages/core/src/physics/Collider.ts
Length of output: 2137
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/src/core/physics/Collider.test.ts (1)
49-64: Add JSDoc documentation and improve type safety.The utility functions would benefit from:
- JSDoc documentation explaining their purpose and parameters
- Return type annotations for better type safety
+/** + * Creates a box entity with physics collider + * @param cubeSize - The size of the box + * @param type - The type of collider to add + * @param pos - The position to place the box + * @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): Entity { +/** + * Formats a number to 5 decimal places for consistent assertions + * @param value - The number to format + * @returns The formatted number + */ -function formatValue(value: number) { +function formatValue(value: number): number {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/physics/PhysicsMaterial.ts(2 hunks)tests/src/core/physics/Collider.test.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/physics/PhysicsMaterial.ts
🔇 Additional comments (3)
tests/src/core/physics/Collider.test.ts (3)
Line range hint 68-90: LGTM! Setup modifications are well-structured.
The changes to the setup hooks improve the test environment by:
- Streamlining root entity assignment
- Adding proper physics material properties
341-363: LGTM! Comprehensive testing of inactive and destroy scenarios.
The tests thoroughly verify:
- Shape management in inactive state
- Proper cleanup on destruction
- Component state after destruction
287-339: 🛠️ Refactor suggestion
Reduce duplication in clone tests.
The clone tests share similar setup code and assertions. Consider extracting common logic into helper functions.
interface ColliderTestSetup {
box: Entity;
collider: DynamicCollider;
}
function setupColliderTest(position: Vector3): ColliderTestSetup {
const box = addBox(new Vector3(1, 1, 1), DynamicCollider, position);
const collider = box.getComponent(DynamicCollider);
collider.automaticInertiaTensor = false;
collider.inertiaTensor.set(10000000, 10000000, 10000000);
collider.applyForce(new Vector3(1000, 0, 0));
collider.shapes[0].material.dynamicFriction = 1;
return { box, collider };
}| // @ts-ignore | ||
| engine.sceneManager.activeScene.physics._update(1); | ||
|
|
||
| expect(collisionScript.onCollisionEnter.mock.calls.length).toBeGreaterThan(1); | ||
| expect(collisionScript.onCollisionStay.mock.calls.length).toBeGreaterThan(1); | ||
| expect(collisionScript.onCollisionExit.mock.calls.length).toBe(1); | ||
| expect(collisionScript.onCollisionExit.mock.calls.length).eq(1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider creating a test utility for physics updates.
The repeated @ts-ignore comments and magic numbers could be encapsulated in a utility function.
+/**
+ * Updates physics simulation for testing
+ * @param engine - The game engine instance
+ * @param deltaTime - Time step in milliseconds
+ */
+function updatePhysics(engine: WebGLEngine, deltaTime: number): void {
+ // @ts-ignore - Accessing internal method for testing
+ engine.sceneManager.activeScene.physics._update(deltaTime);
+}
-// @ts-ignore
-engine.sceneManager.activeScene.physics._update(1);
+updatePhysics(engine, 1);Also, consider adding constants for the time steps:
const PHYSICS_TIME_STEPS = {
COLLISION_CHECK: 1,
TRIGGER_CHECK: 2,
KINEMATIC_MOVE: 16
} as const;Also applies to: 132-137, 151-157, 212-214, 254-256
GuoLei1990
added
enhancement
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
CharacterController,Collider, andDynamicColliderclasses.IDynamicColliderandPhysXDynamicCollider.setContactOffsetmethod inPhysXCharacterController.PhysXColliderandLiteCollider.PhysXBoxColliderShapeandPhysXCapsuleColliderShapeto utilize_worldScale.HingeJointandSpringJoint.Bug Fixes
Documentation
Tests
CharacterController,DynamicCollider, and various joint classes, improving coverage and error handling.ColliderShapeand verified the handling ofcontactOffset.