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

Replace Tangram with Maplibre #5693

Open
wants to merge 292 commits into
base: master
Choose a base branch
from

Conversation

westnordost
Copy link
Member

@westnordost westnordost commented Jun 17, 2024

by @Helium314 and me.

This PR is the same as Helium314#516 only that it merges to vanilla StreetComplete.

Visible differences to Tangram-ES

  • improved style, e.g. private roads, rails and paths look better now, some issues with the old style were solved and bridges / tunnels render more correct now
  • can zoom in much further
  • quest dots are clustered on low zoom
  • no 3D building except in the buildings overlay. (They don't look so good with MapLibre)
  • icons in building overlay have no white outline anymore (technical limitation)
  • color of icons in places and things overlay adapts to day/night mode
  • color of text in overlays adapt to day/night mode
  • animation on selecting (quest) pin
  • (better) animation on selecting elements in an overlay
  • improved memory consumption
  • location accuracy circle doesn't get blurry when it is large
  • (of course, no tangram bugs. For example those flickering quest pins when zooming back out, that was horrible)
  • (... anything more?)

Blocked by

Known bugs

Remarks

  • displaying overlay and quests in the currently visible map section is implemented manually. Could potentially be removed when CustomGeometrySourceOptions.withClip always enabled? maplibre/maplibre-native#2262 is fixed
  • location dot, accuracy circle and view direction has been implemented manually because MapLibre's own LocationComponent didn't look good and felt jerky. If that changes in the future, the own rendering could be replaced

Helium314 and others added 30 commits February 20, 2024 20:57
- the single components own the sources now
- make sure that all of this UI stuff is called on the UI thread
@westnordost
Copy link
Member Author

The PR that maybe solves maplibre/maplibre-native#2410 has been merged and a new version was released. Would you test if you can still reproduce it now? I'd love to finally merge this to master.

(I tested whether it solves maplibre/maplibre-native#2372 - it doesn't. But in an emulator, I was never able to reproduce that also e.g. the cycleway overlay would not display in certain tiles, so maybe (hopefully) it really is a different issue.)

@westnordost
Copy link
Member Author

Also, when I first started it with the new version, it crashed with. I dimly remember that this was mentioned before, but I thought that we solved it?

Process: de.westnordost.streetcomplete.debug, PID: 10770
java.lang.IllegalStateException: Already resumed
	at kotlin.coroutines.SafeContinuation.resumeWith(SafeContinuationJvm.kt:44)
	at de.westnordost.streetcomplete.screens.main.map.maplibre.OfflineManagerKt$awaitDownload$2$1.onStatusChanged(OfflineManager.kt:84)
	at org.maplibre.android.offline.OfflineRegion$setObserver$1.onStatusChanged$lambda$0(OfflineRegion.kt:249)
	at org.maplibre.android.offline.OfflineRegion$setObserver$1.$r8$lambda$ppSc6Vjf3zgbrFuf7K1etl42M0w(Unknown Source:0)
	at org.maplibre.android.offline.OfflineRegion$setObserver$1$$ExternalSyntheticLambda2.run(D8$$SyntheticClass:0)
	at android.os.Handler.handleCallback(Handler.java:958)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loopOnce(Looper.java:205)
	at android.os.Looper.loop(Looper.java:294)
	at android.app.ActivityThread.main(ActivityThread.java:8177)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:971)

@Helium314
Copy link
Collaborator

but I thought that we solved it?

I also thought so. Maybe there are other ways to resume twice?

@Helium314
Copy link
Collaborator

Would you test if you can still reproduce it now?

Still happening, unfortunately.

@westnordost
Copy link
Member Author

westnordost commented Jul 3, 2024

Darn it! But thanks for testing!

@westnordost
Copy link
Member Author

@Helium314 As per suggestion by a maplibre developer, I attempted a workaround for the blocking issue maplibre-native#2410 in the branch maplibre-workaround - always remove all layers and source and re-add it whenever a new geojson is set. Would you test if this circumvents the issue?

@Helium314
Copy link
Collaborator

When switching from address overlay to some other overlay, I can reliably trigger that bug.

Other issues:
No clusters are displayed when zooming out.
No addresses are shown in address overlay.
Places overlay doesn't show any places (but things overlay works).

very interesting: everything works after starting the app once while connected to the internet.
I'll still try to reproduce that bug by switching between overlays more often, but this hidden internet dependency could explain why it sometimes works for me and sometimes not.
Though I'm not sure why (whatever is needed) isn't always downloaded together with map data. Or maybe it has only a very short cache lifetime?

Also got a crash first time switching overlays (never again afterwards)

java.util.ConcurrentModificationException
    at java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:757)
    at java.util.LinkedHashMap$LinkedValueIterator.next(LinkedHashMap.java:785)
    at de.westnordost.streetcomplete.screens.main.map.components.StyleableOverlayMapComponent.set(StyleableOverlayMapComponent.kt:458)
    at de.westnordost.streetcomplete.screens.main.map.StyleableOverlayManager.setStyledElements(StyleableOverlayManager.kt:167)
    at de.westnordost.streetcomplete.screens.main.map.StyleableOverlayManager.access$setStyledElements(StyleableOverlayManager.kt:33)
    at de.westnordost.streetcomplete.screens.main.map.StyleableOverlayManager$setStyledElements$1.invokeSuspend(Unknown Source:15)
    at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
    at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:104)
    at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:584)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:811)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:715)
    at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:702)
    Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@bc50a8e, Dispatchers.Default]

@Helium314
Copy link
Collaborator

Possibly the bug is related to this thing I had in the original PR:

  • 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

@westnordost
Copy link
Member Author

So, for the record, the issue is still reproducible with maplibre-workaround, right? On the contrary, it triggers some other issues that may or may not be additional bugs either because the workaround is not perfect or because of other bugs in MapLibre?

@Helium314
Copy link
Collaborator

So, for the record, the issue is still reproducible with maplibre-workaround, right?

Yes

On the contrary, it triggers some other issues that may or may not be additional bugs either because the workaround is not perfect or because of other bugs in MapLibre?

Here I'm not sure. I remember at least the clustering not working once or twice (but most of the testing is done on high zoom, so possibly I just didn't notice). Now that I noticed the connection to being offline, I can better re-check on the maplibre branch.

@Helium314
Copy link
Collaborator

One more test (maplibre branch), I did everything here sequentially (did not yet test what are the actually necessary parts to get into a broken state):

  1. reset app
  2. start when offline
  3. wait some time (minutes)
  4. go online
  5. zoom to city
  6. download data
  7. go offline
  8. clustering looks fine
  9. overlays work fine (all overlays, and switching between them)
  10. zoom out, move, zoom in
  11. switch to address overlay -> no addresses
    • that broken overlay bug triggers, e.g. when switching to sidewalk overlay
    • tile is still broken when switching overlay on broken zoom level
    • zoom in or out (more than 1 zoom level) and change overlay to make it look like it works
    • but changing overlay does not work properly from now on, the bug comes back again
    • I was able to fix it by going online, zoom out and change to address overlay

My first guess: there is some necessary data that is not downloaded (but then why did the address overlay work initially?)

@westnordost
Copy link
Member Author

Just to understand. You write "all overlays work fine", but then you say that when switching to Address overlay, it stops working. So you mean all overlays except the address overlay?

(I will also try to reproduce the issue given your recipe)

@Helium314
Copy link
Collaborator

Just to understand. You write "all overlays work fine", but then you say that when switching to Address overlay, it stops working. So you mean all overlays except the address overlay?

The address overlay is included here, it really did work initially. For whatever weird reason, after zooming out (iirc lit overlay was active), moving to a different (already downloaded) area, and zooming in, switching to address overlay did not show addresses any more.

@westnordost
Copy link
Member Author

Wow, I can reproduce this! So, this is not device-specific at all!

(And jesus, my phone turned into a heater... I hope it is just the debug mode and/or the issue that is triggering this)

@westnordost
Copy link
Member Author

For me, the address overlay did not show up at all. Switching back to other overlays produced the effect described in the issue. This seems to only happen on the first start of the app, i.e. when I remove the app from recents and start again, even if it is still offline, everything looks fine. Right?

@Helium314
Copy link
Collaborator

For me, the address overlay did not show up at all.

I just tried again, and for me the addresses showed up, but only in the tile where I first enabled an overlay. (and no zoom out was necessary)

@FloEdelmann FloEdelmann linked an issue Jul 20, 2024 that may be closed by this pull request
because users might add POIs between zoom 18 and 20 that already exist if they don't see them displayed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked blocked by another issue enhancement iOS necessary for iOS port
Projects
Status: Blocked
Development

Successfully merging this pull request may close these issues.

Explore migration from tangram to MapLibre
3 participants