Rotate glTF models on import.#1505
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1505 +/- ##
=======================================
Coverage 92.05% 92.05%
=======================================
Files 35 35
Lines 6039 6039
=======================================
Hits 5559 5559
Misses 480 480 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| const halfRoot2 = Math.sqrt(2) / 2; | ||
| const original = sourceNode.getRotation(); | ||
| const rotated = multiplyQuat(original, [-halfRoot2, 0, 0, halfRoot2]) |
There was a problem hiding this comment.
I might just use euler2quat here for readability.
| if (mesh.position(i)[2] > 0) above++; | ||
| } | ||
|
|
||
| expect(above).toEqual(1); |
There was a problem hiding this comment.
The only trouble with this test is it doesn't check the X and Y axes. We should document which axis we rotate about, and ensure it comes back to the original after an export/import cycle.
There was a problem hiding this comment.
That's fair.
I've called out the rotation and scaling specifically at both import and export.
Now the test uses an object that has no rotational or mirrored symmetry.
| sourceNode.setScale([scale[0] * 1000, scale[1] * 1000, scale[2] * 1000]); | ||
|
|
||
| const original = sourceNode.getRotation(); | ||
| const rotated = multiplyQuat(original, euler2quat([90, 0, 0])); |
There was a problem hiding this comment.
Can you explain why this round-trip works when it looks like the same rotation is applied on import and export, rather than opposites? Your test seems to imply it's all working, but I just want to double check.
There was a problem hiding this comment.
Y'know, I thought it was because multiplying quaternions is not transitive. But flipping the sign and reversing didn't work, so I've got some deeper flaw here.
It should be the inverse of exportTransform just to adhere to the principle of least astonishment.
There was a problem hiding this comment.
It's a quaternion WXYZ vs XYZW thing. Time for more documentation. (Ouch! That hurt me as a developer! 😁 )
But while I'm at it, euler2quat currently works in ZYX order, while Manifold.rotate() works in XYZ order. Should I change this to match? I'll have to mess with the Tetrahedron Puzzle.
There was a problem hiding this comment.
Ouch, yeah euler2quat should definitely match Manifold.rotate. Why will that change tet puzzle? Were we actually doing the rotations wrong compared to our documentation? EDIT: ah, indeed. You know, I think that actually confused me when I was writing that example, but I just messed around until it worked instead of noticing the actual problem.
elalish
left a comment
There was a problem hiding this comment.
Good excuse to catch that bug!
GLTF defines 'up' as +Y, while ManifoldCAD defines it as '+Z'.
This PR rotates models on import so that up is up. It also documents that behaviour.
Part of the Villianous Task List.
(Smaller than usual 😅 )