Load culled tiles with low priorty

[kring]

Fixes #4894

What is happening in #4894 is basically a failure of spatial coherence. We use a tile's bounding volume for hierarchical culling, but the bounding volume is computed from the vertices of the tile. So there is no guarantee that:

1. The children of the tile, or
2. The real tile geometry once it is loaded

are contained within the tile's bounding volume. In the particular case in #4894, a clear cache resulted in an upsampled tile with vertices (and hence bounding volume) far above the real terrain surface at that location. The upsampled geometry was deemed to be outside the view frustum and culled, and as a result the real geometry was never loaded.

The solution implemented here is to make sure that culled tiles are still loaded with low priority so that the mistake is eventually rectified. A much better solution (but orders of magnitude more work) would be to fix the spatial coherence.

[jbo023]

I just checked this for our code and it this solved our problem. Thanks.

Hmm, if i understood it correctly we could also solve this in the converter if we change the quantized mesh and propagate the minimumHeight and maximumHeight from the lowest terrain level to the hightest terrain level.
So that a child tile is always inside the boundingvolume of the parent tile ?

A high level tile would than potentially have a flat terrain at height 0m, but the minHeight and maxHeight in the tile would be something like 0m and 1200m depending on the children.

Would this be correct ?

[kring]

change the quantized mesh and propagate the minimumHeight and maximumHeight from the lowest terrain level to the hightest terrain level.

You should absolutely do this. The minimumHeight and maximumHeight properties are meant to capture the "real" (i.e. most detailed) min/max in the area covered by the tile. STK Terrain Server (and therefore STK World Terrain) already does this.

It won't fix the problem described here, though, because Cesium still computes bounding volumes from the vertices instead of from the min/max heights.

[pjcozzi]

For typical views (top down, horizon, 45 degree), how many more tiles does this load?

because Cesium still computes bounding volumes from the vertices instead of from the min/max heights.

Is it reasonable to use the min/max heights?

A much better solution (but orders of magnitude more work) would be to fix the spatial coherence.

What else is required other than fixing the bounding volumes to always include all the children's geometry? Isn't this possible when a tile has the min min and max max heights for all of its descendants?

[kring]

For typical views (top down, horizon, 45 degree), how many more tiles does this load?

I didn't measure, but it doesn't really matter, because:

1. It's more important that it work than that it be fast, and it currently is pretty broken.
2. The new tiles are loaded with low priority, which means that under normal circumstances (i.e. when we don't see this bug), the new tiles will be loaded after all the important ones are loaded. So it won't affect the user's perception of the performance even if it does mean a few more tiles are loaded in the background.

Is it reasonable to use the min/max heights?
What else is required other than fixing the bounding volumes to always include all the children's geometry? Isn't this possible when a tile has the min min and max max heights for all of its descendants?

I'm not sure. I plan to investigate this more when I get a chance, but it seemed important to fix the bug first.

[pjcozzi]

Given that we've only ran into this problem once in practice, I am hesitant to merge this without knowing the before/after numbers. We could leave this branch open until we know the impact or have a better fix. If the impact is small, merging this now would be fine.

[mramato]

I've been running into this issue more lately. It can most easily be reduced by entering 110 45 in the geocoder, which produces really bad culling issues in master but are fixed in this branch. I'm all for improving correctness first (since there are no obvious performance issues), but I'll let @pjcozzi decide when to merge since he has concerns.

[denverpierce]

Quick and dirty test:

Camera sandcastle, terrain on, starting from home and clicking "Fly in a City":

master:

• 323 .terrain requests
• 1.4MB

missingTiles:

• 334 requests
• 1.5MB

I didn't test frame rates but didn't notice any issues (not that there'd be any).

[pjcozzi]

@denverpierce your initial numbers are promising, thanks for checking.

I would still like someone to test a few common cases: top-down, 45 degree, horizon, flight with throttling on to be sure here. This has become standard practice for changes with a potential core performance impact.

[markerikson]

This has been sitting around for almost a year, and at first glance from an outside observer, seems pretty small and low-risk. Any reason this hasn't been merged in yet?

[hpinkos]

Thanks @markerikson, I would also like to push this through because it solves some problems I've run into. I think we want to run some performance tests to make sure it's not making a ton of extra requests.

@pjcozzi can you elaborate on this?

[pjcozzi]

I would also like to merge this, but please do not confuse a small amount of code with low risk - this is pretty high risk without the testing I suggested in #4969 (comment) and again in #4969 (comment).

There was some testing here: #4969 (comment)

If anyone has the time to carefully and broadly test this, please post the before/after number of tiles downloads here and we will review before merging.

[lilleyse]

Master

Static horizontal
  2.47 seconds
  437 requests
Static top down
  1.59 seconds
  306 requests
Static 45 degrees
  2.36 seconds
  395 requests
Zoom
  3.67 seconds
  462 requests
Pan
  10.33 seconds
  969 requests

missingTiles
Static horizontal
  4.59 seconds
  488 requests
static top down
  3.31 seconds
  368 requests        
Static 45 degrees
  4.31 seconds
  507 requests        
Zoom
  4.72 seconds
  515 requests
Pan
  10.09 seconds
  1029 requests

Notes:

• Roughly 50 more requests per test on this branch (with some anomalies, see below)
• Browser cache disabled
• Throttling at 3G speeds produced the same number of requests so I stopped running those tests because they were taking too long.
• I often get inconsistent results on master with a wide variance in number of requests between runs. For example, images like this are really common in master and result in a lot less requests overall. I chose times from the runs that didn't have any visible holes in them.
• No missing tiles on this branch and loading seemed more consistent.

[lilleyse]

Here is the Sandcastle code for this. You'll need to refresh the demo before each test so that results are accurate:

var viewer = new Cesium.Viewer('cesiumContainer');
var scene = viewer.scene;
var camera = scene.camera;
var globe = scene.globe;
var statistics = Cesium.RequestScheduler.statistics;

var cesiumTerrainProviderMeshes = new Cesium.CesiumTerrainProvider({
    url : 'https://assets.agi.com/stk-terrain/v1/tilesets/world/tiles',
    requestWaterMask : true,
    requestVertexNormals : true
});
viewer.terrainProvider = cesiumTerrainProviderMeshes;

var startTime;
var flightComplete;

function startTest() {
    flightComplete = false;
    statistics.numberOfActiveRequestsEver = 0;
    startTime = window.performance.now();
    window.setTimeout(function() {
        scene.postRender.addEventListener(viewReady);
    }, 500);
}
                      
function viewReady(scene, time) {
    var ready = globe._surface.tileProvider.ready && globe._surface._tileLoadQueueHigh.length === 0 && globe._surface._tileLoadQueueMedium.length === 0 && globe._surface._tileLoadQueueLow.length === 0 && globe._surface._debug.tilesWaitingForChildren === 0;
    if (flightComplete && ready) {
        var endTime = window.performance.now();
        var duration = endTime - startTime;
        console.log((duration / 1000).toFixed(2) + ' seconds');
        console.log(statistics.numberOfActiveRequestsEver + ' requests');
        scene.postRender.removeEventListener(viewReady);
    }
}

function goToEverestHorizontal() {
    camera.position = new Cesium.Cartesian3(302950.1757410969, 5637093.359233209, 2976894.491577989);
    camera.direction = new Cesium.Cartesian3(-0.9648960658153797, -0.24110066659365145, -0.10414437451009724);
    camera.right = new Cesium.Cartesian3(-0.02152846103178338, 0.46781654381873394, -0.8835633574877908);
    camera.up = new Cesium.Cartesian3(-0.26174817580950865, 0.8503047394302772, 0.456584868959543);
    flightComplete = true;
}

function goToEverestTopDown() {
    camera.position = new Cesium.Cartesian3(301989.1870802739, 5637745.915399717, 2977153.0443453398);
    camera.direction = new Cesium.Cartesian3(0.021398841015326783, -0.8909524564021135, -0.45359211857597476);
    camera.right = new Cesium.Cartesian3(0.21237352569072232, 0.4473925820246778, -0.8687562161705573);
    camera.up = new Cesium.Cartesian3(-0.9769542339275126, 0.07774058129659328, -0.19878839712310903);
    flightComplete = true;
}

function goToEverest45Degrees() {
    camera.position = new Cesium.Cartesian3(302760.41072832496, 5637092.977453635, 2977284.6758398763);
    camera.direction = new Cesium.Cartesian3(-0.7254568510163212, -0.3330925403210976, -0.6022970337764594);
    camera.right = new Cesium.Cartesian3(0.4750641658993092, 0.39087207931336604, -0.7883736778277414);
    camera.up = new Cesium.Cartesian3(-0.49802248502640617, 0.8580608237157107, 0.12532049797395203);
    flightComplete = true;
}

function zoomToEverest() {
    var position = new Cesium.Cartesian3(302955.90876054496, 5639614.4908250235, 2981096.1048591887);
    camera.flyTo({
        destination : position,
        easingFunction : Cesium.EasingFunction.QUADRATIC_OUT,
        complete : function() {
            flightComplete = true;
        }
    });
}

function panAroundEverest() {
    camera.position = new Cesium.Cartesian3(302950.1757410969, 5637093.359233209, 2976894.491577989);
    camera.direction = new Cesium.Cartesian3(-0.9648960658153797, -0.24110066659365145, -0.10414437451009724);
    camera.right = new Cesium.Cartesian3(-0.02152846103178338, 0.46781654381873394, -0.8835633574877908);
    camera.up = new Cesium.Cartesian3(-0.26174817580950865, 0.8503047394302772, 0.456584868959543);

    var startCartographic = Cesium.Cartographic.fromCartesian(camera.position);
    var longitude = startCartographic.longitude;
    var endLongitude = longitude + 0.01;
    var latitude = startCartographic.latitude;
    var height = startCartographic.height;
    var startTime = window.performance.now();
    var removeCallback = scene.preRender.addEventListener(function(scene, time) {
        var endTime = window.performance.now();
        var delta = endTime - startTime;
        startTime = endTime;
        longitude += delta * 0.000001;
        if (longitude >= endLongitude) {
            flightComplete = true;
            removeCallback();
        }
        camera.position = Cesium.Cartesian3.fromRadians(longitude, latitude, height);
    });
}

Sandcastle.addToolbarButton('Timer Static Horizontal', function() {
    startTest();
    goToEverestHorizontal();
});

Sandcastle.addToolbarButton('Timer Static Top Down', function() {
    startTest();
    goToEverestTopDown();
});

Sandcastle.addToolbarButton('Timer Static 45 degrees', function() {
    startTest();
    goToEverest45Degrees();
});

Sandcastle.addToolbarButton('Timer Zoom', function() {
    startTest();
    zoomToEverest();
});

Sandcastle.addToolbarButton('Timer Pan', function() {
    startTest();
    panAroundEverest();
});

Sandcastle.addToolbarButton('Save camera', function() {
    var cameraString = 'camera.position = new Cesium.Cartesian3(' + camera.positionWC.x + ', ' + camera.positionWC.y + ', ' + camera.positionWC.z + ');\n'+
                       'camera.direction = new Cesium.Cartesian3(' + camera.directionWC.x + ', ' + camera.directionWC.y + ', ' + camera.directionWC.z + ');\n'+
                       'camera.right = new Cesium.Cartesian3(' + camera.rightWC.x + ', ' + camera.rightWC.y + ', ' + camera.rightWC.z + ');\n'+
                       'camera.up = new Cesium.Cartesian3(' + camera.upWC.x + ', ' + camera.upWC.y + ', ' + camera.upWC.z + ');\n';
    console.log(cameraString);
});

[lilleyse]

Sandcastle link: https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/?src=Hello%20World.html&label=Showcases&gist=c85b4921c7979df76e4934ef2eaecbcd

[hpinkos]

@lilleyse do you think this can be merged in or do we still need more testing?

[lilleyse]

I want to see what @pjcozzi thinks about the numbers, but since it sounds like a better solution would require a lot more work that no one may be able to get too soon, we should just merge this now.

[denverpierce]

I ran some of the sandcastle tests @lilleyse developed and came up with similar numbers. There was a lot of variability in the scenarios based on initial view. Master was generally higher, but missingTiles came out ahead some of the time as well. Overall pretty close.

[kring]

A few thoughts on this...

@pjcozzi asked if we can use the min/max heights rather than computing them from the vertices. First, for actual loaded (not upsampled) tiles, that won't help. It might be a good idea anyway, but it won't fix the problem we see here. STK Terrain Server, at least, tends to preserve the min and max vertices because of the way its simplification algorithm works, so using the min/max computed from vertices should be the same in all but the most pathological cases.

What about using the min/max heights for upsampled tiles, rather than computing them from upsampled vertices? Well, that'd be great! But STK Terrain Server currently doesn't provide access to min/max heights without loading tile data. This is something I'd really like to fix; it would also allow us to skip terrain levels during load without upsampling and risking loading the wrong stuff. If we want to improve terrian load performance, I think this is the number 1 way to do it. But it's a change to STK Terrain Server, and we also need a fallback for terrain sources that don't have this new feature.

In the absence of actual min/max knowledge for an upsampled tile, the next best thing we can do is use the min/max of the tile we're upsampling from for all upsampled tiles. I haven't tried it, but I'm confident this would fix #4894. But would it be better?

I'm pretty sure it would be worse. Using min/max heights from large, low-detail details would, in many cases, make the bounding volumes for small upsampled tiles quite huge. A big bounding volume means:

1. The tiles are more likely to be considered visible and rendered.
2. They're more likely to be considered close to the viewer and refined.
3. They're more likely to be loaded with high priority (this is really a consequence of the above 2).

These things are much worse than the effect of this PR. Effectively, we can think of this PR as "optimistically assume that if an upsampled tile's vertices aren't visible, then the real vertices once loaded won't be either, but load the tile with low priority so that we eventually verify that assumption." In contrast, by using a worst-case bounding volume for upsampled tiles, we're saying, "pessimistically assume that upsampled tiles are visible if there's any chance that the real tiles, once loaded, will be, and render and aggressively load them based on that assumption."

We could potentially mitigate some of the negative affects of this approach by having two bounding volumes: one (A) used for culling and load prioritization, which reflects the actual vertices and does not necessarily guarantee spatial coherence, and another (B) used for deciding-whether-to-load that does have proper spatial coherence. In other words, starting with this PR ase a base, we would add a check of the (B) volume prior to adding the tile to the low-priority load queue. We don't need to load tiles that have no chance of being visible, and we could determine that with a pessimistic, spatially-coherent bounding volume.

But having two bounding volumes makes tiles bigger, and its a fair bit of work to implement. I really think the sensible plan here is:

1. merge this PR (preferably a year ago 😆)
2. extend STK Terrain Server to allow access to the BVH without downloading tiles.
3. teach Cesium how to use the extra data in 2 and terrain/imagery loading will be super awesome.

[markerikson]

It's also worth taking into account that there are tile sources besides STK Terrain Server (particularly https://github.com/geo-data/cesium-terrain-builder , which already can generate heightmaps, and just had a PR opened to add quantized-mesh generation).

[jbo023]

I am also in favor of merging this. We are already merged this to our cesium version and used this for the last 10 month. We didn't find any Problems.

[pjcozzi]

@lilleyse thanks for the numbers. @kring thanks for the explanation.

We are cooking up a new terrain dataset with drastically different tiles so I want to rerun the performance tests before making a decision. This will probably a few weeks. The current numbers are not great, but given that this is becoming more of an issue, we would likely go with it for now.

I also wonder if the performance tests should include

• a flight path or some type of camera movement - I know it is hard to get consistent results but this is the common use case.
• what % of tiles downloaded actually needed to be downloaded

CC @tfili who is doing the testing.

[kring]

Ok, your call of course @pjcozzi. I've been using this in all my projects for almost a year now, so it doesn''t really affect me whether it's merged or not, or when.

By the way, if you're doing more performance testing, you might consider looking at "time to fully-detailed scene", even though that's hard to measure automatically. This PR will undoubtedly result in more tiles loaded in total, but because they should all be loaded after the improtant tiles, the time to fully detailed scene should be the same. Plus with this PR it'll actually load all the tiles it needs for a fully-detailed scene, which is nice. 😉

[hpinkos]

Without this fix I was constantly seeing views like this:

We need to fix that one way or another.

[lilleyse]

a flight path or some type of camera movement - I know it is hard to get consistent results but this is the common use case.

The zoom and pan modes in the Sandcastle code involve camera movement.

By the way, if you're doing more performance testing, you might consider looking at "time to fully-detailed scene"

I believe that's what the Sandcastle code is measuring (but maybe it's not?):
var ready = globe._surface.tileProvider.ready && globe._surface._tileLoadQueueHigh.length === 0 && globe._surface._tileLoadQueueMedium.length === 0 && globe._surface._tileLoadQueueLow.length === 0 && globe._surface._debug.tilesWaitingForChildren === 0;
It might also be useful to measure each of these groups separately.

[mramato]

I think @lilleyse's numbers are a little off because it doesn't remove imagery tiles from the scenario and they heavily affect the numbers, it also isn't using the built version of Cesium (which can skew performance). We also need to make sure effective screen resolution is the same across runs.

I'm going to throw together a separate non-sandcastle benchmark that uses the combined/minified Cesium and get the results directly from the debugger instead of trying to count requests with Cesium itself. We really should create our own ChromeHeadless app that we can use to generate repeatable performance runs across branches, but that's at the bottom of a very long wishlist.

[kring]

I believe that's what the Sandcastle code is measuring

No, that code will wait until all tiles are loaded. To measure what I called "time to fully-detailed scene" you'll need a sharp eye and a stopwatch. When all tiles appear to be loaded and the scene stops changing, hit stop. You'll probably need to do this a few times before you can stop at the right time reliably. More tiles will be loaded after you hit stop, but they won't affect the appearance of the scene. I guess maybe you could do this automatically by capturing screenshots periodically and comparing. 😬

So yeah, total PITA to measure, and probably not worth it, but my claim is that the times before and after this PR would be similar. Except that before this PR some scenes (like the one in @hpinkos's screenshot above) will never achieve full detail, so this PR will result in infinite speedup. 👍

[ggetz]

What's the status of this getting merged for the release? Is there still performance testing to be done?

[hpinkos]

I'm 100% in favor of merging this in for 1.42. Both @lilleyse and @mramato ran performance tests and the results seem to show that there is not a significant increase in number of requests or decrease in performance.

Plus it fixes this, so like @kring said, it's an infinite speed up since all the tiles actually load:

@pjcozzi do you have any problems with merging this in for 1.42, or would you like to see more performance numbers first?

[pjcozzi]

Please hold on merging - we are still running tests - this can significantly increase requests and we are better quantifying it.

[pjcozzi]

Update from some offline discussion: the new terrain dataset that we are creating has larger tiles, which has a pretty bad impact on this change for some cases, e.g., ~25% more tiles. We are going to build a tileset with smaller tiles, which should make this change have less of an impact, but still more of an impact than with the current tileset.

We also hope to be able to optimize terrain rendering in general along the lines of what @kring said above, #4969 (comment), and any other optimizations in the not-too-distant future.

So it is OK to go with this PR as a stopgap in the short term.

Totally fine with me to merge any time for the upcoming 1.42 release.

[hpinkos]

Sounds good @pjcozzi

Thanks @kring! And thanks for testing the change @denverpierce @lilleyse @mramato!