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

First Round of 3D Tiles 1.0 updates #301

Merged
merged 45 commits into from
May 9, 2018
Merged

First Round of 3D Tiles 1.0 updates #301

merged 45 commits into from
May 9, 2018

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Apr 17, 2018

First round of edits for 3D Tiles 1.0, tried to minimize organizational and formatting changes to be tackled later.

Fixes #298
Fixes #295
Fixes #280
Fixes #272
Fixes #263
Fixes #225
Fixes #211
Fixes #184
Fixes #140
Fixes #94
Fixes #60
Fixes #8

@ggetz ggetz requested a review from lilleyse April 17, 2018 23:13
@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 17, 2018

Please target a new 1.0 branch.

@ggetz ggetz changed the base branch from master to 1.0 April 18, 2018 13:32
@lilleyse
Copy link
Contributor

For #8, add extras and extensions to boundingVolume.schema.json, batchTable.schema.json/hierarchy/items, and featureTable.schema (even though b3dm, i3dm, and pnts derive from this and include these properties themselves, it would be good to be explicit in the feature table schema).

Can you also open a PR making any spec changes into the vector tiles branch?

@@ -121,7 +121,9 @@ Per-point normals are an optional property that can help improve the visual qual
The normals will be transformed using the inverse transpose of the tileset transform.

#### Oct-encoded Normal Vectors
Oct-encoding is described in [*A Survey of Efficient Representations of Independent Unit Vectors* by Cigolle et al.](http://jcgt.org/published/0003/02/01/). An implementation for encoding and decoding these unit vectors can be found in Cesium's
Oct-encoding is described in [*A Survey of Efficient Representations of Independent Unit Vectors* by Cigolle et al.](http://jcgt.org/published/0003/02/01/). Oct-encoded values are stored in unsigned, unnormalized range ([0, 65535] or [0, 255]) and then converted to a signed normalized range ([-1.0, 1.0]) at runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

A few tweaks for this line

  • pnts only uses oct16 encoding so only the [0, 255] range needs to be mentioned
  • Minor wording

Oct-encoded values are stored in unsigned, unnormalized range [0, 255] and then mapped to a signed normalized range [-1.0, 1.0] at runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

A similar explanation needs to be added i3dm in the Oct-encoded Normal Vectors section but using the range [0, 65535].

@@ -64,18 +64,19 @@ These semantics define global properties for all features.
| Semantic | Data Type | Description | Required |
| --- | --- | --- | --- |
| `BATCH_LENGTH` | `uint32` | The number of distinguishable models, also called features, in the batch. If the Binary glTF does not have a `batchId` attribute, this field _must_ be `0`. | :white_check_mark: Yes. |
| `RTC_CENTER` | `float32[3]` | A 3-component array of numbers defining the center position when instance positions are defined relative-to-center. | :red_circle: No. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop the word instance since it is specific to i3dm.


`.i3dm`
Instanced 3D modela tiles use the `.i3dm` extension and `application/octet-stream` MIME type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instanced 3D modela -> Instanced 3D Model

"type": "object",
"description": "Dictionary object with extension-specific objects.",
"properties": {
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to "properties": {},

README.md Outdated
`b3dm` and `i3dm` tiles embed glTF. According to the [glTF spec](https://github.com/KhronosGroup/glTF/tree/master/specification/1.0#coordinate-system-and-units) glTF uses a right-handed coordinate system and defines the y axis as up. By default embedded models are considered to be y-up, but in order to support a variety of source data, including models defined directly in WGS84 coordinates, embedded glTF models may be defined as x-up, y-up, or z-up with the `asset.gltfUpAxis` property of `tileset.json`. In general an implementation should transform glTF assets to z-up at runtime to be consistent with the z-up coordinate system of the bounding volume hierarchy.
#### Tile content

`b3dm` and `i3dm` tiles embed glTF. According to the [glTF spec](https://github.com/KhronosGroup/glTF/tree/master/specification/1.0#coordinate-system-and-units) glTF uses a right-handed coordinate system and defines the y axis as up. By default embedded models are considered to be y-up, but in order to support a variety of source data, including models defined directly in WGS84 coordinates, embedded glTF models may be defined as x-up, y-up, or z-up with the `asset.gltfUpAxis` property of the tileset JSON.
Copy link
Contributor

Choose a reason for hiding this comment

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

## MIME Type

_TBA, [#60](https://github.com/AnalyticalGraphicsInc/3d-tiles/issues/60)_
> Implementation Note: Point cloud styling engines may often use a shader (GLSL) implementation, however some features of the expression language are not possible in pure a GLSL implementation. Features that must account for these inconsistencies include evaluation of `isNan` and `isFinite` (GLSL 2.0+ supports `isnan` and `isinf` for these functions respectively); the types `null` and `undefined`; strings, including accessing object properties (`color()['r']`) and batch table values; regular expressions; arrays of lengths other than 2, 3, or 4; mismatched type comparisons (`float` to `bool`); and handling of "index out of bounds" errors.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to have a bulleted list within a quoted section? That might be easier to read.

`application/json`
## File Extensions and MIME Types

* Tileset styles use the `.json` extension and the `application/json` mime type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note as before where the extension is not required.

@@ -119,16 +120,20 @@ The vertex shader can be modified at runtime to use `a_batchId` to access indivi

When a Batch Table is present or the `BATCH_LENGTH` property is greater than `0`, the `batchId` attribute (with the parameter semantic `_BATCHID`) is required; otherwise, it is not.
Copy link
Contributor

Choose a reason for hiding this comment

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

The gltf snipped above is still based on 1.0 techniques/shaders. For 2.0 all that's required is a primitive contains a _BATCHID attribute.


The file extension is optional. Valid implementations ignore it and identify a content's format by the `magic` field in its header.
> Implementation Note: Clients may also use the glTF [CESIUM_RTC](https://github.com/KhronosGroup/glTF/tree/master/extensions/1.0/Vendor/CESIUM_RTC) extension and define points relative to center.
Copy link
Contributor

Choose a reason for hiding this comment

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

We are phasing away from this extension, so I wouldn't include this note.


`.b3dm`
Vertex positions may be defined relative-to-center for high-precision rendering [1]. If defined, `RTC_CENTER` specifies the center position and all vertex positions are treated as relative to this value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it should be noted that RTC_CENTER is z-up whereas vertex positions are y-up.

In general I think our coordinate system messaging is still not quite there... still need to think about the clearest way possible to describe it.

@lilleyse
Copy link
Contributor

lilleyse commented Apr 24, 2018

Is #225 (padding) in here? moved to #301 (comment)

@lilleyse
Copy link
Contributor

I made some edits as I went along in 192a632. More to come later.

@lilleyse
Copy link
Contributor

#295 and #280 will need a bit of work before they are done, maybe we can discuss offline
#184 - might be good to explicitly mention that the top level json is not required to have a specific name
#225 - doesn't seem to be included here. It would be good if each tile format has a easy to find link to the padding requirements

I think the rest of the issues look good or are covered by comments.

@ggetz
Copy link
Contributor Author

ggetz commented May 9, 2018

Added glTF CESIUM_z_up extension: KhronosGroup/glTF#1338

@ggetz
Copy link
Contributor Author

ggetz commented May 9, 2018

Updated figures.

@ggetz ggetz force-pushed the tileset.json-edits branch from 5268eb6 to 384e749 Compare May 9, 2018 19:13
@lilleyse
Copy link
Contributor

lilleyse commented May 9, 2018

Awesome work! Ready to merge to 1.0.

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