Fix axis not effect when component changed && set connectedAnchor should not effect when automaticConnectedAnchor is true
#2516
Conversation
WalkthroughThis pull request introduces modifications to the physics joint system, focusing on the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
GuoLei1990
changed the title
GuoLei1990
changed the title
connectedAnchor should not effect when automaticConnectedAnchor is true
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2516 +/- ##
==========================================
+ Coverage 69.01% 69.03% +0.01%
==========================================
Files 957 957
Lines 100052 100062 +10
Branches 8554 8560 +6
==========================================
+ Hits 69054 69076 +22
+ Misses 30742 30730 -12
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/physics/joint/Joint.ts (3)
Line range hint
238-252: Consider using a guard flag instead of handler removalWhile the current implementation works, temporarily removing and reattaching event handlers could be fragile. Consider using a guard flag to prevent recursive updates instead.
private _calculateConnectedAnchor(): void { + private _isCalculating = false; // ... - // @ts-ignore - connectedAnchor._onValueChanged = null; + if (this._isCalculating) return; + this._isCalculating = true; if (connectedCollider) { // ... calculation logic ... } else { // ... calculation logic ... } - // @ts-ignore - connectedAnchor._onValueChanged = this._handleConnectedAnchorChanged; + this._isCalculating = false; this._updateActualAnchor(AnchorOwner.Connected); }
254-260: LGTM! Clear handling of connected anchor changesThe implementation correctly prevents manual changes when automaticConnectedAnchor is true and provides appropriate feedback.
Consider enhancing the warning message to be more informative:
- console.warn("Cannot set connectedAnchor when automaticConnectedAnchor is true."); + console.warn("Cannot manually set connectedAnchor when automaticConnectedAnchor is true. Set automaticConnectedAnchor to false first if you need manual control.");
Line range hint
67-74: Add JSDoc documentation for the updated behaviorThe connectedAnchor property's behavior has changed significantly, but the documentation hasn't been updated to reflect these changes. Please update the JSDoc comments to:
- Explain the warning when setting connectedAnchor with automaticConnectedAnchor enabled
- Document the simplified getter behavior
/** * The connected anchor position. * @remarks If connectedCollider is set, this anchor is relative offset, or the anchor is world position. - * The connectedAnchor is automatically calculated, if you want to set it manually, please set automaticConnectedAnchor to false + * The connectedAnchor is automatically calculated when automaticConnectedAnchor is true. + * To manually set the connectedAnchor: + * 1. Set automaticConnectedAnchor to false + * 2. Set your desired connectedAnchor value + * @throws Warns if attempting to set connectedAnchor while automaticConnectedAnchor is true */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/physics/joint/Joint.ts(4 hunks)tests/src/core/physics/Joint.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/src/core/physics/Joint.test.ts
🔇 Additional comments (2)
packages/core/src/physics/joint/Joint.ts (2)
73-73: LGTM! Simplified getter implementationThe simplified getter improves code clarity by removing unnecessary calculations and deferring the automatic anchor handling to the dedicated method.
183-185: LGTM! Proper event handler initializationThe initialization correctly sets up the connected anchor change handling with proper context binding.
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
Release Notes
Improvements
Bug Fixes
Tests
automaticConnectedAnchorproperty.Note: These changes primarily enhance the physics system's functionality and may not directly impact most end-users.