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

FlyControls: Introduce enabled property. #26154

Merged
merged 2 commits into from
Jun 27, 2023
Merged

Conversation

andredsm
Copy link
Contributor

@andredsm andredsm commented May 27, 2023

Description

Recently, I came across a bug in FlyControls class. This class has a variable named "status" which controls whether the user is dragging the mouse/pointer. This variable is a counter, which is incremented on pointerDown event, and decremented on pointerUp event. On pointerMove event, the camera only rotates if this "status" variable is greater than 0 (when dragToLook is set to True).

In my application, I have an input form of file type. When the file dialog opens up, if it is above the threejs canvas, and the user selects any file by double-clicking, this dispatch only one pointerUp event to the FlyControls. As a consequence, the FlyControls remains in an invalid state, because the "status" variable is negative, and the user can never rotate the camera again.

I've seen similar issues with this missing pointer/mouse up event: example. This issue occurs in all majors browsers. So, to avoid this problem, I change this control variable to a boolean that only indicates whether the user is dragging the pointer or not. I can't see any reason for this variable to be a counter.

A JSFiddle example to reproduce the bug: just remind to reposition the file dialog over the canvas, in such a way the double click to the file is also over the canvas. After this, you are not allowed to rotate the camera.

@andredsm andredsm changed the title Fix FlyControls pointer dragging state Fix FlyControls dragging state May 29, 2023
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 26, 2023

Sorry for the late feedback!

I can confirm the invalid state of FlyControls in your fiddle however it seems there is an issue with this PR. Assume somebody is using two fingers to use the controls. When the user lifts one finger, the previous implementation would not stop the camera transformation. With this PR, dragging is stopped since the controls do not count the pointers anymore. So in some sense the PR fixes your issue but introduces a potentially unwanted change of behavior for someone else.

Would it be helpful to have controls.enabled similar to OrbitControls? With this you could disable FlyControls when the file input pops up and enable the controls again when a file has been selected.

@andredsm
Copy link
Contributor Author

I agree with your comments and your suggestion. I haven't thought about the case of the user using a device with a touch screen, but indeed my current PR introduced this undesired behavior. Today later, I will update this PR to turn the manipulator on/off. Thank you for reviewing and suggestions.

@andredsm andredsm force-pushed the controls-state branch 2 times, most recently from c881543 to dff22b6 Compare June 27, 2023 16:42
@andredsm
Copy link
Contributor Author

@Mugen87 I've just pushed the changes to meet your suggestions. Now I can control when to turn the camera manipulator on/off. I've also merged the dev branch onto my branch to keep it synced, and then I rebased my commits to make it linear (the reason for my forced-pushed commits). 🙃

@Mugen87 Mugen87 added this to the r154 milestone Jun 27, 2023
@Mugen87 Mugen87 changed the title Fix FlyControls dragging state FlyControls: Introduce enabled property. Jun 27, 2023
@Mugen87 Mugen87 merged commit 992fa06 into mrdoob:dev Jun 27, 2023
17 checks passed
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.

2 participants