-
-
Notifications
You must be signed in to change notification settings - Fork 334
Fix useGravity not effect when cloned #2538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis pull request introduces modifications in both the core physics package and its lightweight counterpart. In the core package, a new gravity setting is applied to the native collider via the Changes
Sequence Diagram(s)sequenceDiagram
participant DC as DynamicCollider
participant NC as NativeCollider
DC->>NC: setUseGravity(_useGravity)
sequenceDiagram
participant LDC as LiteDynamicCollider
participant Logger
LDC->>Logger: error("Gravity not supported")
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2538 +/- ##
==========================================
+ Coverage 68.60% 69.05% +0.44%
==========================================
Files 957 957
Lines 100125 100126 +1
Branches 8569 8566 -3
==========================================
+ Hits 68688 69139 +451
+ Misses 31181 30731 -450
Partials 256 256
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/physics-lite/src/LiteDynamicCollider.ts (1)
132-134: LGTM! Consider making error handling consistent across all methods.The change to use
Logger.errorinstead of throwing an error is good as it prevents exceptions from breaking the execution flow. However, some methods in the class still throw errors while others useLogger.error. Consider making the error handling consistent across all methods.Apply this diff to make error handling consistent:
- addForce(force: Vector3): void { - throw "Physics-lite don't support addForce. Use Physics-PhysX instead!"; - } + addForce(force: Vector3): void { + Logger.error("Physics-lite don't support addForce. Use Physics-PhysX instead!"); + } - addTorque(torque: Vector3): void { - throw "Physics-lite don't support addTorque. Use Physics-PhysX instead!"; - } + addTorque(torque: Vector3): void { + Logger.error("Physics-lite don't support addTorque. Use Physics-PhysX instead!"); + } - move(positionOrRotation: Vector3 | Quaternion, rotation?: Quaternion): void { - throw "Physics-lite don't support move. Use Physics-PhysX instead!"; - } + move(positionOrRotation: Vector3 | Quaternion, rotation?: Quaternion): void { + Logger.error("Physics-lite don't support move. Use Physics-PhysX instead!"); + } - sleep(): void { - throw "Physics-lite don't support putToSleep. Use Physics-PhysX instead!"; - } + sleep(): void { + Logger.error("Physics-lite don't support putToSleep. Use Physics-PhysX instead!"); + } - isSleeping(): boolean { - throw "Physics-lite don't support isSleeping. Use Physics-PhysX instead!"; - } + isSleeping(): boolean { + Logger.error("Physics-lite don't support isSleeping. Use Physics-PhysX instead!"); + return false; + } - wakeUp(): void { - throw "Physics-lite don't support wakeUp. Use Physics-PhysX instead!"; - } + wakeUp(): void { + Logger.error("Physics-lite don't support wakeUp. Use Physics-PhysX instead!"); + }tests/src/core/physics/DynamicCollider.test.ts (1)
371-373: LGTM! Consider adding more test cases.The test changes properly verify that gravity behaves correctly with cloned colliders. However, consider adding more test cases to cover edge cases and improve coverage.
Consider adding these test cases:
- Test that re-enabling gravity on the cloned collider works correctly
- Test that cloning a collider with gravity already enabled works correctly
- Test that cloning preserves the gravity setting
Example:
// Test re-enabling gravity on clone const cloneCollider = boxClone.getComponent< 8000 /span>(DynamicCollider); cloneCollider.useGravity = true; cloneCollider.wakeUp(); engine.sceneManager.activeScene.physics._update(1); expect(formatValue(boxClone.transform.position.y)).lessThan(10); // Test cloning with gravity enabled boxCollider.useGravity = true; const boxClone2 = box.clone(); expect(boxClone2.getComponent(DynamicCollider).useGravity).toBeTruthy();Also applies to: 377-377
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/physics/DynamicCollider.ts(1 hunks)packages/physics-lite/src/LiteDynamicCollider.ts(1 hunks)tests/src/core/physics/DynamicCollider.test.ts(1 hunks)
🔇 Additional comments (1)
packages/core/src/physics/DynamicCollider.ts (1)
446-446: LGTM! The gravity setting is now properly synchronized.The addition of
setUseGravityin the_syncNativemethod ensures that the gravity setting is properly synchronized with the native collider, which fixes the issue with gravity not working correctly when cloning colliders.
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
Bug Fixes
Tests