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

Add panning to two-finger gesture in OrbitControls.js #10597

Closed

Conversation

pixnblox
Copy link

This adds an option to OrbitControls.js to allow both panning and zooming (dollying) with two finger gestures, on touch devices. Currently two finger gestures control only zooming, and three fingers must be used for panning. The flag to control this is enableTwoFingerPanZoom, disabled by default.

This was tested with the misc_controls_orbit example. See the Sketchfab viewer for one example of the intended behavior.

@WestLangley
Copy link
Collaborator

On my iPhone at least, with the misc_controls_orbit example, if both zooming and panning are enabled, then the two-finger panning is extremely jittery. So it appears there is something wrong with the implementation.

Also, I see you are trying to replicate the Sketchfab user experience, but when two-finger touches are used for both zooming and panning, it is essentially impossible to do one without the other. In other words, zooming always results in the center of rotation being changed.

@pixnblox
Copy link
Author

@WestLangley, thanks for trying it. I also noticed that is it jittery, and am open to suggestions for fixing it. It doesn't happen with panning alone. I have noticed that zoom (existing behavior) does not feel completely smooth, so I am suspecting that.

I agree that the center of rotation can change, but it feels relatively easy (not having to switch finger count) to compensate adjust as you interact.

@pixnblox
Copy link
Author

@WestLangley, I checked the zoom code, and the jittering does appear to be caused by the zoom changing by a fixed amount on each event, i.e. it doesn't matter when you move a lot or a little, the zoom change is the same for each event. When you are panning, you are roughly (but not exactly) keeping your fingers the same distance apart. If I change zoom code to take the actual finger distance into account, it works much better. However, this probably means giving the user an extra option to control the distance-to-scale conversion, i.e. how much it scales based on distance moved, or perhaps reusing zoomSpeed for this purpose.

@WestLangley
Copy link
Collaborator

Yes, the implementation in this PR is problematic.

@pixnblox
Copy link
Author

I added smooth changes to zoom for both mouse and touch. This corrects the jittering behavior caused by having the same zoom amount for every update.

@XanderLuciano
Copy link
Contributor

@MauricioVives Thank you for this, this is a great improvement over the standard implementation. Even the zoom smoothing code you added was a noticeable improvement. This feels a lot more natural now.

@mrdoob
Copy link
Owner

mrdoob commented Apr 18, 2017

@WestLangley looks good now?

@WestLangley
Copy link
Collaborator

First, is two-finger zoom and pan a feature we want to add?

Feedback appreciated from anyone willing to try it -- preferably on a variety of devices, and on different scenes having different scales and camera types.

If there is interest, we can discuss the implementation details.

@XanderLuciano
Copy link
Contributor

I have it implemented on my website (ncviewer.com) and have tested on an LG G4 and iPhone 5 without issue. Comment from my friend, he said that the 2 finger pan felt "more normal." I noticed that trying to pan with 3 fingers blocks the view of the screen, making panning even more difficult.

I also think the zooming with new code is much smoother as well. Previously there was noticeable jitter even when trying to hold my fingers steady.

I have an app from Autodesk that is a 3D model viewer and it uses the same controls. 1 finger to rotate, 2 fingers to zoom and pan. They also use 3 fingers up/down to set the focal length which is interesting, but not extremely useful.

Final thoughts, with 2 fingers to zoom and pan, I can hold my phone with both hands and only need my thumbs to navigate the view. If I have to use 3 fingers though, I have to take one hand off the phone to position my fingers well enough.

@WestLangley
Copy link
Collaborator

The 2-finger touch pan/move feature does seem nice to me.

I am not sure I understand what the mouse-dolly changes in this PR are for.

@XanderLuciano
Copy link
Contributor

XanderLuciano commented Apr 19, 2017

The getDomElement() function is just code simplification, the rest of the modifications are the zoom/dolly smoothing which prevents jitter when you zoom in/out by accounting for zoom speed instead of just zooming a fixed amount.

@WestLangley
Copy link
Collaborator

the rest of the modifications are the zoom/dolly smoothing which prevents jitter when you zoom in/out by accounting for zoom speed instead of just zooming a fixed amount.

How, exactly, does "accounting for zoom speed" "prevent jitter"?

Why is a similar change not made in handleMouseWheel()?

@XanderLuciano
Copy link
Contributor

Because on mobile if you try to zoom in just a small amount and your fingers slide up and down then the camera will zoom in and out creating a jittery effect. By account for the zoom speed of your fingers, then the zoom in/out is much smaller and there is no noticeable jitter caused by the camera zooming in and out.

By putting it where he did this handles zoom smoothing for all zoom events though. So here's an example where I use middle-click to show the zoom jitter in the current version vs the PR. The PR handles zoomer in a much smoother fashion.

gif of zooming behavior: http://imgur.com/a/veWgV

@pixnblox
Copy link
Author

@WestLangley, the "speed" here is really the change in distance between the touch points, and that is used to modulate the existing zoomSpeed property. Instead of the change being simply positive or negative, the amount of change is taken into account. My second comment on Jan 18 goes into more detail.

I did originally try doing this with the mouse wheel (WheelEvent), but it has no obvious effect because the wheel events are fairly low-precision. I think we would need to animate between successive events to get a smooth result, which is beyond the scope of this change.

@luyaor
Copy link

luyaor commented Jul 15, 2018

Dear MauricioVives & WestLangley,

We are researchers working on identifying redundant development and duplicated pull requests. We have found there is a pull request: #13706 which might be similar to this one and already merged into main stream. So maybe this pull request should be closed ?

We would really appreciate if you could help us to validate and give us some feedback.
Thank you very much for your time!

Sincerely,
Luyao

@WestLangley
Copy link
Collaborator

Closing. An alternate implementation of this feature was merged in #13706.

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.

None yet

6 participants