Bugfix at matrix calculation at FBX import #19093
Conversation
|
Is it somehow possible to demonstrate the issue with an asset? I mean is it actually visible when loading a FBX file or is it more an internal thing? |
|
@looeee any chance you could test this? |
|
matrix1.multiply( matrix2 ) modifies matrix1 in place, which i guess works as intended. In the FBXLoader this is now used as matrix3 = matrix1.multiply( matrix2 ), which becomes a problem, since matrix1 is used in a later calculation during the quite complicated fbx pose calculation process. However, this only becomes apparent if the objects in a fbx have a local rotation. Issues #15745 and #15097 might be related. I have a simple test file of two goats staring at a tree: Blender Three.JS viewer |
|
Also i only dabble as a js developer, and my solution feels somewhat inelegant. I wouldn't mind some hints on how to refactor that... |
Yikes. I would avoid using that pattern. It is very confusing. One alternative is: matrix3.copy( matrix1 ).multiply( matrix2 );The matrix methods return |
| var lParentGSM = new Matrix4(); | ||
| var lParentGRSM = new Matrix4(); | ||
|
|
||
| var lParentTM_inv = new Matrix4().getInverse( lParentTM ); |
There was a problem hiding this comment.
Just new Matrix4() is probably OK here since getInverse is called on the identity matrix.
There was a problem hiding this comment.
Oh, I've just noticed this comment. Well, we can do further clean up PRs 🙂
|
|
||
| var lLocalTWithAllPivotAndOffsetInfo = new THREE.Matrix4().copyPosition( lTransform ); | ||
|
|
||
| var lGlobalTranslation = lParentGX.multiply( lLocalTWithAllPivotAndOffsetInfo ); | ||
| var lGlobalTranslation = new Matrix4().multiply(lParentGX).multiply( lLocalTWithAllPivotAndOffsetInfo ); |
There was a problem hiding this comment.
Formatting: needs spaces in parens multiply( lParentGX )
| lGlobalT.copyPosition( lGlobalTranslation ); | ||
|
|
||
| lTransform = lGlobalT.multiply( lGlobalRS ); | ||
| lTransform = new Matrix4().multiply(lGlobalT).multiply( lGlobalRS ); |
|
Works great, good job! This does fix #15745. Unfortunately, files on #15097 were shared on some site that doesn't work now, but I think we can assume this fixes that too.
Yeah, I think I did it that way to avoid an extra copy. That was definitely premature optimization though. I've done some timing and this code is only adding a couple of ms to the total load time. @WestLangley do you think the PR is OK as is, or would you prefer to use the |
|
I'll clean up the formatting post merge. |
|
@looeee I prefer what I suggested, but I think you should do whatever you prefer. :-) |
|
@WestLangley I think the code you're talking about is the previous code and not the new code. |
|
OK, then I think this PR is fine. We can look at improving the readability of this function another time. Thanks @crest01! |
|
Just wanted to say, this bugfix works great: no more issues with fbx generated from Cinema 4D, and no regression on other models. |
During FBX import the transformation matrix calculation uses the Matrix4.multiply() - function wrong, which causes objects to have a wrong local rotation.
New Pull request, since #19092 was created from the wrong branch.