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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

MapControls #14186

Merged
merged 1 commit into from Jun 18, 2018
Merged

MapControls #14186

merged 1 commit into from Jun 18, 2018

Conversation

moroine
Copy link
Contributor

@moroine moroine commented Jun 1, 2018

This PR replace #14065

Live: https://cdn.rawgit.com/moroine/three.js/MapControls/examples/misc_controls_map.html

This MapControls is identical to OrbitControls excepting:

  • Change default mouseButtons (orbit is now right click and pan is left).
  • Change touch behavior:
    • 1 finger: pan
    • 2 fingers:
      • spread or squish: zoom
      • rotate: orbit left
      • drag vertically: orbit up

The limitations of the implementation are:

  • Starting orbit up will disable orbit left & zoom if order to prevent flickering
  • The way to detect vertical drag might not be optimal and there is probably some constant fine tunning (I don't like that kind of stuff stuck_out_tongue ).
  • Almost all code is copy / paste from OrbitControls this is not optimal for maintenance.

As specified by @WestLangley in comment it would be interesting to extends OrbitControls.

In my opinion it's not possible given the current structure of examples. IMO the three options to refactor this properly are :

  • Base Controls class shipping common code and both Orbit & Map extending Base
    • Will require to expose a lot of methods which should remain internal
    • The user will need to include both OrbitControls/Mapcontrols & BaseControls file...
  • MapControls extending OrbitControls
    • Will require to expose a lot of methods which should remain internal
    • The user will need to include both OrbitControls & MapControls file...
  • Creating a Helper with common code and use it in Orbit & Map
    • The user will need to include both Helper and MapControls/OrbitControls

IMO creating a Helper would be nice, but only if example move one day to import syntax but that's another debate 馃槣

@moroine
Copy link
Contributor Author

moroine commented Jun 1, 2018

To help reviewing here is the diff between OrbitControls and MapControls

@mrdoob
Copy link
Owner

mrdoob commented Jun 1, 2018

Looking good!
How do you orbit up on desktop?

@mrdoob mrdoob added this to the r94 milestone Jun 1, 2018
@WestLangley
Copy link
Collaborator

How do you orbit up on desktop?

right-mouse. This is another place where I think also supporting ctrl-mouse makes sense.

What happens with a track-pad?

@moroine
Copy link
Contributor Author

moroine commented Jun 2, 2018

I think also supporting ctrl-mouse makes sense.

Me too, but I think I'll add it in another PR later on maybe in the same time than #13972 to have the same implementation.

What happens with a track-pad?

Good point ! Tested on mine under Windows and it doesn't seems to react to touch events. I'm not sure we have trackpad touch events.

@moroine
Copy link
Contributor Author

moroine commented Jun 17, 2018

@WestLangley do you think it's ready to be merged ?

@WestLangley
Copy link
Collaborator

I think this is good to go. Improvements can be added in later.

@mrdoob mrdoob merged commit c0e6692 into mrdoob:dev Jun 18, 2018
@mrdoob
Copy link
Owner

mrdoob commented Jun 18, 2018

Thanks!

@moroine moroine deleted the MapControls branch June 18, 2018 05:15
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

3 participants