Helium314 / SCEE

OpenStreetMap surveyor app for experienced OSM contributors
GNU General Public License v3.0
114 stars 8 forks source link

Use CustomGeometrySource for quests and overlays #535

Closed Helium314 closed 2 months ago

Helium314 commented 2 months ago

@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 commented 2 months ago

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

Helium314 commented 2 months ago

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: Screenshot_20240414-130815_Street­Complete_Dev This might be because the glitchy way doesn't have any nodes one of the tiles, and thus isn't returned by getMapDataWithGeometry.

Helium314 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Is this what you mean?

Yes

Helium314 commented 2 months ago

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 commented 2 months ago

How come you use 14 rather than TILE_ZOOM?

Helium314 commented 2 months ago

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

westnordost commented 2 months ago

oh, too bad

Helium314 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

I opened https://github.com/maplibre/maplibre-native/issues/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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

That's what I was hoping for

Helium314 commented 2 months ago

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