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: Fix offscreen and cleanup #27522

Merged
merged 3 commits into from
Jan 9, 2024

Conversation

RenaudRohlinger
Copy link
Collaborator

@RenaudRohlinger RenaudRohlinger commented Jan 7, 2024

#27446 broke the offscreen canvas support (yes it is possible to use controls such as OrbitControls in workers by using virtual dom elements).

Also code syntax wasn't correct so I cleaned up the file to align with the lint rules.

This contribution is funded by Utsubo

@Mugen87 Mugen87 requested a review from sciecode January 7, 2024 09:56
@Mugen87 Mugen87 added this to the r161 milestone Jan 7, 2024
@sciecode
Copy link
Contributor

sciecode commented Jan 7, 2024

Attaching the ctrl intercept to the document and not domElement was done with a specific purpose of keeping the event listeners higher-up in the dom hierarchy, so stopPropagation calls wouldn't interfere with the the underlying logic.

This PR's solutions is not sufficient to guarantee correct interception, as can be shown in the following webgl_controls_map example

By clicking any of the options in UI and holding ctrl while scrolling, you can notice the zoom speed gets much faster. This is because the GUI is above domElement in the hierarchy and stops the propagation of the events controlActive logic.

This is part of the reason of why I was unsure about using this solution in the first place, it is finnicky.

#27446 broke the offscreen canvas support (yes it is possible to use controls such as OrbitControls in workers by using virtual dom elements).

Are you able to provide a bit more information on how it broke, and perhaps an example of the usage so we can see if there's a different solution here?

@RenaudRohlinger
Copy link
Collaborator Author

I see, would it work if instead of document we were using domElement.parentNode then? Otherwise it is also possible to just wrap the document with the condition to check if it is defined in the current context typeof document !== 'undefined'.

The current issue is that document is not available in web workers. /cc @sciecode

@sciecode
Copy link
Contributor

sciecode commented Jan 7, 2024

I see, would it work if instead of document we were using domElement.parentNode

I'm afraid parentNode is also not sufficient, however Node.getRootNode might be exactly what we need in this scenario.

We could try scope.domElement.getRootNode().addEventListener ..., it should give us HTMLDocument/iFrame on standard DOM, and the highest available DOMNode on web workers hierarchy.

@RenaudRohlinger
Copy link
Collaborator Author

RenaudRohlinger commented Jan 8, 2024

scope.domElement.getRootNode() now returns document in a normal context.
I make sure scope.domElement.getRootNode is defined as in some cases, if using a virtual element for example, it might be a hard-coded method.

@sciecode can you confirm if the behavior is now correct please?
https://raw.githack.com/renaudrohlinger/three.js/orbitcontrols-offscreen/examples/misc_controls_orbit.html

@sciecode
Copy link
Contributor

sciecode commented Jan 8, 2024

Sure, the behaviour on main thread will be correct. However it may not be correct when using a ProxyDispatcher element for OffscreenCanvas. We don't want to make it optional, we want to make it mandatory (no if).

If you are implementing your virtual handler based on the manual guide, you'll need to make some changes:

Worker

  • The ElementProxyReceiver needs to implement getRootNode() which returns a member dispatcher rootProxy.
  • Add a new method handleRootEvent that dispatches the incoming event from rootProxy and not from this.
  • Create a new type of web-worker message root-event, and modify ProxyManager so that when it receives a root-event it will call handleRootEvent.

Main-Thread

  • Add a new keydown event-listener to render.domElement.getRootNode() that posts this new root-event message to the worker.

Everything should work as intended, afterwards.

I'll make these changes to the manual soon.

examples/jsm/controls/OrbitControls.js Outdated Show resolved Hide resolved
examples/jsm/controls/OrbitControls.js Outdated Show resolved Hide resolved
@RenaudRohlinger
Copy link
Collaborator Author

Alright, I removed the condition and added a comment to prevent developers from replacing the getRootNode() by document in the future:

const document = scope.domElement.getRootNode(); // offscreen canvas compatibility

Looking forward to your PR on the manual @sciecode, thanks for that.

@sciecode
Copy link
Contributor

sciecode commented Jan 9, 2024

Thank you ❤️

I'll try to get the Manual changes done over the weekend, and will tag you when the PR is done.

@RenaudRohlinger RenaudRohlinger merged commit d75bb9b into mrdoob:dev Jan 9, 2024
11 checks passed
AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
* fix offscreen canvas and lint

* feedbacks

* added comment and removed if condition based on feedback
@ben-fly2success
Copy link

Thank you ❤️

I'll try to get the Manual changes done over the weekend, and will tag you when the PR is done.

@sciecode have you made the change somewhere ? Can't see it in last release.

@davidwinter
Copy link

@sciecode @RenaudRohlinger Thanks for everyones work with this. It would be great to have the manual updated with the required changes. As it stands, following the manual does not work, as I'm encountering the issue with getRootNode not existing. I tried to follow along with the comments but I'm a little stuck in my understanding. Any help would be much appreciated.

Also, @RenaudRohlinger are you using an alternative implementation different to that of the manual? Or you were able to implement the changes that were mentioned above?

@Peters69
Copy link

Peters69 commented Aug 8, 2024

@RenaudRohlinger: I got it running! Thanks a lot for your hard work in explaining something complex in such a great way. Should this also support zoom and pan on mobile/touch devices as mouse movement handling outside the window on desktop devices? If this is currently missing are there any plans to implement it or is this a current limitation we have to live with?

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.

7 participants