Refactor TileDataMonitors

[kkaefer]

No description provided.

[kkaefer]

This solves #123 🎉

[kkaefer]

/cc @tmpsantos @jfirebaugh

[kkaefer]

Remaining tasks:

• Add C++ based (not node!) render tests that load parent tiles

[tmpsantos]

This really looks great. I have a few comments:

Seems to me that this might potentially flood the cache with requests when browsing offline. On a slow file system the optional requests could hurt the response latency of the required requests. We don't need to solve this on this PR but we should probably implement some sort of request priority.

I'm wondering if the cache knowing that it has already a region cached at certain zoom level, it could return it immediately, but we would still keep going through the pyramid.

There is one corner case that I'm not sure it is covered, but if we have sources with different max zoom levels (A, 20 & B, 15), if we are at the zoom 20, we are going to try to get an optional tile at zoom 15 for B, but IMO this one should be a required even if overscaled.

[kkaefer]

Seems to me that this might potentially flood the cache with requests when browsing offline.

The offline requests are made one after another, so there aren't requests for all tiles in the pyramid at the same time; new requests are only generated when the previous one didn't yield a result.

I'm wondering if the cache knowing that it has already a region cached at certain zoom level, it could return it immediately, but we would still keep going through the pyramid.

I think the implementation cost of something like that isn't worth the minimal gains from not doing a few intermediate SQLite queries.

There is one corner case that I'm not sure it is covered, but if we have sources with different max zoom levels (A, 20 & B, 15), if we are at the zoom 20, we are going to try to get an optional tile at zoom 15 for B, but IMO this one should be a required even if overscaled.

No, we calculate what tiles to load per source, so if B has a maxzoom of 15, the ideal tiles are going to be z15 as well, so they're loaded as required.

[kkaefer]

This is pretty hard to test: Our current way of testing is to render images, but this feature is only supposed to work when we're rendering in continuous mode. @jfirebaugh any ideas?

[jfirebaugh]

I'm not sure about some of these renames:

• *TileMonitor ⇢ *TileSource -- this overloads the meaning of the term "source". I get that there's a parallel with FileSource, but we already have a "vector tile source" concept from the style specification, and VectorTileSource isn't that. This will become more of an issue with the next round of runtime style API changes, which will introduce subclasses of Source, including one named VectorSource.

  What is the rationale for moving away from `*TileMonitor? I like moving this to its own header, but can we keep the existing name?

• onPlacementRedone ⇢ onNeedsRepaint. One of the purposes of the observer pattern is to decouple the source of a change from the effect, so that thing being observed doesn't need to know details about what happens as the result of a notification. "Resource loaded" and "placement redone" are the notifications; "repainting" is one specific thing that might need to happen as a result. So it seems to me that the existing names are better.

[jfirebaugh]

I spent today digesting the changes here. Overall this looks like a good direction. I have some ideas about how to restructure the inheritance and class relationships, but I don't think they need to be a blocker. I have been gearing up to do a deep dive on refactoring Source-related code in support of the runtime styling API anyway, and I should do that on top of these changes, or there will be big conflicts.

I don't have any great ideas about testing. I agree that this is an area that doesn't fit into our rendering test pipeline nicely, and it's also difficult to unit test because of the complex object dependencies. I have a feeling a deep-dive refactor would reveal ways to make this area of the code easier to test as well.

What I suggest as next actions are:

• Do as much manual testing as you can to feel more comfortable with the changes.
• Wait for the iOS 3.3.0 release, then merge. Waiting for the iOS release branch will give us more breathing room to work out any kinks once it hits master.
• Follow up with step-by-step refactoring in support of/preparation for runtime styling of sources and better testability.

[jfirebaugh]

Check out 5143-refactor-tile-source for my refactor of your refactor. I introduced a complete set of TileData subclasses, merged all TileSources except FileBasedTileSource into those subclasses, and then merged FileBasedTileSource and TileSource. I think the class relationships are cleaner:

• One TileData subclass for each source type
• GeometryTileData has the code shared between vector, GeoJSON, and annotation sources
• TileSource<T> has the code shared between raster and vector sources

[kkaefer]

*TileMonitor ⇢ *TileSource

I guess this is mostly a matter of personal preference, and I found the term "Monitor" awkward, but we can change it back if it means less ambiguity.

[jfirebaugh]

iOS release branch is active. @kkaefer 🚢 ?