Helium314 / SCEE

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

implement sdf icons #537

Closed westnordost closed 2 months ago

westnordost commented 2 months ago

I implemented SDF icons. Basically, you have to blur a black-and-white graphic to achieve an SDF.

But should we use this, after all?

Pro:

Contra:

westnordost commented 2 months ago

this PR:

image

image

without this PR:

image

image

Helium314 commented 2 months ago

I think it looks better with SDF, but on my Galaxy A3 the SDF creation takes 1.8s (in addition to the 1.6s for bitmap icon creation). Maybe the icons could be created in parallel, similar to scanning for quests? The 100 lines of code are fine imo, as they are really in a separate place and don't add complexity.

westnordost commented 2 months ago

Oof. So it's 3+ seconds until you first see the map(?)

One thing that comes... ah, I see you edited your posts. Yes, that could be an option. I had that at the very start when I created the MapIcons file, but the gain was miniscule to not-existing. Not sure if it would be different with calculating the SDFs.

Helium314 commented 2 months ago

Oof. So it's 3+ seconds until you first see the map(?)

From pressing the SC (debug) icon until I see the map it's about 6 seconds (without SDF icons).

Helium314 commented 2 months ago

Not sure if it would be different with calculating the SDFs.

Also not sure, quite possibly there is enough other stuff happening in parallel anyway.

westnordost commented 2 months ago

From pressing the SC (debug) icon until I see the map it's about 6 seconds (without SDF icons).

For me, it makes a world of a difference whether the debugger is connected or not. Try without connecting the debugger to find the proper times.

Helium314 commented 2 months ago

For me, it makes a world of a difference whether the debugger is connected or not. Try without connecting the debugger to find the proper times.

It's only debug APK, not connected to the PC.

westnordost commented 2 months ago

Hui. For me, cold start takes 2.5s until I see the map with icons. (1s I see the loading-screen, 1s I see the app but not the map and 0.5s I see the map but not the icons.)

westnordost commented 2 months ago

Anyway, given you are the "performance guy" due to preferring old(er) devices, I feel like you should decide on whether the additional startup time is worth it. You could also test if parallelization would decrease the time. My guess is that it would, because the SDF calculation is completely a CPU thing, no IO involved.

Another solution I could imagine would reduce loading time pretty much is to create some kind of MapIconsCache which remembers which icons have already been loaded and only loads the graphics as they are about to be displayed in the StyleableOverlayComponent (etc.). Building icons then would usually not be loaded and only a tiny fraction of preset icons would be loaded when the overlay is displayed. I suspect that also only one quarter of quest pins would be loaded initially. My estimation is that this would lead to a loading time reduction of 80%. The only concern I have (other than introducing more code) is that I am not sure MapLibre will handle the situation correctly that a layer is already displayed and an image used by that layer is added asynchronously (will it re-render?).

Helium314 commented 2 months ago

Release APK takes about 3 seconds, so that should be ok.

westnordost commented 2 months ago

You mean cold start, or loading of graphics?

Helium314 commented 2 months ago

You mean cold start, or loading of graphics?

Cold start, same thing where the debug APK takes 6 seconds.

Anyway, given you are the "performance guy" due to preferring old(er) devices, I feel like you should decide on whether the additional startup time is worth it. You could also test if parallelization would decrease the time. My guess is that it would, because the SDF calculation is completely a CPU thing, no IO involved.

I'll test the parallelization (edit: but tomorrow, or whenever the cleaner is has run)

westnordost commented 2 months ago

Note that the cleaner does not clean the tiles because it needs MapLibre.getInstance() on the main thread and the main thread is not available in a worker. Sadly, this means that cleaning old offline tiles will only work if it runs while the app is running. (I.e. I think the cleaner cleans one hour after the app was started? So, have the app open for one hour or something...)

So, maybe the by-age tilecache is not worthwhile to implement ourselves (because it does only work in rare circumstances) and instead it should be done in maplibre-proper as OfflineManager::setMaximumAmbientCacheAge.

Helium314 commented 2 months ago

I'll test the parallelization

Now creating the bitmaps takes 1.5s (down from 3.4 in a single thread). (edit: 1s on release APK)) However, I didn't notice any effect on the time until I first see the map.

westnordost commented 2 months ago

Cool!

This means that more initialization work is done on the UI thread, so the other CPU cores are not used to capacity (unlike you suspected earlier).

westnordost commented 2 months ago

So this can be merged now.

Regarding the other or related topics that were discussed here:

westnordost commented 2 months ago

Oh, actually, there is a TODO left here. You are testing with a Galaxy A3, right?

According to gsmarena, it has a display density of 245ppi, which should translate to hdpi, i.e. x1.5. This is half of what I am testing with. This is relevant because after testing, I assumed that there is a certain bug in MapLibre that leads to the icon halo width not being scaled. See

https://github.com/streetcomplete/StreetComplete/blob/3202e5405e9e9676e5a51219932c766de76a06c1/app/src/main/java/de/westnordost/streetcomplete/screens/main/map/components/StyleableOverlayMapComponent.kt#L180-L181

After having read the background info posted in https://github.com/maplibre/maplibre-native/issues/2281 , I am not so sure about that my conclusion is correct. Maybe the issue is rather that the SDF icons should be created by using a certain "default" blur radius instead (that coincides with the blur radius used for font SDF creation).

Anyway, TLDR: Would you make a screenshot with your device of a preset icon+text and see if the halo width is the same?

Helium314 commented 2 months ago

I remember having shortly tested it, but I remember not seeing much of a difference so I didn't add it. Could be different on your phone.

I would not expect any difference, given that the change in createPresetBitmaps didn't affect time until I see the map at all.

I also wonder if getting the bitmap from resources is an IO operation?

I think it is, at least the first time reading it. I can change it to use Dispatchers.IO.

According to gsmarena, it has a display density of 245ppi, which should translate to hdpi, i.e. x1.5.

Going by displayMetrics.density I get 1.5 on my S4 Mini and 1.8 for the A3. But that could also be connected to using a non-default "display size" setting.

Would you make a screenshot with your device of a preset icon+text and see if the halo width is the same?

Um... nearby elements don't show any halo, things overlay does but doesn't have text, and places overlay doesn't show any icons at all (I had thought it's because they are blocked by quest pins, but even when hiding quests the places don't appear)

westnordost commented 2 months ago

and places overlay doesn't show any icons at all (I had thought it's because they are blocked by quest pins, but even when hiding quests the places don't appear)

Huh.... that's new...