SCEE icon indicating copy to clipboard operation
SCEE copied to clipboard

Maplibre

Open Helium314 opened this issue 2 years ago • 132 comments

For simple comparison with master, and for discussions.

to do

  • crash after reentering after a while, see https://github.com/Helium314/SCEE/pull/516#issuecomment-2099448562
  • quest pins: try replacing quest dots with clustering
  • ~quest pins: replace homebrew solution for quests and overlays with CustomGeometrySource (draft started by @Helium314)~ blocked by at least maplibre-native#2262 and not strictly necessary
  • clear cache directory after upgrade to MapLibre (aka clean old tiles folder)

to do SCEE

  • re-implement aerial imagery style: style json, adding and changing aerial imagery layer
  • (optional:) if quest pin size has considerable impact on performance with MapLibre, a SCEE setting to half the size of quest pins would be conceivable (maybe some sort of ancient-phone-mode)

upstream blockers

  • minor: maplibre-native#2255 - annoying, but of little consequence
  • ~cosmetic: maplibre#2259~ - worked around this issue
  • crash on adreno 3xx GPUs: maplibre-native#2206
  • critical: maplibre#2372 - users MUST see all available data for the places and things overlays to work as intended. Unknown how many devices it affects and/or how often this occurs for these.

old observations below

Issues / Bugs:

  • Error setting property messages on start, possibly related to style json (don't remember seeing them in previous versions)
  • Dashed lines have an awkward effect every time zoom crosses an integer border, maybe play with expression?
  • Offset lines (e.g. cycleway overlay) are not clickable

Performance regarding icons (symbol layers, mostly about quest pins):

  • Mostly fine at the current state, comparable to Tangram
  • Choppy zoom, especially if there are many quests displayed (on S4 mini and A3)
    • likely this is just MapLibre, but in principle it could also be some SC code (listener or whatever)
    • zooming during high CPU load (on UI thread) makes it much worse, e.g. when opening a quest
  • Image (quest pin) size has a considerable effect on performance (try reducing icon size in the layer using a property, instead of when adding the icon bitmap)
  • Clustering improves performance (dots will also be clustered!)
  • Using some sort of ordering for the symbols from a property is (noticeably) slow. Currently the order is set to the order of adding symbols to the source. This requires a sorted list of quest pins, but still it's fast enough. No need for a pre-quest order like in Tangram.

Other things:

  • MapLibre always does some downloads right after start. Possibly glyphs, considering that when you start SC without WiFi there is a message This request was cancelled (https://api.jawg.io/glyphs/Roboto%20Regular%2cNoto%20Regular/0-255.pbf). This is expected for tiles that were being prefetched but are no longer needed for the map to render.. It's not horrible, but downloading the same thing on every start seems like a waste of bandwidth and resources.
    • maybe goes away when properly pre-loading tiles for offline use
  • for deciding which icons can overlap, consider difference between iconIgnorePlacement and iconAllowOverlap
    • iconIgnorePlacement: If true, other symbols can be visible even if they collide with the icon.
    • iconAllowOverlap: If true, the icon will be visible even if it collides with other previously drawn symbols.
  • properties like line width and circle radius don't have a unit, unclear if it's pixels or dp or anything else

Helium314 avatar Feb 26 '24 14:02 Helium314

Nice! I do intend to work with you together on this, this week however probably not because I don't have so much time and first should review and give feedback on some ready-to-review PRs (osmfeatures KMP, osmapi KMP).

Next thing I intend to do on this (unless you do it first) is to remove some more commented-out tangram-functionality that has been replaced by or would need to be implemented differently in maplibre anyway because it's easier to compare in diff view if there are not massive amounts of to-be-deleted comments floating around. I am not confident to delete your research-notes though, not sure what is already implemented and what is still relevant. Also, next thing for me would be to try out if defining the layers in the json comes without issues. If there are no issues, consider defining them in the JSON instead (plus probably write a small build script that merges the background map style json with the streetcomplete layers json) and if not, have a look at what would be the best api for moving the layer style definition into the components.

westnordost avatar Feb 27 '24 11:02 westnordost

I am not confident to delete your research-notes though, not sure what is already implemented and what is still relevant.

I'll go through the comments and try to move them into a comment here (possibly sort of todo list in first post).

plus probably write a small build script that merges the background map style json with the streetcomplete layers json

That should work easily, you probably just need to merge the layers in the json (the Style.Builder uses a json string anyway).

Helium314 avatar Feb 27 '24 12:02 Helium314

Btw the style could use placeholder colors in the json, and replace them with the actual colors before loading the style. This would not require a separate json for night (but on the other hand, you can't conveniently test the style in some other editor)

Helium314 avatar Feb 27 '24 12:02 Helium314

Looks like you made the downloaded area map component work. I set the color to a more bearable transparent black. Not 100% sure, but my impression is that displaying the (not) downloaded area has an effect on performance when zooming.

Helium314 avatar Feb 28 '24 19:02 Helium314

Try make it non-transparent. Does it still have this effect?

westnordost avatar Feb 28 '24 20:02 westnordost

Opacity or color don't have any effect on this.

Helium314 avatar Feb 28 '24 20:02 Helium314

I was told that shaders also work with maplibre gl (but differently than in tangram), so the downloaded area stripes could be implemented that way, too. (Or by tiling a graphic that looks like that.)

westnordost avatar Feb 28 '24 20:02 westnordost

I made a very basic implementation of CustomGeometrySource for quest pins: https://github.com/Helium314/SCEE/compare/maplibre...Helium314:SCEE:custom_geometry_source

It's mostly for testing, but good enough to see how often getFeaturesForBounds is called. I haven't checked anything, it might be better or worse than the current GeoJsonSource or possibly it depends on what the user does...

Helium314 avatar Feb 28 '24 21:02 Helium314

Well, good! I think this is the way to go on the mid term. My priority is to get out of the "we are just testing" phase, i.e. solve all those todos and clean up the code.

For this, I deleted the KtMapController, the MapBoxMap is now used directly. The wrapper does not make sense for MapLibre anymore, any adapted/simplified methods can be also implemented with extension functions. So, for now, I put most of the previous functionality into extension functions, most of it could probably be removed in the next step.

westnordost avatar Mar 02 '24 12:03 westnordost

I'll also pull in the layer definitions into the components because I think it makes sense to have the style definitions close to the source definitions and IMO defining them in code is not that verbose after all. (Defining expressions in code is actually better readable).

westnordost avatar Mar 02 '24 17:03 westnordost

Regarding SelectedPinsMapComponent, I think at least without further changes, this is still necessary. Your current (hacky) solution to hide all pins except of one element:

            pinsLayer?.setFilter(eq(get("element_id"), questKey.elementId.toString()))
            pinsDotLayer?.setFilter(eq(get("element_id"), questKey.elementId.toString()))

The filter is not complete because it would also need to include the quest type and element type. But even if it was, since the PinsMapComponent is also used by the EditHistoryPinsManager and there may also be pins not actually referring to any element, this kind of filtering will not work.

So, for now I will make the SelectedPinsMapComponent take care of this again. To change this, maybe the two managers would need to not use the same component for displaying their data. Not sure what would be the best replacement.

westnordost avatar Mar 03 '24 14:03 westnordost

I've ended up changing, fixing and (probably) breaking more things in the process than what is titled in the git commit.

image

(The -600 lines of code have to come from somewhere, right?) :-)

Anyway, I think I will probably not have time to continue to work on this in the next days, so feel free to familiarize yourself with the changes. Some things are not working properly, but I am not sure if I broke them or they have been like this already anyway:

  • [x] quest pins except the selected one are not hiding when opening one quest
  • [x] line and point features are not highlighted on selection
  • [x] download always claims that the area is too large
  • [x] zoom buttons don't work
  • [x] dashed lines in overlays are dashed with color+black instead of color+transparent
  • [x] left/right strokes in overlays extend towards the center of an intersection, which is fugly (the same issue existed for tangram implementation, which is why "transparent" was rendered as mimicking the background map style)
  • [x] some maplibre warnings/errors that certain layer property definitions may not be expressions (but must be literal values)

Before I move on with more refactor, I will probably want to look at these. I will write here when I intend to do work on any of this, so that we don't come into conflict each other when we work on things at the same time.

westnordost avatar Mar 03 '24 16:03 westnordost

I think some of these issues are also in the top post (frequently updated!), at least the bottom 2 are not new and the highlighting was broken after a commit from yesterday. The zoom button should actually work again after https://github.com/Helium314/SCEE/pull/516/commits/8c9a33156ce4262dbc587375cecba2faf4aad160.

I'll have a look at the current issues

Helium314 avatar Mar 03 '24 19:03 Helium314

The -600 lines of code have to come from somewhere, right?

And we didn't even (yet) remove the style yaml files.

Helium314 avatar Mar 04 '24 18:03 Helium314

I guess that's it for me today. Btw we could already test the next MapLibre release: https://github.com/maplibre/maplibre-native/releases/tag/android-v11.0.0-pre0 [Edit: the downloadable .aar file apparently doesn't contain Feature and related classes, so the app doesn't compile]

Helium314 avatar Mar 04 '24 19:03 Helium314

[Edit: the downloadable .aar file apparently doesn't contain Feature and related classes, so the app doesn't compile]

Better report a bug. If they didn't notice this on releasing the alpha, who knows when they will notice themselves.

And we didn't even (yet) remove the style yaml files.

Oh! Begone!

westnordost avatar Mar 04 '24 22:03 westnordost

Do you think it would make sense to create the map style json in SC instead of copying the json files? If it's slow it could be done only on version upgrade. Advantage would be that we could just take the road colors for the most likely necessary fix for

left/right strokes in overlays extend towards the center of an intersection, which is fugly (the same issue existed for tangram implementation, which is why "transparent" was rendered as mimicking the background map style)

(I might do it anyway for SCEE as it allows customizing map colors)

Helium314 avatar Mar 06 '24 19:03 Helium314

It's probably not slow but the Kotlin code in the linked map style is more of a "my quick script" quality and I don't really want to introduce a layer on top of MapLibre just to have a nicer API for defining styles. The few number of lines of code that needs to be duplicated for the invisible roads to look like the ones in the background map is really not worth it defining the whole style in code in the app, IMO.

westnordost avatar Mar 07 '24 00:03 westnordost

some maplibre warnings/errors that certain layer property definitions may not be expressions (but must be literal values)

Will look at this now.

westnordost avatar Mar 07 '24 16:03 westnordost

Looking into why darken and addTransparency doesn't work now.

westnordost avatar Mar 07 '24 21:03 westnordost

Btw we could already test the next MapLibre release: https://github.com/maplibre/maplibre-native/releases/tag/android-v11.0.0-pre0 [Edit: the downloadable .aar file apparently doesn't contain Feature and related classes, so the app doesn't compile]

The geojson stuff is a transitive dependency. See gradle dependencies

+--- org.maplibre.gl:android-sdk:10.0.2
|    +--- org.maplibre.gl:android-sdk-geojson:5.9.0
|    |    \--- com.google.code.gson:gson:2.8.6
|    +--- com.mapbox.mapboxsdk:mapbox-android-gestures:0.7.0
|    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.7.20 -> 1.9.0 (*)

westnordost avatar Mar 07 '24 23:03 westnordost

left/right strokes in overlays extend towards the center of an intersection, which is fugly (the same issue existed for tangram implementation, which is why "transparent" was rendered as mimicking the background map style)

I will look at this now.

westnordost avatar Mar 08 '24 11:03 westnordost

Wow, that was... a lot of work. In the end, I solved it by just rendering the left and right side behind the road(s) layer. This required to make sure that for roads on bridges, the left-and-right side highlighting is also rendered on top of other roads. I also fixed all the widths, so that, well, it should be like on master now.

westnordost avatar Mar 08 '24 19:03 westnordost

@westnordost could you put some minimla style json on your server? I would like to work on the offline part, see https://github.com/Helium314/SCEE/blob/ae53926512a65fb18c67632188b539d7b6a4e903/app/src/main/java/de/westnordost/streetcomplete/data/maptiles/MapTilesDownloader.kt I'm not sure what actually needs to be in the style, other than the tile URL. But the commented version in MapTilesDownloader did work when I put it in my home network (onyl api key needs to be changed).

Helium314 avatar Mar 12 '24 14:03 Helium314

Put what where?

westnordost avatar Mar 12 '24 15:03 westnordost

If the style definition does not have to be the exact same as available locally, you could use the one on https://streetcomplete.app/map-jawg/streetcomplete.json (for now)

(The difference is the API key used)

westnordost avatar Mar 12 '24 15:03 westnordost

If the style definition does not have to be the exact same as available locally, you could use the one on https://streetcomplete.app/map-jawg/streetcomplete.json (for now)

(The difference is the API key used)

Thanks! API key should not be an issue I hope.

Helium314 avatar Mar 12 '24 21:03 Helium314

By the way, the json is from this:

https://community.openstreetmap.org/t/minutely-updated-vector-tiles-demo/110121/26

I created a shortbread version of the map style to check out the upcoming openstreetmap-hosted vector tiles, compared it with Jawg. Looks ok, though geometry simplification and compression are missing or lacking. (Test tiles are 65 times the size of jawg tiles or when compared uncompressed, still 15 times the size of jawg tiles)

westnordost avatar Mar 13 '24 00:03 westnordost

(The difference is the API key used)

Looks like it is an issue. The tiles are now twice in the offline database, once with online API key, once with offline API key. Is it ok if I change the API key in the locale styles to match the one from https://streetcomplete.app/map-jawg/streetcomplete.json?

Helium314 avatar Mar 16 '24 07:03 Helium314

During development, that's okay, sure

westnordost avatar Mar 16 '24 13:03 westnordost