fix: controller refcount out of sync with view #205
Conversation
| if (riveAnimationView.controller.refCount == 0) { | ||
| riveAnimationView.controller.acquire(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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?
| 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
Fix is taken from rive-app#205 We will be using this forked repo until the PR is merged to the original repo
Fixes: #198
This may need to be revisited on native Android, as there could be some discussion on making the current controller refcount more decoupled to how it is currently handled. But I could not replicate the issue natively, and it could be that ReactNative is managing the view is unique and not something that would cause issues in native use.
It may also be that we change the view creation logic in ReactNative in the future, so I'm opting in for a patch that ensures RN behaves sensibly with regards to the controller ref count on view navigation. Also adding a lot of code comments + considerations/alternatives.