matrix.extractRotation() returns incorrect rotation matrix when scale is negative

[arodic]

setFromRotationMatrix is incorrect when scale is negative on any axis. matrix.decompose returns correct rotation.

I assume this is because the following code is missing in setFromRotationMatrix (present in matrix.decompose):

// if determine is negative, we need to invert one scale
var det = this.determinant();
if ( det < 0 ) sx = - sx;

[WestLangley]

In your case, are you referring to Quaternion or Euler?

In either case, you have to pass in a pure rotation matrix. (That is where the method name comes from.)

See the code comment:

// assumes the upper 3x3 of m is a pure rotation matrix (i.e, unscaled)

[arodic]

I'm referring to Quaternion. What I did was:

_worldRotation.setFromRotationMatrix( _tempMatrix.extractRotation( this.object.matrixWorld ) );

Here I made a quick jsfiddle showing the problem: https://jsfiddle.net/y6ktuvs4/4/

[WestLangley]

You are trying to extract a pure rotation from a matrix that represents a rotation and a reflection. I'm not sure if doing so is valid, or not.

I expect the issue is with extractRotation(), no?

[arodic]

Right, I assumed that is what extractRotation() was supposed to do, right?

[arodic]

Also, I updated the fiddle to show that object.matrixWorld.decompose() returns correct quaternion after reflection.

[arodic]

Just confirmed, extractRotation() is the problem. The returned rotation matrix is not clean when any scale axis is negative.

[WestLangley]

As soon as you allow negative scale, the implied quaternion is no longer unique.

Consider matrix.makeScale( - 1, 1, - 1 ).

That can be interpreted as a reflection through X and Z with rotation zero; it can also be interpreted as a pure rotation around Y of 180 degrees.

[arodic]

That makes sense, yet I notice that decompose(), which returns expected quaternion, internally does something similar. It creates what looks like rotation matrix, then creates a quaternion from that matrix with quaternion.setFromRotationMatrix( matrix );. But it appears it is not a pure rotation matrix, and it inverts elements 0, 1 and 2 when determinant is negative. That is why I assumed that determinant inversion was causing the discrepancy between decompose() and quarternion from extracted rotation matrix.

[WestLangley]

Actually, extractRotation() is not even correct because it fails to set all the matrix elements.

@arodic Maybe you can use decompose(), instead, extracting the quaternion.

/ping @Mugen87 Can you avoid using the method in Reflector, Refractor, and Water?

[arodic]

@WestLangley I ended up using decompose instead but I find it less semantically intuitive because it mutates the arguments and I didn't really need translate and scale so I had to pass two dummy vectors to make it work.

[Mugen87]

Can you avoid using the method in Reflector, Refractor, and Water?

Basically yes. Instead of:

three.js/examples/js/objects/Reflector.js

Lines 81 to 85 in 4fab7a7

	rotationMatrix.extractRotation( scope.matrixWorld );
	normal.set( 0, 0, 1 );
	normal.applyMatrix4( rotationMatrix );

we could do this:

scope.matrixWorld.decompose( position, quaternion, scale );

normal.set( 0, 0, 1 );
normal.applyQuaternion( quaternion );

[WestLangley]

For historical perspective, the three.js paradigm has always been to control an object's transform by specifying position, quaternion, and scale. Scale was assumed to be positive.

This paradigm only supports pure rotations, and it is more limiting than specifying a matrix transform, which additionally supports reflections and shearing.

Due to demand, we more recently added support for negative scale factors. We can clearly see the problems that arise when a feature does not work universally. (FWIW, another example would be Mesh.drawMode.)

We can try to 'fix' extractRotation(), but I am not sure that is possible based on my earlier comments.

We can also remove it.

@arodic and @Mugen87 will you consider using alternate methods in the code you are maintaining, so this method is no longer used in the library?

[arodic]

Yes, I I'm in favor of removing extractRotation() since decompose does work.

[Mugen87]

TBH, I don't see the need to remove this method. I think it's okay when methods make assumptions about their usage. Besides, .decompose() is computationally more expensive.

Actually, extractRotation() is not even correct because it fails to set all the matrix elements.

This should be an easy fix, right?