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: support 2-finger zoom/pan #13706

Merged
merged 1 commit into from
Mar 29, 2018
Merged

Conversation

WestLangley
Copy link
Collaborator

Three-finger panning has been removed with this PR, and replaced with the more-familiar 2-finger zoom/pan.

@WestLangley
Copy link
Collaborator Author

Please try it and let me know if touch is jittery.

/ping @wuweiweiwu, @ranbuch, @huj2, @moroine, @XanderLuciano

Apologies to @MauricioVives and @smcllns for the delay.

@wuweiweiwu
Copy link
Contributor

LGTM 👍!

@moroine
Copy link
Contributor

moroine commented Mar 28, 2018

Looks good also on my side 👍

I'll see later to add a PR to be able to customize the touch behavior like we have with the option mouseButtons to have the ability to customize the behavior like:

  • 1 finger: pan
  • 2 fingers: rotate + zoom

@pixnblox
Copy link

Great, thanks for getting this in!

@XanderLuciano
Copy link
Contributor

XanderLuciano commented Mar 28, 2018

Initial review looks good on my end 👍 (tested on an LG V30 with Chrome).

Works well with the perspective camera, no jitter like I've seen previously. Will do some more testing and report back though.

I do have a few nit-picky general comments though.

  1. I think the default orbit speed is too high. In my opinion a swipe from one side of the screen to the other side of the screen should be a complete rotation. E.g. swiping from left to right should only orbit once (360 degrees). The current speed makes orbiting become more difficult as you zoom in closer.
  2. Related to 1 - rotating up / down maxes out after only an inch or so of movement. It's like trying to use a gaming mouse on max DPI - it feels a little too twitchy / sensitive.
  3. (Orthographic Camera) - Panning & Zooming should be a 1:1 scale action. The best way to show this would be trying to zoom into a specific point / cone. It's easy to 'overshoot' the target currently. I'll create a quick example with the ortho cam for testing.
  4. Zooming in / out feels slower than panning forward / backwards - I'm thinking panning with ScreenSpacePanning would fix or improve this though.

Nit picking aside though, I'm not seeing any real issues or stopship problems currently, and the above complaints can be addressed in a future Issue / PR.

I'll see about creating a few example files tonight, but if anyone wants to help here's a few different options I'd like to test.

  • A version with an orthographic camera.
  • An option to change between panning modes: HorizontalPanning and ScreenSpacePanning

Also, if someone can test this PR with an iOS device and/or Touchscreen laptop that'd be helpful.
Jitter was previously most noticeable when 2 finger zooming.

@WestLangley
Copy link
Collaborator Author

@XanderLuciano Settings on one device are likely to yield different behavior than the identical settings on a different device.

I would prefer to keep the defaults at 1, and instead focus on code that help achieve more consistent behavior across devices -- to the extent that is even possible.

OrbitControls is an example, remember. Users are free to modify it according to their intended use case.

@mrdoob
Copy link
Owner

mrdoob commented Mar 29, 2018

@XanderLuciano Settings on one device are likely to yield different behavior than the identical settings on a different device.

I was thinking about this a bit the other day. I think we may want to divide the speed by window.devicePixelRatio or something along the lines.

@mrdoob mrdoob added this to the r92 milestone Mar 29, 2018
@mrdoob mrdoob merged commit dc66e41 into mrdoob:dev Mar 29, 2018
@mrdoob
Copy link
Owner

mrdoob commented Mar 29, 2018

LGTM! Thanks!

@WestLangley WestLangley deleted the dev-orbit branch March 29, 2018 02:41
@huj2
Copy link

huj2 commented Apr 20, 2018

@WestLangley First of all, thank you very much for your work!

I just implemented the new version from r92 in my project and found a problem.
I can not select objcets with raycaster on touch devices anymore. If I try, nothing is happening.

@WestLangley
Copy link
Collaborator Author

@huj2 hmm... likely competing event handlers.

I expect this line should be removed.

@huj2
Copy link

huj2 commented Apr 22, 2018

@WestLangley That's the line. Removed it and now it works again.

@moroine
Copy link
Contributor

moroine commented May 15, 2018

A little bit late, but as mentioned in #13706 (comment) new PR for adding a new touch behavior.

If you can test/review this could be awesome

@Pl4n3
Copy link

Pl4n3 commented May 24, 2018

New event.preventDefault() in touchStart() broke some stuff here, where I use both OrbitControls and ClickEvents. Whats the best approach (currently using now older version of OrbitControls) ?

@XanderLuciano
Copy link
Contributor

@Pl4n3 Did you try removing the event.preventDefault() line?

@Pl4n3
Copy link

Pl4n3 commented Jun 27, 2018

@XanderLuciano Now using an older version (from 81) without event.preventDefault(), also liked the old touch behavior better for my projects. I guess the threejs-examples-files are generally more subject to change then the threejs-core and shouldnt be directly referenced in a project.

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

8 participants