Fix gizmo not update when rotate around #329
Conversation
|
|
此外，Gizmo 的一些命名和逻辑待优化：
|
WalkthroughThis pull request updates three modules to refine internal state management and event handling. In the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SphereScript
participant Camera
User->>SphereScript: onPointerBeginDrag(eventData)
Note right of SphereScript: Begin drag operation
User->>SphereScript: onPointerDrag(eventData)
SphereScript->>Camera: Update camera transformation continuously
User->>SphereScript: End drag (event trigger)
SphereScript->>Camera: onPointerEndDrag(eventData) finalizes changes
sequenceDiagram
participant Camera
participant Gizmo
participant Controls
Note over Camera,Gizmo: Camera property changes occur
Camera->>Gizmo: Invoke _onCameraModifyListener(flag)
Gizmo->>Gizmo: Update _cameraModifyDirtyFlag internally
Gizmo->>Gizmo: onUpdate checks _cameraTransformFlag & projection changes
Gizmo->>Controls: Traverse controls if conditions met
Possibly related PRs
Suggested labels
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 (2)
packages/navigation-gizmo/src/SphereScript.ts (1)
119-119: Consider reintroducing or removing_isTriggered.No code sets
_isTriggeredtotrueinonPointerBeginDrag(), so the_isTriggeredproperty never updates to reflect a drag state. This might cause confusion with the usage inonPointerExit()and leads to dead code. Evaluate if_isTriggeredis needed or remove it for clarity.packages/gizmo/src/Gizmo.ts (1)
58-59: Naming clarity for new state fields.
_cameraTransformFlagand_cameraModifyDirtyFlagare well-named but consider documenting their roles and how they differ if there's a possibility of confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/gizmo/src/Gizmo.ts(5 hunks)packages/gizmo/src/Group.ts(3 hunks)packages/navigation-gizmo/src/SphereScript.ts(2 hunks)
🔇 Additional comments (10)
packages/navigation-gizmo/src/SphereScript.ts (2)
145-152: Ensure camera transformations remain consistent with scale or pivot.These lines directly invert the lookAt matrix and assign it as the camera's worldMatrix, which might ignore any existing camera scale or pivot offsets. Verify if this approach is intentional and confirm it doesn't conflict with other transformations in the scene.
161-171: Confirm logic for hiding entities only when raycast fails.If the raycast does not collide with anything, the code hides the round and axis entities. Consider whether the same logic should apply when the raycast hits non-gizmo objects.
packages/gizmo/src/Group.ts (3)
1-1: Import addition looks good.
36-36: Good move to use aBoolUpdateFlag.This approach standardizes dirty state detection and helps sync with external cameras or gizmos.
246-246: Ensure repeated calls won't cause performance overhead.Repeatedly dispatching the transform flag can trigger multiple re-renders if there's no throttling or deduplication. Confirm that the system gracefully handles consecutive dispatches.
packages/gizmo/src/Gizmo.ts (5)
2-4: Importing new flags looks good.These new imports align with the shift to a more robust state-based approach for camera modifications.
64-70: Handle disposal or re-initialization logic carefully.When switching cameras, the code clears
_cameraTransformFlagfrom managers, sets it to null, and unregisters the modify listener. Ensure that subsequent calls on the old camera won't cause stale references or memory leaks.
75-79: Confirm synergy between_cameraTransformFlagand_registerModifyListener.These lines ensure the camera transform updates can be tracked. Verify that the
FramebufferPickerusage doesn't conflict with the new camera-based flag logic.
93-99: Camera modify listener is well-defined.By tracking all camera modification flags in
_cameraModifyDirtyFlag, the code can handle multiple changes in a single frame. Just ensure large changes don't cause performance bottlenecks if flags accumulate.
185-205: Cohesive approach to camera and group transform changes.The updated conditional logic checks both
_transformFlag.flagand_cameraModifyDirtyFlag. This ensures the gizmo updates promptly whenever the group or camera changes. Confirm that resettingthis._cameraModifyDirtyFlagto0won't skip subsequent flags if multiple modifications happen within the same frame.
| this._cameraTransformFlag = cameraEntity.registerWorldChangeFlag(); | ||
| // @ts-ignore | ||
| camera._registerModifyListener(this._onCameraModifyListener); | ||
| this._framebufferPicker = cameraEntity.addComponent(FramebufferPicker); |
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
Refactor
New Features