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

Feature Hierarchy extension #2

Closed
wants to merge 9 commits into from
Closed

Conversation

lilleyse
Copy link

@lilleyse lilleyse commented Mar 5, 2020

Note: this extension is not up-to-date with the latest draft of EXT_feature_metadata

Direct link to extension: EXT_3dtiles_feature_hierarchy

This extension is built on top of EXT_3dtiles_feature_metadata to allow for feature classes and metadata hierarchies.

The current 3D Tiles batch table hierarchy spec is here. It didn't take too much work to move it to glTF, the only difference is that binary properties can now use glTF accessors. Other changes include:

  • The length property of a class is renamed to instanceCount
  • The instances property of a class is renamed to properties to match the regular feature table
  • The instancesLength top-level property is renamed to instanceCount

Notes:

  • I removed the declarative styling section. I think this belongs at a different level of the spec.
  • Should this be its own extension or should it be combined into EXT_3dtiles_feature_metadata?
  • Needs more well defined behavior when using the same property names across different classes. This is what the spec says now:

In some cases multiple ancestors may share the same property name. This can occur if two ancestors are the same class or are different classes with the same property names. For example, if every class defined the property "id", then it would be an overloaded property. In such cases it is up to the implementation to decide which value to return.

Instead should properties could be preceded by their class name (e.g. Building#name or Building.name) to avoid collisions?

@mramato @kring is there anything else that would be nice to have for declarative styling purposes, specifically for the feature hierarchy?

@Samulus
Copy link

Samulus commented Mar 5, 2020

  • Maybe change length to instanceCount ? I think it's clearer (completely subjective).
  • I don't think stacking the buildings on top of each other shows a hierarchy clearly, the relationship wasn't obvious to me. I think if the picture showed buildings in different neighborhoods in an overarching city the hierarchical nature would be more immediately apparent. Illustration doesn't have to be crazy, just circles on the floor around buildings with labels saying Vectorville / XYZTown, and a bigger circle encompassing everything.
  • I would define and use the terms implicit id and explicit id when referring to using classIds.values[i] (the explicit id) and i (the implicit id / instance id) just to make the relationship between the two more obvious.
  • You have an implementation notice explaining the implicit id cannot be used to directly look up the values in the classes[x].instances.whatever.values[x] table. You also mention that it can be inferred by counting. I would go a step further and provide an implementation for doing so at runtime. Unless I'm misunderstanding the spec that would look something like:
// for the car example
var implicitId = 5;
var ext = gltf.extensions.CESIUM_3dtiles_batch_table;
var classInstance = ext.classIds.values[implicitId];
var offset = 0;
for (var i = 0; i < classInstance; ++i) {
  offset += ext.classes[i].batchLength;
}

var sedan = ext.classIds[classInstance].instances.carType.values[implicitId - offset];
var color = ext.classIds[classInstance].instances.carColor.values[implicitId - offset];
  • Nitpicky, but the red arrow that curves and points to 0 in the batchTable with a hierarchy example should be almost touching the 0, like in the example with the cars. It's ambiguous which circle you should start at for traversal otherwise.
  • I think the arrows are kind of misleading in the parent lookup section image. The arrows imply you go from 7 directly to 9, to 9 again in the parentIds array. You actually have to zig zag between both arrays, so the arrows should reflect that. The text is clearer about this.

@sanjeetsuhag
Copy link

We should be consistent with the glTF naming scheme i.e. use ...length for size and ...count for item counts.

@pjcozzi
Copy link

pjcozzi commented Mar 5, 2020

@lilleyse great to see this. Please bump to me when ready.

@lilleyse lilleyse force-pushed the batch-table-hierarchy branch 3 times, most recently from ab137be to 4bd3baa Compare March 10, 2020 18:00
@lilleyse lilleyse changed the title 3D Tiles Batch Table Hierarchy extension 3D Tiles Feature Hierarchy extension Mar 10, 2020
@lilleyse
Copy link
Author

lilleyse commented Mar 10, 2020

Renamed the extension to CESIUM_3dtiles_feature_hierarchy and updated the PR description.

@lilleyse
Copy link
Author

  • instanceCount is used in the class object and in the top level object. Is this ok or should we use a different name?

@lilleyse
Copy link
Author

lilleyse commented Apr 9, 2020

Renamed to EXT_3dtiles_feature_hierarchy and updated the spec to match the latest changes in #1 (comment).

@lilleyse lilleyse changed the title 3D Tiles Feature Hierarchy extension Feature Hierarchy extension Sep 21, 2020
@pjcozzi
Copy link

pjcozzi commented Oct 11, 2020

@lilleyse is this now built into the latest feature metadata extension? Or will this be a separate extension?

@lilleyse
Copy link
Author

@pjcozzi this is separate from the metadata extension. I'm going to close this PR since it's out of sync with the latest EXT_feature_metadata and the design might change a lot.

@lilleyse lilleyse closed this Oct 20, 2020
kring pushed a commit that referenced this pull request Jan 11, 2021
Update to latest Khronos master
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