Fix axis not effect when component changed #2515
Conversation
luzhuang
added
physics
WalkthroughThe pull request introduces changes to the Changes
Sequence DiagramsequenceDiagram
participant HJ as HingeJoint
participant Axis as Axis Vector
participant Native as Native Joint
HJ->>Axis: Set axis value
HJ->>HJ: _setAxis() method
HJ->>Axis: Normalize axis
HJ->>Native: Update native joint axis
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 (
|
![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/HingeJoint.ts (3)
28-28: Enhance the axis property documentation.The current documentation could be more descriptive. Consider adding:
- The expected format/type of the axis vector
- Valid ranges or constraints for the axis values
- Whether the axis will be automatically normalized
- Example usage
- * The Direction of the axis around which the hingeJoint. + * The normalized direction vector representing the axis of rotation for the hinge joint. + * This vector will be automatically normalized when set. + * @example + * ```typescript + * joint.axis = new Vector3(0, 1, 0); // Rotation around Y-axis + * ```
145-146: Improve type safety by properly typing the Vector3 _onValueChanged property.The use of @ts-ignore suggests that _onValueChanged is not properly typed in the Vector3 class. Consider adding proper type definitions instead of suppressing the TypeScript error.
Consider one of these approaches:
- Add proper type definitions to Vector3:
// In Vector3 class _onValueChanged?: () => void;
- Create a custom type that extends Vector3:
interface ObservableVector3 extends Vector3 { _onValueChanged?: () => void; } private _axis: ObservableVector3 = new Vector3(1, 0, 0) as ObservableVector3;
202-210: LGTM! The axis normalization implementation looks correct.The implementation properly handles:
- Prevention of recursive updates by temporarily removing the listener
- Normalization of the axis vector
- Updating the native joint
- Restoring the listener
Consider adding a comment explaining why the listener is temporarily removed to prevent future maintenance confusion:
@ignoreClone private _setAxis(): void { + // Temporarily remove the listener to prevent recursive updates during normalization //@ts-ignore this._axis._onValueChanged = null; this._axis.normalize(); (<IHingeJoint>this._nativeJoint)?.setAxis(this._axis); //@ts-ignore this._axis._onValueChanged = this._setAxis.bind(this); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/physics/joint/HingeJoint.ts(3 hunks)packages/physics-physx/src/joint/PhysXHingeJoint.ts(0 hunks)tests/src/core/physics/HingeJoint.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/physics-physx/src/joint/PhysXHingeJoint.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (1)
tests/src/core/physics/HingeJoint.test.ts (1)
63-64: LGTM! Test changes align with the new axis change detection mechanism.The modification from direct vector assignment to component-wise assignment properly tests that individual component changes trigger the axis normalization and update.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2515 +/- ##
==========================================
- Coverage 69.01% 68.58% -0.44%
==========================================
Files 957 957
Lines 100052 100062 +10
Branches 8554 8565 +11
==========================================
- Hits 69054 68625 -429
- Misses 30742 31181 +439
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. |
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
Refactor