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

fix: controller refcount out of sync with view #205

Merged
merged 3 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 51 additions & 1 deletion android/src/main/java/com/rivereactnative/RiveReactNativeView.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package com.rivereactnative
import android.widget.FrameLayout
import app.rive.runtime.kotlin.PointerEvents
import app.rive.runtime.kotlin.RiveAnimationView
import app.rive.runtime.kotlin.controllers.ControllerState
import app.rive.runtime.kotlin.controllers.ControllerStateManagement
import app.rive.runtime.kotlin.controllers.RiveFileController
import app.rive.runtime.kotlin.core.*
import app.rive.runtime.kotlin.core.errors.*
Expand All @@ -23,7 +25,7 @@ import java.io.UnsupportedEncodingException


class RiveReactNativeView(private val context: ThemedReactContext) : FrameLayout(context) {
private var riveAnimationView: RiveAnimationView = RiveAnimationView(context)
private var riveAnimationView: RiveAnimationView
private var resId: Int = -1
private var url: String? = null
private var animationName: String? = null
Expand All @@ -35,6 +37,8 @@ class RiveReactNativeView(private val context: ThemedReactContext) : FrameLayout
private var shouldBeReloaded = true
private var exceptionManager: ExceptionsManagerModule? = null
private var isUserHandlingErrors = false
@OptIn(ControllerStateManagement::class)
private var controllerState: ControllerState? = null;

enum class Events(private val mName: String) {
PLAY("onPlay"),
Expand All @@ -51,6 +55,8 @@ class RiveReactNativeView(private val context: ThemedReactContext) : FrameLayout
}

init {
riveAnimationView = RiveAnimationView(context)

val listener = object : RiveFileController.Listener {
override fun notifyLoop(animation: PlayableInstance) {
if (animation is LinearAnimationInstance) {
Expand Down Expand Up @@ -108,6 +114,50 @@ class RiveReactNativeView(private val context: ThemedReactContext) : FrameLayout
addView(riveAnimationView)
}

@OptIn(ControllerStateManagement::class)
fun dispose() {
removeView(riveAnimationView)
controllerState?.dispose();
}

@OptIn(ControllerStateManagement::class)
override fun onAttachedToWindow() {
// The below solves: https://github.com/rive-app/rive-react-native/issues/198
// When the view is returned to, we reuse the view's resources.
// For example, navigating to a new page and returning, or a TabView.
Comment on lines +124 to +127
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I guess this is due to RiveReactNativeView being still alive.
Yeah, with RiveAnimationView we currently make sure that all resources get cleared out and fully re-initialized onAttach. Restoring a controller is indeed more efficient because you don't have to rebuild a bunch of resources.

To handle these lifecycle situations you can also use a LifecycleObserver, which we use in RiveAnimationView

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@umberto-sonnino Exactly this! I was initially thinking of using LifecycleObserver but I can't override the default Rive LifecycleObserver and add additional behaviour on to that (unless I missed something). And longer term I wouldn't want these two implementation to get out of sync, for example if Rive Android adds additional logic to it.

I was thinking that instead Android should move its acquire call to the LifecycleObserver's onCreate, as it's calling release in dispose.

controllerState?.let {
riveAnimationView.restoreControllerState(it);
// The controller refCount is a combination of the creation of the controller and the
// initialization of the RiveArtboardRenderer (which calls acquire on the controller).

// This could be decoupled in future versions of the Android runtime, and this code
// may require updating.
//
// For now we're doing a safety check to only add an additional acquire if the refCount is 0.
// As the RiveFileController would automatically have incremented the refCount on instantiation,
// and decreased when navigating away. It's safe to assume it should not be 0 at this point as
// the view still exists.
if (riveAnimationView.controller.refCount == 0) {
riveAnimationView.controller.acquire();
Comment on lines +140 to +141
Copy link
Contributor

@umberto-sonnino umberto-sonnino Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just taking a peek at this, I'm not entirely sure this is a good idea. If a controller ref-count reached 0, all the resources associated with it have been released (i.e. File, Artboard, etc. have all been deleted). So if you restore a deleted controller, you're basically handing over broken/null resources to the View.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @HayesGordon and @zplata, this bit here is still problematic.
If this case is verified, the View will crash: once refCount reached 0, all the resources associated with it will be released

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@umberto-sonnino That doesn't appear to be the case though. With the sample provided the view works fine, also for the case where you navigate to a new screen and back

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmh, when is this branch being called?
Controllers are initialized with a refCount of 1, so it won't be called on first run.
When you navigate away (i.e. onDetachedFromWindow) this calls saveControllerState(), which ups the count by 1.
Does the controller ever reach 0? When will it be released?

// Another approach would be to recreate the entire RiveAnimationView and call addView again.
// But this does not work great due to this issue: https://github.com/facebook/react-native/issues/17968
// Additionally, it may not be the most optimal route as the entire view needs to be recreated
// including all of the attached resources.
}

// Re-add the view
addView(riveAnimationView)
}
super.onAttachedToWindow()
}

@OptIn(ControllerStateManagement::class)
override fun onDetachedFromWindow() {
controllerState = riveAnimationView?.saveControllerState();
removeView(riveAnimationView)
super.onDetachedFromWindow()
}

fun onPlay(animationName: String, isStateMachine: Boolean = false) {
val reactContext = context as ReactContext

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package com.rivereactnative

import android.util.Log
import com.facebook.react.bridge.ReadableArray
import com.facebook.react.common.MapBuilder
import com.facebook.react.uimanager.SimpleViewManager
Expand Down Expand Up @@ -115,6 +114,11 @@ class RiveReactNativeViewManager : SimpleViewManager<RiveReactNativeView>() {
return RiveReactNativeView(reactContext)
}

override fun onDropViewInstance(view: RiveReactNativeView) {
view.dispose();
super.onDropViewInstance(view)
}

override fun onAfterUpdateTransaction(view: RiveReactNativeView) {
super.onAfterUpdateTransaction(view)
view.update()
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "rive-react-native",
"version": "6.1.0",
"version": "6.1.1",
"description": "Rive React Native",
"main": "lib/commonjs/index",
"module": "lib/module/index",
Expand Down Expand Up @@ -149,4 +149,4 @@
]
]
}
}
}
Loading