Replace RasterOverlayTileProvider cache with SharedAssetDepot

[azrogers]

There is currently a cache in the QuadtreeRasterOverlayTileProvider to hopefully prevent the same images from being downloaded multiple times when they're used by multiple different geometry tiles. There is currently a bug that will result in the overlay provider believing that the cache is full when it's not, making the cache eventually all but useless. As mentioned in #958, the issue for this bug, now that we have the SharedAsset system, it makes sense to just get rid of that old cache code and use a SharedAssetDepot instead. That's what this PR is for.

I previously opened this PR with a different branch in #987, but @kring suggested a simpler solution which this branch implements.

[kring]

Super nitpicky stuff below. The bigger issue is that there are some subtle lifetime / thread safety issues here.

Sorry I didn't think of this when we talked earlier, but having the factory function hold an intrusive pointer to the tile provider won't work because it creates a circular reference. RasterOverlayTileProvider holds _pTileDepot, which has a factory function, while holds a pointer back to the RasterOverlayTileProvider. Because of this cycle, none of these objects will ever be destroyed.

I also don't think it's needed. RasterOverlayTileProvider exclusively owns the asset depot, and never hands a pointer out to anyone externally. So if the RasterOverlayTileProvider is destroyed, the only other references to the shared asset depot that can possibly exist are ones within the shared asset depot itself.

And these shouldn't be a problem because the larger raster overlay system already ensures that the tile provider outlives all of the things that use it. We can count on the fact that a RasterOverlayTileProvider will not be destroyed while one of its requests are in progress. See the comment on _pTileProvider in RasterOverlayTile.h. And the same reasoning should allow the factory to hold a raw pointer to the tile provider as well.

However, it's very important that this raw pointer not be misused. RasterOverlayTileProvider is not a thread safe object in the slightest, so it may only be used from the main thread. There was a comment in the old version to this effect:

// Tile failed to load, try loading the parent tile instead.
// We can only initiate a new tile request from the main thread,
// though.

The comment has been retained in this PR, but the behavior is no longer consistent with the comment. getQuadtreeTile is called from a thenImmediately continuation.

The old code jumped through some hoops to avoid capturing a pointer to a RasterOverlayTileProvider in a lambda that is not allowed to use it. That's the reason for the loadParentTile function. This is a good practice because it not only helps us avoid thread safety issues today, it decreases the chances that someone accidentally introduces one later by using a conveniently-captured this without realizing it's dangerous.

That's probably confusing, so let me know if you want to discuss.

[azrogers]

Besides the nitpicks, am I correct that the solution here is changing pThis back to a raw pointer, and using thenInMainThread rather than thenImmediately? I don't think we can use the solution of creating a lambda for fetching parent tiles to avoid capturing the this pointer here, because it would have to be constructed in the factory lambda itself, and that would defeat the point.

[kring]

changing pThis back to a raw pointer

👍

using thenInMainThread rather than thenImmediately

No, that will have performance implications.

I don't think we can use the solution of creating a lambda for fetching parent tiles to avoid capturing the this pointer here, because it would have to be constructed in the factory lambda itself, and that would defeat the point.

You should be able to create the loadParent lambda in the QuadtreeRasterOverlayTileProvider constructor and capture it in the factory function lambda. Let me know if that doesn't work.

[azrogers]

Taking another look at it, it's actually pretty straightforward to create that lambda in the constructor and capture it in the factory lambda. Made the change @kring

[kring]

I made one small change to loadParentTile. It was overwriting the parent tile subset instead of creating a new LoadedQuadtreeImage and setting the subset on that the way the old implementation was. This led to an assertion when zooming in close to the surface.

With that fixed, this looks great! Merging!