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

Allow trackpad to truck on non-pinch gesture. #66

Merged
merged 2 commits into from Jan 10, 2020

Conversation

@andrewplummer
Copy link
Contributor

andrewplummer commented Jan 6, 2020

This is a PR to allow camera trucking for non-pinch trackpad gestures. I left comments but this relies on two tricks... the first is detection of a trackpad based on wheelDeltaX/Y (which typescript doesn't seem to like) here: https://stackoverflow.com/questions/10744645/detect-touchpad-vs-mouse-in-javascript/56948026#56948026

This may be only for Mac trackpads... please modify if there is something better...

The second trick is detecting pinch vs. non-pinch via ctrlKey as shown here: https://stackoverflow.com/questions/15416851/catching-mac-trackpad-zoom/28685082#28685082...

Also I would prefer to have this as an option but I didn't want to guess as to how you would like the API so I leave that to you.. maybe something as simple as cameraControls.trackpad.emulateTruck? I'm not sure.

Last it seems I need to call scope._getClientRect to make truckInternal work, however I am calling this many times, which shouldn't be necessary, but I didn't know the best way to hack a dragStart event for a wheel event.

@yomotsu

This comment has been minimized.

Copy link
Owner

yomotsu commented Jan 7, 2020

Thanks for your contribution.
What you'd like to do is, bind 2-finger-move on macbook to camera track movement?

@andrewplummer

This comment has been minimized.

Copy link
Contributor Author

andrewplummer commented Jan 7, 2020

Yes exactly! But while keeping pinch-to-zoom.
Fantastic library btw... I switched to this as it's much simpler to use than OrbitalControls.

@yomotsu

This comment has been minimized.

Copy link
Owner

yomotsu commented Jan 7, 2020

Actually, I have some concerns.

  1. I think this feature is too specific.
  2. The workaround for detecting a horizontal-2-finger-move may cause another problem since it's kind of trick.
  3. This lib has a button config feature, and this change may conflict with it.

Yet this kind of modification may be needed sometime.
Todo so, the lib could provide an interrupt to mouse-event feature.

(Actually this lib was started with a fork of THREE.OrbitalControls 😁)

@andrewplummer

This comment has been minimized.

Copy link
Contributor Author

andrewplummer commented Jan 7, 2020

  1. Well, it's definitely specific as it basically only pertains to MacBooks, however I think those are ubiquitous enough (you probably use one no?) and I think for a 3D environment two finger trackpad to "truck" feels a lot more natural. However because it is specific I DO think it should be opt-in... I just didn't want to assume how to do that in your library.

  2. It's possible, however you're already detecting isMac as part of your handling of the trackpad, so it is essentially using this trick already (actually the line with isMac and the one I added could probably be refactored into one).

  3. I agree. Similar to #1 my hope was that all default button configs could be kept the same as now and this feature should be wrapped in an if(...) with a new config. I just thought you would want to think yourself about the best way that config could work.

@yomotsu

This comment has been minimized.

Copy link
Owner

yomotsu commented Jan 7, 2020

How about just bind truck move to mouse wheel event by mouse config? Then we don't need to detect whether trackpad or not.

cameraControls.mouseButtons.wheel does not take ACTION.TRUCK as of now, but it's possible.

@andrewplummer

This comment has been minimized.

Copy link
Contributor Author

andrewplummer commented Jan 7, 2020

Well, the problem is that if an actual mouse is plugged in it wouldn't really make sense to have the actual mouse wheel truck, no? If there were a way to do this app-side (distinguish between mouse and trackpad) and have it handle them separately I would be fine with that, but it seems like this can only be determined in the actual wheel event itself. (They could have a mouse plugged in and also be using a trackpad for example).

@yomotsu

This comment has been minimized.

Copy link
Owner

yomotsu commented Jan 9, 2020

Then, could we make another config property called something like this.trachpad.two or so?
https://github.com/yomotsu/camera-controls/blob/master/src/CameraControls.ts#L189-L207

The default value of this.trachpad.two will be the same as this.mouseButtons.wheel but also takes

  • ACTION.TRUCK
  • ACTION.ROTATE

Sounds good?

@yomotsu

This comment has been minimized.

Copy link
Owner

yomotsu commented Jan 9, 2020

Also, leave a comment that the two-finger scroll gesture does not work in Edge and IE11 with precision touchpad such as Windows Surface.
https://developercommunity.visualstudio.com/content/problem/191727/two-point-touch-scrolling-in-code-viewer-doesnt-wo.html

@andrewplummer

This comment has been minimized.

Copy link
Contributor Author

andrewplummer commented Jan 9, 2020

Then, could we make another config property called something like this.trachpad.two or so?

Yes I think that's perfect! And very clear what it's doing.

Also, leave a comment that the two-finger scroll gesture does not work in Edge and IE11 with precision touchpad such as Windows Surface.

Ok I added a comment here, although I can't confirm this myself as I don't have a surface... it may be possible to workaround...

@yomotsu

This comment has been minimized.

Copy link
Owner

yomotsu commented Jan 10, 2020

Thanks!
I'll merge this and add the config then.

I have tested on Surface. It works in Chrome and Firefox but does not work in Edge and IE11 (as of today). But I think it's okay. They will release chromium-based edge soon!

@yomotsu yomotsu merged commit 8797945 into yomotsu:master Jan 10, 2020
@andrewplummer

This comment has been minimized.

Copy link
Contributor Author

andrewplummer commented Jan 10, 2020

Thank you! Also do you have an idea about the todo comment I put in for the line scope._getClientRect( elementRect ); ... basically this needs to get fired on the first time only like dragStart but I'm unsure how to do this (might require a hack)... if you move the trackpad around fast it has a little bit of "jolting" which might be related... I'm not sure yet...

@yomotsu yomotsu mentioned this pull request Jan 10, 2020
@yomotsu

This comment has been minimized.

Copy link
Owner

yomotsu commented Jan 10, 2020

I think there is no way to detect wheel-start event😢
I will leave this as it is for now. I'll think about this and let me know if you figure out.

@yomotsu

This comment has been minimized.

Copy link
Owner

yomotsu commented Jan 10, 2020

a little bit of "jolting"

probably, due to the type difference between wheelDeltaY and deltaY.
wheelDeltaY is int while deltaY is double.

according to the soucecode of chromium
Screen Shot 2020-01-11 at 8 00 23 AM

To detect the trackpad move, it compares them using
event.wheelDeltaY ? event.wheelDeltaY === - 3 * event.deltaY
But sometime it has an error like below.

Screen Shot 2020-01-11 at 1 00 46 AM

This commit may solve the problem.
6b3926e

@andrewplummer

This comment has been minimized.

Copy link
Contributor Author

andrewplummer commented Jan 11, 2020

I think there is no way to detect wheel-start event😢

I think you're right, but what about just a simple timeout so at least it's not hammering getClientRect? This is kind of a hack so not sure how you feel about it. I'm OK either way.

This commit may solve the problem.

Yup, just updated, this works perfectly!
Really love how this feels... I think it's a great update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.