mapbox / mapbox-gl-native

Interactive, thoroughly customizable maps in native Android, iOS, macOS, Node.js, and Qt applications, powered by vector tiles and OpenGL
https://mapbox.com/mobile
Other
4.36k stars 1.33k forks source link

Dynamic MapView in a RecyclerView Viewholder #13529

Closed ulusoyca closed 5 years ago

ulusoyca commented 5 years ago

I have a use case where I have to include a dynamic Mapbox MapView in a Recycler View. I know there have been threads regarding this, and the most common answer is as @tobrun says in this issue Interactive maps are too heavyweight for a fluent UX. Don't consider this issue actionable. However, I believe there are certainly use cases where this heavyweight UX can be tolerable. Here is my use case:

  1. There is a horizontal recycler view (carousel) with few items (let's say 5)
  2. Each item in recycler view is full screen. One of the items is MapView which shows the tracking of the user using LocationComponent
  3. User swipes left and right on the carousel to switch between the recycler view items.

I implemented this behavior so that when

This implementation works in the following way:

  1. Swiping to the MapView item, I can see the location tracking on mapview (yes it takes time to load)
  2. When MapView item is visible and the app put on the background and then foreground again, everything works fine. Location tracking continues.
  3. Swiping from MapView item to other items works fine as well.
  4. Finally, when revisiting the Mapview item the app crashes with the following log:

    A/libc: Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x6b in tid 3249 (GLThread 10877), pid 2746 ... dev/ashmem/dalvik-classes2.dex extracted in memory from /data/app/com.soy.android.wear.mock.debug-2VcGZQvZJpbQSfkq8rAZsw==/base.apk!classes2.dex (deleted) (com.mapbox.mapboxsdk.maps.renderer.MapRenderer.onSurfaceCreated) at com.mapbox.mapboxsdk.maps.MapView$5.onSurfaceCreated(MapView.java:315) What is causing this crash?

I set the number of ViewHolder in RecycledViewPool to be 1 so that only one instance of the Viewholder is present.

Here is the call list: (Map: ${(System.identityHashCode(this))}: Starting mapview)

Map: 24206533: Creating mapview Map: 24206533: Starting mapview Map: 24206533: Resuming mapview Map: 24206533: Stopping mapview Map: 24206533: Starting mapview Map: 24206533: Resuming mapview

ulusoyca commented 5 years ago

I am using Mapbox version 6.7.1

tobrun commented 5 years ago

@ulusoyca an example implementation of RecyclerView can be found here. Note that you need to use the textureView configuration vs the standard GLSurfaceView and it does require some changes that haven't been released yet. For this to work we will have to cherry-pick https://github.com/mapbox/mapbox-gl-native/commit/bb5b558f21c56872127540c7c5b0324d6286e505 and make a new 6.8 release. Keeping this issue open to track this.

ulusoyca commented 5 years ago

@tobrun Thanks for informing. I realized that PR after I opened the issue. I upgraded the version to 7.0.0-alpha to try out. I will see if it works in my specific case.

ulusoyca commented 5 years ago

Note that you need to use the textureView configuration vs the standard GLSurfaceView and it does require some changes that haven't been released yet.

@tobrun can you elaborate more about the changes that I should make in case I use the latest alpha version (7.0.0-alpha.3)?

tobrun commented 5 years ago

Make sure to call onDestroy in https://github.com/mapbox/mapbox-gl-native/blob/master/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/maplayout/RecyclerViewActivity.kt#L47

Hook into https://github.com/mapbox/mapbox-gl-native/blob/master/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/maplayout/RecyclerViewActivity.kt#L75 for start/resume flow

Hook into https://github.com/mapbox/mapbox-gl-native/blob/master/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/maplayout/RecyclerViewActivity.kt#L75 for stop/pause flow

ulusoyca commented 5 years ago

@tobrun thanks for great support. How important to call onLowMemory() and onSaveInstanceState mapView methods? I don't want to call those methods. What would be the cost? I am asking these questions because I can detect lifecycle methods that you mentioned above using the (context as LifecycleOwner).lifecycle.addObserver(this)

tobrun commented 5 years ago

they are not critical though onLowMemory would help out the Android system as we can clear out some memory so it can be reclaimed by the system.

OnSaveInstacneState is solely for convenience of saving state. eg. after rotation, your camera position is pointing to the same location on the map.

ulusoyca commented 5 years ago

@tobrun I implemented very similar logic with the example above. However, I still recieve the same error as I received in version 6.7.1

A/libc: Fatal signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0 in tid 11868 (GLThread 651)

Except this time it doesn't show com.mapbox.mapboxsdk.maps.MapView$5.onSurfaceCreated(MapView.java:315)

To reproduce the error:

  1. View attached, map is visible
  2. View detached, map is invisible
  3. Revisited the map item in the recycler view:
    override fun onViewAttachedToWindow(holder: RecyclerView.ViewHolder) {
        super.onViewAttachedToWindow(holder)
        if (holder is MapHolder) {
            val mapView = holder.mapView
            mapView.onStart()
            mapView.onResume()
        }
    }

    After above method is run, I see a black screen and then crash happens.

    A/DEBUG: signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0
    A/DEBUG: Cause: null pointer dereference
    ...
    018-12-10 16:39:55.247 11886-11886/? A/DEBUG: backtrace:
    2018-12-10 16:39:55.247 11886-11886/? A/DEBUG:     #00 pc 00000000  <unknown>
    2018-12-10 16:39:55.248 11886-11886/? A/DEBUG:     #01 pc 00021ffd  /data/app/{packageName}-fn4eA4JyrtJ-fnv043EPcw==/lib/arm/libmapbox-gl.so

    Any idea?

tobrun commented 5 years ago

@ulusoyca please compare your setup with the reference implementation found in https://github.com/mapbox/mapbox-gl-native/blob/master/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/maplayout/RecyclerViewActivity.kt

ulusoyca commented 5 years ago

@tobrun thanks for your support. I missed the app:mapbox_renderTextureMode="true" detail in item_map. Now it works! I am closing this since my question is solved. One minor detail though, when the map item is visible again, screen is temporarily black. Do you have any suggestions to solve this?

tobrun commented 5 years ago

Could you add a video or gif? happy to track that down with you or find ways to optimise it

ulusoyca commented 5 years ago

@tobrun I will send you soon a zip file in which there is a sample project that I can replicate it. Meanwhile, I have another question. According to the recyclerview documentation: OnViewRecycled:

A view is recycled when a RecyclerView.LayoutManager decides that it no longer needs to be attached to its parent RecyclerView. This can be because it has fallen out of visibility or a set of cached views represented by views still attached to the parent RecyclerView. If an item view has large or expensive data bound to it such as large bitmaps, this may be a good place to release those resources.

I have following calls:

... Detach Map: 217361391: OnViewRecycled Map: 217361391: DestroyMap ... Attach Map: 217361391: IsMapDestroyed: true Map: 217361391: OnCreate is called Map: 217361391: Starting mapview

The result is black screen. After onDestroy is called calling onCreate again doesn't crash but gives only black screen. Any suggestions? Should onDestroy be called only when activity and fragment onDestroy(), onDestroyView() events? Calling what method would be useful performance-wise on OnViewRecycled?

tobrun commented 5 years ago

As shown in the sample code, OnDestroy should only be called when the hosting activity is destroyed. Once a map is destroying you can restore it's state. On a new instance would work but for integration in a RecyclerView this is not advised.

ulusoyca commented 5 years ago

@tobrun I realized that the temporary black screen I mentioned in my above comment while swiping fast between RV items (stress test) is not coming from the Mapbox API. It is the background color when the mapview is not visible. Apparently, it takes time for mapview to become visible after the item is attached. Are there any tips to decrease the mapview loading time after on attached to the window?

Note that in my setup recyclerview is a carousel and each item is w:matchParent & h:matchParent.

ulusoyca commented 5 years ago

Currently my solution is this:

        <com.mapbox.mapboxsdk.maps.MapView
            android:id="@+id/mapView"
            style="@style/mapbox_Location"
            android:layout_width="match_parent"
            android:layout_height="match_parent"
            android:background="@drawable/map_placeholder"
            mapbox:mapbox_renderTextureMode="true" />

Adding a blurred static map placeholder image as background to avoid black background screen.

tobrun commented 5 years ago

You can also call MapView#setForeground(new ColorDrawable(someColorInt)); and hide it with MapView#setForeground(null)

ulusoyca commented 5 years ago

@tobrun thanks for the tip. Let's say I call the MapView#setForeground(new ColorDrawable(someColorInt)), how do I detect that Map is actually visible and I call MapView#setForeground(null)? Would be very useful also to detect this loading time for analytics purposes.

ulusoyca commented 5 years ago

I think this merged PR gives some tip about what I am after: https://github.com/mapbox/mapbox-gl-native/pull/12377/files.