Add physicsMaterial loader and joint component parser and change radians to degrees in physics #2475
Conversation
WalkthroughThis pull request introduces several enhancements to the physics and asset management systems. Key changes include adding a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 #2475 +/- ##
===========================================
+ Coverage 68.68% 68.91% +0.22%
===========================================
Files 920 922 +2
Lines 95350 95661 +311
Branches 8116 8121 +5
===========================================
+ Hits 65494 65924 +430
+ Misses 29602 29484 -118
+ Partials 254 253 -1
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: 3
🧹 Nitpick comments (11)
packages/loader/src/resource-deserialize/resources/parser/ParserContext.ts (1)
29-36: MethodaddComponentneatly abstracts component registration.
Consider additional safety checks or logs for duplicate component IDs to prevent accidental overrides.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-35: packages/loader/src/resource-deserialize/resources/parser/ParserContext.ts#L33-L35
Added lines #L33 - L35 were not covered by testspackages/core/src/physics/joint/Joint.ts (2)
134-134: Inertia scale clarity (connectedInertiaScale).
Ensure the naming convention is consistent with the mass scale properties for uniformity and easy maintainability.
148-148: Inertia scale clarity (inertiaScale).
Maintain consistent referencing for inertia scaling to reduce confusion between connected and self colliders.packages/loader/src/index.ts (1)
25-25: Expanded loader registry with PhysicsMaterialLoader.
AddingPhysicsMaterialLoaderto the index file ensures clients can seamlessly load physics materials. Make sure that usage instructions for this new loader are documented (e.g., in a README or developer guide) so users know how to take advantage of the new functionality.packages/loader/src/PhysicsMaterialLoader.ts (1)
15-38: Add test coverage for the load method.
The new loader is untested according to static analysis. Ensure test scenarios confirm that the fetched JSON data is correctly mapped to thePhysicsMaterialproperties (bounciness, friction, etc.) and that errors are handled gracefully.Would you like help creating unit tests for coverage?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-38: packages/loader/src/PhysicsMaterialLoader.ts#L16-L38
Added lines #L16 - L38 were not covered by testspackages/physics-physx/src/PhysXCharacterController.ts (1)
84-84: Consider clamping extremely high or negative slope values.Although converting degrees to cosine is correct, passing angles above 90 degrees yields negative cosines, which may lead to unintended behavior. Adding input validation or clamping could prevent out-of-range issues.
packages/core/src/physics/shape/ColliderShape.ts (1)
69-69: Documentation clarification.The docstring states “material can’t be null,” but consider clarifying the rationale for this limitation. This helps maintainers appreciate why null or optional materials are disallowed.
packages/core/src/physics/CharacterController.ts (1)
64-65: Eliminate confusion in the slope limit’s documentation.The phrase "the value is the cosine value of the maximum slope angle" is now misleading, as the code uses degrees. Consider removing that part to reduce confusion.
packages/design/src/physics/IDynamicCollider.ts (1)
135-139: Add more documentation on default behavior for gravity.It would be helpful to specify the default setting (e.g., gravity enabled or disabled) and clarify how this method interacts with existing physics defaults in your environment.
packages/physics-lite/src/LiteDynamicCollider.ts (1)
139-141: Throw structured error and add test coverage.
- Prefer throwing an Error object rather than a string literal for better stack tracing, e.g.,
throw new Error("Physics-lite...").- Lines 140-141 are uncovered. Consider adding a test to verify or expect the thrown error.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 140-141: packages/physics-lite/src/LiteDynamicCollider.ts#L140-L141
Added lines #L140 - L141 were not covered by testspackages/core/src/physics/DynamicCollider.ts (1)
244-257: Add tests for default gravity usage
The introduced getter/setter foruseGravityis well-structured. However, static analysis indicates lines 248-249 have insufficient coverage. Consider adding a small test or assertion that checks the defaulttruesetting of_useGravityimmediately upon collider creation.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 248-249: packages/core/src/physics/DynamicCollider.ts#L248-L249
Added lines #L248 - L249 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/core/src/asset/AssetType.ts(1 hunks)packages/core/src/physics/CharacterController.ts(2 hunks)packages/core/src/physics/DynamicCollider.ts(2 hunks)packages/core/src/physics/joint/Joint.ts(8 hunks)packages/core/src/physics/joint/JointLimits.ts(1 hunks)packages/core/src/physics/joint/JointMotor.ts(4 hunks)packages/core/src/physics/shape/ColliderShape.ts(1 hunks)packages/design/src/physics/ICharacterController.ts(1 hunks)packages/design/src/physics/IDynamicCollider.ts(1 hunks)packages/loader/src/PhysicsMaterialLoader.ts(1 hunks)packages/loader/src/index.ts(1 hunks)packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts(1 hunks)packages/loader/src/resource-deserialize/resources/parser/ParserContext.ts(2 hunks)packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts(3 hunks)packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts(1 hunks)packages/physics-lite/src/LiteDynamicCollider.ts(1 hunks)packages/physics-physx/src/PhysXCharacterController.ts(1 hunks)packages/physics-physx/src/PhysXDynamicCollider.ts(1 hunks)packages/physics-physx/src/shape/PhysXCapsuleColliderShape.ts(1 hunks)tests/src/core/physics/CharacterController.test.ts(4 hunks)tests/src/core/physics/DynamicCollider.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/design/src/physics/ICharacterController.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/core/src/physics/DynamicCollider.ts
[warning] 248-249: packages/core/src/physics/DynamicCollider.ts#L248-L249
Added lines #L248 - L249 were not covered by tests
packages/loader/src/PhysicsMaterialLoader.ts
[warning] 16-38: packages/loader/src/PhysicsMaterialLoader.ts#L16-L38
Added lines #L16 - L38 were not covered by tests
packages/core/src/physics/shape/ColliderShape.ts
[warning] 77-78: packages/core/src/physics/shape/ColliderShape.ts#L77-L78
Added lines #L77 - L78 were not covered by tests
packages/physics-lite/src/LiteDynamicCollider.ts
[warning] 140-141: packages/physics-lite/src/LiteDynamicCollider.ts#L140-L141
Added lines #L140 - L141 were not covered by tests
packages/core/src/physics/joint/JointLimits.ts
[warning] 44-44: packages/core/src/physics/joint/JointLimits.ts#L44
Added line #L44 was not covered by tests
packages/loader/src/resource-deserialize/resources/parser/ParserContext.ts
[warning] 33-35: packages/loader/src/resource-deserialize/resources/parser/ParserContext.ts#L33-L35
Added lines #L33 - L35 were not covered by tests
[warning] 39-52: packages/loader/src/resource-deserialize/resources/parser/ParserContext.ts#L39-L52
Added lines #L39 - L52 were not covered by tests
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts
[warning] 98-98: packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts#L98
Added line #L98 was not covered by tests
🪛 Biome (1.9.4)
packages/loader/src/resource-deserialize/resources/parser/ParserContext.ts
[error] 17-17: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (31)
packages/loader/src/resource-deserialize/resources/parser/ParserContext.ts (2)
2-2: Import usage confirmed.
No issues identified; the import of IComponentRef aligns with new capabilities.
9-9: Visibility annotation adjusted to @internal.
This signals that ParserContext is no longer part of the public API surface. Please ensure documentation references reflect this change.
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (1)
95-99: New IComponentRef type is well-structured.
Having distinct ownerId and componentId clarifies referencing.
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (3)
1-11: Imports and type references confirmed.
All referenced types appear to be in use for the enhanced parsing logic.
97-98: Add test coverage for _isComponentRef branch.
Ensure that the path to resolve a component via getComponentByRef is tested to prevent regressions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 98-98: packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts#L98
Added line #L98 was not covered by tests
169-170: Consistent type-guard method _isComponentRef.
The implementation aligns well with other _is* checks.
packages/loader/src/resource-deserialize/resources/parser/HierarchyParser.ts (1)
96-96: Switched to this.context.addComponent.
This maintains consistency with the new asynchronous resolution flow in ParserContext.
packages/core/src/physics/joint/Joint.ts (7)
9-9: Ensure import usage is necessary.
The new import for DynamicCollider is a solid addition and aligns with the new dependency injection in the @dependentComponents decorator. Verify that the DynamicCollider is effectively utilized in this file or any subclasses based on your usage requirements.
14-14: Confirm automatic component addition logic.
Switching from CheckOnly to AutoAdd automatically appends a DynamicCollider component if it's missing. While this streamlines usage for many scenarios, it can also create unwanted components if an entity was never intended to have a DynamicCollider. Confirm this design choice is intentional and beneficial for typical workflows.
40-40: Good use of optional chaining.
This line safely sets the connected collider in _nativeJoint without throwing if value is null or undefined. The code is concise, clear, and avoids null-pointer pitfalls.
68-77: Clever listener detachment and reattachment approach.
Temporarily removing _onValueChanged during _calculateConnectedAnchor() prevents unnecessary triggers. Consider verifying that other operations do not depend on that same callback in concurrent contexts.
82-82: Helpful warning for manual connectedAnchor override.
Emitting a warning if automaticConnectedAnchor is true gives developers immediate feedback about misconfiguration. This is a good user-focused approach.
106-106: Mass scale clarity (connectedMassScale).
Renaming the property to be more explicit about which collider it refers to is a welcome clarity improvement. Verify that logic in external calls or user scripts referencing this property is also updated.
120-120: Mass scale clarity (massScale).
Likewise, ensure that references to the mass scale for the self collider are consistently updated across the codebase to avoid confusion.
packages/loader/src/PhysicsMaterialLoader.ts (2)
1-10: Solid design for new PhysicsMaterialLoader.
The imports and class declaration are consistent with the Galacean loading architecture. This is a good start to integrating physics material assets.
13-14: Resource loader decorator.
Specifying [ "mesh" ] in the second parameter might be misleading if this loader doesn’t truly require a mesh dependency. If the PhysicsMaterial never interacts directly with mesh data, consider removing or revising "mesh" to keep dependencies clear.
packages/core/src/asset/AssetType.ts (1)
63-65: Addition of PhysicsMaterial asset type.
This enum extension is straightforward. Confirm that custom usage of AssetType.PhysicsMaterial in the resource manager is fully implemented to handle physics asset queries.
packages/core/src/physics/joint/JointMotor.ts (4)
25-28: Avoid redundant updates in targetVelocity setter.
Well done checking that _targetVelocity is changing before dispatching an update, preventing unnecessary overhead.
39-42: Avoid redundant updates in forceLimit setter.
Consistent with targetVelocity, this pattern of conditional dispatch helps keep performance overhead minimal.
53-56: Gear ratio setter optimization.
The same approach for gear ratio ensures updates occur only when needed. Good practice for property setting.
67-70: Conditional freeSpin setter.
Matches the conditional update pattern. This approach is uniformly applied across all property setters, improving maintainability.
packages/core/src/physics/shape/ColliderShape.ts (1)
76-78: Ensure runtime coverage and test handling when null material is provided.
This check is vital to maintain a valid physics environment, but lines 77-78 are not covered by tests. Add or expand tests to confirm that an error is thrown if material is set to null.
Would you like me to help write a test case that validates throwing an error for null materials?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 77-78: packages/core/src/physics/shape/ColliderShape.ts#L77-L78
Added lines #L77 - L78 were not covered by tests
packages/core/src/physics/CharacterController.ts (1)
18-18: Double-check the default slope limit value.
Switching from 0.707 (cosine of 45 degrees) to 45 changes the meaning. Confirm that existing users won’t experience unexpected behavior from this more permissive slope value in degrees.
packages/physics-physx/src/shape/PhysXCapsuleColliderShape.ts (1)
107-108: Double-check rotation parameter order.
Changing from (rotation.x, rotation.y, rotation.z) to (rotation.y, rotation.x, rotation.z) may alter the collider’s orientation. Confirm that the new order provides the intended yaw/pitch/roll transformation.
packages/physics-physx/src/PhysXDynamicCollider.ts (1)
200-202: Gravity toggling logic looks correct.
Using eDISABLE_GRAVITY with !value correctly reflects the inverse relationship. Good job adding explicit gravity control for this physics engine.
tests/src/core/physics/CharacterController.test.ts (4)
103-103: Confirm correct slope limit default in constructor
This line ensures the slope limit defaults to 45 degrees instead of the previous cosine-based value. The test appears valid and consistent with the new slope limit approach.
182-183: Ensure slope limit modifications are tested thoroughly
Here, the test first checks the slope limit (45) and then sets it to 0, simulating a scenario with no slope climbing ability. Verify the physics engine’s behavior remains aligned with these test conditions.
197-197: Re-validate slopeLimit in pass condition
Confirming that the slope limit defaults to 45 is consistent with the design. No issues here, but ensure you have enough tests covering varying slope limits.
298-298: Clone test ensures slope limit is preserved
This verifies that cloned character controllers inherit a slope limit of 45. The test is valuable and appears well constructed.
packages/core/src/physics/DynamicCollider.ts (1)
26-26: Initialize _useGravity to true
Initializing gravity to true by default is a sensible choice. Consider documenting any scenario in which you might want to disable it initially.
tests/src/core/physics/DynamicCollider.test.ts (1)
367-380: Verify gravity toggle logic
This new test successfully validates that disabling gravity keeps the box aloft, and re-enabling gravity drops it. The logic is correct, and clearly demonstrates how useGravity can be toggled at runtime.
packages/loader/src/resource-deserialize/resources/parser/ParserContext.ts
Show resolved
Hide resolved
| */ | ||
| export class ParserContext<T extends IHierarchyFile, I extends EngineObject> { | ||
| entityMap: Map<string, Entity> = new Map(); | ||
| entityConfigMap: Map<string, IEntity> = new Map(); | ||
| components: Map<string, Component> = new Map(); | ||
| rootIds: string[] = []; | ||
| strippedIds: string[] = []; | ||
| componentWaitingMap: Map<string, Function[]> = new Map(); |
There was a problem hiding this comment.
Avoid using Function as a type.
This can be a source of bugs due to overly broad acceptance of function-like values. Use a more precise signature (e.g., (component: Component) => void).
- componentWaitingMap: Map<string, Function[]> = new Map();
+ componentWaitingMap: Map<string, Array<(component: Component) => void>> = new Map();Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/loader/src/resource-deserialize/resources/parser/ParserContext.ts
Show resolved
Hide resolved
luzhuang
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/core/src/physics/DynamicCollider.ts (1)
Line range hint
401-413: Add useGravity sync in _syncNative methodThe _syncNative method doesn't synchronize the useGravity property with the native collider, which could lead to inconsistent behavior when the collider is recreated or reset.
protected override _syncNative(): void { super._syncNative(); (<IDynamicCollider>this._nativeCollider).setLinearDamping(this._linearDamping); (<IDynamicCollider>this._nativeCollider).setAngularDamping(this._angularDamping); (<IDynamicCollider>this._nativeCollider).setLinearVelocity(this._linearVelocity); (<IDynamicCollider>this._nativeCollider).setAngularVelocity(this._angularVelocity); + (<IDynamicCollider>this._nativeCollider).setUseGravity(this._useGravity); if (this._automaticCenterOfMass || this._automaticInertiaTensor) { this._setMassAndUpdateInertia(); } else {
🧹 Nitpick comments (8)
tests/src/core/physics/ColliderShape.test.ts (1)
271-274: Consider verifying precision in floating-point comparisons.
You are using fixed numeric expectations here, which might cause test flakiness if the underlying computations slightly change due to floating-point rounding. Consider using tolerance-based comparisons (e.g.,expect(Math.abs(distance - 6.897)).to.be.lessThan(0.001)) to improve test stability.tests/src/core/physics/Joint.test.ts (4)
201-201: Check for potential floating-point driftThe updated expected value (477.06462) is significantly higher than a typical angular velocity range seen in older tests. If this result is correct due to recalibrated physics formulas, it’s fine; however, consider using a tolerance-based assertion (“closeTo” with a reasonable tolerance) to guard against floating-point drift or platform inconsistencies.
214-214: Ensure stable floating-point comparisonsUsing ".closeTo(86.74174, 0.01)" is appropriate, but please confirm that the 0.01 tolerance remains robust across various platforms and GPU/CPU settings. A slightly looser tolerance might reduce sporadic test failures if there are small floating-point differences.
261-261: Guard against floating-point variationsAs with other lines, consider verifying whether 0.01 remains sufficient margin for floating-point variations. If tests occasionally fail, increasing the tolerance or using relative error thresholds might help maintain stability.
276-276: Promote reusability in repeated torque-application testsThis test pattern reoccurs multiple times. Consider extracting torque + update + assert logic into a helper function to reduce duplication, enhance clarity, and ensure consistent floating-point tolerances across similar checks.
packages/core/src/physics/joint/Joint.ts (1)
85-85: Preventing connectedAnchor override
Logging a warning when attempting to setconnectedAnchorwhileautomaticConnectedAnchoris true reinforces the intended usage. Possibly, throwing an error or toggling the property might be more explicit.packages/physics-physx/src/joint/PhysXHingeJoint.ts (1)
80-85: Consider centralizing angle conversion logic for maintainability.
Repeated conversions could be refactored into a helper method to avoid future duplication.packages/core/src/physics/DynamicCollider.ts (1)
244-256: Consider enhancing the useGravity property documentationWhile the implementation is correct, the documentation could be more detailed to help developers understand:
- The default value
- The impact of enabling/disabling gravity
- Common use cases (e.g., floating objects, custom gravity systems)
/** * Controls whether gravity affects the dynamic collider. + * @default true + * @remarks When disabled, the collider will not be affected by scene gravity, + * useful for floating objects or implementing custom gravity systems. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/core/src/physics/DynamicCollider.ts(4 hunks)packages/core/src/physics/joint/Joint.ts(10 hunks)packages/core/src/physics/joint/JointLimits.ts(1 hunks)packages/core/src/physics/shape/ColliderShape.ts(1 hunks)packages/loader/src/PhysicsMaterialLoader.ts(1 hunks)packages/physics-lite/src/shape/LiteColliderShape.ts(2 hunks)packages/physics-physx/src/PhysXDynamicCollider.ts(4 hunks)packages/physics-physx/src/joint/PhysXHingeJoint.ts(3 hunks)packages/physics-physx/src/shape/PhysXColliderShape.ts(3 hunks)tests/src/core/physics/ColliderShape.test.ts(2 hunks)tests/src/core/physics/DynamicCollider.test.ts(3 hunks)tests/src/core/physics/HingeJoint.test.ts(11 hunks)tests/src/core/physics/Joint.test.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/physics/joint/JointLimits.ts
- tests/src/core/physics/DynamicCollider.test.ts
- packages/core/src/physics/shape/ColliderShape.ts
- packages/loader/src/PhysicsMaterialLoader.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/physics/joint/Joint.ts
[error] 28-28: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (42)
tests/src/core/physics/ColliderShape.test.ts (1)
500-503: Ensure numeric expectations remain valid across physics engines.
Similar to the previous comment on floating-point precision, these updated numeric expectations may differ between PhysX and Lite physics engines. If the physics simulation updates in future versions, the test could fail. Use an approximate comparison to safeguard against minor calculation variations.
tests/src/core/physics/HingeJoint.test.ts (12)
71-71: Confirm correctness of the new angle in degrees.
The updated angle from the torque application changed from an earlier small value to approximately 6.58, suggesting it might now be measured in degrees (or another unit). Please verify if this is the correct intended unit and magnitude for the new physics calculations.
97-97: Validate hard limit angle.
An angle of 1.57019 is extremely close to π/2 in radians, if that was the original system. Since other parts of the code have switched to degrees, ensure that this test's expected value is consistent with the chosen unit.
102-102: Double-check negative hard limit angle.
Similar to the positive limit, confirm that -1.57019 is the correct target for the chosen unit system.
124-125: Switch to degrees for min/max limits.
These changes from radians to degrees make sense given the updated context in the rest of the PR. Ensure all other limiting logic (especially animations or calculations) expects degrees consistently throughout the codebase.
135-135: Angle unexpectedly large at 6.27762.
After applying torque for 1 second, examine whether ~6.28 degrees is the expected angle for the applied forces (or if additional factors, e.g., damping, are at play).
156-157: Consistent use of degrees in stiffness test.
These changes to -90 and 90 confirm that you’re using degrees for the joint limits. Ensure that the rest of the calculations, including inertia or torque, also remain consistent in degrees.
167-167: Confirm the negative angle outcome.
A result of -12.93617 degrees suggests significant rotation under stiffness constraints. Verify that it aligns with your new limit logic.
188-189: Damping limits changed to degrees.
These updated limits from -90 to 90 align with degree-based constraints. This is consistent with the other tests and presumably correct.
199-199: High angle due to damping test.
The resulting angle of ~50 degrees is substantial for a single torque update. Ensure the physics parameters (damping, torque, and update delta time) yield the intended result.
222-222: Motor-driven large rotation.
278.87335 degrees in 1 second suggests far more than 30 deg/sec. Double-check whether other factors (e.g., inertia or initial angular velocity) are being accounted for.
307-318: Exceptionally high angular velocity.
The torque application yields a velocity in the thousands, which may be expected in a low-mass scenario or if torque is high. If not, consider verifying mass and inertia values.
358-358: Cloning logic angle check.
The test confirms the newly cloned joint angle also switches to ~6.58 degrees. Given the shift to degree-based calculations, this consistency looks correct.
tests/src/core/physics/Joint.test.ts (2)
229-229: Maintain consistency in test approach
This line again tests ".closeTo(86.74174, 0.01)". Ensure that all similar physics tests consistently apply the same tolerance strategy so they all pass reliably under the same conditions.
248-248: Confirm expected high angular velocity change
Switching from ~8 to 477 is a massive jump. Verify that this increase aligns with the new physics behavior. If so, no problem; otherwise, double-check that inertia or torque parameters are being applied correctly.
packages/core/src/physics/joint/Joint.ts (11)
9-9: Import usage
The import of DynamicCollider is valid because joints specifically require dynamic colliders to function properly with the new dependency mode. This import is necessary for correct joint behavior.
14-14: Expanded dependency from Collider to DynamicCollider
Switching to @dependentComponents(DynamicCollider, DependentMode.AutoAdd) ensures dynamic colliders are automatically added rather than just being checked. Verify that static colliders are not inadvertently auto-added, as that might break existing scenarios using static colliders for certain joint types.
42-42: Connected collider assignment
Using optional chaining (value?._nativeCollider) prevents runtime errors. Confirm that null or undefined connectedCollider scenarios are truly valid for all joint types and always handled gracefully.
70-70: Documentation clarity
This doc line clarifies that connectedAnchor is automatically computed if automaticConnectedAnchor is true. This helps new contributors and users avoid confusion.
73-81: Automatic anchor recalculation
Nullifying _onValueChanged before recalculating prevents unwanted side effects. This is a sensible pattern, but remember to reassign _onValueChanged immediately afterward to ensure future changes are captured.
109-109: Document mass scale usage
The improved comment clarifies that this scale applies to the connected collider’s mass. This is a welcome enhancement for code clarity.
123-123: Document mass scale usage for the main collider
Consistent phrasing with connectedMassScale helps keep these properties intuitive.
137-137: Refined inertia doc
It’s good practice to clarify that this scale applies only to the connected collider’s inertia. Helps separate it from local collider inertia properties.
151-151: Add inertia scale doc
As with connectedInertiaScale, clarifying that this is for the local collider’s inertia fosters better usability.
196-196: Assignment pattern
Binding _updateActualAnchor to AnchorOwner.Connected is a clean approach. Just ensure there are no unexpected references to the wrong context.
198-198: Immediate _onValueChanged assignment
Immediately assigning _updateConnectedActualAnchor ensures anchor updates remain consistent whenever connectedCollider anchor changes. This is a well-structured approach.
packages/physics-physx/src/joint/PhysXHingeJoint.ts (3)
2-2: Ensure consistent usage of MathUtil throughout.
Good addition. This import aligns with the rest of the code that converts between radians and degrees.
59-59: Validate unit consistency for getAngle.
Converting from radians to degrees is correct, but confirm all consumers of getAngle() expect degrees.
73-73: Use of MathUtil.degreeToRadian in setHardLimit.
Proper approach for internally storing limits in radians.
packages/physics-lite/src/shape/LiteColliderShape.ts (2)
1-1: Confirmed new import alignment.
Importing MathUtil ensures consistent conversions across Lite physics shapes.
46-56: Properly converting rotation from degrees to radians.
This is correct as Quaternion.rotationEuler expects radians. Using degreeToRadian avoids confusion.
packages/physics-physx/src/shape/PhysXColliderShape.ts (3)
1-1: New import for MathUtil.
Bringing MathUtil in is consistent with the engine’s approach to unit conversions.
38-38: Initialization of _rotation to a non-null Vector3.
Good defensive change. This ensures _rotation is always available for manipulation.
61-66: Degree-to-radian inside setRotation.
Correctly updates internal quaternion. This ties in nicely with the engine’s convention for storing rotations in radians.
packages/physics-physx/src/PhysXDynamicCollider.ts (5)
2-2: New import for MathUtil.
Integral for unit conversions related to angular velocity and other geometry transformations.
81-85: Converting angular velocity from radians to degrees on get.
Double-check all consumer code expects degrees for angular velocity.
92-97: Converting angular velocity input from degrees to radians on set.
Ensures internal storage remains consistent in radians.
148-148: Convert max angular velocity from degrees to radians.
Matches the global approach of storing angular units in radians.
206-211: New setUseGravity method.
Implementation is straightforward and ensures correct toggling of gravity with PxActorFlag.
packages/core/src/physics/DynamicCollider.ts (3)
23-23: LGTM: Correct conversion from radians to degrees per second
The new value (18000/Math.PI) correctly converts the previous 100 radians/sec limit to its equivalent in degrees/sec, maintaining the same rotational speed limit while aligning with the new degree-based angular velocity units.
26-26: LGTM: Sensible default for gravity effect
The _useGravity property is initialized to true, which is a logical default as most physical objects should be affected by gravity unless explicitly specified otherwise.
189-190: LGTM: Clear documentation of angular velocity units
The documentation clearly specifies that maxAngularVelocity is measured in degrees per second, which is consistent with the implementation.
| @ignoreClone | ||
| private _updateConnectedActualAnchor: Function; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using 'Function' as a type
Using Function can lead to unsafe behavior, as it does not specify arguments or return types. Consider using a more specific function type signature (e.g., () => void) for better type safety and clarity.
- private _updateConnectedActualAnchor: Function;
+ private _updateConnectedActualAnchor: () => void;Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
| * The angular velocity vector of the dynamic collider measured in degrees per second. | ||
| */ |
There was a problem hiding this comment.
💡 Codebase verification
Breaking change in angular velocity units requires documentation update
Based on the code analysis, this is indeed a significant change that needs proper documentation:
- The tests show angular velocity values being used in degrees (e.g., 45 degrees, 200 degrees) which confirms the change to degrees per second
- Multiple components and tests rely on angularVelocity, including:
- DynamicCollider.ts (core implementation)
- Joint.test.ts and HingeJoint.test.ts (joint behavior)
- physx-select.ts (example code)
- No changelog entries were found documenting this breaking change
The change from radians to degrees needs to be properly documented to prevent issues for API consumers. Please:
- Add a changelog entry documenting this breaking change
- Update the API documentation to explicitly mention this is a breaking change from radians
- Consider adding migration notes for users upgrading from previous versions
🔗 Analysis chain
Verify impact of angular velocity unit change
The change from radians to degrees per second is a breaking change that could affect existing code. Let's verify if this change is properly documented and if there are any direct consumers that need to be updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of angularVelocity to identify potentially affected code
echo "Searching for direct usage of angularVelocity..."
rg -l "angularVelocity" --type ts
# Search for any tests that might need updating
echo "Searching for related tests..."
rg -l "angularVelocity" --type test
# Search for documentation that might need updating
echo "Searching for documentation files..."
fd -e md -e mdx | xargs rg -l "angular.*velocity"
Length of output: 607
Script:
#!/bin/bash
# Let's examine the actual usage of angularVelocity in the identified files
echo "Checking DynamicCollider.ts implementation..."
rg "angularVelocity" packages/core/src/physics/DynamicCollider.ts -B 2 -A 2
echo -e "\nChecking test implementations..."
rg "angularVelocity" tests/src/core/physics/DynamicCollider.test.ts -B 2 -A 2
rg "angularVelocity" tests/src/core/physics/Joint.test.ts -B 2 -A 2
rg "angularVelocity" tests/src/core/physics/HingeJoint.test.ts -B 2 -A 2
echo -e "\nChecking example usage..."
rg "angularVelocity" examples/physx-select.ts -B 2 -A 2
# Let's also check for any migration guides or changelog entries
echo -e "\nChecking for migration guides and changelogs..."
fd "CHANGELOG|MIGRATION" --type f | xargs rg -l "angular.*velocity"
Length of output: 8517
GuoLei1990
added
ignore for release
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
PhysicsMaterial.DynamicColliderandPhysXDynamicColliderclasses.Bug Fixes
JointLimitsto automatically adjust limits when invalid values are set.Documentation
Tests
CharacterControllerto reflect changes in slope limit handling.DynamicCollider.