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: Introduce listenToPointerEvents(). #21224

Closed
wants to merge 1 commit into from

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Feb 8, 2021

Related issue: #20575 (comment)

Description

Introduce listenToPointerEvents() which must be used to add event listeners for point events. The second parameter of OrbitControls is now deprecated (but still functional).

To me, the change has one minor flaw. listenToPointerEvents() does not only register event listeners to pointer events but to contextmenu, wheel and touch* events, too.

However, as far as I understand the point is to create an instance of OrbitControls without adding event listeners in the ctor. Still, the name of listenToPointerEvents() is not quite right...

@Mugen87 Mugen87 marked this pull request as draft February 8, 2021 13:48
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 8, 2021

@mrdoob
Copy link
Owner

mrdoob commented Feb 8, 2021

/cc @gaearon @drcmda

@mrdoob
Copy link
Owner

mrdoob commented Feb 8, 2021

To me, the change has one minor flaw. listenToPointerEvents() does not only register event listeners to pointer events but to contextmenu, wheel and touch* events, too.

Ah, I forgot about the wheel one...

Still, the name of listenToPointerEvents() is not quite right...

Does addPointerEvents( renderer.domElement ) sound better?

@mrdoob mrdoob added this to the r126 milestone Feb 8, 2021
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 8, 2021

It's actually the term pointer that feels not right^^. I mean considering the fact that the method adds an event listener to contextmenu and wheel. The touch events might not be that important since we eventually can replace them with pointer events some day.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 8, 2021

Maybe as suggested in #20575 and use connect()? listenToKeyEvents() could be renamed to connectKeys() or something similar.

@drcmda
Copy link
Contributor

drcmda commented Feb 8, 2021

This is fantastic, thanks!

@mrdoob mrdoob modified the milestones: r126, r127 Feb 23, 2021
@drcmda
Copy link
Contributor

drcmda commented Feb 28, 2021

@Mugen87 is there a reason key events are not added by default but through a connect function? i wonder, couldn't these two just be combined?

@mrdoob mrdoob modified the milestones: r127, r128 Mar 30, 2021
@mrdoob mrdoob modified the milestones: r128, r129 Apr 23, 2021
@mrdoob
Copy link
Owner

mrdoob commented May 27, 2021

How about listenToUserEvents( element )?

@mrdoob mrdoob modified the milestones: r129, r130 May 27, 2021
@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 27, 2021

How about listenToUserEvents( element )?

In this case we would have OrbitControls.listenToUserEvents() and OrbitControls.listenToKeyEvents(). For simplicity, how about this combination:

controls.listenToEvents( renderer.domElement ); // default method for setup event listeners
controls.listenToKeyEvents( window ); // additional method for specific events.

The connect(), connectKey() and disconnect() pattern seems out of question, right^^?

@Mugen87 is there a reason key events are not added by default but through a connect function? i wonder, couldn't these two just be combined?

Yes, key event listeners are better placed at window whereas pointer events better fit to renderer.domElement.

@mrdoob mrdoob modified the milestones: r130, r131 Jun 30, 2021
@haywirez
Copy link

haywirez commented Jul 2, 2021

Don't forget about domElement.style.touchAction = 'none', I think it should be moved to the listenToUserEvents() setup call as well.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jul 2, 2021

This PR is heavily outdated. I'll file a new one when we have agreed on the new API of OrbitControls.

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.

4 participants