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

TransformControls: Use in-plane rotation when eye and selected gizmo axis are parallel #26860

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

alankalb
Copy link
Contributor

Description

I found this bug when using the rotate TransformControls in a web-app that allows users to set a camera to be perfectly parallel to a cardinal axis. When the TransformControls eye and the selected axis for rotation are parallel, the resulting rotation is zero as the cross product of two parallel vectors is the zero vector and the dot product with the zero vector is zero. This was fixed by performing the cross product and checking if the resulting vector has a length of zero, indicating that the vectors are parallel. If the vectors are parallel, the rotation logic for the E handle is used.

This bug does not occur very often as setting the value of this.eye from the camera position usually small rounding errors causing the vector to not be perfectly parallel with a cardinal axis. For the app I am working on, the camera can be set perfectly inline with a cardinal axis when the rotation control is displayed causing the bug to occur frequently.

Other notes/questions:

  • I am not sure if _inPlaneRotation is the best name for the variable. I tend to refer to the E handle as the in-plane handle but I am open to other suggestions.
  • I did not check if _tempVector is equal to _zeroVector as the resulting vector could include -0 and may not be strictly equal to _zeroVector even when _tempVector and this.eye are parallel

Before (using a modified version of the transform controls example where I have hardcoded the camera position to be parallel to the x-axis):

Screen.Recording.2023-09-27.at.4.52.49.PM.mov

After:

Screen.Recording.2023-09-27.at.4.53.17.PM.mov

@alankalb alankalb changed the title Use in-plane rotation when eye and selected gizmo axis are parallel TransformControls: Use in-plane rotation when eye and selected gizmo axis are parallel Sep 28, 2023
_tempVector.cross( this.eye );

// When _tempVector is 0 after cross with this.eye the vectors are parallel and should use in-plane rotation logic.
if ( _tempVector.length() === 0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be compared with an epsilon?

Copy link
Contributor Author

@alankalb alankalb Sep 28, 2023

Choose a reason for hiding this comment

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

By epsilon, do you mean a small threshold value? That could certainly help when the camera and axis are not strictly parallel but are practically parallel. When this happens the resulting cross product is normalized making a seemingly random vector that is used as the reference for rotation. If we we were to use an epsilon (assuming I am understanding it correctly) we would probably need to update the line this.rotationAxis.copy( this.eye ); in the in_plane rotation logic to be something like _inPlaneRotation ? this.rotationAxis.copy( _unit[ axis ] ) : this.rotationAxis.copy( this.eye ); to ensure you rotate around the intended axis rather than this.eye which may be very slightly off-axis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LeviPesin Pinging you to get feedback on my previous response to your question.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not very sure, but I think using this.eye would be fine if it would be in any case within distance of epsilon from the axis?

@mrdoob
Copy link
Owner

mrdoob commented Sep 28, 2023

/ping @arodic

@arodic
Copy link
Contributor

arodic commented Oct 9, 2023

lgtm. Looking at the code this should work fine but please make sure there are no regressions.

@Mugen87 Mugen87 added this to the r158 milestone Oct 10, 2023
@Mugen87 Mugen87 merged commit 856c330 into mrdoob:dev Oct 10, 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.

5 participants