Skip to content

Matrix4: Avoid NaN values when scale is zero #32440

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.

Sign up for GitHub

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

Merged
Mugen87 merged 8 commits into from
Dec 8, 2025
+29 −4
Merged

Matrix4: Avoid NaN values when scale is zero #32440

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
605b589
Fix: Handle zero scale in Matrix4.extractRotation
Om-Mishra09 Dec 1, 2025
2c2c43a
Fix: Apply zero-scale safety to decompose() for consistency
Om-Mishra09 Dec 1, 2025
d5d18b4
Refactor: Use identity basis fallback in extractRotation, revert deco…
Om-Mishra09 Dec 3, 2025
1843933
Feat: Apply identity fallback to extractBasis as requested
Om-Mishra09 Dec 6, 2025
0111597
Style: Fix ESLint padded-blocks errors
Om-Mishra09 Dec 6, 2025
7bf0c6f
Refactor: Use determinant check for zero-scale robustness
Om-Mishra09 Dec 6, 2025
b96ed88
Refactor: Add determinant guard to decompose() as requested
Om-Mishra09 Dec 6, 2025
2fa0485
Update Matrix4.js
Mugen87 Dec 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/math/Matrix4.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,9 +277,9 @@ class Matrix4 {
const te = this.elements;
const me = m.elements;

const scaleX = 1 / _v1.setFromMatrixColumn( m, 0 ).length();
const scaleY = 1 / _v1.setFromMatrixColumn( m, 1 ).length();
const scaleZ = 1 / _v1.setFromMatrixColumn( m, 2 ).length();
const scaleX = 1 / ( _v1.setFromMatrixColumn( m, 0 ).length() || 1 );
const scaleY = 1 / ( _v1.setFromMatrixColumn( m, 1 ).length() || 1 );
const scaleZ = 1 / ( _v1.setFromMatrixColumn( m, 2 ).length() || 1 );
Copy link
Collaborator

@Mugen87 Mugen87 Dec 3, 2025

Choose a reason for hiding this comment

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

After some thinking about this change I see it as follows:

I don't think the proposed solution for extractRotation() is ideal. When the scale is zero and thus the length of the respective matrix column, the method should use the identity basis vector for that column. You can't derive a meaningful rotation from a zero scaled basis vector. The identity basis which is essentially the untransformed default direction makes most sense as a fallback to me. I would try to implement the method like so:

/**
 * Extracts the rotation component of the given matrix
 * into this matrix's rotation component.
 *
 * Note: This method does not support reflection matrices.
 *
 * @param {Matrix4} m - The matrix.
 * @return {Matrix4} A reference to this matrix.
 */
extractRotation( m ) {

	const te = this.elements;
	const me = m.elements;

	const lengthX = _v1.setFromMatrixColumn( m, 0 ).length();
	const lengthY = _v1.setFromMatrixColumn( m, 1 ).length();
	const lengthZ = _v1.setFromMatrixColumn( m, 2 ).length();

	// x

	if ( lengthX === 0 ) {

		te[ 0 ] = 1;
		te[ 1 ] = 0;
		te[ 2 ] = 0;
		te[ 3 ] = 0;

	} else {

		const scaleX = 1 / lengthX;

		te[ 0 ] = me[ 0 ] * scaleX;
		te[ 1 ] = me[ 1 ] * scaleX;
		te[ 2 ] = me[ 2 ] * scaleX;
		te[ 3 ] = 0;

	}

	// y

	if ( lengthY === 0 ) {

		te[ 4 ] = 0;
		te[ 5 ] = 1;
		te[ 6 ] = 0;
		te[ 7 ] = 0;

	} else {

		const scaleY = 1 / lengthY;

		te[ 4 ] = me[ 4 ] * scaleY;
		te[ 5 ] = me[ 5 ] * scaleY;
		te[ 6 ] = me[ 6 ] * scaleY;
		te[ 7 ] = 0;

	}

	// z

	if ( lengthZ === 0 ) {

		te[ 8 ] = 0;
		te[ 9 ] = 0;
		te[ 10 ] = 1;
		te[ 11 ] = 0;

	} else {

		const scaleZ = 1 / lengthZ;

		te[ 8 ] = me[ 8 ] * scaleZ;
		te[ 9 ] = me[ 9 ] * scaleZ;
		te[ 10 ] = me[ 10 ] * scaleZ;
		te[ 11 ] = 0;

	}

	te[ 12 ] = 0;
	te[ 13 ] = 0;
	te[ 14 ] = 0;
	te[ 15 ] = 1;

	return this;

}

@WestLangley What do you think?

Copy link
Collaborator

@WestLangley WestLangley Dec 3, 2025

Choose a reason for hiding this comment

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

I do not see how we can support setting scale to zero.

You can't extract a quaternion from a matrix that is not of full rank -- nor from one that is not orthogonal. And the approach that @Mugen87 has proposed is not invertible.

It would be best to add that this method requires the matrix, at a minimum, to be of full rank.

Also, the method name extractRotation is misleading. A rotation in three.js represents a pure rotation. The columns in the input matrix may not be orthogonal.

Copy link
Collaborator

@Mugen87 Mugen87 Dec 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I understand there are limitations but we should change something to at least avoid NaN values. Can you please recommend an approach with the most plausible result? As far as I understand, the suggested version from my previous comment produces at least an orthogonal matrix.

Copy link
Contributor Author

@Om-Mishra09 Om-Mishra09 Dec 3, 2025

Choose a reason for hiding this comment

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

@Mugen87 I have applied your suggested implementation (Identity fallback) to extractRotation.

I reverted my changes to decompose() for now to keep this consistent. Should I leave decompose as-is, or should I apply this same Identity fallback logic to it as well?

Copy link
Collaborator

@WestLangley WestLangley Dec 3, 2025

Choose a reason for hiding this comment

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

I understand you want to support setting matrix.scale( 0, 0, 0 ). When users do that, the quaternion and position are retained. Restoring the scale to the previous value restores the transform.

You are trying to get this particular method to work on an invalid input transform having less than full rank.

Instead of avoiding NaN's, I think we should avoid invalid inputs.

As far as I understand, the suggested version from my previous comment produces at least an orthogonal matrix.

I do not think that is true. You can end up with two basis vectors being identical. Here is a sample input matrix with zero scale on one axis.

1  0  0
0  0  1
0  0  0

Copy link
Collaborator

@Mugen87 Mugen87 Dec 3, 2025

Choose a reason for hiding this comment

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

My point is: We usually do not check user input or sanitize arguments. And it's hard to forecast how APIs are used and how transformation matrices end up. My goal was to make certain math methods more robust so we do not end up with values that completely break rendering. My feeling is sometimes you expect a consistency level in our math modules which is not always "required" in a 3D engine. So even if from a mathematical standpoint an implementation might produce "incorrect" results, it can be still be beneficial from a development point of view.

So I vote to merge a solution that avoids NaN values.

Copy link
Collaborator

@WestLangley WestLangley Dec 3, 2025

Choose a reason for hiding this comment

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

You are arguing that it is better to silently return an incorrect answer than to flag invalid inputs.

If you want to support zero scale values, I suggest starting with mesh.matrix.decompose() and (somehow) ensure it avoids NaNs with mesh.scale.set( 0, 1, 1 ).

That method is attempting to extract a pure rotation from a non full-rank matrix, which is not possible.


te[ 0 ] = me[ 0 ] * scaleX;
te[ 1 ] = me[ 1 ] * scaleX;
Expand Down Expand Up @@ -1041,9 +1041,9 @@ class Matrix4 {
// scale the rotation part
_m1.copy( this );

const invSX = 1 / sx;
const invSY = 1 / sy;
const invSZ = 1 / sz;
const invSX = 1 / ( sx || 1 );
const invSY = 1 / ( sy || 1 );
const invSZ = 1 / ( sz || 1 );

_m1.elements[ 0 ] *= invSX;
_m1.elements[ 1 ] *= invSX;
Expand Down