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

Add Extras to 3DTile and 3DTileset #6974

Merged
merged 16 commits into from
Aug 31, 2018
2 changes: 2 additions & 0 deletions Apps/SampleData/Cesium3DTiles/Tilesets/Tileset/tileset.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"version": "1.0",
"tilesetVersion": "1.2.3"
},
"extras": { "hey":12 },
"properties": {
"id": {
"minimum": 0,
Expand Down Expand Up @@ -61,6 +62,7 @@
]
},
"geometricError": 0,
"extras": { "special": true },
"content": {
"uri": "ll.b3dm"
}
Expand Down
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Change Log
* Added `GeocoderViewModel.destinationFound` for specifying a function that is called upon a successful geocode. The default behavior is to fly to the destination found by the geocoder. [#6915](https://github.com/AnalyticalGraphicsInc/cesium/pull/6915)
* Added optional `width` and `height` to `Scene.drillPick` for specifying a search area.
* Added `Cesium3DTileset.root` for getting the root tile of a tileset. [#6944](https://github.com/AnalyticalGraphicsInc/cesium/pull/6944)
* Added `Cesium3DTileset.extras` and `Cesium3DTile.extras` for getting application specific metadata from 3D Tiles. [#6974](https://github.com/AnalyticalGraphicsInc/cesium/pull/6974)
* Added `heightReference` to `BoxGraphics`, `CylinderGraphics` and `EllipsoidGraphics`, which can be used to clamp these entity types to terrain [#6932](https://github.com/AnalyticalGraphicsInc/cesium/pull/6932)

##### Fixes :wrench:
Expand Down
21 changes: 19 additions & 2 deletions Source/Scene/Cesium3DTile.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ define([
var contentHeader = header.content;

/**
* The local transform of this tile
* The local transform of this tile.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing these!

* @type {Matrix4}
*/
this.transform = defined(header.transform) ? Matrix4.unpack(header.transform) : Matrix4.clone(Matrix4.IDENTITY);
Expand All @@ -99,8 +99,10 @@ define([
var parentInitialTransform = defined(parent) ? parent._initialTransform : Matrix4.IDENTITY;
this._initialTransform = Matrix4.multiply(parentInitialTransform, this.transform, new Matrix4());

this._extras = header.extras;

/**
* The final computed transform of this tile
* The final computed transform of this tile.
* @type {Matrix4}
* @readonly
*/
Expand Down Expand Up @@ -429,6 +431,21 @@ define([
}
},

/**
* Application specific metadata.
*
* @memberof Cesium3DTile.prototype
*
* @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.}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments here.

*/
extras : {
get : function() {
return this._extras;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
},

/**
* Gets or sets the tile's highlight color.
*
Expand Down
18 changes: 18 additions & 0 deletions Source/Scene/Cesium3DTileset.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ define([
this._selectedTilesToStyle = [];
this._loadTimestamp = undefined;
this._timeSinceLoad = 0.0;
this._extras = undefined;

this._cullWithChildrenBounds = defaultValue(options.cullWithChildrenBounds, true);
this._allTilesAdditive = true;
Expand Down Expand Up @@ -710,6 +711,7 @@ define([
that._geometricError = tilesetJson.geometricError;
that._extensionsUsed = tilesetJson.extensionsUsed;
that._gltfUpAxis = gltfUpAxis;
that._extras = tilesetJson.extras;
that._readyPromise.resolve(that);
}).otherwise(function(error) {
that._readyPromise.reject(error);
Expand Down Expand Up @@ -1237,6 +1239,22 @@ define([
get : function() {
return this._ellipsoid;
}
},

/**
* Application specific metadata.
Copy link
Contributor

@lilleyse lilleyse Aug 29, 2018

Choose a reason for hiding this comment

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

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.

*
* @memberof Cesium3DTileset.prototype
*
* @type {Object}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

* @readonly
*
* @see {@link https://github.com/AnalyticalGraphicsInc/3d-tiles/tree/master/specification#specifying-extensions-and-application-specific-extras|Extras in the 3D Tiles specification.}
*/
extras : {
get : function() {
return this._extras;
}
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 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');

Copy link
Contributor

Choose a reason for hiding this comment

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

Only needed for the Cesium3DTileset one, not Cesium3DTile.

}
});

Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
124 changes: 124 additions & 0 deletions Specs/Data/Cesium3DTiles/Tilesets/TilesetWithExtras/tileset.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"asset": {
"version": "1.0",
"tilesetVersion": "1.2.3"
},
"properties": {
"id": {
"minimum": 0,
"maximum": 9
},
"Longitude": {
"minimum": -1.3197192952275933,
"maximum": -1.319644104024109
},
"Latitude": {
"minimum": 0.698848878034009,
"maximum": 0.6989046192460953
},
"Height": {
"minimum": 6.161747192963958,
"maximum": 85.41026367992163
}
},
"geometricError": 240,
"extras": {
"key": "value"
},
"root": {
"boundingVolume": {
"region": [
-1.3197209591796106,
0.6988424218,
-1.3196390408203893,
0.6989055782,
0,
88
]
},
"geometricError": 70,
"extras": {
"key": "value"
},
"refine": "ADD",
"content": {
"uri": "parent.b3dm",
"boundingVolume": {
"region": [
-1.3197004795898053,
0.6988582109,
-1.3196595204101946,
0.6988897891,
0,
88
]
}
},
"children": [
{
"boundingVolume": {
"region": [
-1.3197209591796106,
0.6988424218,
-1.31968,
0.698874,
0,
20
]
},
"geometricError": 0,
"content": {
"uri": "ll.b3dm"
}
},
{
"boundingVolume": {
"region": [
-1.31968,
0.6988424218,
-1.3196390408203893,
0.698874,
0,
20
]
},
"geometricError": 0,
"content": {
"uri": "lr.b3dm"
}
},
{
"boundingVolume": {
"region": [
-1.31968,
0.698874,
-1.3196390408203893,
0.6989055782,
0,
20
]
},
"geometricError": 0,
"content": {
"uri": "ur.b3dm"
}
},
{
"boundingVolume": {
"region": [
-1.3197209591796106,
0.698874,
-1.31968,
0.6989055782,
0,
20
]
},
"geometricError": 0,
"content": {
"uri": "ul.b3dm"
}
}
]
}
}
Binary file not shown.
Binary file not shown.
12 changes: 12 additions & 0 deletions Specs/Scene/Cesium3DTilesetSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ defineSuite([
var pointCloudUrl = 'Data/Cesium3DTiles/PointCloud/PointCloudRGB/tileset.json';
var pointCloudBatchedUrl = 'Data/Cesium3DTiles/PointCloud/PointCloudBatched/tileset.json';

var tilesetWithExtras = 'Data/Cesium3DTiles/Tilesets/TilesetWithExtras/tileset.json';

beforeAll(function() {
scene = createScene();
});
Expand Down Expand Up @@ -356,6 +358,16 @@ defineSuite([
});
});

fit('loads tileset with extras', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Change fit to it

return Cesium3DTilesTester.loadTileset(scene, tilesetWithExtras).then(function(tileset) {
expect(tileset.extras).toBeDefined();
Copy link
Contributor

@lilleyse lilleyse Aug 31, 2018

Choose a reason for hiding this comment

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

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.

expect(tileset.extras).toEqual({'key': 'value'});
Copy link
Contributor

Choose a reason for hiding this comment

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

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


expect(tileset.root.extras).toBeDefined();
expect(tileset.root.extras).toEqual({'key': 'value'});
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

});
});

it('gets root tile', function() {
var tileset = scene.primitives.add(new Cesium3DTileset({
url : tilesetUrl
Expand Down