Optimize drag and click of Pointer
#2042
Conversation
cptbtptpbcptdtptp
added
bug
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2042 +/- ##
==========================================
- Coverage 67.46% 66.07% -1.39%
==========================================
Files 471 462 -9
Lines 23809 23131 -678
Branches 3402 3413 +11
==========================================
- Hits 16062 15284 -778
- Misses 6556 6669 +113
+ Partials 1191 1178 -13 ☔ View full report in Codecov by Sentry. |
cptbtptpbcptdtptp
changed the title
onPointerUp and onPointerClickdrag and click of Pointer
packages/core/src/Script.ts
Outdated
| * This function will be called when the pointer is pressed on the collider. | ||
| * @param pointer | ||
| */ | ||
| onPointerStartDrag(pointer: Pointer): void {} |
There was a problem hiding this comment.
Use onPointerBeginDrag name, like onBeginRender ane onEndRender
WalkthroughThe updates enhance pointer event handling in the Changes
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 as PR comments)
Additionally, you can add 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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/core/src/Script.ts (1 hunks)
- packages/core/src/input/pointer/Pointer.ts (5 hunks)
- packages/core/src/input/pointer/PointerManager.ts (7 hunks)
- tests/src/core/input/InputManager.test.ts (5 hunks)
Additional comments not posted (27)
packages/core/src/input/pointer/Pointer.ts (13)
5-5: Import of HitResultThe import statement for
HitResultis necessary for the newhitResultproperty. This change is appropriate and follows good practice for managing dependencies.
29-29: Initialization of hitResultThe
hitResultproperty is initialized correctly. This property is essential for storing the results of raycasting operations, which are crucial for pointer interactions in 3D environments.
33-33: Initialization of _eventsMapThe
_eventsMapproperty is initialized toPointerEventType.None, which is a safe default ensuring that no pointer events are mistakenly flagged at initialization.
45-47: Private entity tracking propertiesThe introduction of
_pressedEntity,_enteredEntity, and_draggedEntityis crucial for managing the state of entities during different pointer interactions. This change enhances the tracking capabilities of thePointerclass.
52-54: Getter methods for entity propertiesThe getter methods for
pressedEntity,enteredEntity, anddraggedEntityare implemented correctly. These methods provide safe, read-only access to the internal state, which is a good practice in encapsulation.Also applies to: 59-61, 66-68
73-75: Constructor implementationThe constructor correctly initializes the
idproperty of thePointerinstance. This is essential for uniquely identifying each pointer, especially in multi-touch scenarios.
Line range hint
81-102: Method to handle pointer exit and enter eventsThis method efficiently manages transitions between different entities during pointer interactions. The use of
_scriptsto trigger specific script actions is a robust design choice, facilitating complex interactions within the game or application environment.
109-122: Method to handle pointer down and start drag eventsThis method consolidates the actions related to pointer down and drag start, reducing redundancy and enhancing the clarity of event handling. The simultaneous update of
_pressedEntityand_draggedEntityis a sensible approach, reflecting the start of a drag action.
127-140: Method to handle pointer up and click eventsThe conditional check for
sameTargetbefore firing theonPointerClickevent is a crucial detail that ensures the click event is only triggered when the pointer up event occurs on the same entity as the pointer down event. This prevents false positives in click detection.
147-148: Method to fire pointer drag eventsThis method correctly triggers the
onPointerDragevent for the_draggedEntity. It's well-implemented to support continuous drag interactions, crucial for user interfaces and interactive applications.
162-174: Method to handle pointer end drag eventsThe separation of concerns is well maintained here, with clear handling of the end drag event. The conditional check for
receivingEntityto triggeronPointerDropis a thoughtful addition, allowing for more complex drag-and-drop interactions.
181-183: Dispose method implementationThe
_disposemethod correctly nullifies all relevant properties, ensuring that the pointer instance is cleaned up properly. This is crucial for managing memory and preventing leaks in applications with complex interactions.
187-194: PointerEventType enumThe introduction of the
PointerEventTypeenum is a significant enhancement for managing different types of pointer events. This structured approach facilitates clearer and more maintainable event handling code.packages/core/src/Script.ts (2)
166-167: Implementation of onPointerEndDragThe method is implemented correctly, providing a clear and specific handler for the end of a drag event. This addition enhances the script's capabilities to react to complex pointer interactions.
172-173: Implementation of onPointerDropThe
onPointerDropmethod is a new addition that handles the scenario where a pointer is lifted from a collider during a drag operation. This method is crucial for implementing drag-and-drop functionality.packages/core/src/input/pointer/PointerManager.ts (7)
12-12: Import of Pointer and PointerEventTypeThe import of
PointerandPointerEventTypeis necessary for the updated functionality in this file. This change is appropriate and integrates well with the rest of the codebase.
76-76: Pointer disposal in the update loopThe addition of the
_disposecall within the cleanup loop is a critical improvement. It ensures that pointer instances are properly cleaned up, preventing memory leaks and ensuring that the system's state is correctly maintained.
116-116: Reset of _eventsMap in the update loopResetting
_eventsMaptoPointerEventType.Noneat the start of each update cycle is a good practice. It ensures that event states do not persist incorrectly across frames.
130-135: Pointer raycasting method callThe modification to include
hitResultin the_pointerRayCastmethod call is a necessary update to support the new functionality in thePointerclass. This change allows for more accurate and detailed handling of pointer interactions.
139-142: Handling of pointer move eventsThe handling of pointer move events with the new
_firePointerDragmethod call is an appropriate update. It ensures that drag events are processed correctly, enhancing the responsiveness and accuracy of the interface.
148-153: Enhanced event handling for pointer down and upThe updates to handle pointer down and up events more comprehensively, including the initiation and termination of drag operations, are well-implemented. These changes make the event handling more robust and intuitive.
159-159: Handling of pointer leave and cancel eventsThe addition of
_firePointerEndDragin the context of pointer leave and cancel events is a crucial update. It ensures that drag operations are correctly terminated even when unexpected events like pointer leave occur.tests/src/core/input/InputManager.test.ts (5)
84-86: Setup of camera and box entities in testsThe adjustments to the camera and box entities, including their positions and sizes, are appropriate for testing the new pointer functionality. These changes ensure that the tests are relevant and effective for the new features.
Also applies to: 89-89, 92-92
108-110: Implementation of pointer event handlers in TestScriptThe new methods
onPointerUp,onPointerStartDrag,onPointerEndDrag, andonPointerDropin theTestScriptclass are correctly implemented. These methods enhance the test coverage for the new pointer event functionality.Also applies to: 116-118, 124-126, 128-129
138-138: Spying on methods and dispatching events in testsThe use of
chai.spyon new methods and the dispatching of pointer events to test these methods are correctly done. These tests effectively verify the functionality of the new event handlers.Also applies to: 142-154, 162-162
182-211: New tests for onPointerClickThe new tests for the
onPointerClickevent handler are comprehensive and well-structured. They effectively test various scenarios to ensure that the handler behaves as expected under different conditions.
235-261: Hit result testsThe tests for hit results are crucial for verifying the accuracy of pointer interactions with 3D objects. These tests are well-implemented, covering various scenarios to ensure the functionality is robust and reliable.
packages/core/src/Script.ts
Outdated
| * This function will be called when the pointer is pressed on the collider. | ||
| * @param pointer | ||
| */ | ||
| onPointerStartDrag(pointer: Pointer): void {} |
There was a problem hiding this comment.
Method naming consistency
The previous comment suggests renaming onPointerStartDrag to onPointerBeginDrag for consistency with other method names like onBeginRender. This is a valid suggestion to maintain a consistent naming convention throughout the API.
- onPointerStartDrag(pointer: Pointer): void {}
+ onPointerBeginDrag(pointer: Pointer): void {}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onPointerStartDrag(pointer: Pointer): void {} | |
| onPointerBeginDrag(pointer: Pointer): void {} |
| * This function will be called when the pointer is lifted on the collider. | ||
| * @param pointer - The pointer that triggered | ||
| */ | ||
| onPointerDrop(pointer: Pointer): void {} |
There was a problem hiding this comment.
Are you sure the comments is right and clearly
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/core/src/Script.ts (1 hunks)
- packages/core/src/input/pointer/Pointer.ts (5 hunks)
- tests/src/core/input/InputManager.test.ts (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/core/src/Script.ts
- tests/src/core/input/InputManager.test.ts
Additional comments not posted (11)
packages/core/src/input/pointer/Pointer.ts (11)
28-29: LGTM! Addition ofhitResultproperty.The
hitResultproperty captures raycasting results for the current frame, enhancing pointer interaction tracking.
45-47: LGTM! Renaming and addition of properties.The renaming of
_currentPressedEntityto_pressedEntityand_currentEnteredEntityto_enteredEntityimproves readability. The addition of_draggedEntityis logical for tracking drag events.
52-68: LGTM! Addition of getter methods.The addition of getter methods for
pressedEntity,enteredEntity, anddraggedEntityprovides convenient access to these properties.
73-75: LGTM! Addition of constructor.The constructor initializes the
idproperty, which is necessary for thePointerclass.
Line range hint
81-102:
LGTM! Update to_firePointerExitAndEnter.The update to use
_enteredEntityimproves consistency with the renamed property.
109-122: LGTM! Renaming and update to_firePointerDownAndStartDrag.The renaming and update to initialize
_draggedEntityare logical for handling drag events.
127-140: LGTM! Addition of_firePointerUpAndClick.The addition of
_firePointerUpAndClickis logical for managing pointer up events and clicks.
Line range hint
147-152:
LGTM! Update to_firePointerDrag.The update to use
_draggedEntityis logical for handling drag events.
162-174: LGTM! Addition of_firePointerEndDrag.The addition of
_firePointerEndDragis logical for handling the end of dragging.
181-183: LGTM! Addition of_dispose.The addition of
_disposeis logical for resetting the state of the pointer.
187-194: LGTM! Addition ofPointerEventTypeenum.The addition of
PointerEventTypeenum is logical for categorizing pointer events.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/core/src/Script.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/core/src/Script.ts
Scriptnew callback about drag and dropPointernew APISummary by CodeRabbit
New Features
onPointerBeginDrag,onPointerEndDrag, andonPointerDrop).Bug Fixes
Tests
Refactor