-
Notifications
You must be signed in to change notification settings - Fork 470
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
Copyedit #302
Copyedit #302
Conversation
schema/tile.schema.json
Outdated
@@ -25,7 +25,7 @@ | |||
}, | |||
"transform" : { | |||
"type" : "array", | |||
"description" : "A floating-point 4x4 affine transformation matrix, stored in column-major order, that transforms the tile's content, i.e., its features and content.boundingVolume, and boundingVolume and viewerRequestVolume from the tile's local coordinate system to the parent tile's coordinate system, or tileset's coordinate system in the case of the root tile. transform does not apply to geometricError nor does it apply any volume property when the volume is a region, which is defined in WGS84 / EPSG:4326 coordinates.", | |||
"description" : "A floating-point 4x4 affine transformation matrix, stored in column-major order, that transforms the tile's content--i.e., its features as well as content.boundingVolume, boundingVolume, and viewerRequestVolume--from the tile's local coordinate system to the parent tile's coordinate system, or, in the case of a root tile, from the tile's local coordinate system to the tileset's coordinate system. transform does not apply to geometricError, nor does it apply any volume property when the volume is a region that is defined in WGS84 / EPSG:4326 coordinates.", |
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.
Please check these edits to be sure they're right.
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.
nor does it apply any volume property when the volume is a region that is defined in WGS84 / EPSG:4326 coordinates.
All regions are defined in WGS84 / EPSG:4326 coordinates, not just exceptional ones.
Styling/README.md
Outdated
* Number expressions | ||
* Vector expressions of the same type | ||
* Binary equality operators `===` and `!==` operate on any expressions. The operation returns `false` if the expression types do not match. | ||
* Binary regexp operators `=~` and `!~` requires one argument to be a string expression and the other to be a RegExp expression. | ||
* Binary `regexp` operators `=~` and `!~` require one argument to be a string expression and the other to be a `RegExp` expression. |
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, we use regexp
and RegExp
. Which is preferred?
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.
@@ -1299,7 +1290,7 @@ For example, the following style will color all doors, but not features that are | |||
isClass(name : String) : Boolean | |||
``` | |||
|
|||
Returns `true` is the feature's class, or any of its ancestors' classes, are equal to `name`. | |||
Returns `true` if the feature's class, or any of its ancestors' classes, are equal to `name`. |
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 "ancestors" be "parents"?
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.
"Ancestors" is right since a feature inherits properties of not just its parents class, but also its parent's parent's class, etc.
|
||
The `parentCounts` and `parentIds` arrays form an instance hierarchy. A feature's properties includes those defined by its own class and any properties from ancestor instances. | ||
The `parentCounts` and `parentIds` arrays form an instance hierarchy. A feature's properties include those defined by its own class and any properties from ancestor instances. |
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 "ancestor" be "parents"? ... see also next line
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 as above, "ancestors" is right since a feature inherits properties of not just its parents class, but also its parent's parent's class, etc.
@@ -66,10 +70,10 @@ var positionArray = new Float32Array(featureTableBinary.buffer, byteOffset, feat | |||
var position = positionArray.subarray(featureId * 3, featureId * 3 + 3); // Using subarray creates a view into the array, and not a new array. |
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.
In the README preview here on GitHub, these comments run off the page.
@@ -204,7 +195,7 @@ A meta property expression can evaluate to any type. For example: | |||
} |
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.
"can evaluate to any type." Is there a word missing after "type"?
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 valid. If we wanted to be more specific we could say "data type", but I don't think that's necessary.
README.md
Outdated
* Has several properties that match the external tileset's root tile: | ||
* `root.geometricError === tile.geometricError`, | ||
* `root.refine === tile.refine`, and | ||
* `root.boundingVolume === tile.content.boundingVolume` (or `root.boundingVolume === tile.boundingVolume` when `tile.content.boundingVolume` is `undefined`). | ||
* `root.viewerRequestVolume === tile.viewerRequestVolume` or `root.viewerRequestVolume` is `undefined`. | ||
* Cannot be used to create cycles, for example, by pointing to the same tileset.json containing the tile or by pointing to another tileset.json that then points back to the tileset.json containing the tile. | ||
* Both the tile's `transform` and root tile's `transform` are applied. For example, in the following tileset referencing an external tileset, the computed transform for `T3` is `[T0][T1][T2][T3]`. | ||
* Will by transformed by both the tile's `transform` and root tile's `transform`. For example, in the following tileset referencing an external tileset, the computed transform for `T3` is `[T0][T1][T2][T3]`. |
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.
"Will be" ?
|
||
All angles are in radians. | ||
|
||
3D Tiles do not explicitly store Cartographic coordinates (longitude, latitude, and height); these values are implicit in WGS84 Cartesian coordinates, which are efficient for the GPU to render since they do not require a non-affine coordinate transformation. A 3D Tiles tileset can include application-specific metadata, such as Cartographic coordinates, but the semantics are not part of the 3D Tiles specification. | ||
3D Tiles does not explicitly store cartographic coordinates (longitude, latitude, and height); these values are implicit in WGS84 Cartesian coordinates, which are efficient for the GPU to render since they do not require a non-affine coordinate transformation. A 3D Tiles tileset can include application-specific metadata, such as cartographic coordinates, but the semantics are not part of the 3D Tiles specification. |
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 we add to the style guide that "3D Tiles" should be treated as singular?
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.
Yes, I have a couple of things to add since going through this. Thanks for the reminder!
@@ -3,5 +3,5 @@ | |||
"id" : "style.colorExpression.schema.json", | |||
"title" : "color expression", | |||
"type" : "string", | |||
"description" : "3D Tiles style expression that evaluates to a Color." | |||
"description" : "3D Tiles style expression that evaluates to a color." |
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 a type, Color
, so it should still be capitalized I think.
@ggetz Thanks for your comments! I made the changes. Please double-check my |
Thanks @slchow! I should have been more specific - |
Thanks @ggetz! |
@slchow Can you merge in master? |
README.md
Outdated
@@ -1,6 +1,6 @@ | |||
<p align="center"><img src="figures/Cesium3DTiles.png" /></p> | |||
|
|||
Specification for streaming massive heterogeneous **3D** geospatial datasets. | |||
Specification for streaming massive heterogeneous 3D geospatial datasets. |
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 believe the bold style was intentionally added for emphasis.
README.md
Outdated
[Batched 3D model](TileFormats/Batched3DModel/README.md) (*.b3dm)<br /><br />Textured terrain and surfaces, 3D building exteriors and interiors, massive models, ... | :white_check_mark: **Solid base**, only minor, if any, changes expected | ||
[Instanced 3D model](TileFormats/Instanced3DModel/README.md) (*.i3dm)<br /><br />Trees, windmills, bolts, ... | :white_check_mark: **Solid base**, only minor, if any, changes expected | ||
[Point cloud](TileFormats/PointCloud/README.md) (*.pnts)<br /><br />Massive number of points | :white_check_mark: **Solid base**, only minor, if any, changes expected | ||
[Vector data](TileFormats/VectorData/README.md) (*.vctr)<br /><br />Polygons, polylines, and placemarks | :white_circle: **In progress**, [#124](https://github.com/AnalyticalGraphicsInc/3d-tiles/pull/124/files) |
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 these are the tile format / "special" names, I think the capitalization changes should be undone throughout:
Batched 3D model
-> Batched 3D Model
Instanced 3D model
-> Instanced 3D Model
Point cloud
-> Point Cloud
Vector data
-> Vector Data
composite
-> Composite
Batch table
-> Batch Table
Batch table hierarchy
-> Batch Table Hierarchy
Feature table
-> Feature Table
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 I went back and forth on this. Are these proper names just for the tile format we've developed? e.g., would you say, "The Point Cloud tile format is for tiling 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.
"The Point Cloud tile format is for tiling point clouds"
Yeah that is the way I would write that sentence. In the case of Point Cloud and Vector Data it just happens that the tile format and the type of data have the same name, but the others are unique proper names.
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.
OK, thanks. I'll go back to capitalized, then.
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 why this was ambiguous now, it seemed like it was split half and half between uppercase and lowercase.
README.md
Outdated
@@ -412,7 +411,7 @@ The top-level object in tileset.json has four properties: `asset`, `properties`, | |||
|
|||
`asset` is an object containing properties with metadata about the entire tileset. Its `version` property is a string that defines the 3D Tiles version. The version defines the JSON schema for tileset.json and the base set of tile formats. The `tilesetVersion` property is an optional string that defines an application-specific version of a tileset, e.g., for when an existing tileset is updated. The `gltfUpAxis` property is an optional string that specifies the up-axis of glTF models contained in the tileset. | |||
|
|||
`properties` is an object containing objects for each per-feature property in the tileset. This tileset.json snippet is for 3D buildings, so each tile has building models, and each building model has a `Height` property (see [Batch Table](TileFormats/BatchTable/README.md)). The name of each object in `properties` matches the name of a per-feature property, and defines its `minimum` and `maximum` numeric values, which are useful, for example, for creating color ramps for styling. | |||
`properties` is an object containing objects for each per-feature property in the tileset. This tileset.json snippet is for 3D buildings, so each tile has building models, and each building model has a `height` property (see [Batch Table](TileFormats/BatchTable/README.md)). The name of each object in `properties` matches the name of a per-feature property, and the name defines its `minimum` and `maximum` numeric values, which are useful, for example, for creating color ramps for styling. |
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.
height
should stay as Height
because the property name in the tileset.json snippet is upper case.
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.
Whoops! Good catch.
README.md
Outdated
@@ -412,7 +411,7 @@ The top-level object in tileset.json has four properties: `asset`, `properties`, | |||
|
|||
`asset` is an object containing properties with metadata about the entire tileset. Its `version` property is a string that defines the 3D Tiles version. The version defines the JSON schema for tileset.json and the base set of tile formats. The `tilesetVersion` property is an optional string that defines an application-specific version of a tileset, e.g., for when an existing tileset is updated. The `gltfUpAxis` property is an optional string that specifies the up-axis of glTF models contained in the tileset. | |||
|
|||
`properties` is an object containing objects for each per-feature property in the tileset. This tileset.json snippet is for 3D buildings, so each tile has building models, and each building model has a `Height` property (see [Batch Table](TileFormats/BatchTable/README.md)). The name of each object in `properties` matches the name of a per-feature property, and defines its `minimum` and `maximum` numeric values, which are useful, for example, for creating color ramps for styling. | |||
`properties` is an object containing objects for each per-feature property in the tileset. This tileset.json snippet is for 3D buildings, so each tile has building models, and each building model has a `height` property (see [Batch Table](TileFormats/BatchTable/README.md)). The name of each object in `properties` matches the name of a per-feature property, and the name defines its `minimum` and `maximum` numeric values, which are useful, for example, for creating color ramps for styling. |
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 name of each object in
properties
matches the name of a per-feature property, and the name defines itsminimum
andmaximum
numeric values, which are useful, for example, for creating color ramps for styling.
The wording here needs to be tweaked since it is not the name that defines the minimum
/maximum
but rather the object's value. Maybe something like below?
The name of each object in
properties
matches the name of a per-feature property, and its value defines itsminimum
andmaximum
numeric values, which are useful, for example, for creating color ramps for styling.
README.md
Outdated
|
||
Each tile's `content.url` property points to a tile that is one of the formats listed in the [Status section](#spec-status) above. | ||
|
||
A tileset can contain any combination of tile formats. 3D Tiles may also support different formats in the same tile using a [Composite](TileFormats/Composite/README.md) tile. | ||
|
||
## Declarative styling | ||
## Declarative Styling |
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 be Declarative styling
?
README.md
Outdated
|
||
At runtime, a tile's `geometricError` is used to compute the Screen-Space Error (SSE) to drive refinement. We expect to expand this, for example, by using the [_Virtual Multiresolution Screen Space Error_](http://www.dis.unal.edu.co/profesores/pierre/MyHome/publications/papers/vmsse.pdf) (VMSSE), which takes occlusion into account. This can be done at runtime without streaming additional tile metadata. Similarly, fog can also be used to tolerate increases to the SSE in the distance. | ||
At runtime, a tile's `geometricError` is used to compute the screen space Error (SSE) to drive refinement. We expect to expand this, for example, by using the [_virtual multiresolution screen space error_](http://www.dis.unal.edu.co/profesores/pierre/MyHome/publications/papers/vmsse.pdf) (VMSSE), which takes occlusion into account. This can be done at runtime without streaming additional tile metadata. Similarly, fog can also be used to tolerate increases to the SSE in the distance. |
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.
screen space Error
-> screen space error
@@ -71,42 +71,37 @@ The following style assigns the default show and color properties to each featur | |||
} | |||
``` | |||
|
|||
Instead of showing all features, `show` can be an expression dependent on a feature's properties, for example: | |||
Instead of showing all features, `show` can be an expression dependent on a feature's properties, for example, the following expression will show only features in the 19341 zip code: |
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 nice, I like these edits. Makes the examples much easier to read.
Styling/README.md
Outdated
|
||
In addition to a string containing an expression, `color` and `show` can be an array defining a series of conditions (think of them as `if...else` statements). Conditions can, for example, be used to make color maps and color ramps with any type of inclusive/exclusive intervals. | ||
In addition to a string containing an expression, `color` and `show` can be an array defining a series of conditions (think of them as `if...else` statements). Conditions can, for example, be used to make color maps and color ramps with any type of inclusive/exclusive intervals. |
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 trailing space.
README.md
Outdated
|
||
## Tile formats | ||
## 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.
Should this be Tile formats
?
Everything looks good - I just made a few edits where I saw them, mainly related to Batch Table Hierarchy. |
Thank you @lilleyse! |
Copyedit of spec as-is. Will copyedit remaining changes as they come in.