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

TransformControls PointerLock support #16093

Merged
merged 4 commits into from
Sep 19, 2019

Conversation

jbaicoianu
Copy link
Contributor

This pull request extends TransformControl's getPointer method to allow it to be used when pointer lock is enabled. Without this patch, the TransformControl will react to the mouse at the position it was when the pointer was locked, which leads to confusing behavior. The patch fixes this behavior by returning the center of the screen as the pointer location whenever the pointer is locked.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 29, 2019

/ping @arodic

@arodic
Copy link
Contributor

arodic commented Mar 29, 2019

Hey thanks @Mugen87

I'll take a look at it. Just wondering @jbaicoianu, in what scenario would you use TransformControls and pointerlock at the same time since there is no cursor?

Anyway, it should be ok to pick with some sort of visible crosshair in the middle of the screen. Without a visual aid, I think it would be very awkward to use.

@arodic
Copy link
Contributor

arodic commented Mar 29, 2019

As a side note, I have custom email filter for "TransformControls" but I appreciate the pings @Mugen87 :) Also, I apologise for slow response sometimes.

@jbaicoianu
Copy link
Contributor Author

jbaicoianu commented Mar 30, 2019

@arodic yes, in our case when pointerlock is enabled, our engine is already drawing a crosshair at the center, so it works intuitively. I don't think this is something the TransformControls needs to handle itself, since many people who use pointerlock controls have a crosshair of some sort, for other purposes - this change just lets TransformControls play nicely when the pointer is already locked via an external mechanism.

This tweet has a video which shows off how we're using TransformControls in first person mode - works like a charm, feels very intuitive to be able to grab an object then walk off while holding it

https://twitter.com/matrixscene/status/1111522564692566016?s=20

@arodic
Copy link
Contributor

arodic commented Apr 5, 2019

This looks good to me. I am going to add pointer lock to example and send you a PR into jbaicoianu:transformcontrols-pointerlock then we can merge it to dev as one PR.

I'm not sure why the CI is throwing error. Some kind of timeout?

@arodic
Copy link
Contributor

arodic commented Apr 5, 2019

Ugh... turns out PointerLockControls doesn't play nice with other controls and its kind of broken. Unlike other controls it does not emit change event, in constructor it sets camera rotation to (0,0,0) for some reason, and after pointer unlock, OrbitControls goes haywire since PointerLockControls adds camera to a rotated Object3D perhaps it changes camera's rotation axis order to YXZ?.

Not sure what is the best approach to test pointerlock behavior with TransformControls. I'd fix PointerLockControls but not sure how much friction i'd face.

@jbaicoianu if you have a working example I'm in favor of merging this, but ideally, transform controls example should have pointerlock option.

@arodic
Copy link
Contributor

arodic commented Apr 17, 2019

Since fixing #16160 is taking longer than expected, I suggest to merge this and I'll add PointerLock in the example later.

Not sure why LGTM analysis is failing. @jbaicoianu perhaps you can amend your branch to trigger another CI pass to see if its just a fluke.

Verified

This commit was signed with the committer’s verified signature.
jkuester Joshua Kuestersteffen
…/three.js into transformcontrols-pointerlock
@jbaicoianu
Copy link
Contributor Author

jbaicoianu commented Apr 17, 2019

Not sure what happened with the LGTM analysis originally, but when I submitted this pull request I noticed there were a couple other pull requests that failed in the same way, so I suspect it may have just been a temporary issue with the service. I've pushed a minor change and it's rechecking, let's see if it works this time around.

We're not using PointerLockControls in our engine, we've built our own implementation so I'm not as familiar with PointerLockControls' internals - all of the issues you pointed out sound familiar from when we evaluated it a few years back though. Sounds like some clean-up would be nice to have, but as you noted, it's always hard to change something when it's worked that way for so long. Maybe since these are examples anyway, it might make sense to implement it as a separate control, like PointerLock2Controls, or something more creatively named - maybe specifically FPSControls?

Right now my only example of TransformControls and pointer lock working together is in my local dev copy, I don't have a publicly accessible link yet but I'm working on finalizing the code for a release soon.
I'll let you know when I have a live example I can share.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 17, 2019

maybe specifically FPSControls?

There is already FirstPersonControls...

@arodic
Copy link
Contributor

arodic commented Apr 17, 2019

There is already FirstPersonControls

True, I tried using that but noticed it does not implement PointerLock, and also lacks change event, etc.

Sounds like some clean-up would be nice to have, but as you noted, it's always hard to change something when it's worked that way for so long. Maybe since these are examples anyway, it might make sense to implement it as a separate control...

This brings up a larger issue with examples/js/controls and examples/js/ in general. The code varies significantly in style, quality and lacks constant API. I understand that examples are regarded as non-essential and nothing more than "examples" but I believe that controls are actually quite important and could benefit from unified API. Wouldn't it be nice if you could hot-swap any control in your app with a few lines of code and have them just work without application-side fixups? In theory, refactoring controls is easy. In practice, there is a quite a lot of friction because "it's always hard to change something when it's worked that way for so long".

@arodic
Copy link
Contributor

arodic commented May 2, 2019

This PR has been tested and its OK to merge. I submitted the example on top of it from this branch

@Mugen87 Mugen87 merged commit d3e8a2b into mrdoob:dev Sep 19, 2019
@Mugen87 Mugen87 added this to the r109 milestone Sep 19, 2019
@mrdoob
Copy link
Owner

mrdoob commented Sep 19, 2019

Thanks!

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

4 participants