ModelViewer + SurfaceView within a RecyclerView crash

[srMarlins]

Describe the bug When attaching a SurfaceView to a ModelViewer within a RecyclerView, the view lifecycle isn't properly handled. A SurfaceView is detached from the window but not destroyed. When the SurfaceHolder.Callback attached within the UiHelper callbacks are invoked, the engine has been already been destroyed resulting in the provided stack trace.

To Reproduce Steps to reproduce the behavior:

1. Put multiple SurfaceView backed by ModelViewer into a RecyclerView
2. Scroll up or down until a view is detached from the window, but not destroyed, then scroll back to that view

Expected behavior Any cleanup/configuration (specifically with the engine) should be properly cleaned up/recreated when the view detaches/reattaches itself without relying on complete destruction/reconstruction of the view itself.

Logs

java.lang.IllegalStateException: Calling method on destroyed Engine
        at com.google.android.filament.Engine.getNativeObject(Engine.java:649)
        at com.google.android.filament.Engine.createSwapChain(Engine.java:293)
        at com.google.android.filament.Engine.createSwapChain(Engine.java:271)
        at com.google.android.filament.utils.ModelViewer$SurfaceCallback.onNativeWindowChanged(ModelViewer.kt:362)
        at com.google.android.filament.android.UiHelper.createSwapChain(UiHelper.java:570)
        at com.google.android.filament.android.UiHelper.access$100(UiHelper.java:120)
        at com.google.android.filament.android.UiHelper$1.surfaceCreated(UiHelper.java:405)
        at android.view.SurfaceView.updateSurface(SurfaceView.java:1173)
        at android.view.SurfaceView.lambda$new$0$SurfaceView(SurfaceView.java:173)
        at android.view.-$$Lambda$SurfaceView$w68OV7dB_zKVNsA-r0IrAUtyWas.onPreDraw(Unknown Source:2)
        at android.view.ViewTreeObserver.dispatchOnPreDraw(ViewTreeObserver.java:1093)
        at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:3096)
        at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1959)

Smartphone (please complete the following information):

• Device: Sony Xperia 5ii
• OS: Android 11

Additional context Right now it seems the only solution around this (while still using ModelViewer) is to force the RecyclerView to not recycle those views. This is possible via

override fun onBindViewHolder(holder: ViewHolder, position: Int) {
        holder.setIsRecyclable(false)
}

[romainguy]

The problem is deeper than this: ModelViewer should not destroy an Engine it didn't create, and creating an Engine per ModelViewer is expensive. If would be even more expensive to destroy/create the Engine on every attach/detach. The added problem here is that there's no good time other than at detach time to know when to destroy resources.

For the specific case of showing objects in a RecyclerView I would highly recommend handling this yourself as in this example (although it's in Compose): https://github.com/romainguy/sample-materials-shop/. This sample uses its own ModelViewer and does not destroy the Engine ever when the View is detached from the window. It also shares the Engine and various other resources between multiple Views, which is far more efficient.

CC @prideout

[romainguy]

tl;dr I recommend we completely change how ModelViewer work to be either:

• Not self-contained (like in the sample app I linked to)
• Safe but incredibly expensive in a RecyclerView (and properly document that it should probably not be used that way)

Alternatively we could try to add caches but this solution has its own issues.

[braveBo]

Hey @romainguy. Sorry repost here to prevent you from not seeing my message in that close PR :)

1. Could I understand that you suggest creating our own ModelViewer to share the Engine and various other resources between multiple Views? with names such as 'singleEngineModelViewer' or something like that.
2. Is it okay for me to create the new 'xxModelViewer' class under google/android/utils/ folder, making it shareable?

[romainguy]

1. That or change ModelViewer so it takes an Engine as a parameter instead of creating its own.
2. Why makes a new one instead of modifying the existing one?

[srMarlins]

Hey @romainguy my idea right now is to make 2 changes to ModelViewer to allow for more precise resource management:

1) Only allow the ModelViewer to create and manage the resources directly related to the scope of the view's rendering. It would be the View, Camera, and Renderer that would be managed by the ModelViewer (in relation to the engine).

2) Move model loading logic into a class called ModelLoader which handles the loading and management of the subsequent Asset. The idea here is to allow for the AssetLoader & ResourceLoader to be managed at a similar scope as the Engine & to eventually allow for caching of the assets/scenes by the caller

When planning these changes is it okay to modify the public api or should we make sure it remains backwards compatible?

[romainguy]

I'd like @prideout to participate in this discussion when he's back next week. But yes, overall it makes sense to me.

[prideout]

For the first baby step in fixing these issues, I think we should change the ModelViewer constructor so that clients MUST pass their own engine (currently it is optional), and it should definitely not call destroy in the detach handler.

For the next step after that, I think we should somehow add support for "list" scenarios while retaining the simplicity of ModelViewer. This might mean a new lower-level class that users don't need to create if they don't need.

We should also look into leveraging the Jetpack Lifecycle library. More info at:

https://developer.android.com/topic/libraries/architecture/lifecycle

[prideout]

@pixelflinger this bug has some notes that describe baby steps towards JetPack, it might be worth keeping.