-
Notifications
You must be signed in to change notification settings - Fork 367
Add double-tap zoom for Environment Controls #1439
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
gkjohnson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've left a few comments about the code structure - I think a few things can be simplified. Regarding that actual usability, here are two things I've noticed:
- It's currently not possible to "cancel" the animation. As in if you place your finger on the screen to draw while the "zoom" animation is occurring then the animation should cancel so the user can interact.
- If you drag your finger a long distance, then release and tap again in the same spot this counts as a "double tap".
| // double-tap state | ||
| this._lastTapTime = 0; | ||
| this._lastTapPoint = new Vector2(); | ||
| this._doubleTapAnim = null; // stores { startTime, startValue, targetValue, zoomPoint } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets initialize this during initialization rather than setting it to null.
|
|
||
| _detectDoubleTap( pointerTracker ) { | ||
|
|
||
| const currentPoint = new Vector2(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets use an existing scratch variable for this.
|
|
||
| const now = performance.now(); | ||
| const timeSinceLastTap = now - this._lastTapTime; | ||
| const moveDistance = pointerTracker.getMoveDistance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"getMoveDistance" seems to always be "0" because the "pointerup" event is fired without any "pointerMove" events on the same frame. And the "pointerTracker.updateFrame()" call in "update()" resets the distances. Have you checked if this is returning what you expect it to?
| adjustedPointerToCoords( _pointer, domElement, _pointer ); | ||
| setRaycasterFromCamera( raycaster, _pointer, camera ); | ||
| const hit = this._raycast( raycaster ); | ||
| const zoomPoint = hit ? hit.point.clone() : camera.position.clone().addScaledVector( raycaster.ray.direction, 100 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the scratch variables
| // Ease-out cubic: t = 1 - (1-t)³ | ||
| const progress = Math.min( elapsed / duration, 1 ); | ||
| const t = 1 - Math.pow( 1 - progress, 3 ); | ||
| const currentValue = startValue + ( targetValue - startValue ) * t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use MathUtils.lerp here?
|
|
||
| const { camera } = this; | ||
| const { startTime, startValue, targetValue, zoomPoint } = this._doubleTapAnim; | ||
| const elapsed = this.clock.getElapsedTime() - startTime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should take a "deltaTime" passed into the function and thread it through the "update" function, as is done elsewhere in the class. Then the animation time can be counted down on each update.
| // Don't reset state if we're animating a double-tap zoom | ||
| if ( ! this._doubleTapAnim ) { | ||
|
|
||
| this.resetState(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary? It doesn't seem to impact anything relating to the animation
| if ( ! this._doubleTapAnim ) { | ||
|
|
||
| this._updateZoom(); | ||
| this._updatePosition( deltaTime ); | ||
| this._updateRotation( deltaTime ); | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guard shouldn't be needed, either. If the zoom animation has begun / is animating then the mouse / touch should be up and there should be no way for these functions to do anything
| this.inertiaTargetDistance = _vec.copy( pivotPoint ).sub( camera.position ).dot( _forward ); | ||
|
|
||
| } else if ( state === NONE ) { | ||
| } else if ( state === NONE && ! this._doubleTapAnim ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - this check should not be needed. if the double tap has begun then all the residual inertia field should have been reset. Otherwise we'll run into a case where the "inertia" just begins again once the animation completes.
#973
Implements double-tap to zoom functionality for EnvironmentControls, inspired by Google Maps and other modern mapping platforms. This adds an intuitive touch gesture for zooming in on mobile and tablet devices.
@gkjohnson the animation is in
_updateDoubleTapAnimationComplete Implementation