Move batch table hierarchy to extension #6780
Conversation
|
Thanks for the pull request @ggetz!
Reviewers, don't forget to make sure that:
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
| } | ||
| this._extensions = defaultValue(extensions, {}); | ||
| this._extras = defined(batchTableJson) ? batchTableJson.extras : undefined; |
There was a problem hiding this comment.
extras can removed since it isn't used
| * @private | ||
| */ | ||
| this.batchTableJson = batchTableJson; | ||
|
|
||
| /** | ||
| * @private | ||
| */ | ||
| this.batchTableBinary = batchTableBinary; |
There was a problem hiding this comment.
It seems like in master as well as here this property isn't used, so it should be fine to remove.
|
|
||
| var legacyHierarchy = jsonHeader.HIERARCHY; | ||
| if (defined(legacyHierarchy)) { | ||
| Cesium3DTileBatchTable._deprecationWarning('batchTableHierarchyExtension', 'The batch table HIERARCHY property has been moved to an extension. Use extension.3DTILES_batch_table_hierarchy instead.'); |
There was a problem hiding this comment.
Change Use extension.3DTILES_batch_table_hierarchy to Use extensions.3DTILES_batch_table_hierarchy
| * @returns {Object|undefined} The contents of the batch table extension, or <code>undefined</code> | ||
| * if the extension is not present. | ||
| */ | ||
| Cesium3DTileBatchTable.prototype.getExtension = function(extensionName) { |
There was a problem hiding this comment.
This function isn't used outside this class. Should it be a local function, or are there plans to use it externally?
| var extensions = this._extensions; | ||
| if (!defined(extensions)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This check isn't required since _extensions defaults to an empty object.
| function getBinaryProperties(featuresLength, properties, binaryBody) { | ||
| if (!defined(properties)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This check isn't needed. properties defaults to an empty object.
| } | ||
| } | ||
| var propertyNames = Object.keys(this._properties); | ||
| results.push.apply(results, propertyNames); |
There was a problem hiding this comment.
The reason for the more verbose form before was to avoid allocating an array (like Object.keys would) if results is supplied. In practice I'm not sure if it really matters for this specific function though, so the change is ok.
Source/Scene/Cesium3DTileset.js
Outdated
| @@ -1915,6 +1917,20 @@ define([ | |||
| } | |||
| }; | |||
|
|
|||
| /** | |||
| * <code>true</code> if this the tileset JSON file lists the extension in extensionsUsed; otherwise, <code>false</code>. | |||
There was a problem hiding this comment.
if this the tileset JSON file - small typo. Same below.
| @@ -0,0 +1,53 @@ | |||
| { | |||
There was a problem hiding this comment.
Replace BatchTableHierarchyExtension with BatchTableHierarchy after the name change in 3d-tiles-tools.
| @@ -54,6 +54,9 @@ defineSuite([ | |||
|
|
|||
| beforeAll(function() { | |||
| scene = createScene(); | |||
|
|
|||
| // Keep the error from logging to the console when running tests | |||
| Batched3DModel3DTileContent._deprecationWarning = function() {}; | |||
There was a problem hiding this comment.
Instead can this be a spy?
|
Thanks @lilleyse , updated! |
|
Thanks. Just made some small edits in ab89ce1, but otherwise looks solid. |
|
Just waiting for CI since it looks like it failed for some other reason on the previous commit. |
Part of #6697
To reflect updates to the 3D Tiles spec, deprecated support for the 3D Tiles pre-version 1.0 Batch Table Hierarchy in favor of the
3DTILES_batch_table_hierarchyextension.TODO: