Use CustomGeometrySource for quests and overlays

[Helium314]

@westnordost I'll try this in a separate branch, so both versions are easier to compare.

Currently I'm not sure how to best arrange QuestPinsManager and PinsMapComponent, but haven't had much time to think it through.

Overlays are still missing, but should work very similar to quests.

[westnordost]

Not having looked at the code yet, this is hopefully helpful:

• The *Component is basically the View / ViewController, i.e. this code could also be in the fragment, it is only in the separate class to separate the different things that are being set on the view in the fragment

• A *CustomGeometrySource is basically the ViewModel, i.e. it delivers the data in the format that is convenient for the View and is otherwise passive, i.e. it only responds to queries. So the way it should be structured IMO is that the fragment queries the data from the source and then puts the data into the *Component. This means that Pin and StyledElement to Feature functions would need to be moved out of the Components but since they are still strongly coupled, maybe leave them in the same file as static functions?

• The *Manager is currently doing what the source but pushes the data onto the component actively. I think the composition is better / cleaner with a passive *Source class

[Helium314]

For overlays it looks a little glitchy. On zooming I sometimes get short flashes of a previously enabled overlay, before the data is reloaded. (edit: it's not related to loading / creating overlay data, but seems to be a MapLibre thing)
And it looks like there are issues near tile edges:

This might be because the glitchy way doesn't have any nodes one of the tiles, and thus isn't returned by getMapDataWithGeometry.

[Helium314]

Getting the CustomGeometrySource to work properly for overlays might be tricky, at least I don't see a simple way of returning the partially white way when querying data for the bottom left tile only...
(besides modifying the cache, but iirc the current behavior is wanted)

[Helium314]

For overlays it looks a little glitchy. On zooming I sometimes get short flashes of a previously enabled overlay, before the data is reloaded. (edit: it's not related to loading / creating overlay data, but seems to be a MapLibre thing)

Also happens when disabling an overlay, and then zooming, which is mildly annoying.

I'm now checking whether there are issues with quest pins if a long way has pins in two different tiles.

[westnordost]

For vector tile sources it is possible to define from which to which zoom level they are available. If this is possible for the quest pin source and overlay source too, it could be made so that they are only available at one level. This would at least lead to that at the edges of tiles, it does not flash but stays consistently either rendered, or not rendered.

[Helium314]

I'm now checking whether there are issues with quest pins if a long way has pins in two different tiles.

There are cases where pins disappear when zooming in (I guess because pin is in same tile as quest (center) on z14, but not on z15.
When only loading z14 tiles (withMaxZoom(14)), this is better. Still there are cases where a pin at the end of a road never shows up, while it's displayed normally when using the GeoJsonSource.

[Helium314]

For vector tile sources it is possible to define from which to which zoom level they are available. If this is possible for the quest pin source and overlay source too, it could be made so that they are only available at one level. This would at least lead to that at the edges of tiles, it does not flash but stays consistently either rendered, or not rendered.

Is this what you mean?

        options = CustomGeometrySourceOptions()
            .withMaxZoom(TILES_ZOOM)
            .withMinZoom(TILES_ZOOM),

That's the case for overlays.

[westnordost]

Is this what you mean?

Yes

[Helium314]

I set the zoom for quest pins to 14 now, so at least pins now don't disappear on zoom in.
But long ways still often have fewer pins than with a GeoJsonSource.

[westnordost]

How come you use 14 rather than TILE_ZOOM?

[Helium314]

When I use TILES_ZOOM, the quests are only displayed at z16 or higher (there is no "min" equivalent to maxOverscaleFactorForParentTiles).

[westnordost]

oh, too bad

[Helium314]

I noticed withClip, and it sounds like when setting to false it should fix the issue of certain tiles not containing a node of the way passing through.
Only issue is that it already defaults to false, and explicitly setting it doesn't change anything.

Btw with CustomGeometrySource overlays noticeably load tile by tile, instead of for the whole visible area at once, which is not ideal for performance (at least when data isn't cached).

[Helium314]

I'm not sure whether the CustomGeometrySource can be nicely used for overlays due to the glitches.
For quests it's mostly fine, except for ways with quest pins in different tiles. Here usually users would not notice the missing pins, probably except for power users. Maybe there are also workarounds... like putting the pins in a SpatialCache?

[Helium314]

I opened maplibre/maplibre-native#2262, maybe the issue in the sreenshot is just a bug. But if not, we should use the CustomGeometrySource for overlays.

If you want I can add it for quests only, but we'll still have the sometimes missing pins for long ways.

[westnordost]

Are you sure that this is a bug in maplibre-native? To me, it looks like Maplibre is just querying the data tile-by-tile and since our CustomGeometrySource will just not return any line for the blue road on the lower-left tile, it is naturally not part of the tile.

withClip sounds like it is unrelated to that. E.g. in our case, for the bbox query done for the lower left tile, there is not even a geometry in the data in the first place, so nothing can be clipped.

[Helium314]

withClip sounds like it is unrelated to that. E.g. in our case, for the bbox query done for the lower left tile, there is not even a geometry in the data in the first place, so nothing can be clipped.

But data exists in neighboring tiles and is clipped at tile boundaries, which in my understanding it should not.

[Helium314]

Are you sure that this is a bug in maplibre-native?

No, that's why in the linked issue I wrote "Is this a bug, or am I misunderstanding what withClip is doing?"

[westnordost]

But data exists in neighboring tiles and is clipped at tile boundaries, which in my understanding it should not.

Right, understood, you set withClip to false and expect geometries that overlay to the next tile to be in doubt duplicate.

[Helium314]

That's what I was hoping for

[westnordost]

isVolatile=false here and for overlays could potentially improve performance, as documentation reads like other tiles will not be re-fetched when one tile changes. Worth trying it out, at least.

[Helium314]

I had planned some performance test possibly using different settings, and comparison with GeoJsonSource, but if the issue in the screenshot can't be solved I don't need to bother...

[westnordost]

Mh, yeah, I mean, it could be solved anyway on our side, by having the bbox call return all the elements whose geometry intersects with the bbox, but that would require a lot of refactoring, especially also in the cache, plus, there were good reasons for why we changed it to the current behavior.

Another thing I noticed is that it is possible to pass null for the GeometryTileProvider in a CustomGeometrySource. Any update would then need to be triggered manually with CustomGeometrySource::setTileData. This would be pretty close to the solution we have now with the following differences:

• more complex, as we 1. need to calculate the tiles from the bbox in view and 2. slice up the data of that bbox manually into the tiles bboxes (because withClip doesn't work the way we thought it would) before repeatedly calling setTileData for the different tiles
• when data is updated, only the tile(s) in which the changed element(s) is/are contained need to be updated

Whether this is a performance improvement at all hinges on whether re-setting all data vs re-setting it only in one tile (on each edit) does actually make a difference, performance-wise. (Who knows, maybe internally, the whole data is pushed to the GPU anyway).

For this reason, I think it is worthwhile to compare the performance of the current approach with CustomGeometrySource even if there is this rendering issue from the screenshot right now.

[Helium314]

when data is updated, only the tile(s) in which the changed element(s) is/are contained need to be updated

But can't we achieve this by only invalidating the region covered by the update?

[Helium314]

Another thing I noticed is that it is possible to pass null for the GeometryTileProvider in a CustomGeometrySource

While technically it's possible, this is useless as it results in an NPE.

[Helium314]

I did a very basic performance comparison. Note that this does not include MapLibre internal stuff, this appears to happen on a different thread (was blocking on Tangram).

CustomGeometrySource

• enabling of the overlay (nothing in cache), 4 tiles: 4 updates running in parallel, last one finished after 2.1s. There is a clear tile-by-tile buildup visible.
• disable and enable the overlay: 4 updates in parallel, last one finished after 0.1s
• move to the next tiles: 2 updates in parallel, last one finished after 0.5s. Didn't notice tile-by-tile buildup.

GeoJsonSource

• enabling of the overlay (nothing in cache), 4 tiles: finished after 1.6s
• disable and enable the overlay: finished aftr 0.2s
• move to the next tiles: finished after 0.4s

Fetching the map data for more than a single tile is faster with GeoJsonSource, but that's expected.
My feeling is that also the visual response (i.e. including MapLibre internal data updates) is faster with GeoJsonSource. For disabling and enabling the overlay I didn't notice a clear difference.
The tile-by-tile buildup of CustomGeometrySource doesn't look appealing, but I think it would be acceptable.

[westnordost]

What's also relevant is the time for a single edit to one element to go through, as this is the (one) point where CustomGeometrySource should be much faster (as only one tile should be updated, rather than all that is in view and more)

[Helium314]

Test with 64 tiles in cache after scrolling around, at lowest possible zoom where overlays are still displayed, in a well mapped city center (probably worst possible case for GeoJsonSource).

For GeoJsonSource the update takes 60 ms, updated overlay is visible after estimated 0.2 to 0.4 s.
For CustomGeometrySource the update (calculating bbox and fetching map date) takes ~100 ms (because styled elements are not cached), updated overlay is visible after estimated 0.1 to 0.2 s (definitely faster than with GeoJsonSource).

[westnordost]

The update after the user edited something is the most important statistic, IMO, because it is done a lot and is part of the user workflow.

In tangram-es, the update of the pins / overlay was quite noticeable because I think something was done on the UI thread, leading to jerky camera-zoom-back animation, and there was a flicker visible.
In MapLibre, I noticed neither, so while up to 300ms (=usual animation time in Android) is in a range of what may be noticed, your measurements and the fact that MapLibre does not flicker when updating the geojson source make me think that we don't really need CustomGeometrySource. It would only be worthwhile if this method could be deployed without issues and leads to less code on our side.

What's your opinion on this?

[Helium314]

I think a CustomGeometrySource would be really nice to have, mainly because we could kick out a lot of code (also I'd like to do a performance comparison on S4 Mini, but...).

With the current issues with CustomGeometrySource and with the current code working well with GeoJsonSource, I think we could just leave it as it is for now. GeoJsonSource simply works well enough.

Switching to CustomGeometrySource can still be done at a later point, and I think it's worth trying again (also depending on whether the withClip thing really is a bug and gets fixed).

[Helium314]

I'll close this as it's not necessary for reasonable performance. We can still have a look at it later (definitiely after maplibre/maplibre-native#2262)