streetcomplete / StreetComplete

Easy to use OpenStreetMap editor for Android
https://streetcomplete.app
GNU General Public License v3.0
3.82k stars 348 forks source link

Explore migration from tangram to MapLibre #3123

Closed smichel17 closed 3 weeks ago

smichel17 commented 3 years ago

Originally posted by @westnordost in https://github.com/streetcomplete/StreetComplete/issues/1892#issuecomment-889991925

MapLibre (ex Mapbox SDK) now supports Metal, so an iOS port would probably (need to) use MapLibre instead of tangram-es. This requres to

  • create a mapstyle for mapbox / maplibre (for jawg.io map tiles) that matches the map style for tangram
  • check it out, see if it has all features required by StreetComplete (probably yes)

As tangram is not really developed further, it might be a good choice to change to using the maplibre SDK for Android too. Or - bluesky - use/create a map view that directly renders the OSM data.

Additionally, there's issues like https://github.com/streetcomplete/StreetComplete/issues/3101 which are complicated to fix due to working around tangram limitations.

From the parts I am familiar with, I think StreetComplete already has a decent wrapper around Tangram (e.g. KtMapController), so in many places it would "just" be a matter of swapping out the implementation of those interfaces, and then plugging any leaks in the abstraction. There's also the scene files; I don't yet know how they map to MapLibre features. And any other tangram features we use that I'm not familiar with.

westnordost commented 3 years ago
smichel17 commented 3 years ago

My first impression was that MapLibre isn't quite ready for use on Android— I wasn't able to find any docs besides this README, which still contains many links back to the mapbox docs. Based on the commit history it also seems like the Android version is not under particularly more active development than tangram (equivalent history). (That could be misleading, though, if most of the work is happening in code that's shared between platforms.)

smichel17 commented 3 years ago

Reading the MapBox camera guide, it seems similar enough to tangram's api, and perhaps the center the camera within a map area feature could be used to fix #3101. So if these are still accurate, perhaps it will not be as much effort as I thought.

westnordost commented 3 years ago

I consider MapLibre / MapBox SDK somewhat more mature than tangram-es though. (Without having had a closer look though.): It is older, i.e. had some years headstart, and now for 3 years, tangram-es wasn't really much developed on.

smichel17 commented 3 years ago

I'm reading through the MapBox docs now, and I agree it seems that way. Particularly interesting to me: it looks like dynamic display of quests may be possible without re-rendering the entire map. The main problem is that I'm not sure what features have changed since the MapLibre split; I didn't find any MapLibre documentation, and I don't see any way to reference the MapBox guides at the version before the split (the javadocs are versioned, so that's fine).

edit: Here's a good overview: https://docs.mapbox.com/help/getting-started/add-markers/. Unfortunately,

  • Mapbox Maps SDKs v10 includes wrappers supporting this approach.
  • Mapbox Maps SDKs less than v10 offer plugins supporting a limited version of this approach.

The MapLibre fork happened just before version 9.3, so we might be limited to the latter.

Even still, combined with data driven styling, I think we could easily enough do things like make the quest dots the same color as the background of the pin that will appear when they're expanded.

smichel17 commented 3 years ago

They have a few different render & camera modes, which I think could replace our custom "compass mode" https://docs.mapbox.com/android/maps/guides/location-component/#rendermode (although with this approach, we give up some control / ability to tweak later)

westnordost commented 2 years ago

So the following steps need to be taken here:

Phase 1

  1. create a requirements catalogue, i.e. what features must the library provide, what would be good but is not required, what is extra
  2. create a minimal Android app and implement/demo all the features mentioned above
  3. finally, evaluate. Are all necessary features covered? Estimate how long it would take to migrate and evaluate what we gain from that. (Do we need less code because there are less workarounds? Do we need to add additional workarounds? etc.)

After the evaluation, if we decide it is worth it, there may be a phase 2 - to actually do the migration. I can help with the requirements catalogue, i.e. when it is almost complete, I'll review it and also add anything you may not have thought of (because features that require this don't exist yet).

matkoniecz commented 2 years ago

create a requirements catalogue, i.e. what features must the library provide, what would be good but is not required, what is extra

generic mandatory features:

specific mandatory features:

nice to have:

What is missing in this listing?

pinging @westnordost as it is relatively important

pinging @smichel17 as help with finding missed issues is likely here

westnordost commented 2 years ago

Hmm, the following more, otherwise I think it is complete

further mandatory:

nice to have:

westnordost commented 1 year ago

@matkoniecz did not start working on it yet, so for the time being, this is free for the taking / can be done by other people interested in this. If you do, I can assign you to "reserve" this.

adrianclay commented 1 year ago

I've made a start exploring this.

I've hacked together a version of SC with a side-by-side of Tangram / MapLibre, as I believe it will be (for me) an easier way of demoing desired requirements. ![image](https://user-images.githubusercontent.com/976274/200693981-8652abbb-d547-4640-8f25-5fe396bfcbcd.png)

For now I'll be focusing on exploring the list of requirements above.

westnordost commented 1 year ago

Whoa, cool!

Note that for some time, I have been playing with the thought whether it would not be less effort / more flexible / faster to skip vector map libraries and directly render the OSM data (etc.) onto the map like Vespucci or iD does. I.e. facilitate some (hardware accelerated) canvas-drawing of sorts.

Advantages:

Not sure:

Disadvantages:


Anyway, these were my thoughts so far for the unicorny outlook of maybe rendering the map by oneself, using the Canvas or somesuch. I felt I needed to record this here before looking into MapLibre starts in earnest, just to mention another alternative.

That considered, I am of course very happy that someone looks into this, as tangram-es issues unfortunately have been piling up and piling up, to the point where I do not even bother to open bug reports in their repo anymore as it looks like it has finally been abandoned. Moving to the widely used MapLibre looks very much like the step in the right direction here.

smichel17 commented 1 year ago

tilting the map and possibly rotating the map will probably be much harder to implement (when thinking about simply drawing all the stuff on a simple 2D canvas)

I wonder if we could borrow significant amounts of code from MapLibre/Tangram for this.

westnordost commented 1 year ago

If you have any updates or news, I'd be glad to hear about it! This is one remaining abandonware thorn in the sides of StreetComplete it would be very nice to get rid of (or explore how it can be done at least). If you have the code somewhere public, it can also help for future attempts even if you do not continue to look into it.

adrianclay commented 1 year ago

Hey - I'm afraid to say I've been unable to prioritise this issue and give it the effort it needs (which is a lot).

I've pushed up the changes I'd made here to 8f2d59678f5e14409284f00c3783a36239de53e4

I've documented some of my thinking within the commit message. If anybody is interested in taking this over, I'd be happy to spend some time talking through the code and answering questions over a video call.

My super summarised view is that I can't see any major reason not to migrate, other than the effort required in doing so 😄. Any time spent on this is time not spent on other changes, conversely the sooner this migration happens the less painful it will be.

Bonne chance.

Helium314 commented 1 year ago

I played a bit with the mapLibre work of @adrianclay, and added some more stuff: https://github.com/Helium314/SCEE/tree/maplibre After updating mapLibre to 10.0.2 it was necessary to update OkHttp to 4.10, leading to some more small changes.

As for the feature list

generic mandatory features:

specific mandatory features:

nice to have:

not in the list, but relevant and not tested:

I think the next steps should be:

westnordost commented 1 year ago

display of rotated image at specific point (to display direction of view, pointer to the current location when offscreen)

This would not really be necessary, as the location and view direction could also be shown as normal views on top of the map, just like that arrow that points to your location if it is out of the screen. It may even be favourable, as AFAIK the fact that these things are currently drawn by tangram-es trigger a re-draw basically every frame

But since camera movements with animation are supported, I guess smooth zooming is possible.

I mean, they are not really supported by tangram-es (not multiple properties at the same time) which is why I built this wrapper around it. There is at least one bug associated with this which is why it would be better if maplibre supported this out of the box.

Helium314 commented 1 year ago

Current possible issues:

westnordost commented 1 year ago

I didn't (yet) find a way of downloading map tiles for offline use, unless the map style is downloaded together with the map tiles, https://github.com/maplibre/maplibre-gl-native/issues/609

Oh, that'd be bad. There are two possibilities to do this:

Helium314 commented 1 year ago

via some kind of cache control

This will probably not work. Tiles are in a SQLite database, and if we add tiles to the database, we would somehow need to make sure the database is not opened / accessed by MapLibre while the tiles are persisted.

via some kind of offline datasource

So far I haven't found anything useful. There is a CustomGeometrySource, but this seems to expect Features and not tiles.

mnalis commented 1 year ago

This will probably not work. Tiles are in a SQLite database, and if we add tiles to the database, we would somehow need to make sure the database is not opened / accessed by MapLibre while the tiles are persisted.

Don't know about specific MapLibre case, but AFAICT SQLite should generally support multiple clients reading/writing to the database at the same time. e.g. https://stackoverflow.com/questions/34409275/concurrency-in-sqlite-database

westnordost commented 1 year ago

Tiles are in a SQLite database, and if we add tiles to the database, we would somehow need to make sure the database is not opened / accessed by MapLibre while the tiles are persisted.

You mean other than tangram-es, maplibre does not use a http cache at all? Are you sure? This sounds a bit over-engineered, especially if some kind of offline-feature is not supported at all.

Helium314 commented 1 year ago

Don't know about specific MapLibre case, but AFAICT SQLite should generally support multiple clients reading/writing to the database at the same time. e.g. https://stackoverflow.com/questions/34409275/concurrency-in-sqlite-database

Thanks! I remember I had read some warning to not do this, but looks like it's worth a try.

This sounds a bit over-engineered, especially if some kind of offline-feature is not supported at all.

Well, there is real offline functionality (though this is definielty a bit over-engineered for what we need). But as far as I understand, the entire OfflineRegion that is downloaded is connected to a specific map style, and this style can only be dowloaded (https://github.com/maplibre/maplibre-gl-native/issues/609). I haven't tried downloading and then accessing the map tiles with default style... let's see if I can put a minimal style JSON with the correct tile URL somewhere online (not exposing the API key complicates this a bit).

Actually for some part I like having a database for the cached tiles, because this eliminates the potentially huge file system overhead, see #3417. Though it can't be simply removed by Android when running out of disk space...

Helium314 commented 1 year ago

I haven't tried downloading and then accessing the map tiles with default style... let's see if I can put a minimal style JSON with the correct tile URL somewhere online (not exposing the API key complicates this a bit).

Ok, this seems to work. Tiles downloaded for a style that shows nothing are visible on a map that's using default style. see https://github.com/streetcomplete/StreetComplete/commit/93ab626a0abc0160759871cb26054ec58decef04 So I see this as working and won't do more on the download now.

HolgerJeromin commented 1 year ago

I have no insight, but could the offline-manager be used for pushing the already downloaded osm data into mapLibre?

Helium314 commented 1 year ago

Many things already work now, with some issues:

I have no insight, but could the offline-manager be used for pushing the already downloaded osm data into mapLibre?

I don't think so, you can't define an OfflineRegion without an URL (well you can, but then it's useless). Though maybe if SC would setup a local http server from which MapLibre could download? Most likely not worth the work.

Helium314 commented 1 year ago

If anyone wants to test and find issues, a working version (with different package name) is here

Already known issues:

Semi issues (fix known, but too much work for now):

If anyone wants to help: the most important thing to be solved is how to best (or at all) add things to the map (geometry, quests, overlays). Best means it should

Currently annotations are used, but there are other ways that may or may not work for SC. What's probably worth investigating are layers, sources and filters (they are all connected, but I don't see through this yet).

Edit: Having many annotations clearly has a bad effect on performance. When there are many quests, zooming is noticeably stuttering (on my old device), while it's fine when all quests are disabled. This is not an issue in tangram. Even without quests tangram feels a bit smoother than MapLibre, though this might be caused by the more detailed map style used for the MapLibre map. Also MapLibre pan/zoom behavior doesn't feel right. I have the impression there is some delay before move/zoom is happening and it doesn't seem to be a performance issue.

So my impression is that we shouldn't use annotations, because of performance and because of the overlay issues,

Helium314 commented 1 year ago

I had a closer look at how the annotations work. Each AnnotationManager adds a GeoJsonSource and a layer (symbol, circle, fill or line). Adding / updating source data could be done in a more efficient way, but so far I didn't even test whether that is actually necessary. But after filling the data, I think there is no / little difference to other ways I investigated, so I see no way of improving the general map performance over the current state. Which is really not great if many quests are in view, especially if you have a smoothly working tangram map next to it...

westnordost commented 1 year ago

On slack us, there is a channel called #maplibre on which you may ask maplibre related questions, fyi.

Helium314 commented 1 year ago

Looks like it's more or less as fast as it can get now. The main performance issue when scrolling/zooming appears to be sorting, which is noticeably faster (though still not great) when using one unique imporance value per QuestType, instead of one per Quest. And even faster, to a point where it's equal to unordered, is when we set the z-Order to the order of the source. Since for every change in displayed quest pins we need to provide all features to the GeoJsonSource, this is feasible (and sorting pin list by importance is fast).

Note: Single pins can be added/removed using AnnotationManagers, but internally they set the full list of Annotations in a GeoJsonSource, so essentially it's the same.

As for performance of updating a lot of pins, I'd need a way of testing. The update is done in background by MapLibre, so there is no simple way of measuring time. Appears to be fast enough though.

Helium314 commented 1 year ago

Checklist updated, remaining things:

What I don't like about MapLibre:

So I think I'm more or less done with exploring, and will leave another apk for everyone who's interested. The actual code is incomplete in some places (camera, offline maps, actual pins instead of round icons, overlays,...). Here it should be clear what needs to be done from the existing code and comments. I think it's a reasonable base if someone wants to work on it (I will not, at least for quite a while).

matkoniecz commented 1 year ago

The comment implies that Tangram has bad battery performance, which in my opinion is not true. But anyway, MapLibre definitely generates higher CPU load than Tangram during normal operation, even without any pins being displayed

SC seems to be eating battery faster than other apps which require continuous screen display and some people commented on how fast battery disappears when using it

Helium314 commented 1 year ago

For me, the biggest battery eaters were auto-upload and the theme (on OLED display). When using auto-upload, auto-downlad (can switch it off in SCEE) and light theme, my phone used to get very warm. Now with dark theme and auto-up/download off this doesn't happen any more. My guess is that actually auto-download might be the main reason for high battery usage (network traffic, and high CPU load when persisting data and creating quests).

Btw the performance difference between Tangram and MapLibre seems to be smaller in non-debug version, but still MapLibre isn't as smooth as Tangram.

mnalis commented 1 year ago

My guess is that actually auto-download might be the main reason for high battery usage (network traffic, and high CPU load when persisting data and creating quests).

Data traffic is huge battery eater, definitely. For example, using offline OsmAnd with online showing of notes (which downloads them as-you-go, similar to SC), I can roughly double battery time by disabling showing of notes / data traffic.

Even in SC-only situation, I can greatly extend battery life when I have data disabled, and then stop every few kilometers to enable data for a minute or two, load new areas, and then disable data again for next hour of walking; instead of having data enabled all the time and let SC autodownload stuff every dozen seconds.

SC seems to be eating battery faster than other apps which require continuous screen display and some people commented on how fast battery disappears when using it

Another big power user (as opposed to say notepad app with screen on) is GPS (and bluetooth/wifi if used for positioning) and CPU, as noted.

Helium314 commented 1 year ago

I lack the motivation of continuing here, but I updated the comments a little and merged the latest SC changes into https://github.com/Helium314/SCEE/tree/maplibre in case someone is interested on working on this.

The comments contain some information on how things work and what is still to be done. Some short and incomplete summary:

1ec5 commented 1 year ago

Note: Single pins can be added/removed using AnnotationManagers, but internally they set the full list of Annotations in a GeoJsonSource, so essentially it's the same.

If you’re working with a lot of annotations (like several dozen or more) and rendering performance matters to you, you should avoid AnnotationManager in favor of setting up a lower-level symbol layer.

AnnotationManager is optimized for the use case of a single marker on screen, or several markers that share nothing in common and pop in and out at different times. Each instance of this class manages a separate GeoJSON source and style layer, so you could increase the style’s complexity very rapidly by adding many annotations to the map.[^ios]

StreetComplete’s use case calls for a single GeoJSON source and a single symbol layer, using the data-driven styling technique to vary each symbol according to each underlying feature’s properties. Just as with the icons and labels in the basemap, the performance impact of this approach would be negligible unless you animate the camera continuously on a steep tilt (common in navigation applications but uncommon in editors like StreetComplete).

[^ios]: MGLAnnotation on iOS doesn’t work like this. It’s much more performant because it relies on a shared, spatially indexed source type that isn’t exposed publicly. The Android annotations plugin doesn’t have access to this source type.

Helium314 commented 1 year ago

If you’re working with a lot of annotations (like several dozen or more) and rendering performance matters to you, you should avoid AnnotationManager in favor of setting up a lower-level symbol layer.

That's the current state of the test implementation, with exception for the layer showing the current user position. But it looks like currently the whole annotation / symbol thing is being worked on. It might be good to wait until MapLibre guys are done with this...

What could be done now and pretty much independent from other progress is creating a StreetCompete map style json. I really didn't want to work on this because being unable to simply comment some parts of the file is absolutely horrible.

westnordost commented 1 year ago

It would make sense if we know that sooner or later we will switch to Maplibre. Is that certain? Your comments so far seemed to be somewhat critical of this.

Helium314 commented 1 year ago

There are some checkmarks missing in https://github.com/streetcomplete/StreetComplete/issues/3123#issuecomment-1454681659, but I'm not sure whether this is really the latest state (didn't re-read other comments).

I'm not sure about animations, here we'll have to wait for the next release which should contain some improvements (at very least for the fling animation, maybe more). And my impression is that performance will not be better than Tangram, at best it's the same.

westnordost commented 8 months ago

I recreated the StreetComplete map style for Tangram-ES in MapLibre. Not everything is possible with MapLibre that was possible in Tangram, but since I spent so much time on this, I thought I could also make the stylesheet a bit prettier, too:

One thing that doesn't render yet is the oneway-arrow. Don't know what's wrong, will have a look at it some other time.

In general I ended up with a little less thick lines at the same zoom level, and also in general features tend to be visible about one zoom level later. But for both, I think it is a good thing.

Test it here: https://www.westnordost.de/misc/map.html

Repo is here: https://github.com/streetcomplete/maplibre-streetcomplete-style Writing styles for MapLibre in JSON is actually so cumbersome and involves a lot of copy&paste that I rather specified the style in a simple DSL I whipped up in Kotlin. So that Kotlin app outputs the style (normal and night-style).

Maybe we need to end up completely defining this style in code later on to be able to dynamically generate it at runtime, because I didn't find a way to inject into the currently loaded map style such things as which language for places to prefer, font scaling etc. and we certainly want to avoid having a streetcomplete.json for every preferred language. We'll see, for now I think it is fine.


This is just the styling for the Jawg background map. The style for whatever StreetComplete renders additionally can hopefully be defined in code and doesn't need to be "baked" into the style json like it was necessary for Tangram.

1ec5 commented 8 months ago

❌ no shore wave animation (MapLibre has no shader support), instead a simple white outline that fades in at zoom 19 or so

You can bring in a custom shader using a custom style layer.

Writing styles for MapLibre in JSON is actually so cumbersome and involves a lot of copy&paste that I rather specified the style in a simple DSL I whipped up in Kotlin. So that Kotlin app outputs the style (normal and night-style).

Yes, if you aren’t using a visual design studio like Fresco, then code generation is a sound approach.

Maybe we need to end up completely defining this style in code later on to be able to dynamically generate it at runtime, because I didn't find a way to inject into the currently loaded map style such things as which language for places to prefer, font scaling etc. and we certainly want to avoid having a streetcomplete.json for every preferred language. We'll see, for now I think it is fine.

Yes, this is why OSM Americana builds the style at runtime. It also outputs a prebuilt stylesheet, but that stylesheet can’t adapt to the user’s language and other environment settings.

westnordost commented 8 months ago

Fresco... it looks really good. Too bad, it wasn't (really) linked from the OSM wiki on the topic of MapBox styles, so I used Maputnik but on first glance I like Fresco more, because it is closer to the code. (And anyway, I was using Maputnik basically only to view the style I have been editing in code.)

Maybe we need to end up completely defining this style in code later on to be able to dynamically generate it at runtime, because I didn't find a way to inject into the currently loaded map style

Oh, actually, someone in #maplibre-android told me that one can access and edit properties of layers from the style:

maplibreMap.style?.getLayerAs<FillLayer>("someLayer")?.setProperties(...)

1ec5 commented 8 months ago

Good point, added it to https://wiki.openstreetmap.org/wiki/Mapbox_styles#Editors

westnordost commented 8 months ago

Regarding the status of the (exploration of the) migration to MapLibre:

I asked in the chat recently about the next version of MapLibre, and it seems to mainly just contain some renamings of classes (replace MapBox with MapLibre) and some work on Metal (for iOS). In any case, nothing big. (I didn't look at the unmerged PRs, though)

@Helium314, would you consider the work you did to advance this in the maplibre branch on SCEE to be a good base to finish the implementation, or would you say that it was rather a prototype to have a stab at how (and if) things could be done with MapLibre? Regarding your post from April, have you lost motivation to work on it for good, or is this something you may carry on with later? (Maybe spurred by the creation of that MapLibre style? 😛)


By the way, I am amazed how much more smoother and cleaner the map looks with maplibre, especially in direct comparison between the style for Tangram and MapLibre. Yes, I've made some style changes, but look at for example the font rendering.

Helium314 commented 8 months ago

@Helium314, would you consider the work you did to advance this in the maplibre branch on SCEE to be a good base to finish the implementation, or would you say that it was rather a prototype to have a stab at how (and if) things could be done with MapLibre? Regarding your post from April, have you lost motivation to work on it for good, or is this something you may carry on with later? (Maybe spurred by the creation of that MapLibre style? 😛)

I'd definitely call it a prototype. Some parts are probably fine, while others are more experimental. E.g. all the stuff in MainMapFragment is mostly for testing how to do things, contains a lot of comment on what works well and what doesn't, contains overlay styles that should better be in some style json, contains stuff that should not be in there,...

Helium314 commented 8 months ago

I think I would work on it again, but can't really say whether I'll have the necessary time

westnordost commented 8 months ago

Cool! It would also be the most efficient way to go about this, because you already familiarized yourself somewhat with the framework. I'll consider this ticket reserved for now (there is enough other things that potential contributors could do).

Helium314 commented 7 months ago

I started checking the current state again and worked on some stuff that was still missing.

Anyway, I remembered the main issue with the current state: interaction with the map (sources and annotation managers) must be done on UI thread, and for testing I simply exposed the MainActivity in its companion object for runOnUiThread. This works, but is not the way to go. How should it be handled properly?

westnordost commented 7 months ago

Hm well, any interaction with the UI has to be done on the UI thread in Android. So I guess, the answer is, to

westnordost commented 5 months ago

FYI work in progress here: https://github.com/Helium314/SCEE/pull/516