-
Notifications
You must be signed in to change notification settings - Fork 459
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
1.0 Organization and formatting updates #308
Conversation
Given the new organization we should scrub through the Cesium code base, or anywhere else really, for any |
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 new organization is great!
I'll take a look at the Q&A section a bit later.
README.md
Outdated
|
||
Created by the <a href="http://cesiumjs.org/">Cesium team</a> and built on <a href="https://www.khronos.org/gltf">glTF</a>.<br/> | ||
Bringing techniques from graphics research, the movie industry, and the game industry to 3D geospatial, 3D Tiles define a spatial data structure and a set of tile formats designed for 3D and optimized for streaming and rendering. |
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'm not sure this overview adds much. The first part of the sentence feels like the wrong tone. The second half is good, but could be developed further. Maybe it should be followed by a list of domains 3D Tiles may be used in - BIM/CAD, Phogrammetry, Point Clouds, etc. Or the section could be dropped?
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.
Since this is the new landing page for 3D Tiles, I figured there should be some sort of introductory section. I'l edit the tone and add a sample of domains.
README.md
Outdated
|
||
<a href="http://cesiumjs.org/"><img src="figures/cesium.jpg" height="40" /></a> <a href="https://www.khronos.org/gltf"><img src="figures/gltf.png" height="40" /></a> | ||
Additional tile formats are under development, including Vector Data (`vctr`) [[#124](https://github.com/AnalyticalGraphicsInc/3d-tiles/pull/124)] for geospatial features like points, lines, and polygons. |
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.
Better to link directly to the vector tiles README: https://github.com/AnalyticalGraphicsInc/3d-tiles/tree/3d-tiles-next/TileFormats/VectorData
Q-and-A.md
Outdated
|
||
[glTF](https://www.khronos.org/gltf) is an open standard for 3D models from Khronos (the same group that does WebGL and COLLADA). Cesium uses glTF as its 3D model format, and the Cesium team contributes heavily to the glTF spec and open-source COLLADA2GLTF converter. We recommend using glTF in Cesium for individual assets, e.g., an aircraft, a character, or a 3D building. | ||
|
||
We created 3D Tiles for streaming massive geospatial datasets where a single glTF model would be prohibitive. Given that glTF is optimized for rendering, that Cesium has a well-tested glTF loader, and that there are existing conversion tools for glTF, 3D Tiles use glTF for some tile formats such as [b3dm](TileFormats/Batched3DModel/README.md) (used for 3D buildings). |
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.
b3dm is a broken link
Q-and-A.md
Outdated
|
||
3D Tiles supports heterogeneous data by allowing different tile formats in a tileset, e.g., a tileset may contain tiles for 3D buildings, tiles for instanced 3D trees, and tiles for point clouds, all using different tile formats. | ||
|
||
3D Tiles also supports heterogeneous datasets by concatenating different tile formats into one tile using the [Composite](TileFormats/Composite/README.md) tile format. In the example above, a tile may have a short header followed by the content for the 3D buildings, instanced 3D trees, and point clouds. |
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.
Composite - fix link
Q-and-A.md
Outdated
|
||
#### How are cracks between tiles with vector data handled? | ||
|
||
Unlike 2D, in 3D, we expect adjacent tiles to be from different LODs so, for example, in the distance, lower resolution tiles are used. Adjacent tiles from different LODs can lead to an artifact called _cracking_ where there are gaps between tiles. For terrain, this is generally handled by dropping _skirts_ slightly angled outward around each tile to fill the gap. For 3D buildings, this is handled by extending the tile boundary to fully include buildings on the edge; [see above](#Quadtrees). For vector data, this is an open research problem that we need to solve. This could invole boundary-aware simplification or runtime stitching. |
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.
Fix see above
link.
@@ -2,6 +2,8 @@ | |||
|
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.
See tile formats for a list of tile formats.
- link is broken in this line
specification/Styling/README.md
Outdated
* Gabby Getz, [@ggetz](https://github.com/ggetz) | ||
* Matt Amato, [@matt_amato](https://twitter.com/matt_amato) | ||
* Tom Fili, [@CesiumFili](https://twitter.com/CesiumFili) | ||
* Sean Lilley, [@lilleyse](https://github.com/lilleyse) | ||
* Patrick Cozzi, [@pjcozzi](https://twitter.com/pjcozzi) | ||
|
||
Contents: | ||
## Contents |
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.
Below, the Schema Reference
link should be fixed.
specification/Styling/README.md
Outdated
* Gabby Getz, [@ggetz](https://github.com/ggetz) | ||
* Matt Amato, [@matt_amato](https://twitter.com/matt_amato) | ||
* Tom Fili, [@CesiumFili](https://twitter.com/CesiumFili) | ||
* Sean Lilley, [@lilleyse](https://github.com/lilleyse) | ||
* Patrick Cozzi, [@pjcozzi](https://twitter.com/pjcozzi) | ||
|
||
Contents: | ||
## Contents |
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.
Remove the Acknowledgments entry.
specification/Styling/README.md
Outdated
@@ -63,6 +47,25 @@ Styles are defined with JSON and expressions written in a small subset of JavaSc | |||
|
|||
## Examples | |||
|
|||
_This section is non-normative_ |
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 note is too broad since some of the content in this section is normative including conditions and meta. Maybe the note should be Examples in this section are non-normative
specification/README.md
Outdated
@@ -0,0 +1,749 @@ | |||
# 3D Tiles Format Specification | |||
|
|||
**Version 1.0**, May 11, 2018 |
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.
Add a TODO to change this date later.
Also replaces #312 |
@lilleyse This has been updated. Last thing to look at is the Q&As. |
Updated Q&A. It didn't require too many changes. |
I made some tweaks to the overview in 9c7ae49 |
specification/Styling/README.md
Outdated
* [`expression`](#reference-expression) | ||
* [`number expression`](#reference-number-expression) | ||
* [`Point Cloud Style`](#reference-point-cloud-style) (root 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.
What does (root object)
mean?
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 missed removing that. wetzel adds that to the schema object the command was run on.
##### Extras | ||
|
||
Application-specific data. | ||
### Property Reference |
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.
Organizationally, I think it makes the document easier to read if the property reference section is the last rather than somewhere in the middle. Same for any other READMEs.
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.
Makes sense, I'll make them an "Appendix" and add a link from the Header section.
@lilleyse Updated |
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.
Thanks @ggetz. I went through the property reference sections. Mostly minor comments.
specification/README.md
Outdated
* [`Properties`](#reference-properties) | ||
* [`Tile`](#reference-tile) | ||
* [`Bounding Volume`](#reference-bounding-volume) | ||
* [`Tile Content`](#reference-tile-content) |
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.
These links aren't working.
|
||
* **Type**: `object` | ||
* **Required**: No | ||
* **Type of each property**: Extension |
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 line wasn't included for tileset.extensions
. Which one is the preferred form?
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 other occurrences of this line to watch out for as well.
specification/README.md
Outdated
|
||
##### asset.version :white_check_mark: | ||
|
||
The 3D Tiles version. The version defines the JSON schema for tileset.json and the base set of tile formats. |
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.
It looks like the schema files still reference tileset.json
in some places. Change all occurrences to tileset JSON
.
specification/README.md
Outdated
|
||
#### Bounding Volume | ||
|
||
A bounding volume that encloses a tile or its content. Exactly one property is required. |
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 Exactly one property is required
sentence should be changed to say exactly one of box
, region
, or sphere
is required, since we don't want to exclude extensions
/extras
specification/README.md
Outdated
|
||
This Specification is licensed under a [Creative Commons Attribution 4.0 International License (CC BY 4.0)](http://creativecommons.org/licenses/by/4.0/). | ||
|
||
Some parts of this Specification are purely informative and do not define requirements necessary for compliance and so are outside the Scope of this Specification. These parts of the Specification are marked as being non-normative, or identified as **Implementation Notes**. |
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.
Should this sentence be in a separate section? Maybe a Notes
section?
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 is where it's mentioned in the Khronos documents: https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#appendix-d-full-khronos-copyright-statement
But I would be fine with moving it if there's a good argument for it.
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 distinction might be that the glTF document has it under the copyright section whereas this is under the license section. I don't want it to be implied that this paragraph is part of the license.
Though speaking of, do we need a Copyright section ourselves?
<a name="reference-property"></a> | ||
##### Property | ||
|
||
A user-defined property which specifies per-feature application-specific metadata in a tile. Values either can be defined directly in the JSON as a numeric array, or can refer to sections in the binary body with a [`BinaryBodyReference`](#reference-binarybodyreference) 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.
Another random schema mistake from before - change numeric array
to just array
|---|----|-----------|--------| | ||
|**extensions**|`object`|Dictionary object with extension-specific objects.|No| | ||
|**extras**|`any`|Application-specific data.|No| | ||
|**BATCH_LENGTH**|`object`, `number` `[1]`, `number`|A [`GlobalPropertyScalar`](#reference-globalpropertyscalar) object defining a numeric property for all features. See the corresponding property semantic in [Semantics](/specification/TileFormats/PointCloud/README.md#semantics).| :white_check_mark: Yes| |
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.
Throughout, make sure all of these Semantics
links point to the correct tile format. Here the link points to pnts but should be b3dm.
|**extensions**|`object`|Dictionary object with extension-specific objects.|No| | ||
|**extras**|`any`|Application-specific data.|No| | ||
|
||
Additional properties are not allowed. |
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 feature table schema should allow for additional properties.
|
||
Additional properties are allowed. | ||
|
||
* **JSON schema**: [featureTable.schema.json#/definitions/binaryBodyReference](../../schema/featureTable.schema.json#/definitions/binaryBodyReference) |
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.
Fix link text.
Additional properties are allowed. | ||
|
||
* **JSON schema**: [featureTable.schema.json#/definitions/binaryBodyReference](../../schema/featureTable.schema.json#/definitions/binaryBodyReference) |
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.
Fix link text.
Updated. @lilleyse If you're happy with the updates, I'll go ahead and regenerate the PDF with the changes. |
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 updates to the property reference sections look great.
The changes to the spatial data structure section look good too. I made some small edits in 3e8f839. Does this section need to be sooner in the document though? (a short section early
- #315). Maybe take some of the highlight here and put them in their own section right below Coordinate reference system
, and link back to this main section.
specification/Styling/README.md
Outdated
| |Type|Description|Required| | ||
|---|----|-----------|--------| | ||
|**defines**|`object`|A dictionary object of defined [`expression`](#reference-expression) strings mapped to a variable name that may be referenced throughout the style. If an expression references a defined variable, it is replaced with the result of the corresponding evaluated expression.|No| | ||
|**show**|`undefined[]`, `object`|A [`boolean expression`](#reference-boolean-expression) or [`conditions`](#reference-conditions) property which determines if a feature should be shown.|No, default: `true`| |
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 cases where the type is undefined[]
for some reason.
specification/Styling/README.md
Outdated
|
||
Additional properties are allowed. | ||
|
||
* **Type of each property**: [`metaProperty`](#reference-metaproperty) |
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.
Since this doesn't lead to anything anymore, maybe style.metaProperty.schema.json
should be removed and instead each property be of type expression?
"additionalProperties" : {
"$ref" : "style.expression.schema.json"
}
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.
Same comment for style.definesProperty.schema.json
@lilleyse This is all updated, and the latest PDF is included. Let me know if we need anything else, but I think this is ready to merge if you are good with the updates. |
Adding some clarification to padding based on the feedback in #314 |
All the recent changes look good - take a quick look at 22565e7 - after that I also think this is good to merge. |
@lilleyse Looks good! I also added the example to be in the |
Awesome! Whew... is all I can say. |
Fixes #12
Fixes #153
Fixes #297
Fixes #37
Fixes #315
Fixes #37
TODO:
Defn list Add definition list #306