3D Tiles Transform Tests #4256
3D Tiles Transform Tests #4256
Conversation
| // Transform from y-up to z-up | ||
| Matrix3.multiply(yUpToZUpMatrix3, halfAxes, halfAxes); | ||
| Matrix4.multiplyByPoint(yUpToZUpMatrix4, center, center); | ||
| } |
There was a problem hiding this comment.
The spec now defines local bounding volumes as y-up. So I do the y-up to z-up conversion at runtime. I was also wondering about transforms in local space...
Say you have a root and a child. The root has a transform that goes from local to wgs84 and the child has a transform that translates it upward by 10 meters. Intuitively the child's transform should be a translation of (0.0, 10.0, 0.0) to match the y-up convention of local-space. However the translation needed by Cesium is (0.0, 0.0, 10.0). So the transforms are z-up and bounding volumes are y-up.
The main issue is how do we make the distinction that a transform/bounding volume is in local space vs wgs84 space? Should all transforms be y-up including the wgs84 transform? Or should we instead define z-up as the convention?
There was a problem hiding this comment.
To be consistent with glTF, y should be up.
To be consistent with WGS84 (and local east-north-up), z should be up.
If I understand you correctly, to be consistent with local vs WGS84 coordinates, z should be up.
It would be nice to be consistent with glTF, but it sounds like we have a compelling reason to go with z up. Do you agree?
There was a problem hiding this comment.
Yeah you summed it up well. I agree that z-up is better for this.
There was a problem hiding this comment.
Let's do it. Please update the spec as well - including the main spec as well as b3dm and i3dm specs where glTF is explicitly mentioned.
|
Any typed array memory concerns with |
|
Other than these comments, code and tests look good. Did you run coverage? |
Yeah that's correct.
Yeah coverage is mostly solid. I'm curious what you think about the inline comments. |
lilleyse
force-pushed
the
transform-tests
branch
from
fc2625c to
ecef596
Compare
|
Updated. I squashed some commits because of the change to z-up and |
| @@ -45,6 +46,7 @@ define([ | |||
| getExtensionFromUri, | |||
| Intersect, | |||
| joinUrls, | |||
| CesiumMath, | |||
There was a problem hiding this comment.
I don't think this is used.
|
Code looks OK.
What comments? |
The ones you already looked at before. |
|
Thanks @lasalvavida for the updated tiles. @pjcozzi this is ready to merge now. |
|
Tests and Sandcastle example are good! |
Tests for #4130
Sorry about the branch confusion. This is merging into
pnts-updatesinstead of3d-tiles-transformmainly because of all the file renaming that happened in #4228.I reworked
ModelInstanceCollectionextensively to simplify it and fix some bugs related to its interaction with shadows and derived commands. The functionality is the same though.