Terrain and Imagery support for Google Earth Enterprise #5189
Conversation
…n't a tile as opposed to a promise everytime. Also handles negative altitudes in terrain.
…s are showing up which need to be looked at.
…rner skirt vertices.
| */ | ||
| function GoogleEarthEnterpriseMetadata(options) { | ||
| //>>includeStart('debug', pragmas.debug); | ||
| if (!defined(options.url)) { |
There was a problem hiding this comment.
This also throws if options itself is undefined, use options = defaultValue(options, options.EMPTY_OBJECT); as the first line.
|
Why are there no credits? Is there no attribution requirement? |
| buffer : metadata, | ||
| quadKey : quadKey, | ||
| type : 'Metadata' | ||
| }); |
There was a problem hiding this comment.
As discussed offline, you can pass [metadata] as the second parameter here to transfer it and avoid the copy.
There was a problem hiding this comment.
Same comment for anywhere else you're doing scheduleTask and don't need to continue to use the buffer after the fact
| var t = tileInfo[q]; | ||
| // If we have tileInfo make sure sure it is not a node with a subtree that's not loaded | ||
| if (defined(t) && (!t.hasSubtree() || t.hasChildren())) { | ||
| return when(t); |
There was a problem hiding this comment.
I don't think this when needs to be here, you can just return t.
| function GoogleEarthEnterpriseTerrainData(options) { | ||
| //>>includeStart('debug', pragmas.debug); | ||
| if (!defined(options) || !defined(options.buffer)) { | ||
| throw new DeveloperError('options.buffer is required.'); |
There was a problem hiding this comment.
Same comment as earlier about the options object and using Check here, I'll stop commenting on it, just fix it throughout.
| //>>includeEnd('debug'); | ||
|
|
||
| var ellipsoid = tilingScheme.ellipsoid; | ||
| var nativeRectangle = tilingScheme.tileXYToNativeRectangle(x, y, level); |
There was a problem hiding this comment.
Should be be using scratch values and result parameters here (also in cartographicToCartesian below).
| } | ||
|
|
||
| var that = this; | ||
| return when(verticesPromise, function(result) { |
There was a problem hiding this comment.
No reason for when here, since verticesPromise is garuanteed to be a promis, use verticesPromise.then instead.
| return when(verticesPromise, function(result) { | ||
| that._mesh = new TerrainMesh( | ||
| center, | ||
| new Float32Array(result.vertices), |
There was a problem hiding this comment.
Is this making a copy? Why not just use the value directly?
There was a problem hiding this comment.
I don't think so. It's passing in a buffer, not another typed array which should just use the Float32Array as a dataview into the buffer.
| } | ||
|
|
||
| var that = this; | ||
| return when(upsamplePromise, function(result) { |
There was a problem hiding this comment.
upsamplePromise is already a promise, no need for when
| * | ||
| * @param {Object} obj Object with same properties as TileInformation | ||
| */ | ||
| TileInformation.fromObject = function(obj) { |
There was a problem hiding this comment.
The convention that we use for copy functions is to call them clone and provide an optional result parameter.
| type : 'Terrain' | ||
| }); | ||
|
|
||
| return decodePromise |
There was a problem hiding this comment.
To avoid the extra indentation, whenever you return a promise in a chain, end the function and put a then one level up, instead of globbing then on this. Essential, change this to
return taskProcessor.scheduleTask({
buffer : terrain,
type : 'Terrain'
});
})
.then(function(terrainTiles) {
|
I was a little concerned about the new third party libraries increasing build size, but the release version of |
|
That's all of the comments I have. I'm no expert in this area, but this looks good to me. Awesome work @tfili! |
…e had been returned in the parent.
There was a problem hiding this comment.
Very nice @tfili. I didn't look super close at the GEE end of things because I don't know anything about it, but I looked at the points of interaction with the terrain/imagery engine. The couple of comments I had aren't too critical before merging this, especially since I hear @tfili might be a bit busy at the moment. Congratulations dude!
| var parentInfo; | ||
| var q = quadKey; | ||
| var terrainVersion = -1; | ||
| if (terrainState !== TerrainState.NONE) { |
There was a problem hiding this comment.
I don't think this if is necessary. If terrainState were TerrainState.NONE, we would have returned above. And even if not, entering the switch won't do any harm.
There was a problem hiding this comment.
Thanks, @kring. I looked at the code and agree. I'll push a fix.
| promise = requestPromise | ||
| .then(function(terrain) { | ||
| if (defined(terrain)) { | ||
| return taskProcessor.scheduleTask({ |
There was a problem hiding this comment.
This is not ideal because it subverts the terrain system's ability to prioritize the work it does. The way this is written, when a tile request comes back, it will always do work in the web worker, even if the user has since moved the camera and the tile is no longer needed.
A better approach would be to return a TerrainData representing the raw downloaded data, and only decode it on demand in createMesh and similar methods.
The rule of thumb for good terrain loading performance is: do as little as possible in promise resolutions, because they always happen, even if they're not needed.
Up to you if you think this is worth the trouble, of course.
# Conflicts: # CHANGES.md
|
I merged in master and made a tweak suggested by @kring. Since @tfili is indisposed, we discussed offline that this is in good shape to merge and we'll add server-send credit handling and address #5189 (comment) in future PRs. As soon as the tests pass, I'll merge. |
|
Awesome work here @tfili! I still think you should have named the baby Cesium. |
|
Edit: Working great now. Thanks a lot guys! |
|
Awesome @jburr-nc thanks for letting us know and for testing it out. |
|
I didn't see any other good places to ask about this -- hopefully interested parties are still monitoring this PR even though it's closed. The thread here was very helpful in understanding the difference between the GE provider and the GEE provider, but this clarification is sorely lacking in the documentation. Since both providers actually talk to GEE (and not the public GE server), maybe in a future release it would be helpful to merge the two and provide a 2D/3D mode switch? That would clarify the intent. |
Added a
GoogleEarthEnterpriseTerrainProviderandGoogleEarthEnterpriseImageryProvider, which takes aGoogleEarthEnterpriseMetadataobject, so we don't duplicate metadata requests.GoogleEarthEnterpriseTerrainDataandcreateVerticesFromGoogleEarthEnterpriseBufferwere also added to parse terrain data on a web worker.Tests were added for all of these classes.
Update CHANGES.md
One TODO that needs to be investigated