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

OrbitControls: update state on pointer up #27425

Merged
merged 1 commit into from
Dec 24, 2023
Merged

Conversation

sciecode
Copy link
Contributor

@sciecode sciecode commented Dec 22, 2023

Currently, whenever a 2-touch event is in progress and only one 1 pointer is removed, the controls won't properly re-initiate the configured 1-touch event; which leads to unresponsiveness.

This PR addresses this issue, by properly re-initiating states when necessary. At first I thought this could be related to #27333. Although, I'm not too sure anymore.

If anyone with compatible devices could test, I would appreciate it. I would like to investigate that issue, but I can only speculate as to the causes without access to a device that can replicate it.

Live Example - PR

https://rawcdn.githack.com/sciecode/three.js/ec4b8f713bf4cc5238e5c66e5b5d8c66191fe5f7/examples/misc_controls_orbit.html

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 23, 2023

I've tested OrbitControls with a Quest 2 today. #27420 did not fix #27333 so I can still see the interaction bug with r160.

Unfortunately, this PR does also not fix #27333. The problems described in #27333 are still present.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 23, 2023

Currently, whenever a 2-touch event is in progress and only one 1 pointer is removed, the controls won't properly re-initiate the configured 1-touch event; which leads to unresponsiveness.

Is it possible to demonstrate this issue with a mobile phone? TBH, I have not experienced an issue in this context so far. Is there something specific I has to do in order to reproduce the unresponsiveness?

@mrdoob
Copy link
Owner

mrdoob commented Dec 23, 2023

I can definitely reproduce what this PR fixes.

  1. Orbit the camera with one finger
  2. Without releasing the first finger, use to fingers to go to pan/zoom mode.
  3. Release the second dinner to go back to orbit mode.

Without this PR the camera no longer moves even if one finger is still touching:
https://threejs.org/examples/?q=orbit#misc_controls_map

With this PR we are back in orbit mode:
https://rawcdn.githack.com/sciecode/three.js/ec4b8f713bf4cc5238e5c66e5b5d8c66191fe5f7/examples/misc_controls_map.html

@sciecode
Copy link
Contributor Author

What mrdoob described is precisely the use case being fixed here. 👍🏽

@Mugen87 Mugen87 added this to the r161 milestone Dec 23, 2023
@Mugen87 Mugen87 merged commit 1cd56c4 into mrdoob:dev Dec 24, 2023
11 checks passed
@sciecode sciecode deleted the dev-orbit-state branch December 24, 2023 18:53
@mrdoob
Copy link
Owner

mrdoob commented Dec 26, 2023

@sciecode What do you think of this version?

function onPointerUp( event ) {

	removePointer( event );

	switch ( pointers.length ) {

		case 1:

			if ( event.pointerType === 'touch' ) {

				const pointerId = pointers[ 0 ];
				const position = pointerPositions[ pointerId ];

				// minimal placeholder event - allows state correction on pointer-up
				onTouchStart( { pointerId: pointerId, pageX: position.x, pageY: position.y } );

			}

			break;

		case 0:

			scope.domElement.releasePointerCapture( event.pointerId );
			scope.domElement.removeEventListener( 'pointermove', onPointerMove );
			scope.domElement.removeEventListener( 'pointerup', onPointerUp );

			scope.dispatchEvent( _endEvent );

			state = STATE.NONE;

			break;

	}

}

Your code dispatches a _endEvent even when one touch is still active.

@sciecode
Copy link
Contributor Author

sciecode commented Dec 26, 2023

Sure, I actually wasn't certain if we should invoke an end event there or not.

But I don't understand why we need to validate for pointerType === 'touch' there. If the event is triggered by a pen, for example, it would fail and be stuck in an invalid state, cause neither case would run.

Mouse events would never reach the case where there are more than 1 pointer active. Perhaps I misunderstood something?


Hmm, I just noticed we only treat touch events as possible multiple pointers. So a pen event would already be mistreated by other eventListeners. I'll see if I can adjust that on a future PR.

@mrdoob
Copy link
Owner

mrdoob commented Dec 28, 2023

But I don't understand why we need to validate for pointerType === 'touch' there.

It just makes the code easier to understand IMHO.

AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants