Skip to content
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

Calculate I3dm instance rotations by converting basis to a matrix #1047

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

timoore
Copy link
Contributor

@timoore timoore commented Dec 16, 2024

The previous approach tried to create rotations directly from the up and right vectors that encode the instance rotation, but it naïvely didn't handle rotations of 180 degrees of the up and right vector. It is reliable to create the rotation matrix, filling in the Z column by doing a cross-product, and then let the algorithm for converting a rotation matrix to a quaternion do its robust thing.

Addresses #1046. Note: in Cesium for Unreal, the instances in the sample tileset supplied by the user in CesiumGS/cesium#11477 still have missing geometry. This is most likely due to negative scale factors in the instance transformations; it's not clear if this can be addressed in a practical way.

The previous approach tried to create rotations directly from the up
and right vectors that encode the instance rotation, but it naïvely
didn't handle rotations of 180 degrees of the up and right vector. It
is reliable to create the rotation matrix, filling in the Z column
by doing a cross-product, and then let the algorithm for converting a
rotation matrix to a quaternion do its robust thing.
@kring
Copy link
Member

kring commented Dec 17, 2024

@timoore there's code for turning a 3x3 rotation+scale matrix into a quaternion in Transforms::computeTranslationRotationScaleFromMatrix, and it handles the possibility of a negative scale. The trick would be constructing that rotation+scale matrix, which would require deciding which is applied first, rotation or non-uniform scale? The I3DM spec doesn't seem clear on this to me, but maybe I missed it somewhere.

But also, CesiumJS doesn't have the missing geometry problem you mentioned, right? It just has incorrect normals? So we can at least match CesiumJS behavior, which seems like it would be an improvement.

@timoore
Copy link
Contributor Author

timoore commented Dec 17, 2024

@timoore there's code for turning a 3x3 rotation+scale matrix into a quaternion in Transforms::computeTranslationRotationScaleFromMatrix, and it handles the possibility of a negative scale. The trick would be constructing that rotation+scale matrix, which would require deciding which is applied first, rotation or non-uniform scale? The I3DM spec doesn't seem clear on this to me, but maybe I missed it somewhere.

We get translation, rotation, and scale separately from I3dm for each instance, and it they are applied in that order. The bug addressed here is specifically in the process of turning orthonormal right, up vectors into a rotation and is not affected by negative scale factors. The new code here does what Transforms::computeTranslationRotationScaleFromMatrix ultimately does too: call glm::quat_cast.

But also, CesiumJS doesn't have the missing geometry problem you mentioned, right? It just has incorrect normals? So we can at least match CesiumJS behavior, which seems like it would be an improvement.

Well, vsgCs doesn't have the bug either when using this PR. I agree that the problem shouldn't be hand-waved away saying that "Unreal doesn't like negative scaling factors," but we already go to a lot of trouble to avoid right-hand coordinate systems in vertex geometry for Unreal, and at first glance it doesn't seem super practical to deal with instance geometry on an instance-by-instance basis in order to handle negative scale factors.

@kring
Copy link
Member

kring commented Dec 18, 2024

Ok, thanks Tim. I don't quite understand what's going wrong in Unreal, though. Unreal doesn't inherently have a problem with negative scale. Even the shenanigans we added to avoid it were added to make physics work correctly; rendering was happy even with the negative scale (see CesiumGS/cesium-unreal#1126). I spent a little bit of time looking at this, though, and it's not obvious to me what the problem is. Presumably our math is wrong somewhere, but I don't know where. It's certainly more complicated in Unreal than elsewhere because of the inversion of the Y coordinate in vertex positions, and the paired matrix that un-does that and effectively cancels out the negative scale in the right-handed to left-handed transformation.

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.

2 participants