Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement sdf icons #537

Merged
merged 2 commits into from
Apr 19, 2024
Merged

Conversation

westnordost
Copy link
Collaborator

@westnordost westnordost commented Apr 16, 2024

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:

  • same look as now, for the shop and things overlay. However, the building icons continue to not have a halo because the halo only works for SDFs. Arguably, since there are no black elements on the map, the SDF icons don't actually need a halo (for night mode, the icons are white).
    For the buildings, a halo would be nice, but neither necessary (imo) not possible (with maplibre halos at least)
  • (is it faster than not using SDF icons?)

Contra:

  • startup takes 0.5s longer (on my device) because blurring the 730+ preset icons takes that long
  • +100 lines of code

@westnordost
Copy link
Collaborator Author

this PR:

image

image

without this PR:

image

image

@Helium314
Copy link
Owner

Helium314 commented Apr 17, 2024

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
Copy link
Collaborator Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Collaborator Author

westnordost commented Apr 17, 2024

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
Copy link
Owner

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Owner

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

@westnordost
Copy link
Collaborator Author

You mean cold start, or loading of graphics?

@Helium314
Copy link
Owner

Helium314 commented Apr 17, 2024

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
Copy link
Collaborator Author

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
Copy link
Owner

Helium314 commented Apr 18, 2024

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
Copy link
Collaborator Author

westnordost commented Apr 18, 2024

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
Copy link
Collaborator Author

So this can be merged now.

Regarding the other or related topics that were discussed here:

  • I will create a suggestion ticket to have OfflineManager::setMaximumAmbientCacheAge in MapLibre

  • Maybe (at least) the pin creation could also be parallelized using the same method. And actually, I noticed, the Preloader initializes the three lazy fields in MapIcons consecutively (!!), this could also easily be parallelized. 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 also wonder if getting the bitmap from resources is an IO operation? I.e. withContext(IO) {…} should be used? Or is everything in the res directory already in memory when starting the app?) However, this has nothing to do with this directly, but something that you could try out on the main PR, if you like.

@westnordost westnordost merged commit b7d6e74 into Helium314:maplibre Apr 19, 2024
@FloEdelmann FloEdelmann deleted the maplibre-sdf branch April 19, 2024 12:31
@westnordost
Copy link
Collaborator Author

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 maplibre/maplibre-native#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
Copy link
Owner

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
Copy link
Collaborator Author

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...

@westnordost westnordost mentioned this pull request Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants