-
Notifications
You must be signed in to change notification settings - Fork 79
Standardize handedness transform to match other file format (FTV-299) #129
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
Conversation
a08673c to
3464a73
Compare
FBX change handedness in a weird way. It goes from (X,Y,Z) to (-X,Y,Z)
while the convention is usually to go from Z to -Z instead of changing
allong X.
By default, USD will continue to change basis going from (X,Y,Z) to
(X,Y,-Z), but there is now an option to force the change of basis to
match what FBX does ("change handedness: slow and safe as fbx"). This
allow consistency between formats.
3464a73 to
be6efe7
Compare
|
|
||
| /// <summary> | ||
| /// The matrix used to change basis is driven by the import options. This allow to be consistent with the | ||
| /// FBX/Alembic/Obj importers. |
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.
Maybe:
<summary>
Configurable matrix used for change of basis from USD to Unity and vice versa.
</summary>
<remarks>
Allows global configuration of the change of basis matrix, which e.g. is used to make the USD importer conform to the legacy FBX import convention in Unity, swapping the X-axis instead of the Z-axis.
</remarks>
| // The matrix to change the handedness (the basis) is different for FBX and doesn't | ||
| // follow convention. This bit change this basisChange matrix to match the user options. | ||
| // [DEFAULT] SlowAndSafe is the standard and transform (X,Y,Z) basis to (X,Y,-Z) | ||
| // SlowAndSafe_AsFBX is consistent with FBX/Alembic/Obj ans transform (X,Y,Z) basis to (-X,Y,Z) |
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.
Maybe:
The matrix to convert USD (right-handed) to Unity (left-handed) is different for the legacy FBX importer
and incorrectly swaps the X-axis rather than the Z-axis. This changes the basisChange matrix to match the
user options. See SceneImportOptions.BasisTransformation for additional details.
I think there is some way to literally reference that class (BasisTransformation) from the doc string above, but I forget how. It's like <ref> or something.
| // basisChange and basisChange.inverse are identical. Just aliasing here so the math below | ||
| // reads correctly. | ||
| var basisChangeInverse = basisChange; | ||
| var basisChangeInverse = UnityTypeConverter.basisChange; |
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.
It is no longer guaranteed that the inverse is the same as the basisChange now -- not sure if you don't care about that (just leave it as-is) or want to expose basisChangeInverse 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 computed the inverse of the matrix with -1 in [0, 0] and it gave me the same 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.
Yes, for your example they are the same, but now that matrix can be set to anything, there is no guarantee that the matrix is involutory, or even invertable at all.
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.
Right... but do we really allow any matrix?... it's true that I don't do any verification :/
Should I add a check or at least a comment/warning above?
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.
Well, we don't support any matrix, but we allow it because the matrix is public.
You could just expose the inverse matrix also and leave it to the user to set both correctly. That would also have the nice side effect of caching the inverse matrix, instead of computing it every time.
BTW, this matrix probably should be initialized to the default value, otherwise it only has the correct default when calling APIs via SceneImporter.
c3d3577 to
b1edaa0
Compare
Both basis change matrix and inverse matrix are cached. By default the basis change matrix is the one needed to change handedness by swapping the Z axis.
a31f7a5 to
46dd38d
Compare
FBX, Alembic, Obj (and probably other formats) change their handedness to match the left hand coordinate system of Unity by inverting the X vector.
It's not conventional to do that but USD still has to be able to match that. One option is added to the Change Handedness list, "SlowAndSafe_asFBX" that allow to apply the same change of basis as for FBX.
By default, USD will still change basis allong the Z axis and not X.