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 meddles with camera after disabled #19917

Closed
2 tasks done
kingpotter1990 opened this issue Jul 24, 2020 · 4 comments · Fixed by #19925
Closed
2 tasks done

OrbitControls meddles with camera after disabled #19917

kingpotter1990 opened this issue Jul 24, 2020 · 4 comments · Fixed by #19925

Comments

@kingpotter1990
Copy link

kingpotter1990 commented Jul 24, 2020

Description of the problem

I was trying to manually set camera to a position and lookat target and disable control. However the disable does not seem to really shut down control from meddleing with the camera, unless I also set the control's target to be the same as the new camera's lookat target, it will update the camera.

Sample code:

// Try set my camera, then disable orbit control
updateCamera(pos, target) {
    this.camera.position.copy(pos);
    this.camera.lookAt(target);
    // This won't work
    this.orbitControl.enabled = false;
}

Possible cause that I guess:
I have a mainloop calback that keeps calling orbitControls.update() for inertia animation, my guess is that the update function meddles with camera and does not respect that enabled=== false setting.

I found the cure is to add this:

this.orbitControls.target = pos; // I also noted that setting target would re-enable a disabled orbit controls.

I guess my main complaint is that I did not expect orbitControls to be able to change camera at all, after setting disabled. Including setting target, or calling update.

Three.js version

r118

Browser
  • All of them
OS
  • All of them
@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 24, 2020

Setting OrbitControls.enabled to false just prevents that the controls from reacting on certain events like mousedown or mousemove. If you still invocated update() in your code, the controls will perform their update routine and e.g. orient the camera towards the focus point.

Interestingly, not all control classes are implemented that way. FirstPersonControls has this line in its update method:

return function update( delta ) {
if ( this.enabled === false ) return;

I think it's okay adding this line in OrbitControls, too.

@WestLangley
Copy link
Collaborator

Setting OrbitControls.enabled to false just prevents that the controls from reacting on certain events like mousedown or mousemove. If you still invoke update() in your code, the controls will perform their update routine and e.g. orient the camera towards the focus point.

@Mugen87 is correct.

However, disabling update() if .enabled is false will abruptly halt damping, which I do not think is desirable when users maintain the same camera.

Worse, disabling update() if .enabled is false will prevent auto rotation even if .autoRotate is true.

I think this is something you need to handle at the app-level.

Also, OrbitControls is an example. It is OK to modify it to suit your needs.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 25, 2020

However, disabling update() if .enabled is false will abruptly halt damping, which I do not think is desirable when users maintain the same camera.

Worse, disabling update() if .enabled is false will prevent auto rotation even if .autoRotate is true.

Well, one could expect that this the exact behavior if enabled is set to false. That the controls have no influence on the camera anymore.

It makes probably sense to improve the docs so they state OrbitControls.enabled just defines whether the class reacts on user input or not.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 25, 2020

After some testing, user code could actually break if the behavior is changed. Against this backdrop, I vote to just clarifying the docs and not changing OrbitControls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants