Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace RasterOverlayTileProvider cache with SharedAssetDepot #987

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

azrogers
Copy link
Contributor

@azrogers azrogers commented Nov 8, 2024

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.

While this PR currently implements the cache on the RasterOverlayTileProvider base class itself, it will only actually be used when the loadTileImageFromUrl method is called, and only QuadtreeRasterOverlayTileProvider-derived classes seem to call this method.

While this PR is in good working order, at least from my testing, it feels like it loads raster overlays slower than it did previously. This isn't a particularly surprising issue to occur after these changes, but it does need to be fixed. I'll try to get to the bottom of this on Monday.

@kring
Copy link
Member

kring commented Nov 12, 2024

@azrogers I took a look at this, and I'm not sure where the performance regression might be coming from. But there's a lot more complexity than I expected, and more than I think is necessary (though it's always possible I'm missing something!).

I think it should be possible to replace _tilesOldToRecent, _tileLookup, and _cachedBytes in QuadtreeRasterOverlayTileProvider with a SharedAssetDepot<LoadedQuadtreeImage, QuadtreeTileID>. You'll have to make LoadedQuadtreeImage into a shared asset (instead of LoadedRasterOverlayImage). But not much else should have to change. In particular, there shouldn't be any breaking API changes for people that developed their own raster overlay types (and our own shouldn't have to change, either).

The trick (maybe!) is to think of getQuadtreeTile as the factory that the SharedAssetDepot uses to turn a QuadtreeTileID into a LoadedQuadtreeImage. mapRasterTilesToGeometryTile calls the asset depot's getOrCreate instead of calling getQuadtreeTile directly.

Let me know if you want to discuss. Or if I'm crazy and have missed something big.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants