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

Remove CESIUM_z_up extension and add a detailed implementation note #307

Merged
merged 5 commits into from
May 11, 2018

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented May 10, 2018

Specifically check out Coordinate Reference System and contained glTF section.

Thoughts @ggetz @pjcozzi @emackey?

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I prefer the extension to having to specify these implementation notes, as one of the most common use cases, geospatial data which in inherently z-up, has to go through several additional steps. But I'm willing to here other opinions.

README.md Outdated

If the `CESIUM_z_up` glTF extension is not used, the glTF model is considered to by _y_-up and must be transformed to a _z_-up coordinate system at runtime. This is done by rotating the model about the _x_-axis by π/2 radians. Equivalently, apply the following matrix transform:
The [region](#region) bounding volume type is defined in [EPSG 4326](http://nsidc.org/data/atlas/epsg_4326.html) coordinates.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important to state geographic coordinate system (latitude, longitude, height) because everything else is in a right handed (x,y,z) coordinate system.

README.md Outdated
0.0, 1.0, 0.0, 0.0,
0.0, 0.0, 0.0, 1.0
]
[1,0,0,0,0,0,1,0,0,-1,0,0,0,0,0,1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This formatting makes the matrix hard to see at a glance, and is inconsistant with how we format them elsewhere in the spec.

Both here and throughout the remaining changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think I prefer a row-major, line separated matrix too.

The reason I changed it here was to be in the same form as the matrix in the gltf snippet so it was obvious how the two compare. But I'll change it back.

README.md Outdated
> * Mesh data, including positions and normals, is not modified - it can remain _z_-up.
> * The root node matrix specifies a _z_-up to _y_-up transform. This transforms the source data into a _y_-up coordinate system as required by glTF. For example:
>
>```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specify the language for syntax highlighting like json

@emackey
Copy link

emackey commented May 10, 2018

I think letting go of the extension is the right move for both the glTF and the 3D Tiles ecosystems.

Such an extension can't be vendor-specific to Cesium if we expect other vendors to implement 3D Tiles as an open standard and interoperate with Cesium and ion-generated tiles. For example, desktop applications that want to render ion-served tiles will need a desktop implementation of the single-vendor extension.

The idea was poorly received by several participants in the main glTF repo, as it runs counter to the spirit there: glTF asks everyone to agree to use the same conventions (axes selection, texture channel selection, etc) specifically to avoid the pain felt in older formats that allowed exporters to write whatever was convenient for the exporter, introducing too much variance into the resulting library of assets for any one reader to fully cope with.

Also, the extension was marked optional. Sorry, my mistake, it's marked required. But by adding a fallback, it could be converted to optional. Authors of optional extensions are asked to include fallbacks for core readers to do the best they can do with the model. The fallback here is incredibly trivial: Just add the above matrix to the root node, at which point the whole extension becomes moot.

I think letting go of this extension is well worth the effort, particularly to maintain the message that 3D Tiles uses core glTF without any vendor extensions, and the ecosystem is stable.

@lilleyse lilleyse force-pushed the crs-transforms branch 4 times, most recently from 34a0df4 to 90b9602 Compare May 10, 2018 16:01
@lilleyse lilleyse force-pushed the crs-transforms branch 2 times, most recently from 602496f to 25f3b89 Compare May 10, 2018 16:07
@lilleyse
Copy link
Contributor Author

@ggetz updated based on your comments.

@lilleyse lilleyse force-pushed the crs-transforms branch 3 times, most recently from 3c034cd to c7c8614 Compare May 10, 2018 17:47
@ggetz
Copy link
Contributor

ggetz commented May 11, 2018

OK I'm good with this solution. Should we go ahead and close the z-up extension?

@lilleyse I made some tweaks to formatting and wording just for readability, feel free to switch it back don't like any of those changes.

@lilleyse
Copy link
Contributor Author

The formatting tweaks are good.

Yeah let's close the z-up extension.

@ggetz
Copy link
Contributor

ggetz commented May 11, 2018

OK, thanks @lilleyse !

@ggetz ggetz merged commit 12729dd into 1.0 May 11, 2018
@ggetz ggetz deleted the crs-transforms branch May 11, 2018 15:07
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.

3 participants