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

Adds EXT_geopose_basic_euler extension #5

Draft
wants to merge 12 commits into
base: 3d-tiles-next
Choose a base branch
from
Draft

Conversation

sanjeetsuhag
Copy link

@sanjeetsuhag sanjeetsuhag commented May 5, 2021

This extension to implements Target 2: Basic Euler, with the Yaw-Pitch-Roll of the OGC GeoPose 1.0 Standard (draft).

TODO

  • Rename to EXT_geopose_basic_euler
  • Add use cases to overview
  • Add diagram with a JSON example
  • Fix schema
  • Add snippet of that CesiumJS code as a reference implementation in an appendix
  • Add how this extension interacts with the glTF scene graph and coordinate system
  • Add property reference from schema
  • Define lat/lon bounds in schema
  • Add Known Implementations section

"roll"
]
},
"extensions": {},
Copy link

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

Comment on lines 53 to 60
"longitude": 46.7,
"latitude": 25.067,
"height": 691.0,
"ypr": {
"yaw": 0.0,
"pitch": 0.0,
"roll": 0.0
}
Copy link

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..
Copy link

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:

Suggested change
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..

extensions/2.0/Vendor/EXT_geopose/0.0.0/README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,68 @@
<!-- omit in toc -->
# EXT_geopose
Copy link

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

image

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.

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

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/

Copy link

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.

Copy link

@pjcozzi pjcozzi left a 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).
Copy link

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.
Copy link

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.
Copy link

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..
Copy link

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

Copy link

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.

Copy link

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,
Copy link

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

Copy link

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
Copy link

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.

Copy link

Choose a reason for hiding this comment

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

{
"extensions": {
"EXT_geopose": {
"longitude": 46.7,
Copy link

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.",
Copy link

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": {
Copy link

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.

Copy link

@lilleyse lilleyse May 7, 2021

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

image

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

Copy link

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
Copy link

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.

Copy link
Author

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.

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.

@sanjeetsuhag sanjeetsuhag changed the title Adds EXT_geopose extension Adds EXT_geopose_basic_euler extension May 11, 2021
@janerivi
Copy link

janerivi commented Jun 9, 2021

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
image

@lilleyse
Copy link

Synced this branch with 3d-tiles-next and removed the landing page.

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.

4 participants