-
-
Notifications
You must be signed in to change notification settings - Fork 36.1k
Matrix4: Avoid NaN values in certain methods.
#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.
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
base: dev
Are you sure you want to change the base?
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
|
For reproduction: https://jsfiddle.net/0z5hcp4m/ I agree we should handle the @WestLangley Does the fallback to |
|
Did we decide to support such matrices? If so, it must be handled in a consistent manner everywhere. That would include, for example, @Om-Mishra09 What is your use case? |
|
@WestLangley My primary use case is handling objects that are scaled to If any logic (like physics sync or world matrix updates) calls I agree regarding consistency. I can check |
|
I've updated |
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 |
src/math/Matrix4.js
Outdated
| 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 ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
It looks like there is not enough support for this change. Closing. |
|
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! |
I'll add even closed PRs to milestones so we can easier see in what sprint the PR was discussed. |
|
@Mugen87 After thinking about this more, I agree with you that we should do something that causes less friction for users. I see In the case of And in the case of That would meet your requirement of silently handling zero scale. Do you think that would work? |
|
Reopening for now. |
|
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 |
|
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
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! |
|
Thanks for the reconsideration @WestLangley! I agree that reducing friction for users is a great goal. My current implementation of I have just pushed an update to apply the same logic to Ready for review! |
|
@Om-Mishra09 As I suggested above, In In Please fully test it in as many use cases as you can think of, and see if this workaround makes sense to you. |
|
@WestLangley I've refactored both
This implementation is definitely cleaner. I've verified that |
|
Can we do something similar for |
NaN values in certain methods.
|
@Mugen87 Great idea. I've updated
This ensures all three methods ( |

Description
The Issue Currently,
extractRotationassumes 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 becomeNaN. ThisNaNpropagation can crash downstream logic (physics engines, world matrix decompositions) that rely on this method.Solution
Added a safety check
|| 1to 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