-
Notifications
You must be signed in to change notification settings - Fork 7
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
Adds EXT_geopose_basic_euler extension #5
base: 3d-tiles-next
Are you sure you want to change the base?
Conversation
"roll" | ||
] | ||
}, | ||
"extensions": {}, |
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.
longitude
, latitude
, height
, and ypr
should be marked as required
"longitude": 46.7, | ||
"latitude": 25.067, | ||
"height": 691.0, | ||
"ypr": { | ||
"yaw": 0.0, | ||
"pitch": 0.0, | ||
"roll": 0.0 | ||
} |
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.
Is there a document on github that describes the GeoPose JSON encoding? Would be good to link to that.
|
||
## Coordinate Systems | ||
|
||
This extension uses WGS84([EPSG:4979](https://epsg.io/4979)) as the coordinate reference system for specifying the position with longitude and latitude specified in degrees. Height above (or below) the ellipsoid must be specified in meters. The yaw-pitch-roll is provided as a rotation-only transform from a WGS84 referenced local tangent plane East-North-Up coordinate system.. |
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.
More official link to the EPSG:
This extension uses WGS84([EPSG:4979](https://epsg.io/4979)) as the coordinate reference system for specifying the position with longitude and latitude specified in degrees. Height above (or below) the ellipsoid must be specified in meters. The yaw-pitch-roll is provided as a rotation-only transform from a WGS84 referenced local tangent plane East-North-Up coordinate system.. | |
This extension uses WGS84 ([EPSG:4979](https://epsg.org/crs_4979/WGS-84.html)) as the coordinate reference system for specifying the position with longitude and latitude specified in degrees. Height above (or below) the ellipsoid must be specified in meters. The yaw-pitch-roll is provided as a rotation-only transform from a WGS84 referenced local tangent plane East-North-Up coordinate system.. |
@@ -0,0 +1,68 @@ | |||
<!-- omit in toc --> | |||
# EXT_geopose |
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.
The extension intuitively makes sense but there should be a sentence about how this extension interacts with the glTF scene graph and coordinate system.
- The geopose transform would be applied last after the node hierarchy
- +z would be facing east, +x would be facing north, and +y would be facing up. I put together a sandcastle to confirm: Sandcastle
It would be helpful to include a snippet of that CesiumJS code as a reference implementation in an appendix. Also maybe link directly to Transforms.headingPitchRollToFixedFrame
.
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.
Not sure where you are going forward on this topic, but at least OGC has a draft spec for a GeoPose encoding standard out there for review - maybe you can chime in and maybe you can look into aligning theese ideas with the current version of the draft spec. https://github.com/opengeospatial/GeoPose
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.
By the way I am happy to get on a call, or maybe even invite you as a guest to an upcomming Standards Working Group meeting where you can provide input on the current draft from your perspective. You can reach me on linked in or twitter https://twitter.com/Jan_Erik_Vinje https://www.linkedin.com/in/janerikvinje/
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.
@janerivi we are deeply interested in aligning efforts. I apologize for the lack of context in the pull request. @LisaBosCesium will be in touch.
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.
@sanjeetsuhag good start.
How far along is the CesiumJS implementation? That will help inform any corner cases or implementation tips for this spec.
<!-- omit in toc --> | ||
## Dependencies | ||
|
||
Written against the draft of [OGC GeoPose Standard 1.0](https://github.com/opengeospatial/GeoPose/tree/main/standard). |
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.
The intent of the Dependencies is to state, for example, if an extension requires another extension and to state what version of glTF is requires, e.g.
Written against the glTF 2.0 spec.
So OGC GeoPose is not a dependency, but it may be a normative reference. I haven't looked at GeoPose or this extension closely enough yet to know for sure.
|
||
## Overview | ||
|
||
This extension to glTF enables static positioning and orienting of models on the Earth. |
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.
This needs more context and use cases. Probably one to two paragraphs to properly motivate.
|
||
GeoPose 1.0 is an OGC Implementation Standard for exchanging the location and orientation of real or virtual geometric objects (*Poses*) within reference frames anchored to the earth’s surface (*Geo*) or within other astronomical coordinate systems. | ||
|
||
This extension implements [Standardization Target 2: Basic-Euler](https://github.com/opengeospatial/GeoPose/blob/main/standard/standard/standard/clause_7_normative_text.adoc#standardization-target-2-basic-euler) in the OGC GeoPose 1.0 Standard. |
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.
Perhaps this means that EXT_geopose
is too broad of an extension name and that we should use something more narrow. This is something to discuss with the authors of the OGC GeoPose spec - because there could be interest in bringing more of the GeoPose spec into a glTF perhaps via a collection of extensions.
|
||
## Coordinate Systems | ||
|
||
This extension uses WGS84([EPSG:4979](https://epsg.io/4979)) as the coordinate reference system for specifying the position with longitude and latitude specified in degrees. Height above (or below) the ellipsoid must be specified in meters. The yaw-pitch-roll is provided as a rotation-only transform from a WGS84 referenced local tangent plane East-North-Up coordinate system.. |
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.
degrees
Is this consistent with the rest of the glTF ecosystem?
At glance, I see degrees in this vendor extension: https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Vendor/AGI_articulations/README.md#stages
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.
Ah, I probably wrote this years ago. 😄
All angles are in radians.
See https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#coordinate-system-and-units
Ditto for 3D Tiles.
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.
There are a couple benefits to degrees:
- Matches the EPSG:4979 definition
- Human readable and editable, while not necessarily the primary goal of glTF, could be beneficial for an extension like this
I also think the case could be made that even though coordinates are angles, they are a distinct category that could have special treatment.
"latitude": 25.067, | ||
"height": 691.0, | ||
"ypr": { | ||
"yaw": 0.0, |
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.
Diagram would be helpful for yaw pitch roll mapping to east north up.
The spec should also discuss the interaction with positioning a glTF model where the front is +z.
glTF uses a right-handed coordinate system, that is, the cross product of +X and +Y yields +Z. glTF defines +Y as up. The front of a glTF asset faces +Z.
See https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#coordinate-system-and-units
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.
Ah, I see this is basically a duplicate comment with @lilleyse's comment above.
} | ||
``` | ||
|
||
## Schema Updates |
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 I missed a TODO list for the spec, but it should have a property reference generated from the JSON schema.
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.
Also add a Known Implementations section; for example, see https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_texture_basisu#known-implementations
{ | ||
"extensions": { | ||
"EXT_geopose": { | ||
"longitude": 46.7, |
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.
longitude and latitude bounds should be defined, e.g., [-180, 180]
and [-90, 90]
(degrees)
}, | ||
"ypr": { | ||
"type": "object", | ||
"description": "Rotation-only transformation from a WGS-84-referenced local tangent plane east-north-up coordinate system.", |
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.
Perhaps useful to include an appendix in the extension spec on how to compute the transformation matrix.
https://github.com/CesiumGS/cesium/blob/master/Source/Core/Transforms.js#L275
"type": "number", | ||
"description": "Heights are in meters above (or below) the WGS84 ellipsoid." | ||
}, | ||
"ypr": { |
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.
glTF does not use abbreviations in property names...maybe "ypr" is common enough but this might need to be renamed. Also, perhaps the ypr
indirection isn't needed at all, and that yaw
, pitch
, and roll
could be in the top-level object.
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 would also prefer a slightly different schema but this intentionally matches the JSON encoding defined in the GeoPose spec.
https://github.com/opengeospatial/GeoPose/blob/main/standard/pdf/geopose_standard.pdf - page 42
Also on page 10:
GeoPose 1.0 JSON encodings are specified via JSON-Schema:2019-9 and most of the requirements are that conforming encoded data objects shall validate against the corresponding schema
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.
@lilleyse there is a discussion point to be add with the glTF community: if this extension should try to strictly follow GeoPose norms or glTF norms. From a glTF ecosystem perspective, consistency throughout that ecosystem may be preferred, but it is to be discussed. There may also be lessons learned from bringing the XMP spec to glTF.
@sanjeetsuhag could you please start a TODO checklist at the top of this GitHub issue.
@@ -0,0 +1,8 @@ | |||
# EXT_geopose |
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.
Are we sure this changelog is necessary? It is not customary.
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.
We have been following this convention (ex: 3DTILES_implicit_tiling, 3DTILES_metadata, EXT_feature_metadata) to provide direct references and changelogs for specifications that may depend on these extensions.
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.
@sanjeetsuhag for this extension it's fine to not have a changelog. We should consider removing the landing pages for those as well.
By the way the "default" Basic GeoPose intends to use quaternions (not angles) defined on the local tangent plane of the geolocataion using East North Up that might be more convenient for a graphics engine. The correspondance of axis of the kartesian coordinate system at the tangent plane is East North Up -> X Y Z. Schema can be found on page 40 (as of todays writing) https://github.com/opengeospatial/GeoPose/blob/main/standard/pdf/geopose_standard.pdf |
b3f48c2
to
3bf5af9
Compare
Synced this branch with |
2996cb7
to
f34bd1a
Compare
This extension to implements Target 2: Basic Euler, with the Yaw-Pitch-Roll of the OGC GeoPose 1.0 Standard (draft).
TODO
EXT_geopose_basic_euler