Helium314 / SCEE

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

Maplibre #516

Closed Helium314 closed 1 month ago

Helium314 commented 5 months ago

For simple comparison with master, and for discussions.

to do

to do SCEE

upstream blockers


old observations below

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

Other things:

westnordost commented 2 months ago

So, I was unhappy with that the circles would appear and vanish immediately while the text faded in and out, so I made the circles an icon, too, so that it would also fade in and out. Looked kind of buggy. It looks like this

https://github.com/Helium314/SCEE/assets/4661658/ae88a58e-bc5e-4407-802d-3078792c2307

westnordost commented 2 months ago

And I also did

Arguably, the clusters at zoom 16 are also not all that interesting. For me, it mostly displays numbers between 2 and 4. Could just as well go with dots here.

Agree.

but not

We could also make the pin layer visible one or two zoom levels earlier (and fetch the data also at lower zoom levels). I tried it out, the performance is fine on my phone, but it should only be done if it is also smooth on yours.

With one level earlier it's a noticeable difference, but could still be fine.

because I don't know how badly it would affect performance on lower end devices and the app should rather look and work smoothly.

Helium314 commented 2 months ago

So, I was unhappy with that the circles would appear and vanish immediately while the text faded in and out, so I made the circles an icon, too, so that it would also fade in and out. Looked kind of buggy.

I also noticed this but had not tried looking for a solution.

It looks like this

Very nice!

I will play more with reproducing https://github.com/maplibre/maplibre-native/issues/2372, or the similar issue in my comment there. If I can get it at least semi-reliably and on a phone, it should increase the chance of the issue getting fixed.

Helium314 commented 2 months ago

It looks like this

Though maybe it looks a little cluttered with many quests now? Screenshot_20240519-104431_Street­Complete_Dev

Helium314 commented 2 months ago

pin icon scale and memory use

I had a closer look at this.

Memory use is always after forced GC, except for the short jumps. Looks like "something" is using much more memory than the extra 80 MB one would expect from the increased pin bitmap size.

Helium314 commented 2 months ago

A bug I stumbled upon when trying to reproduce MapLibre issues: Sometimes when switching overlays, quest pins are gone until the map is moved. Looks very much that this is because QuestPinsManager.invalidate calls clear and then onNewScreenPosition. Now onNewScreenPosition does nothing if lastDisplayedRect is the same as the current rect. Since clear is setting lastDisplayedRect to null in a coroutine, it's sometimes set to null after the check for changed tiles rect in onNewScreenPosition.

Edit: setting lastDisplayedRect at some other point either in clear or invalidate should help, but I guess there is a good reason for setting this indside questsInViewMutex.withLock

westnordost commented 2 months ago

Though maybe it looks a little cluttered with many quests now?

Oh, yes indeed! I only tested that in the central part of Hamburg, looks like this has already a lower density of quests.

Would you tune the values so that it does not look too cluttered for your test area?

westnordost commented 2 months ago

pin icon scale and memory use

Hm, interesting. I think this is worth posting as an issue on MapLibre. (With some additional explanation.) After all, for us, this is not that relevant anymore, as I already reduced the scale to 1.

Helium314 commented 2 months ago

Uh, and another issue: The quest pin order is not correct. You can check this by switching on and off places overlay in infrequently updated areas. Then on many shops the displayed pin will switch between opening hours and check existence. I'm not sure when this started, I'm pretty sure that it had worked correctly some time ago (not 100% sure though...).

Helium314 commented 2 months ago

Oh, yes indeed! I only tested that in the central part of Hamburg, looks like this has already a lower density of quests.

I found this is mostly an issue when enabling all quests, while on default profile it looks fine. Maybe just setting the clusters to have constant size on map (instead on screen) solves this already.

westnordost commented 2 months ago

A bug I stumbled upon when trying to reproduce MapLibre issues: Sometimes when switching overlays, quest pins are gone until the map is moved.

I'll look into this. The same issue should exist for overlays also

westnordost commented 2 months ago

Actually, your proposed solution works best. There was indeed no reason why the lastDisplayedRect had to be in the mutex, as the mutex is for another data structure.

westnordost commented 2 months ago

Uh, and another issue: The quest pin order is not correct. You can check this by switching on and off places overlay in infrequently updated areas. Then on many shops the displayed pin will switch between opening hours and check existence. I'm not sure when this started, I'm pretty sure that it had worked correctly some time ago (not 100% sure though...).

Checkexistence has higher priority than opening hours quest.

westnordost commented 2 months ago

I remember having issues with tile cache not being cleared (fully?) when it contained a lot of files. Maybe to be safe we should clear the folder more often than just on upgrade? There should not be any side effects when it's empty anyway.

Hm. How about... (pseudo code)

if (!preferences.clearedTangramCache) {
  for (file in listFilesInCacheDir()) {
    file.delete()
  }
  preferences.clearedTangramCache = true
}
Helium314 commented 2 months ago

Checkexistence has higher priority than opening hours quest.

I think the priority is not relevant here, because it does not change when enabling an overlay. Using 92a8b119e433655fc51a0c173c8baa907dde8461 (edit: also with the newer e049c5418b6c5f2fda305a05339111392b1ec06a), the opening hours quest is shown for shops that haven't had an update for a while, and when I hide it the check (shop?) existence quest is shown. With 3589c52a1b90035d05841614079e5a57088d22b8, which quest is shown sometimes changes when switching on/off the places overlay.

Helium314 commented 2 months ago

So it looks like clustering messes with the symbols order... without clustering it works.

Helium314 commented 2 months ago

Hm. How about... (pseudo code)

Possibly using walk is preferable, but otherwise I think this is fine.

Helium314 commented 2 months ago

I did some more investigation on memory use:

Native memory use again with scale 2 and 4 added, here we should note that bitmaps are in memory twice. scale bitmap size from allocationByteCount native memory use after start (not showing pins) native memory use after showing pins and zooming / panning max (short-time) native memory use
1 10 MB 113 MB 160 MB 190 MB
2 40 MB 167 MB 195 MB 290 MB
3 90 MB 261 MB 300 MB 450 MB
4 160 MB 398 MB 419 MB 700 MB

What do you think about not caching the map icons, and instead create them again whenever the view is created? It would likely save a noticeable amount of memory, but I'm not sure when view is created again (only thing I found so far is theme switch).

westnordost commented 2 months ago

MapLibre apparently copies the added images

This is not surprising. The data needs to be copied from Android's Bitmap data structure to whatever MapLibre uses internally in C++.

presetBitmaps appear to waste space

Yup, noticed that, too. MapLibre will even copy the bitmap another time if the bitmap is not a ARGB image:

https://github.com/maplibre/maplibre-native/blob/3f8cdf4d7a5fbcbdfd8614408666992a44c7dd66/platform/android/MapLibreAndroid/src/main/java/org/maplibre/android/maps/Style.java#L1336

What do you think about not caching the map icons, and instead create them again whenever the view is created?

Bad idea, IMO. Generating these pictures takes quite some time. Especially lower-end devices may occasionally destroy the MainActivity when tabbing out (e.g. taking a picture), and then, regenerating the sprites each time would absolutely paralyze users.

Given your numbers, it looks like MapLibre is copying all bitmaps into a new array of MapLibre bitmaps, THEN iterate them, adding them one by one in native code. The short-time memory spike could thus be avoided by calling to add them one by one from the java code. But no idea how big is the performance overhead for JNI calls.

A performance improvement from our side could be to pre-generate a spritesheet and sprites.json (for each display density) via a build task and feed this into the streetcomplete.json. However, this is easier said then done, for different reasons.

westnordost commented 2 months ago

So due to a comment in that issue I posted, we learned, that the issue of the missing tiles for GeoJson sources already existed for v10 of the library. Rather than downgrading to v9, I think it would be better to hope for it being fixed, as many other issues have been fixed for v10 (and I imagine, v11).

Another issue is that the sort order of the quests being inserted is mixed up when clustering is on, even on zoom levels for which clustering is turned off. Would you report that at maplibre? Let's leave whether or not we find a way to somehow work around that to such time when the release-blocking maplibre#2410 is addressed and released but the clustering issue was not fixed in the same release.

Helium314 commented 2 months ago

I switched to using symbolSortKey for fixing the icon order after adding clustering. Either the performance impact has been reduced in MapLibre 11.0, or already the Galaxy A3 is fast enough that there is no noticeable difference to the previous SymbolZOrder setting.

Helium314 commented 2 months ago

A performance improvement from our side could be to pre-generate a spritesheet and sprites.json (for each display density) via a build task and feed this into the streetcomplete.json. However, this is easier said then done, for different reasons.

Would it also work to just use the old Tangram sprites sheet? If no, I'd say we leave it as it is, and only do something if there are issues reported by low-end device users.

westnordost commented 2 months ago

Would it also work to just use the old Tangram sprites sheet?

You mean the old method - generate the necessary sprite sheet plus metadata once after every app update (or each time in debug mode or something) and save it into a file, then load that file together with the style json on start.

According to documentation, it is indeed possible to supply several sprite sources, i.e. there could be one sprite source for the oneway arrow (base style), one sprite source for the quest pins, one for the preset icons, one for the building icons.

This requires to have these sources hardcoded in the style json and also ensure that the necessary files have been generated and saved before the style is loaded. The latter is no problem, the code just needs to move before that. The former I guess would ideally require to change the build script that copies the latest version of the streetcomplete.json into this repo to inject some json at a certain position. (or, there could be some kind of marker in the source file so it is easier to find the correct position.)

Finally, the code in MapIcons needs to be modified and extended a bit: 1. instead of returning a map of icon name to bitmap, create one bitmap with a spritesheet plus a data structure from which the sprites.json is created, 2. create/overwrite the files and 3. (re)add logic so that it is only done once (per app update). Basically, very similar to what the TangramSpriteSheet.kt did before, only mostly that the metadata is in another format.

How much time does creation of the icons at each start take for you currently? I.e. how much time could be saved at most? (Keep in mind that just loading these sprite sheets will also take some time... not sure how to measure how long that would take beforehand... maybe just make the canvas of current sprites.png much bigger and draw some stuff in there so that the file to be loaded is bigger?)

westnordost commented 2 months ago

Thinking more about this, another approach would be to simply not load all icons at the start, but load them dynamically at the point where they are first used. While this may not lead to faster initial startup time then the approach above (but definitely compared to now), it will reduce the used memory by a lot, as most quest types and most preset icons will never be displayed during an app run. Loading images dynamically when necessary is also what every game engine will do.

The code could also result to be a bit cleaner this way, because the images needed for one *Component will then actually also be added by that one component and not somewhere else. The code would look something like this:

val loadedIcons: MutableSet<String>
...
// just before .setGeoJson
for (pin in pins) {
    val name = pin.iconName
    if (name !in loadedIcons) {
        val icon = loadPinIcon(name)
        map.style.addImage(name, icon)
        loadedIcons.add(name)
    }
}

However, at least with the implementation as outlined above: This is the UI thread. To change this, functions that setGetJson like PinsMapComponent::set` would have to be made suspending like I did in that other experiment.

Helium314 commented 2 months ago

How much time does creation of the icons at each start take for you currently? I.e. how much time could be saved at most?

It's around 2.8 s. I guess the savings could be noticeable, but probably would not make a huge difference in startup time.

westnordost commented 1 month ago

I read in Every Door's release notes that it is now also showing the user's walked path, but as little blue dots instead of a blue "marker" line. I didn't see it in action, but in principle, that sounds like a really nice idea because it emphasizes more that it is not necessarily an exact path, it seems to express better this kind of thing (think pirate treasure maps) and it makes the line more visually distinct from highlighted lines and overly lines. That, on paper, of course. Could try it out if one has the time.

mnalis commented 1 month ago

I didn't see it in action, but in principle, that sounds like a really nice idea because it emphasizes more that it is not necessarily an exact path

I didn't see new EveryDoor in action either yet (waiting for F-droid build), but I don't think I'm going to like it.

I've seen it before (e.g. in Tower Collector) and not having the order of dots is losing too much information (e.g. think if mapping a public square, it is very important which of the dots all around you were the ones leading to current position - and in which direction. So relations between dots is very important)

small_Screenshot_20240601_223533_Tower Collector

Can you tell if I've come from WNW, NW, NE, or SE? Because I sure can't (and the whole point of that GPS trail request in https://github.com/streetcomplete/StreetComplete/issues/2209 was to help user orient themselves).

it seems to express better this kind of thing (think pirate treasure maps)

Well, if SC implements much more gamification, and there is big red X at the end marking where treasure chest is, then maybe it would look cool :smile:; but from usability perspective of this particular feature I think it is much better leaving it as it is.

(the linked ticket lists some ideas how to make it more visually appealing while retaining functionality, but I'm afraid the effort would likely far outweigh the benefits)

Helium314 commented 1 month ago

Could try it out if one has the time.

Due to really bad GPS on A3, and known issues on S4 Mini, I can't try it. But maybe you want to test following change:

-            .withProperties(*commonTrackProperties, lineOpacity(0.6f)),
+            .withProperties(*commonTrackProperties, lineOpacity(0.6f), lineCap(Property.LINE_CAP_ROUND), lineDasharray(arrayOf(0f, 2f))),
         LineLayer("old-track", "old-track-source")
-            .withProperties(*commonTrackProperties, lineOpacity(0.2f))
+            .withProperties(*commonTrackProperties, lineOpacity(0.2f), lineCap(Property.LINE_CAP_ROUND), lineDasharray(arrayOf(0f, 2f)))

in https://github.com/Helium314/SCEE/blob/maplibre/app/src/main/java/de/westnordost/streetcomplete/screens/main/map/components/TracksMapComponent.kt#L67

I tested on other lines, but I guess there is no fundamenal difference to tracks.

Helium314 commented 1 month ago

I got around to do it (with modified MIN_TRACK_ACCURACY). Works, but depending on zoom the dots may look like dashes (right screenshot).

westnordost commented 1 month ago

Cool! Hmm, I rather like it! What about you?

I wonder why whether it's dots or dashes varies by zoom level - we don't have this issue witth e.g. steps, private roads etc.

Helium314 commented 1 month ago

Cool! Hmm, I rather like it! What about you?

I also think it's good, except for the dots/dashes.

I wonder why whether it's dots or dashes varies by zoom level - we don't have this issue witth e.g. steps, private roads etc.

Possibly it's related to the steps line width changing with zoom, while the track line width is constant (in pixels).

mnalis commented 1 month ago

Nobody asked me :smiley_cat: but I'd just say that if that had to be implemented (which I'm still not convinced is good idea), I'd rather it were (possibly longer) dashes than dots at all zooms.

That would provide similar visual gimmick, but dashes would lose less of the information than the dots.

Helium314 commented 1 month ago

which I'm still https://github.com/Helium314/SCEE/pull/516#issuecomment-2143587198

You argue against a dotted line with an example of showing each recent location as a dot. Cases where you can't get directional data from a dotted line, but you can get it from a full line seem more like an edge case to me.

westnordost commented 1 month ago

By the way I don't mind if the dots stretch a bit depending on zoom level - I kind of like it, too. First, it still fits the "treasure map" metaphor (doesn't matter if dots or dashes), second, it looks a bit like tracks (footprints) that way. But anyway, if it looks weird that way when zooming, there is always the option to make the line broader as one zooms in, as with all the other lines.

Helium314 commented 1 month ago

closing in favor of https://github.com/streetcomplete/StreetComplete/pull/5693