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

Move Batch Table hierarchy to an extension #305

Merged
merged 6 commits into from
May 7, 2018

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented May 3, 2018

Fixes #303

  • Name - is CESIUM_batch_table_hierarchy the name of the extension we would like to use? Is this a vendor extension? What prefix should we use for core extensions otherwise?
  • Contributors - I copied the list of contributors from the Batch Table, is this list valid here?
  • Should the batch table JSON schema still contain an additionalProprties field? or is that covered by extensions and extras now?

@ggetz ggetz requested a review from lilleyse May 3, 2018 18:14
@lilleyse lilleyse force-pushed the tileset.json-edits branch from 45f11b8 to f0e031b Compare May 7, 2018 15:41
@lilleyse
Copy link
Contributor

lilleyse commented May 7, 2018

Name - is CESIUM_batch_table_hierarchy the name of the extension we would like to use? Is this a vendor extension? What prefix should we use for core extensions otherwise?

Hm good question... maybe core extensions should have the 3DTILES_ prefix? I think this extension qualifies as a core extension, so I'm good with the naming 3DTILES_batch_table_hierarchy.

@lilleyse
Copy link
Contributor

lilleyse commented May 7, 2018

Contributors - I copied the list of contributors from the Batch Table, is this list valid here?

Just include me and Patrick.

@lilleyse
Copy link
Contributor

lilleyse commented May 7, 2018

Should the batch table JSON schema still contain an additionalProprties field? or is that covered by extensions and extras now?

Check what glTF does - but I'm ok either way.

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

Looks great!

@@ -167,342 +158,6 @@ var geographicArray = new Float64Array(batchTableBinary.buffer, byteOffset, geog
var geographicOfFeature = positionArray.subarray(batchId * numberOfComponents, batchId * numberOfComponents + numberOfComponents); // Using subarray creates a view into the array, and not a new array.
```

## Batch Table Hierarchy

The standard batch table is suitable for datasets composed of features with the same sets of properties. However, some datasets have more complex metadata structures such as feature types or feature hierarchies that are not easy to represent as parallel arrays of properties. The Batch Table Hierarchy provides more flexibility for these cases.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worthwhile to keep just this section but point to the extension for more details.


```json
{
"CESIUM_batch_table_hierarchy" : {
Copy link
Contributor

Choose a reason for hiding this comment

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

CESIUM_batch_table_hierarchy should be contained within an extensions objects.


```json
{
"HIERARCHY" : {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use extensions and CESIUM_batch_table_hierarchy objects.


```json
{
"HIERARCHY" : {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment.

"Height" : [...],
"Longitude" : [...],
"Latitude" : [...],
"HIERARCHY" : {...}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment - and change the name in the description above.

@ggetz
Copy link
Contributor Author

ggetz commented May 7, 2018

Thanks @lilleyse ! Updated, and also updated the figures to reflect the changes.

* [Overview](#overview)
* [Motivation](#motivation)
* [Batch table JSON schema updates](#batch-table-json-schema-updates)
* [3DTILES_batch_table_hierarchy](#3DTILES_batch_table_hierarchy)
Copy link
Contributor

Choose a reason for hiding this comment

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

The link isn't working, it should be lower case.


Batch Table Hierarchy, parking lot:

![batch table hierarchy parking lot](figures/batch-table-hierarchy-parking-lot.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh good catch about the pictures!

The json formatting is a little off on this picture. The other picture looks good.

@ggetz
Copy link
Contributor Author

ggetz commented May 7, 2018

Thanks @lilleyse, fixed!

@lilleyse
Copy link
Contributor

lilleyse commented May 7, 2018

👍

@lilleyse lilleyse merged commit c3f7c96 into tileset.json-edits May 7, 2018
@lilleyse lilleyse deleted the hierarchy-extension branch May 7, 2018 20:19
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.

2 participants