Terrain / imagery rendering overhaul

[kring]

This is not ready to merge yet, but I'm opening it now to track remaining problems and ideas, some of which will go on the roadmap rather than being implemented prior to merging this branch.

Required:

• I've modifed the upsample worker to start immediately rather than the first time it's needed, because in this branch it's needed much later and it's a long wait to start it up once we do realize it's needed. Not sure if this is ok. Also I've done that in such a way that it throws an exception.
• Fill mesh / vertex array should be freed once the tile is loaded.
• Fill tile generation allocates more memory than necessary. The worst offender is that it creates two copies of the vertices for... reasons.
• Test failures.
• There's debug code around (sometimes commented out).
• What should the default setting of QuadtreePrimitive.preloadAncestors be?
• Test with the new BVH data from the modified server and make sure that it's still working as expected and providing a benefit. Tested it and found it provided no benefit, removed it.
• Test with terrain exaggeration, with/without water mask, and with all the combinations of possible vertex data, i.e. with/without normals, Web Mercator texture coordinate.
• Code duplication between real loading process and fill generation (e.g. around imagery layers)
• Entire globe flickers when adding a new imagery layer.

Broken Sandcastle examples:

• 3D Tiles feature picking crashes trying to use destroyed vertex array after moving around a bit. Caused by draw command referencing fill tile vertex array but real vertex array has already been loaded and fill freed.
• Clamp to terrain initially loads a black scene.

Nice to have:

• The fill tile generation is buggy in some cases, which can lead to cracks between adjacent fill tiles and (rarely) even flickering as a fill tile switches its geometry back and forth between frames. Mostly it looks ok though.
• For very coarse tiles (e.g. levels 0-4), fill tiles could use some more vertices in order to match the globe curvature. There's a fairly obvious artifact from this when spinning the globe quickly from a global view.
• We're a little too aggressive in waiting for tiles to be ready before rendering them, which can lead to slow imagery layers blocking all refinement.

Future:

• We could reduce the need for fill tiles by upsampling culled tiles (but not loading their terrain or imagery) so they're ready to render when the camera moves and they're no longer culled. We'd only want to do this when the parent is loaded, though, which it really only will be (eventually) if QuadtreePrimitive.preloadAncestors is true.
• Currently fill tiles match adjacent vertices at their edges and have just one vertex in the midle. We could improve their quality by also copying vertices that fall inside the tile from the nearest ancestor, but then we'd need a much more complicated tessellation, like an incremental Delaunay. Using the existing triangles when all three ancestor vertices are fall inside the tile will help. Probably not worth the CPU time, but it would be interesting to investigate.

[mramato]

Awesome, @kring, thanks. Do you have planned changes left on your end, or are you just waiting for more feedback?

Does anyone else have any feedback here? I'm going to take another look now, but barring any issues I think we want to get this merged today or tomorrow to give it as much time in master as possible. Thanks!

[mramato]

@kring I can confirm your update fixed the problem I was seeing with loading, thanks again.

Now I'm seeing some unexpected behavior when setting RequestScheduler.throttleRequests = false

In 1.54, if throttleRequests is false, RequestScheduler.statistics.numberOfActiveRequestsEver is always equal to RequestScheduler.statistics.numberOfAttemptedRequests, since no requests are ever cancelled. In this branch, numberOfAttemptedRequests is still slightly larger (though not as large as when throttling is enabled).

As expected, numberOfCancelledRequests remains at 0 both in master and this branch, so I believe throttling is indeed disabled. Perhaps this is just an accounting issue somewhere?

[mramato]

Just noticed cancelled requests is 0 even with throttling on, so something changed under the hood that maybe I'm just not aware of?

[mramato]

Here are some rough numbers comparing 1.54 and this branch across several different views. Bandwidth/data savings are just as impressive as runtime loading performance. Once this is merged into master, I'll probably re-run these tests again with some additional vetting just to make sure everything is on the up-and-up.

• Numbers are from "best of 3" runs.
• Canvas size of 1920x1080
• Used combined but (not minified) Cesium (next time I'll use minified)
• Default CesiumWidget with Sentinel-2 Imagery and Cesium World Terrain (no water/lighting) from ion.
• Request throttling enabled (the default)
• All tiles from the local cache (to remove network overhead).
• Tests ran in Chrome 72.0.3626.96 (Official Build) (64-bit) on Ubuntu 18.04.
• Times are from call to camera.setView until scene.globe.tilesLoaded gets set to true.

Also note, since I used locally cached tiles, this is the "best" case for master. With a clear cache I would expect master would fair even worse since it has to request more tiles.

Overall, this branch kicks ass, but the savings can be vastly different from view to view. With the views I used, this branch seems to load the tests 1.4x to 2.3x times faster and uses ~33% less data/bandwidth overall. There might be some better views to test with as well.

From the intangibles, this branch also loads things in a way that makes it appear even faster to end users, no more back to front waiting for the world to pop-in. We can't discount user impression when talking about the hard numbers.

My testing methodology or assumptions might get some tweaks, since I have some open questions to Kevin about RequestStatistics differences in this branch.

1.54

Overall: 19.7 MB of tiles loaded. (Via Chrome Network tab)

Grand canyon horizon: 4818 ms
Number of server requests: 352
Number of attempted requests: 1386
Number of cancelled requests: 755
Percentage of throttled requests: 74.60

Everest: 5034 ms
Number of server requests: 544
Number of attempted requests: 4269
Number of cancelled requests: 1734
Percentage of throttled requests: 87.25

AGI top-down: 2787 ms
Number of server requests: 238
Number of attempted requests: 770
Number of cancelled requests: 211
Percentage of throttled requests: 69.09

fill-tiles

Overall: 13.3 MB of tiles loaded. (Via Chrome Network tab)

Grand canyon horizon: 3391 ms
Number of server requests: 210
Number of attempted requests: 635
Number of cancelled requests: 0
Percentage of throttled requests: 66.92

Everest: 3850 ms
Number of server requests: 369
Number of attempted requests: 635
Number of cancelled requests: 0
Percentage of throttled requests: 41.88

AGI top-down: 1166ms
Number of server requests: 98
Number of attempted requests: 132
Number of cancelled requests: 0
Percentage of throttled requests: 25.75

[mramato]

Played with a bunch of Sandcastle examples and didn't run into any other problems, so this PR gets a 👍 from me (outside of that one question about request statistics).

[hpinkos]

Should these just be lessThan, lessThanOrEqualTo, etc? We don't call the function leftIsEqualToRight

[kring]

I thought my names made the use clearer. Order doesn't matter in equals, so being precise is less important there. But if you all feel strongly about it I'm happy to rename them.

[OmarShehata]

I do like this because otherwise I'd have to go look up the documentation whenever using this to check which way it's comparing them. This way it's self-documenting.

[hpinkos]

Right than left is standard, so this seems a little pedantic. In Check we just call these functions greaterThan lessThan etc https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/Check.js#L89

[OmarShehata]

That's fair. Sounds like we should change it for consistency here then @kring .

[tfili]

I honestly don't care either way, but Check isn't public so I wouldn't necessarily use that as the decision maker.

[mramato]

I thought we had functions like this all over the code, but to might surprise, JulianDate is the only place I could find them (outside of Check). While I'm on board with renaming them for consistency, I don't want it to hold up this PR which really needs time to stew in master.

If someone wants to open a separate PR to clean things up before the next release, I'll review and merge it ASAP.

[kring]

@mramato yeah RequestScheduler isn't being used to throttle requests any more in this branch. I mean, it does still drop requests where there are already too many in flight to a particular hostname, but it no longer does any canceling or reordering for terrain or imagery. I removed it because:

• The mechanism was really expensive. There was a seprate function, with its own closure, created per tile 😬. Plus all the calling distance computation functions and sorting every frame.
• It produced bad results. RequestScheduler only knew how to sort by distance. The terrain engine uses an improved prioritization system that slightly favors tiles near the center of the screen. It also has low/medium/high priority queues. Ignoring the queues and sorting by distance causes us to load a bunch of really unimportant low priority tiles earlier than the important ones, just because they happen to be closer.

More discussion of this from back in June:
#6243 (comment)

[kring]

Oh and yeah I think everything has been addressed up to this point.

[mramato]

@mramato yeah RequestScheduler isn't being used to throttle requests any more in this branch.

Fine by me, I actually never liked what we had anyway. @lilleyse @loshjawrence @OmarShehata were you already aware of this and are you okay with it? Any concerns that would prevent us from merging this?

Assuming no one else has anything, I'm fine with merging this today.

[loshjawrence]

The last time I set throttle to false it totally trashed streaming performance for camera-moving scenarios on 3dtiles. I guess I can copy the Request and RequestScheduler files over and remeasure some things.

[mramato]

The last time I set throttle to false it totally trashed streaming performance for camera-moving scenarios on 3dtiles. I guess I can copy the Request and RequestScheduler files over and remeasure some things.

Thanks. Just let us know ASAP if there are any actionable concerns for this PR going into master, while I'm sure it might affect some of the branches you have in-flight, unless there's some sort of architectural showstopper, that shouldn't hold up this PR.

[loshjawrence]

Using the changes in Request and RequestScheduler from this PR on our staging branch looks harmless from my testing. Any diff's seem to be within the noise.

Orig:
attempted: 32962 cancelled: 2793 flight loads: 277 site time: 15.28
attempted: 26767 cancelled: 3600 cancelled active: 35 flight loads: 473 site time: 3.54
attempted: 24293 cancelled: 4422 cancelled active: 138 flight loads: 505 site time: 5.11
Total Time: 23.94

Ring's changes:
attempted: 33636 cancelled: 2881 flight loads: 277 site time: 15.28
attempted: 26695 cancelled: 3760 cancelled active: 32 flight loads: 461 site time: 3.66
attempted: 24370 cancelled: 4573 cancelled active: 136 flight loads: 515 site time: 5.25
Total Time: 24.20

Ready to merge

[OmarShehata]

@mramato I think this gets a ✔️ from me as well. From the earlier discussion @kring pointed to about the request scheduler it sounds inline with the direction we're going.

[lilleyse]

@mramato Good with me as well.

[mramato]

Since this branch was a bit out of date, I merged in master. Assuming it passes without any surprises, I'll merge this up.

[mramato]

@kring are there any new GitHub issues we should write up (such as those unchecked boxes?)

Other than that, I'm going to merge. I encourage everyone to continue to put things through their paces in master and also report any unusual problems you might find over the next few weeks.

[mramato]

While writing up a forum post, I was amused by the fact that this overhaul is going to be released exactly 6 years after initial terrain support was added (3/1/13 in version b14): https://github.com/AnalyticalGraphicsInc/cesium/blob/master/CHANGES.md#b14---2013-03-01

Some guy named @kring did all the work on it back then. How times have changed.