Skip to content
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

Controls Zoom & Pan Bug on Touch Devices #27073

Closed
BenedictLang opened this issue Oct 27, 2023 · 5 comments · Fixed by #27420
Closed

Controls Zoom & Pan Bug on Touch Devices #27073

BenedictLang opened this issue Oct 27, 2023 · 5 comments · Fixed by #27420
Labels

Comments

@BenedictLang
Copy link

Description

When you have one touch point, move this touch point by a larger distance and than add a second touch point without releasing the first, depending on the configuration, dolly/zoom, pan or rotate is triggered in OrbitControls and TrackballControls (+MapControls).
This looks buggy, as the user is not intended to already trigger the controls. This is happening especially in the use-case when you configure controls.touches.ONE = TOUCH.PAN and controls.touches.TWO = TOUCH.DOLLY_ROTATE and when you are panning with one finger and then want to zoom on an object without touchending all touchpoints.

You can see the issue in the basic configurations as well. I don't see this effect on Google Maps or Apple Maps.

I think to change this, one would need to update the dollyStart/panStart with the current position of pointer 1 pointerPosition[pointer[0].pointerId] if a second pointer is added, so that there is no dollyDelta or panDelta. I guess the location is assigned to dollyStart once the touchstart is triggered, which is the wrong position if you have moved meanwhile to calculate a correct delta.

Reproduction steps

  1. Tap on touch screen
  2. Hold and move (drag) down
  3. While holding, add second touchpoint somewhere

Code

    /**
     * Handle the pointer down event.
     * @param {PointerEvent} event - The pointer event.
     */
    function onPointerDown(event: PointerEvent): void {
      if (!scope.enabled) return

      if (pointers.length === 0) {
        scope.domElement.setPointerCapture(event.pointerId)

        scope.domElement.addEventListener('pointermove', onPointerMove)
        scope.domElement.addEventListener('pointerup', onPointerUp)
      }

      //

      addPointer(event)

      if (event.pointerType === 'touch') {
        onTouchStart(event)
      } else {
        onMouseDown(event)
      }
    }

     /**
     * Handle the touch start event.
     * @param {PointerEvent} event - The touch start event.
     */
    function onTouchStart(event: PointerEvent): void {
      trackPointer(event)

      switch (pointers.length) {
        case 1:
          switch (scope.touches.ONE) {
            case TOUCH.ROTATE:
              if (!scope.enableRotate) return

              handleTouchStartRotate()

              state = STATE.TOUCH_ROTATE

              break

            case TOUCH.PAN:
              if (!scope.enablePan) return

              handleTouchStartPan()

              state = STATE.TOUCH_PAN

              break

            default:
              state = STATE.NONE
          }

          break

        case 2:
          switch (scope.touches.TWO) {
            case TOUCH.DOLLY_PAN:
              if (!scope.enableZoom && !scope.enablePan) return

              handleTouchStartDollyPan()

              state = STATE.TOUCH_DOLLY_PAN

              break
              
              
              [...]
    }

    /**
     * Handle the touch start event for dolly (zoom).
     */
    function handleTouchStartDolly(): void {
      const dx: number = pointers[0].pageX - pointers[1].pageX
      const dy: number = pointers[0].pageY - pointers[1].pageY

      const distance: number = Math.sqrt(dx * dx + dy * dy)

      dollyStart.set(0, distance)
    }
    
    /**
     * Track the position of a pointer.
     * @param {PointerEvent} event - The pointer event to track.
     */
    function trackPointer(event: PointerEvent): void {
      let position: null | Vector2 = pointerPositions[event.pointerId]

      if (position === undefined) {
        position = new Vector2()
        pointerPositions[event.pointerId] = position
      }
      if (position) {
        position.set(event.pageX, event.pageY)
      }
    }

Live example

TrackballControls

Screenshots

No response

Version

r158

Device

Desktop, Mobile

Browser

Chrome, Firefox, Safari, Edge

OS

Windows, Android, iOS

@BenedictLang
Copy link
Author

On the other hand, when I add the second pointer, it should call handleTouchStartDollyPan() again, where it will read the location from pointer[0].pageX etc. which are onTouchMove updated through trackPointer, so that should be correct.
I don't get where it is calculating the delta wrong...

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 30, 2023

In general, the controls add pointers sequentially in the same order pointerdown events occur. So when addPointer() is called in onPointerDown(), in just saves the pointer event in an internal array to represent the current active pointers. I think this logic is correct.

I think you are referring to the situation when one pointer is active, a second pointer is added which leads to a state transition. According to the default touch configuration you switch from ROTATE to DOLLY_PAN. This of course means pointer events are processed differently but I think this is actually the intended behavior (works as designed^^).

If you want to disallow state transitions in certain cases, you need something like a timeout in which additional pointer downs are valid and lead to another state. When the time has passed though, additional pointer downs do not change the state anymore. Would something like this fix the issue for you?

@Mugen87 Mugen87 added the Addons label Oct 30, 2023
@BenedictLang
Copy link
Author

BenedictLang commented Oct 30, 2023

Interesting idea, thanks, I will check that out tonight.

It might be on the technical side the intended behaviour if it is that state switching, but on the UX side you will see some jumped zooming/panning/rotating that makes the controls look bugged. This is not only happening when you follow the path from one pointer panning to two pointer zoom, but when you have a large screen with not the best touchscreen, ghost touching will do this also unintentionally.

But I also know that even if it is smoothening the controls out it might be considered as an add on and I've read your opinion in multiple PRs about the simplicity and basic structure that is the intention of the codebase of three.js 😅

@BenedictLang
Copy link
Author

BenedictLang commented Oct 30, 2023

This is the current behaviour which I would not consider as intentional.

Used standard OrbitControls in its (almost) preconfiguration and TrackballControls for zoom with damping.

lv_0_20231030105457.mp4

But the users say they want a behaviour like in Google or Apple Maps, which is why I make a custom OrbitControls with a threshold for tilting with two pointers, circular rotation and this is where I saw, that google maps does not have this jumping effect while switching state from one active to two active pointers.

Btw it would be lovely to have this kind of control behind MapControls which I was hoping to find, not just another setup of OrbitControls^^ 😅

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 30, 2023

If you have a solution, it would be interesting if you could share it here. In this way, we can evaluate its behavior and complexity. This would also make it easier to compare the current state and the fixed one which makes it easier to justify a breaking change/change in behavior.

@BenedictLang BenedictLang changed the title Controls Zoom Bug on Touch Devices Controls Zoom & Pan Bug on Touch Devices Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants