Skip to content

Conversation

@Om-Mishra09
Copy link

Description
The Issue Currently, extractRotation assumes the matrix columns (basis vectors) have a non-zero length. If an object has a scale of 0 on any axis (e.g., mesh.scale.set(0, 0, 0) to hide it), the column length becomes 0.

This results in a division by zero (Infinity), causing the entire matrix to become NaN. This NaN propagation can crash downstream logic (physics engines, world matrix decompositions) that rely on this method.

Solution
Added a safety check || 1 to the divisor. This pattern matches safety checks found elsewhere in the codebase (e.g. Vector3.normalize) to ensure the method remains robust even with degenerate matrices.

Reproduction

const m = new THREE.Matrix4().makeScale(0, 0, 0);
const res = new THREE.Matrix4();
res.extractRotation(m);
console.log(res.elements); 
// Before: [NaN, NaN, NaN...]
// After: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]

@github-actions
Copy link

github-actions bot commented Dec 1, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 350.9
83.23
350.9
83.23
+0 B
+0 B
WebGPU 616.15
171.03
616.15
171.03
+0 B
+0 B
WebGPU Nodes 614.75
170.76
614.75
170.76
+0 B
+0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 482.96
118.03
483.14
118.07
+181 B
+46 B
WebGPU 686.81
186.58
686.99
186.62
+181 B
+41 B
WebGPU Nodes 636.65
173.8
636.83
173.84
+181 B
+40 B

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 1, 2025

For reproduction: https://jsfiddle.net/0z5hcp4m/

I agree we should handle the 0 scale case more robust.

@WestLangley Does the fallback to 1 looks sensible to you?

@WestLangley
Copy link
Collaborator

Did we decide to support such matrices?

If so, it must be handled in a consistent manner everywhere. That would include, for example, Matrix4.decompose().

@Om-Mishra09 What is your use case?

@Om-Mishra09
Copy link
Author

@WestLangley My primary use case is handling objects that are scaled to (0, 0, 0)—for example, during shrink/pop-out animations or to temporarily hide them without removing them from the scene graph.

If any logic (like physics sync or world matrix updates) calls extractRotation during that zero-scale state, the resulting NaNs propagate and can permanently corrupt the object's state, preventing recovery even if the scale returns to 1 later.

I agree regarding consistency. I can check Matrix4.decompose() and apply the same robustness fix there as well. Would you like me to include that in this PR?

@Om-Mishra09
Copy link
Author

I've updated Matrix4.decompose() to include the same robustness fix (using the || 1 pattern) for consistency.
Ready for review!

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 2, 2025

Did we decide to support such matrices?

The zero scale usage is indeed a common thing especially when animating objects. I do think we should support this use case.

A good start would be avoiding matrices with NaN values so the engine handles zero scale more gracefully.

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

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

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

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
Author

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

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

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

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.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 3, 2025

It looks like there is not enough support for this change. Closing.

@Mugen87 Mugen87 closed this Dec 3, 2025
@Mugen87 Mugen87 added this to the r182 milestone Dec 3, 2025
@Om-Mishra09
Copy link
Author

Understood! Thanks @Mugen87 and @WestLangley for the detailed discussion and review.

While this specific solution wasn't the right fit, I'm glad we identified the edge case. I see it's added to the r182 milestone—if you decide on a preferred approach (e.g., throwing a warning instead of a fallback), I'd be happy to help implement that.

Thanks again!

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 4, 2025

I see it's added to the r182 milestone

I'll add even closed PRs to milestones so we can easier see in what sprint the PR was discussed.

@WestLangley
Copy link
Collaborator

@Mugen87 After thinking about this more, I agree with you that we should do something that causes less friction for users.

I see Matrix4.invert() silently returns the zero matrix if the determinant is zero. There have been no user complaints with that workaround as far as I am aware.

In the case of Matrix4.extractRotation(), the basis is lost and not recoverable, if a scale component is zero. Perhaps it would be OK to silently return the identity matrix in that case.

And in the case of Matrix4.extractBasis(), silently return the identity matrix basis vectors.

That would meet your requirement of silently handling zero scale.

Do you think that would work?

@WestLangley
Copy link
Collaborator

Reopening for now.

@WestLangley WestLangley reopened this Dec 5, 2025
@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 5, 2025

That sounds great to me! Thanks for the reconsideration!

@Om-Mishra09 Do you think you can update the PR according to the suggestions made in #32440 (comment)? It's okay to just focus on extractRotation() in this PR.

@hybridherbst
Copy link
Contributor

hybridherbst commented Dec 5, 2025

It just so happens that I made an animated test asset with weird matrices recently (it will go up on the Khronos glTF test assets at some point, as test asset explicitly for matrix decomposition):

20251110-AffineTransformationAnim.glb.zip

image

If you get the worldMatrix of node "Cube", and decompose it, it should a) give some decomposition result and b) be somewhat close to the result of "Decomposed Cube".

You can set the pos/rot/scale of "Decomposed Cube - Interactivity" to visually compare right in the scene how close your decomposition result is to "ground truth" while the animation is running. "Ground truth" here means: it's a recording of the decomposition result of Unity's matrix math to compare against. The matrix is flipped, sheared, and scaled to 0 at some points throughout the animation.

Hope that helps!

@Om-Mishra09
Copy link
Author

Thanks for the reconsideration @WestLangley!

I agree that reducing friction for users is a great goal. My current implementation of extractRotation already handles the identity fallback as described.

I have just pushed an update to apply the same logic to Matrix4.extractBasis(): if a basis vector has zero length, it now defaults to the identity basis vector (e.g., (1, 0, 0)).

Ready for review!

@WestLangley
Copy link
Collaborator

@Om-Mishra09 As I suggested above,

In Matrix4.extractRotation(), if the determinant is zero, silently return the identity matrix.

In Matrix4.extractBasis(), if the determinant is zero, silently return the identity matrix basis vectors.

Please fully test it in as many use cases as you can think of, and see if this workaround makes sense to you.

@Om-Mishra09
Copy link
Author

@WestLangley I've refactored both extractRotation and extractBasis to use the determinant() === 0 check as a guard clause, as suggested.

  • extractRotation: returns this.identity() if the input matrix determinant is 0.
  • extractBasis: sets the provided vectors to the identity basis if this.determinant() is 0.

This implementation is definitely cleaner. I've verified that makeScale(0,0,0) now correctly falls back to identity without producing NaNs.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 6, 2025

Can we do something similar for decompose()? Meaning if the determinant is 0, set scale to (1,1,1) and quaternion to its identity? position is extracted as usual.

@Mugen87 Mugen87 changed the title Fix: Prevent NaN in Matrix4.extractRotation when scale is zero Matrix4: Avoid NaN values in certain methods. Dec 6, 2025
@Om-Mishra09
Copy link
Author

@Mugen87 Great idea. I've updated decompose() to handle the zero-determinant case as requested:

  • Position is extracted first (so translation is preserved).
  • If determinant() === 0, scale defaults to (1, 1, 1) and quaternion defaults to identity.

This ensures all three methods (extractRotation, extractBasis, decompose) are now robust against singular matrices.

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.

4 participants