Improve gizmo api #314
Improve gizmo api #314
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
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 (
|
![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
🧹 Outside diff range and nitpick comments (4)
packages/gizmo/src/Gizmo.ts (4)
30-30: Consider adding JSDoc for the public group propertySince
groupis now a public property, it should be documented with JSDoc to describe its purpose and usage.+/** Group instance that stores selection state and transformation information */ readonly group: Group = new Group();
Line range hint
108-123: LGTM: Constructor properly enforces required camera parameterThe constructor now correctly enforces the camera parameter requirement while making layer and state optional. The error handling for PhysicsManager is maintained.
However, consider adding type validation for the camera parameter:
constructor(entity: Entity, props: { camera: Camera; layer?: Layer; state?: State }) { super(entity); + if (!props.camera) { + throw new Error("Camera is required for Gizmo initialization"); + } if (!this.entity.engine.physicsManager) { throw new Error("PhysicsManager is not initialized"); }
210-223: LGTM: Initialization logic properly encapsulatedThe
_initmethod effectively encapsulates the camera and framebuffer picker initialization logic. The method properly checks for camera changes and initializes controls.However, there's a potential improvement for error handling:
private _init(camera: Camera) { + if (!camera) { + throw new Error("Camera is required for initialization"); + } if (camera !== this._sceneCamera) {
Line range hint
139-152: Consider consolidating duplicate transform update logicThe transform update logic is duplicated between the started and non-started states. Consider extracting this into a separate method.
+private _updateTransforms() { + this._traverseControl(this._type, (control) => { + this._type === State.all ? control.onUpdate(true) : control.onUpdate(false); + }); + this.group._gizmoTransformDirty = false; +} // Then in both places: -this._traverseControl(this._type, (control) => { - this._type === State.all ? control.onUpdate(true) : control.onUpdate(false); -}); -this.group._gizmoTransformDirty = false; +this._updateTransforms();Also applies to: 174-179
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/gizmo/README.md(2 hunks)packages/gizmo/src/Gizmo.ts(10 hunks)
🔇 Additional comments (5)
packages/gizmo/README.md (3)
39-39: LGTM: Example URL updated
The example URL has been correctly updated to the new domain.
64-64: LGTM: Usage example aligns with new API design
The example correctly demonstrates the new initialization pattern requiring explicit camera parameter, which improves API clarity.
70-70: LGTM: Documentation link updated
Documentation link has been properly updated to the new domain.
packages/gizmo/src/Gizmo.ts (2)
Line range hint 274-279: LGTM: Proper cleanup in gizmo end handler
The gizmo end handler properly cleans up the state and dispatches the event. The coordinate dirty flag is correctly set.
🧰 Tools
🪛 Biome (1.9.4)
[error] 273-273: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint 340-351: LGTM: World matrix and position retrieval updated
The code correctly uses the public group property to retrieve world matrix and position information.
|
Because it involves API changes, this PR changes base to dev/1.4 |
|
@cptbtptpbcptdtptp When do you migrate framebuffer picker to raycast? |
This pull request introduces serveral changes:
constructorparameters of gizmogroupof gizmo, while not pass thegroupgizmo.initfunction_initializepropertyForce the developer to set
camerawhile add the gizmo component:Invoking
gizmo.init(camera, group)to create a gizmo is a disconnect in the process.