Skip to content

Conversation

@z4122
Copy link
Contributor

@z4122 z4122 commented Apr 22, 2024

Description
OrbitControls only set up when init, if user update Object's up, it will not take effect.
This PR update up in runtime.
And I have test it in example. In the beginning, the camera's up is (0, 1, 0), three seconds later, camera's up is (1, 0, 0)
https://github.com/mrdoob/three.js/assets/13222615/d4dca1fb-ca79-46ff-b8f3-d1e1b6d5cbb6

Relevant PR
#18829

@z4122 z4122 force-pushed the feat/orbitcontrols_can_change_up_now branch from 6aacfc5 to a071747 Compare April 22, 2024 15:49
@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 22, 2024

This PR actually breaks OrbitControls. E.g. the camera in misc_controls_orbit is initially not oriented correctly anymore.

https://rawcdn.githack.com/z4122/three.js/feat/orbitcontrols_can_change_up_now/examples/misc_controls_orbit.html

@z4122 z4122 force-pushed the feat/orbitcontrols_can_change_up_now branch from a071747 to c666427 Compare April 23, 2024 14:29
@z4122
Copy link
Contributor Author

z4122 commented Apr 23, 2024

This PR actually breaks OrbitControls. E.g. the camera in misc_controls_orbit is initially not oriented correctly anymore.

https://rawcdn.githack.com/z4122/three.js/feat/orbitcontrols_can_change_up_now/examples/misc_controls_orbit.html

Sorry for that mistake, please review again


// so camera.up is the orbit axis
quat.setFromUnitVectors( object.up, yAxisUp );
quatInverse = quat.clone().invert();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clone() usage still creates unnecessary instances.

When fixed, the code is actually like #18829. I don't think we need two open PRs for the same thing with essentially the same implementation.

Copy link
Contributor Author

@z4122 z4122 Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fixed. Shoud we close this PR, and wait #18829 to merge?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that is better. I've just rebased #18829 so it can be merged if approved.

@z4122 z4122 force-pushed the feat/orbitcontrols_can_change_up_now branch from c666427 to e35074c Compare April 23, 2024 15:58
@Mugen87 Mugen87 closed this Apr 23, 2024
@Mugen87 Mugen87 added this to the r164 milestone Apr 23, 2024
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.

3 participants