Add Extras to 3DTile and 3DTileset #6974

OmarShehata · 2018-08-29T17:26:15Z

This resolves #6490. It reads the extras dictionary from tileset.json and from all tiles, and makes it accessible via a read only .extras property.

I tested it by modifying the SampleData/Cesium3DTiles/Tilesets/Tileset/tileset.json to add extras fields and checking them:

var tileset = new Cesium.Cesium3DTileset({
    url: '../../SampleData/Cesium3DTiles/Tilesets/Tileset/tileset.json'
});

tileset.readyPromise.then(function(tileset) {
    console.log(tileset.extras); // Prints { value: 12 }
    for(var child of tileset.root.children){
        console.log(child.extras); // Prints undefined for all except the tile that has the `extras`. 
    }
}

I also added a test. It's basically just checking a getter, so it can easily be combined in another test instead of being its own.

I'm not sure if there's much we can do with extensions right now aside from just making it accessible in the API. Perhaps it's best to do that until more use cases arise? @lilleyse please review when you get a chance.

cesium-concierge · 2018-08-29T17:26:17Z

Thanks for the pull request @OmarShehata!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

lilleyse

Thanks @OmarShehata.

lilleyse · 2018-08-29T19:17:30Z

Specs/Scene/Cesium3DTilesetSpec.js

@@ -356,6 +358,16 @@ defineSuite([
        });
    });

+    fit('loads tileset with extras', function() {


Change fit to it

lilleyse · 2018-08-29T19:17:33Z

Specs/Data/Cesium3DTiles/Tilesets/TilesetWithExtras/tileset.json

@@ -0,0 +1,124 @@
+{


I don't think it's necessary to create a new tileset TilesetWithExtras just for this change. Just add extras properties to Tileset. We did a similar thing when adding the tilesetVersion property.

After that, make sure to keep SampleData/Cesium3DTiles/Tilesets/Tileset/tileset.json and Specs/Data/Cesium3DTiles/Tilesets/Tileset/tileset.json in sync.

key/value is maybe too generic? Maybe name or id instead? Use different values/types for the tileset-level extras and the child extras. This will help make the test more robust.

Open a PR with any modifications to 3D Tiles sample data to https://github.com/AnalyticalGraphicsInc/3d-tiles-tools/tree/master/samples-generator. Then regenerate tileset.json and update here. There are formatting inconsistencies in this hand-edited file.

lilleyse · 2018-08-29T19:18:23Z

Specs/Scene/Cesium3DTilesetSpec.js

+    fit('loads tileset with extras', function() {
+        return Cesium3DTilesTester.loadTileset(scene, tilesetWithExtras).then(function(tileset) {
+            expect(tileset.extras).toBeDefined();
+            expect(tileset.extras).toEqual({'key': 'value'});


Remove expect(tileset.extras).toBeDefined(); - it is redundant.

lilleyse · 2018-08-29T19:18:29Z

Specs/Scene/Cesium3DTilesetSpec.js

+            expect(tileset.extras).toEqual({'key': 'value'});
+
+            expect(tileset.root.extras).toBeDefined();
+            expect(tileset.root.extras).toEqual({'key': 'value'});


Same comment.

lilleyse · 2018-08-29T19:22:38Z

Source/Scene/Cesium3DTileset.js

+         *
+         * @memberof Cesium3DTileset.prototype
+         *
+         * @type {Object}


Change to @type {*}, extras is not necessarily an object, it could be a string, number, etc.

lilleyse · 2018-08-29T19:26:52Z

Source/Scene/Cesium3DTileset.js

+        },
+
+        /**
+         * Application specific metadata.


Expand on the description some more. Something like:

Returns the extras property at the top-level of the tileset JSON, which contains application specific metadata. Returns undefined if extras does not exist.

lilleyse · 2018-08-29T19:27:30Z

Source/Scene/Cesium3DTile.js

+         *
+         * @type {Object}
+         * @readonly
+         * @see {@link https://github.com/AnalyticalGraphicsInc/3d-tiles/tree/master/specification#specifying-extensions-and-application-specific-extras|Extras in the 3D Tiles specification.}


Same comments here.

lilleyse · 2018-08-29T19:28:39Z

Source/Scene/Cesium3DTile.js

+         */
+        extras : {
+            get : function() {
+                return this._extras;


Instead of storing _extras as a member variable, this could just be return this._header.extras

lilleyse · 2018-08-29T19:29:40Z

Source/Scene/Cesium3DTile.js

@@ -88,7 +88,7 @@ define([
        var contentHeader = header.content;

        /**
-         * The local transform of this tile
+         * The local transform of this tile.


Thanks for fixing these!

lilleyse · 2018-08-29T19:33:05Z

Specs/Scene/Cesium3DTilesetSpec.js

+            expect(tileset.extras).toEqual({'key': 'value'});
+
+            expect(tileset.root.extras).toBeDefined();
+            expect(tileset.root.extras).toEqual({'key': 'value'});


Since you mentioned it in the PR description, it might be worthwhile to add a test that checks that extras is undefined for some other tile in the tree that doesn't have extras.

lilleyse · 2018-08-29T19:34:56Z

I'm not sure if there's much we can do with extensions right now aside from just making it accessible in the API. Perhaps it's best to do that until more use cases arise?

Yeah fine with me, I don't think we have a good use case yet for needing to expose tileset extensions to the API.

lilleyse · 2018-08-29T20:00:22Z

Also... add this change to CHANGES.md

OmarShehata · 2018-08-29T20:35:48Z

Also... add this change to CHANGES.md

Just did! (0a0af0f)

Regenerating the tileset.json seems to change the b3dm files as well, which I've included. (I opened the PR here #6974)

But now there's also this warning?

WARN LOG: 'This tileset JSON uses the "content.url" property which has been deprecated. Use "content.uri" instead.'

I'm not sure if I regenerated them incorrectly.

In the new test/spec, I loop over all the children to count the ones with an extras because it seems like the order they are in tileset.json isn't necessarily the order I get them in. I'm open to feedback if there's a better way to test this.

lilleyse · 2018-08-29T20:47:26Z

Sorry, I forgot that CesiumGS/3d-tiles-validator#139 was still open. If you branch CesiumGS/3d-tiles-validator#152 off of that branch and regenerate the tileset most of those problems will go away.

lilleyse · 2018-08-31T14:23:09Z

Source/Scene/Cesium3DTileset.js

+        },
+
+        /**
+         * Returns the `extras` property at the top-level of the tileset JSON, which contains application specific metadata.


Instead of tick marks use <code>extras</code> in documentation for code highlighting.

lilleyse · 2018-08-31T14:27:40Z

Specs/Scene/Cesium3DTilesetSpec.js

+            expect(tileset.root.extras).not.toBeDefined();
+
+            var taggedChildren = 0;
+            for(var i = 0;i < tileset.root.children.length; i++) {


Style things:

change for(var to for (var - add space after for

change i = 0;i to i = 0; i - add space after semicolon

change i++ to ++i - just a convention in the Cesium code base

save length as its own variable before the loop - this convention was originally added for performance reasons since length doesn't have to be re-evaluated every iteration. While it doesn't really matter for tests it's just good to get into that habit.

var length = tileset.root.children.length for (var i = 0; i < length; ++i) {

lilleyse · 2018-08-31T14:28:21Z

Specs/Scene/Cesium3DTilesetSpec.js

+            var taggedChildren = 0;
+            for(var i = 0;i < tileset.root.children.length; i++) {
+                if (defined(tileset.root.children[i].extras)) {
+                    taggedChildren ++;


Similarly add the ++ before the variable here too to match the convention:

++taggedChildren;

lilleyse · 2018-08-31T14:33:07Z

Specs/Scene/Cesium3DTilesetSpec.js

@@ -356,6 +358,22 @@ defineSuite([
        });
    });

+    it('loads tileset with extras', function() {
+        return Cesium3DTilesTester.loadTileset(scene, tilesetUrl).then(function(tileset) {
+            expect(tileset.extras).toBeDefined();


CC #6974 (comment)

Checking that the extras is what we expect is still worthwhile:

expect(tileset.extras).toEqual({'name': 'Sample Tileset'});

And remove the toBeDefined line.

lilleyse · 2018-08-31T14:38:36Z

In the new test/spec, I loop over all the children to count the ones with an extras because it seems like the order they are in tileset.json isn't necessarily the order I get them in. I'm open to feedback if there's a better way to test this.

Ah yeah, that's due to the traversal code sorting the children based on distance from camera. The way you're testing it is fine.

lilleyse · 2018-08-31T14:43:52Z

Source/Scene/Cesium3DTileset.js

+        extras : {
+            get : function() {
+                return this._extras;
+            }


It might be a good idea to check if the tileset is ready before accessing extras and throw an error otherwise. See asset:

//>>includeStart('debug', pragmas.debug); if (!this.ready) { throw new DeveloperError('The tileset is not loaded. Use Cesium3DTileset.readyPromise or wait for Cesium3DTileset.ready to be true.'); } //>>includeEnd('debug');

Only needed for the Cesium3DTileset one, not Cesium3DTile.

OmarShehata · 2018-08-31T15:44:16Z

Thanks for the detailed feedback @lilleyse ! Should be all good now.

Actually, one issue, I noticed is when using the @exception {DeveloperError} it only shows up in the docs if it's used on a function, not on a property. The docs don't say anything about it working for properties. So even though I added it here d4c90e3 it doesn't actually show up in the docs. Same for asset and other properties.

Should we remove @exception tags from properties and maybe embed it in the description? I couldn't find if there was an easy way to add exception tags to properties in jsdoc.

mramato · 2018-08-31T15:59:10Z

Should we remove @exception tags from properties and maybe embed it in the description? I couldn't find if there was an easy way to add exception tags to properties in jsdoc.

In general, properties shouldn't throw exceptions, but if there's a good reason to that's fine; just leave it as it is for now. #832 was written a long time ago to eventually make sure they work (feel free to tackle that if you want 😄 )

Source/Scene/Cesium3DTileset.js

lilleyse · 2018-08-31T18:55:16Z

Specs/Scene/Cesium3DTilesetSpec.js

+    it('loads tileset with extras', function() {
+        return Cesium3DTilesTester.loadTileset(scene, tilesetUrl).then(function(tileset) {
+            expect(tileset.extras).toEqual({ 'name': 'Sample Tileset' });
+            expect(tileset.root.extras).not.toBeDefined();


Alternatively, toBeUndefined.

lilleyse · 2018-08-31T20:57:31Z

Thanks @OmarShehata!

Omar Shehata added 4 commits August 29, 2018 11:23

Added to Tileset

722701d

Made extras read only

9badee4

Added .extras to Cesium3DTile

0c3d711

Added test

1e73469

Omar Shehata added 2 commits August 29, 2018 14:06

Updated changes.md

0a0af0f

Fixed linting issue

e434db4

lilleyse reviewed Aug 29, 2018

View reviewed changes

Omar Shehata added 4 commits August 29, 2018 16:26

Return from header directly

25c8623

Better description

8a24922

Test checks for extras in tileset as well as children

468a1fb

Updated sample data

5de6c3f

OmarShehata mentioned this pull request Aug 29, 2018

Added extras dictionary to tileset CesiumGS/3d-tiles-validator#152

Closed

Updated sample data

4f0d17c

OmarShehata mentioned this pull request Aug 29, 2018

Add extras dictionary to tileset CesiumGS/3d-tiles-validator#153

Merged

lilleyse reviewed Aug 31, 2018

View reviewed changes

Omar Shehata added 4 commits August 31, 2018 10:45

Replace tick marks with <code>

90ee7cf

Add error if not loaded

d4c90e3

Updated test

e88a6c8

Added back taggedChildren test

e683221

lilleyse reviewed Aug 31, 2018

View reviewed changes

Added test for throwing error

e4d20f1

lilleyse merged commit 94e7113 into CesiumGS:master Aug 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Extras to 3DTile and 3DTileset #6974

Add Extras to 3DTile and 3DTileset #6974

OmarShehata commented Aug 29, 2018

cesium-concierge commented Aug 29, 2018

lilleyse left a comment

lilleyse Aug 29, 2018

lilleyse Aug 29, 2018

lilleyse Aug 29, 2018

lilleyse Aug 29, 2018

lilleyse Aug 29, 2018

lilleyse Aug 29, 2018 •

edited

Loading

lilleyse Aug 29, 2018

lilleyse Aug 29, 2018

lilleyse Aug 29, 2018

lilleyse Aug 29, 2018

lilleyse commented Aug 29, 2018

lilleyse commented Aug 29, 2018

OmarShehata commented Aug 29, 2018

lilleyse commented Aug 29, 2018 •

edited

Loading

lilleyse Aug 31, 2018

lilleyse Aug 31, 2018 •

edited

Loading

lilleyse Aug 31, 2018

lilleyse Aug 31, 2018 •

edited

Loading

lilleyse commented Aug 31, 2018

lilleyse Aug 31, 2018

lilleyse Aug 31, 2018

OmarShehata commented Aug 31, 2018

mramato commented Aug 31, 2018

lilleyse Aug 31, 2018

lilleyse commented Aug 31, 2018

Add Extras to 3DTile and 3DTileset #6974

Add Extras to 3DTile and 3DTileset #6974

Conversation

OmarShehata commented Aug 29, 2018

cesium-concierge commented Aug 29, 2018

lilleyse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Aug 29, 2018

lilleyse commented Aug 29, 2018

OmarShehata commented Aug 29, 2018

lilleyse commented Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

lilleyse Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

lilleyse commented Aug 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarShehata commented Aug 31, 2018

mramato commented Aug 31, 2018

Choose a reason for hiding this comment

lilleyse commented Aug 31, 2018

lilleyse Aug 29, 2018 •

edited

Loading

lilleyse commented Aug 29, 2018 •

edited

Loading

lilleyse Aug 31, 2018 •

edited

Loading

lilleyse Aug 31, 2018 •

edited

Loading