Availability from bvh #7196
Availability from bvh #7196
Conversation
… the layer.json available member.
|
@kring could you spare a few seconds to look at this? I'll also take a quick look now. |
|
Wow love the 1m Zion terrain. |
|
Does anyone know why @cesium-concierge didn't comment on this? |
|
@tfili how did this impact Cesium startup time? And doesn't this warrant an update to CHANGES.md? |
|
@tfili please submit an issue about backwards compatibility / deprecation as we discussed? |
Source/Core/CesiumTerrainProvider.js
Outdated
| * @constant | ||
| * @default 3 | ||
| */ | ||
| BVH: 3 |
There was a problem hiding this comment.
Are we sure this shouldn't be spelled out like WATER_MASK? Perhaps similar question for the extension spec if that is relevant.
Source/Core/CesiumTerrainProvider.js
Outdated
| layer.bvhLoaded.addAvailableTileRange(level, x, y, x, y); | ||
| recurseHeights(level, x, y, bvh, 0, maxLevel, provider.availability, layer.availability); | ||
| } else { | ||
| console.log('Incorrect number of heights in tile Level: %d X: %d Y: %d', level, x, y); |
There was a problem hiding this comment.
Is console.log the right approach compared to an exception?
Source/Core/CesiumTerrainProvider.js
Outdated
|
|
||
| if (!isNaN(buffer[index])) { | ||
| // Minimum height isn't a Nan, so the tile exists | ||
| // TODO: Make this more efficient |
There was a problem hiding this comment.
We generally don't merge PRs with TODOs. Write an issue or just do it. Is it a bottleneck in practice?
Source/Core/CesiumTerrainProvider.js
Outdated
| } | ||
| //>>includeEnd('debug'); | ||
|
|
||
| // returns true if we can request vertex normals from the server |
Source/Core/CesiumTerrainProvider.js
Outdated
| * @memberof CesiumTerrainProvider.prototype | ||
| * @type {Boolean} | ||
| */ | ||
| requestBvh : { |
There was a problem hiding this comment.
Not sure that we should use the abbreviation in the public API.
Source/Core/CesiumTerrainProvider.js
Outdated
|
|
||
| var promise = when.resolve(); | ||
| if (level !== 0) { | ||
| var bvhLevels = layer.bvhLevels - 1; |
There was a problem hiding this comment.
Extract out into a helper function given the duplication with the code above?
Source/Core/CesiumTerrainProvider.js
Outdated
| var numberofIncludedLevels = findNumberOfLevels(numberOfHeights/2); | ||
| if (defined(numberofIncludedLevels) && numberofIncludedLevels <= layer.bvhLevels) { | ||
| var maxLevel = numberofIncludedLevels + level - 1; | ||
| layer.bvhLoaded.addAvailableTileRange(level, x, y, x, y); |
There was a problem hiding this comment.
TileAvailability is meant to work with tile ranges, not individual tiles. I don't think it's going to go well to use it this way. One problem is that the availability quadtree is going to grow without bound as you move around. Another problem is that the availability quadtree will be deeper than before, so traversing it will take longer. And finally, the computeBestAvailableLevelOverRectangle will probably be crazy slow in some scenarios because it works by subtracting rectangles from the area of interest. If that's done one tile at a time, it's a lot of rectangles. Plus the remaining rectangle list will become huge because each time we subtract a tile from a rectangle we create 1-3 new rectangles.
Source/Core/CesiumTerrainProvider.js
Outdated
| var level = this.availability._maximumLevel; | ||
| this._tilingScheme.positionToTileXY(position, level, scratchTileXY); | ||
|
|
||
| return when(loadTileAvailability(this, scratchTileXY.x, scratchTileXY.y, level)); |
There was a problem hiding this comment.
So every time you ask the terrain provider to load availability it will create a promise, and then, if availability isn't yet know for the maximum level at that position, it will do a non-throttled request for the closest BVH node to that level (generating 404s if it doesn't exist). I modifed the Terrain sandcastle example to use your Zion terrain and sample heights around Zion. Pressing the "Sample Most Detailed Everest Terrain" button generates thousands of requests all at once and locks up the browser. Here's the Super Hacky Sandcastle.
Source/Core/CesiumTerrainProvider.js
Outdated
| var bvh = new Float32Array(buffer, extensionPos, numberOfHeights); | ||
| extensionPos += Float32Array.BYTES_PER_ELEMENT * numberOfHeights; | ||
|
|
||
| var numberofIncludedLevels = findNumberOfLevels(numberOfHeights/2); |
There was a problem hiding this comment.
The iterative algorithm in findNumberOfLevels is pretty expensive to do on every tile, and I don't think it's necessary. For the error checking part, just calculate the number of tiles that should be in the BVH at this level, and see if the actual number of tiles is the same. For recurseHeights, you just need to adjust your ranges as you recurse. For the loaded tile, the BVH range is 0 through length-1. Then the number of BVH tiles in each child is n = (length - 1) / 4, the southwest child is 1 through n, southeast child is n + 1 through 2n, northwest is 2n + 1 through 3n, and northeast is 3n + 1 through 4n.
Source/Core/CesiumTerrainProvider.js
Outdated
| return this._availability.isTileAvailable(level, x, y); | ||
| var result = loadTileAvailability(this, x, y, level); | ||
|
|
||
| // If we got a result, return it. If it was a promise return false for now. |
There was a problem hiding this comment.
In the fill-tiles branch, returning false here will cause the terrain engine to upsample the tile instead of loading it, and never look back. We should return some sort of sentinel value (null? unfortunately we're already using undefined) to indicate that we don't know yet, rather than returning false.
|
That's all I have. I still wonder if it would be wiser to use some kind of tile-local version of the rectangle ranges we're currently using in layer.json, rather than the BVH extension. |
|
Thanks for the quick review, @kring!
@tfili what do you think of this? Sounds like there are some performance concerns with the current approach. |
|
@pjcozzi We have a metadata extension in the works and mostly ready to go. I'm reviewing that today. I can use that send down subtree available ranges, instead of using the BVH extension. That should address most of the performance issues @kring brings up. The good thing here is that most of the code will be reused and actually slightly simplified. Thanks @kring for the review. I didn't realize the availability bloat with my approach. I have a better understanding of whats going on now. |
|
Cool, good luck @tfili! |
|
@kring If you wouldn't mind taking another look it would be appreciated. I abandoned using BVH for availability and instead store the ranges in the tiles every 10 levels. The only thing that has changed in the example is the asset numbers. I also cache promises for availability requests so we don't make a bunch of the same ones. The one issue with your branch is that we are returning false from Here is the example: |
|
Also turns out we have a helper that converts a buffer to a string in the most efficient way the browser supports. |
…e our built in helper to convert a buffer to a string.
The only time we save is the download, decompression and parsing of the layer.json. The biggest tileset we have is CWT which currently has the size of 200KB gzipped and 1.8MB uncompressed. The time improvement isn't very noticeable on my laptop. However, if we end up with a layer.json that is 1.2MB gzipped and 15MB uncompressed then the improvement seems to be about a 1-1.5 seconds on load.
No. It looks like tiles are loading in about the same time as they did previously. |
Nice!!! |
CHANGES.md
Outdated
| @@ -9,6 +9,7 @@ Change Log | |||
| * Added `Scene.clampToHeightMostDetailed`, an asynchronous version of `Scene.clampToHeight` that uses the maximum level of detail for 3D Tiles. | |||
| * Added `Scene.invertClassificationSupported` for checking if invert classification is supported. | |||
| * Added `computeLineSegmentLineSegmentIntersection` to `Intersections2D`. [#7228](https://github.com/AnalyticalGraphicsInc/Cesium/pull/7228) | |||
| * Added ability to load availability from quantized mesh tiles every few levels instead of upfront in layer.json. This should speed up the load time of large tilesets significantly. [#7196](https://github.com/AnalyticalGraphicsInc/cesium/pull/7196) | |||
There was a problem hiding this comment.
Would be good to be specific here like the examples in the comment and frame it in what the end user experiences - faster load time, less memory usage since this is progressive.
|
@tfili do you know how code coverage is? Does this need any additional tests? |
|
Is there a terrain dataset you can point me to (offline if needed) to test with? |
|
@kring did you need to look at any of this again? Or can we just merge when ready? |
Nevermind, GTOPO + Zion from above is perfect. |
|
The |
|
Looking at the places |
|
@kring In master it sort of looks that way, because the parent's must be loaded before the child. I'll mess around with it an do some more testing. I definitely over complicated the problem. In your branch with LOD skipping, you'll need to take that into account though. @pjcozzi I'll run coverage, but with the simplifications I put in today, I think loading a terrain with metadata availability should cover it, but I'll verify. |
Yeah I haven't totally thought through the implications in the |
|
@tfili I think I found an issue with sampling terrain when parent terrain are involved (not sure if it happens in master or not).
The first time you press it, the samples will all be undeground. The second time it works. This doesn't not happen if you just use the GTOPO terrain so it might only be an issue when parent terrains are used. |
|
@kring returning undefined seems to work well. The one caveat is that if we are using parent terrain, I still need to do a fetch tile for the parent during @mramato I'm looking at your issue now. |
|
Just those last two comments. |
Co-Authored-By: tfili <[email protected]>
We now don't use the availability list from layer.json. Instead we load it from the BVH terrain extension. Here is a Sandcastle example. The GTOPO terrain is stand alone and the Zion UT terrain is cutout terrain on top of GTOPO.
https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/index.html#c=tVZtb+I4EP4rVr9AJWoSJ06cLq2uR3tVqgLblu7tcpxOJjHgNrHZxLyu+t9vQqClqD1dpV1/AcYz88w880xCvY4uM64MaopcTtOrO8SjSOQ5Mhot9TRDUivE81yYvK9KHxxqhWMx5NPEnK2du/pRKHSCKmJ5NR5cRrIjr8L7VWi3ZZiH6pZGzdALHydfvzSvAgxO3+PLx8Lp8Vt6lfbOo0VracvWw8X8unuzbKXfTOcydDpNO213b8ft8xjuRotW2pLXzatJD5K1lnPJ//zDCh/0ot0NSev8nrZXZ/PhDY6GzZvv5mjs9mLX8H++XjysTOBPHdbknWC2WMyH/t3N7ywcxGHl01s93YlsJrKinbExk/y4Xo/F7IhPJI5K30inRWRfzXiGRkZPdFdkGZcFBUrMN1Ti8mNz9TnTMxmLrPqjrxCcaZYcoxfsW5ED2ZHAw0ynZwXdYVz1vMO+ejp8hlrBLH4Rkv8aaSbFfE3BDsiXta1aKUloamUATGSV2gbHvIZHx6+oeU6/0jrt6kJzcZOrpVbV1w1eLIwAOZ5sYW9FZLgaJWXBtzyWXOXVIxsHPvGJZfm2YxHbdUnNwp7rM5cxy3H8wPfBtHYDF+JQ4lPP9pmzdgtcSqntgsXzbLYpYN0fziOhBB4leiCwUHyQiGs5GhupRlCTyaZi7dxXd9BAxHMDhfE47mqdDHjWEmpa/euZkIUBFiqX3c7njmNVaqVZq1wk0BRcDacqMrKgAG1iirMpZJ/Pk1d8fir9n4DXGtoDbF+cIztFeoh6kLyG7rsACgs9KUw/rZodNe4U83dJ5gfYKZSASin876LeFtG7hGxp+GD+ntxLvGluG/NWGdssG9oinoqM42Gy7OrqDkAscpAUX2d53s4mzwx848pZa/1cjDIhCq3bBNuuV2jac2hAiFdDjoct6jCLENdxQcge2Aj1LexQ3wG9O5bLHO+w9gKpMwmLtYXcqaU4YwGLBRJ/rqXFzRgbvd03EmAvgPw2Za4X2D7dzVyciTTR+N3wI8fHlHmwdo5nW4y5zn58ppMEwi1sw1oGsNIeC4jveoy++D1tJlFM4emNQfTkr5xAwGhAHcv3CHOICxPwYSiO7VECxVoWpTXkMpdiBhOwGC1aID+Pf4Jdy3d9yijMlgAvHx2ABw1YLmEucRmhHqHvTgDmQy1mQS8WCaBn9h8T6KuD2kEjN8tEnJZ3v8l0ojNTvHaqGNeNSCcJB1Lrg2n0KAyO8ryIbtS3QY1YzpCMT/oHe2+W/gGKEvjrATfDaZLcyZXoH5w26uD/KizRa+468NJO+BJcykIaY/v0urzCGDfq8LOA3Y825bNpJ/O/