fix: move bitmap recycling to onDetachedFromWindow #1542

WoLewicki · 2021-02-22T15:46:41Z

Summary

This PR fixes the issues in react-native-screens and probably other libraries using native Fragments for transitions on Android (see software-mansion/react-native-screens#773). It moves the recycling and setting mBitMap to null to onDetachedFromWindow when the view is removed from react view hierarchy. invalidate is not always the proper place for doing it since the native view can still be visible for the user after the invalidate is called (see this comment with video: software-mansion/react-native-screens#773 (comment)).

Test Plan

You can use the code example attached in the above comment.

What's required for testing (prerequisites)?

The project with svg view and react-native-screens and native-stack navigator.

What are the steps to reproduce (after prerequisites)?

Go back from the screen with an svg view.

Compatibility

OS	Implemented
Android	✅.

Checklist

I have tested this on a simulator

gustavopch · 2021-04-19T23:36:49Z

Hey, maintainers, please don't forget this PR. It prevents SVG icons from disappearing before the screen is fully hidden. I applied it with patch-package in my app and it seems to work fine. Pretty simple change, probably 2min to review. 😊

WoLewicki · 2021-04-20T10:59:10Z

This PR may not be enough in some cases, e.g. when using tab navigator, so I would treat it as a workaround for some cases right now. I'll try to update it with a more generic solution.

alexco2 · 2021-07-12T20:10:10Z

I tested it with stack navigator and react-native-screens in development environment. SVG elements that I animate with reanimated are not appearing/ animating anymore after applying this PR. I have not testet yet if this fixes the standard problem with static SVG's.

WoLewicki · 2021-07-13T08:42:24Z

@alexco2 can you provide a repo with reproduction of the problem when this PR applied?

alexco2 · 2021-07-13T09:03:04Z

@WoLewicki I tested it with static SVG's. This PR fixed the bug. I will try to provide a repo or post a code snipped with an animated SVG in the coming days.

hadnet · 2021-07-16T03:51:08Z

Hey guys, I'm using the react-native-svg 12.1.1 and my svgs disappears during animation transitions using react-native-screens/native-stack on Android (this issue here software-mansion/react-native-screens#773 (comment)). Has this problem already been resolved or not?

alexco2 · 2021-07-18T15:16:57Z

@WoLewicki sorry for being late. I don't have much time at the moment and don´t know when I will be able to give you an reproduction of the problem. I will try to do it asap.

WoLewicki · 2021-07-19T08:54:52Z

@hadnet does the issue appear when this PR is applied in your code?

hadnet · 2021-07-19T12:45:38Z

@hadnet does the issue appear when this PR is applied in your code?

It doesnt. When apply this PR the problem is solved! (I'm using static SVGs)

WoLewicki · 2021-07-19T13:07:29Z

Ok so it is up to the maintainers of this library to merge it.

alexco2 · 2021-07-31T18:56:46Z

@WoLewicki
As promised, here the snippet with a representation of the SVG animation I was referring to.
If your PR is applied, the animation stops working.

https://snack.expo.dev/@expoco2/animatedsvg

import React, { useEffect } from 'react';
import { Text, View, StyleSheet } from 'react-native';
import Animated, {
  useSharedValue,
  useAnimatedStyle,
  withRepeat,
  withTiming,
} from 'react-native-reanimated';
import { Svg, Circle } from 'react-native-svg';

const AnimatedCircle = Animated.createAnimatedComponent(Circle);

export default function App() {
  const Animation = useSharedValue(1);

  useEffect(() => {
    Animation.value = withRepeat(withTiming(0, { duration: 2000 }), -1, true);
  }, []);

  const animtedProps = useAnimatedStyle(() => {
    const r = Animation.value * 50;
    return { cx: '50', cy: '50', r, fill: 'green' };
  });

  return (
    <View style={styles.container}>
      <Svg width={'100%'} height={'100%'}>
        <AnimatedCircle animatedProps={animtedProps} />
        <Circle cx={200} cy={200} r={50} fill="yellow" />
      </Svg>
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    paddingTop: Constants.statusBarHeight,
    backgroundColor: '#ecf0f1',
    padding: 8,
  },
});

WoLewicki · 2021-08-04T16:05:59Z

@alexco2 it is probably due to the usage of invalidate in VirtualView: https://github.com/react-native-svg/react-native-svg/blob/38f52a5c1404f75f0e0ee390468b6416d1eeab23/android/src/main/java/com/horcrux/svg/VirtualView.java#L113, which is not used only when the component is removed in React tree, but when its props are changed too. When animating, probably invalidate is called many times and it should trigger recycling bitmap, which with my PR it doesn't do. We need to somehow get the information that the whole component has been removed from React hierarchy and then not trigger recycling of bitmap since it will be done in onDetachedFromWindow. I might be wrong though and it should be tested first.

alexco2 · 2021-08-04T16:15:42Z

@WoLewicki
Unfortunatly my skills in native/ java are from from enough to be of real help here. I´m happy though to support you in testing potential fixes.

WoLewicki · 2021-08-27T10:01:20Z

@alexco2 I added another implementation with a check for the source of invalidate action. Could you check if it resolves the problem and does not introduce any new ones?

alexco2 · 2021-08-27T10:17:48Z

@WoLewicki
Thank you! I will probably try it today and give feedback asap.

alexco2 · 2021-08-27T14:52:59Z

@WoLewicki
I tested your PR. It works great. The animations work again and the svg elements do not disappear after unmounting a screen. One thing i noticed is if you hot reload the app after changing something in e.g. a worklet, the path animations are broken again. After reloading the app, this problem is fixed though.

sommcz · 2022-02-10T13:59:17Z

Any updates on this issue?

thomasviaud · 2022-02-14T10:30:51Z

Yes, can you please provide updates please ?

WoLewicki · 2022-02-14T12:45:39Z

I think the only solution now is to use these changes with patch-package.

AlexanderEggers · 2022-04-24T06:09:53Z

android/src/main/java/com/horcrux/svg/RenderableViewManager.java

@@ -1284,6 +1284,8 @@ public void onChildViewAdded(View view, View view1) {
            @Override
            public void onChildViewRemoved(View view, View view1) {
                if (view instanceof VirtualView) {
+                    SvgView svgView = ((VirtualView) view).getSvgView();
+                    svgView.setRemovedFromReactViewHierarchy();


I tested your code and there is a chance that svgView is null. I would suggest doing a null check here - just to avoid potential crashes.

ovidiuwin · 2022-05-06T07:08:12Z

Any updates with this issue?

mordechaim · 2022-07-13T16:01:19Z

I applied this PR with patch-package. Now I'm facing issues with Victory charts not updating on props change.

Instead I changed it to the patch from software-mansion/react-native-screens#773 (comment)

react-native-svg+12.3.0.patch

diff --git a/node_modules/react-native-svg/android/src/main/java/com/horcrux/svg/SvgView.java b/node_modules/react-native-svg/android/src/main/java/com/horcrux/svg/SvgView.java
index 82dc328..fc5e60a 100644
--- a/node_modules/react-native-svg/android/src/main/java/com/horcrux/svg/SvgView.java
+++ b/node_modules/react-native-svg/android/src/main/java/com/horcrux/svg/SvgView.java
@@ -67,6 +67,7 @@ public class SvgView extends ReactViewGroup implements ReactCompoundView, ReactC
     }
 
     private @Nullable Bitmap mBitmap;
+    private boolean mRemovalTransitionStarted = false;
 
     public SvgView(ReactContext reactContext) {
         super(reactContext);
@@ -88,13 +89,33 @@ public class SvgView extends ReactViewGroup implements ReactCompoundView, ReactC
                 return;
             }
             mRendered = false;
-            ((VirtualView) parent).getSvgView().invalidate();
+            SvgView svgView = ((VirtualView) parent).getSvgView();
+            if(svgView != null) {
+                svgView.invalidate();
+            }
             return;
         }
-        if (mBitmap != null) {
-            mBitmap.recycle();
+        
+        if(!mRemovalTransitionStarted){
+            if (mBitmap != null) {
+                mBitmap.recycle();
+            }
+            mBitmap = null;
+        }
+    }
+            
+    @Override
+    public void startViewTransition(View view) {
+        super.startViewTransition(view);
+                    mRemovalTransitionStarted = true;
+    }
+            
+    @Override
+    public void endViewTransition(View view) {
+        super.endViewTransition(view);
+        if (mRemovalTransitionStarted) {
+            mRemovalTransitionStarted = false;
         }
-        mBitmap = null;
     }
 
     @Override

WoLewicki · 2022-08-26T14:04:39Z

Closing since #1844 has been merged. Feel free to comment there if there are any problems. @mordechaim could you post a reproduction of the problem in a form of an issue so we can work on it?

mordechaim · 2022-08-30T17:43:58Z

@WoLewicki I created a reproduction: https://github.com/mordechaim/SvgVictoryTest

There are 2 branches

svg@12+patch uses the patch I posted up here and works flawlessly
svg@13 uses the latest version without the patch, the UI locks up after 3 button presses

fix: move bitmap recycling to onDetachedFromWindow

7a25c17

WoLewicki mentioned this pull request Feb 22, 2021

SVG component becomes transparent when goBack software-mansion/react-native-screens#773

Closed

feat: check the invalidate source

207c660

AlexanderEggers reviewed Apr 24, 2022

View reviewed changes

WoLewicki changed the base branch from develop to main July 15, 2022 09:53

WoLewicki mentioned this pull request Aug 10, 2022

svg blinks on navigation #1830

Closed

WoLewicki mentioned this pull request Aug 23, 2022

fix: bitmap nulled before screen transition #1844

Merged

WoLewicki closed this Aug 26, 2022

mordechaim mentioned this pull request Sep 7, 2022

Android v13 breaks victory-native #1861

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: move bitmap recycling to onDetachedFromWindow #1542

fix: move bitmap recycling to onDetachedFromWindow #1542

WoLewicki commented Feb 22, 2021 •

edited

Loading

gustavopch commented Apr 19, 2021

WoLewicki commented Apr 20, 2021

alexco2 commented Jul 12, 2021 •

edited

Loading

WoLewicki commented Jul 13, 2021

alexco2 commented Jul 13, 2021

hadnet commented Jul 16, 2021

alexco2 commented Jul 18, 2021

WoLewicki commented Jul 19, 2021

hadnet commented Jul 19, 2021 •

edited

Loading

WoLewicki commented Jul 19, 2021

alexco2 commented Jul 31, 2021 •

edited

Loading

WoLewicki commented Aug 4, 2021

alexco2 commented Aug 4, 2021

WoLewicki commented Aug 27, 2021

alexco2 commented Aug 27, 2021

alexco2 commented Aug 27, 2021 •

edited

Loading

sommcz commented Feb 10, 2022

thomasviaud commented Feb 14, 2022

WoLewicki commented Feb 14, 2022

AlexanderEggers Apr 24, 2022

ovidiuwin commented May 6, 2022

mordechaim commented Jul 13, 2022 •

edited

Loading

WoLewicki commented Aug 26, 2022

mordechaim commented Aug 30, 2022

fix: move bitmap recycling to onDetachedFromWindow #1542

fix: move bitmap recycling to onDetachedFromWindow #1542

Conversation

WoLewicki commented Feb 22, 2021 • edited Loading

Summary

Test Plan

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Compatibility

Checklist

gustavopch commented Apr 19, 2021

WoLewicki commented Apr 20, 2021

alexco2 commented Jul 12, 2021 • edited Loading

WoLewicki commented Jul 13, 2021

alexco2 commented Jul 13, 2021

hadnet commented Jul 16, 2021

alexco2 commented Jul 18, 2021

WoLewicki commented Jul 19, 2021

hadnet commented Jul 19, 2021 • edited Loading

WoLewicki commented Jul 19, 2021

alexco2 commented Jul 31, 2021 • edited Loading

WoLewicki commented Aug 4, 2021

alexco2 commented Aug 4, 2021

WoLewicki commented Aug 27, 2021

alexco2 commented Aug 27, 2021

alexco2 commented Aug 27, 2021 • edited Loading

sommcz commented Feb 10, 2022

thomasviaud commented Feb 14, 2022

WoLewicki commented Feb 14, 2022

AlexanderEggers Apr 24, 2022

Choose a reason for hiding this comment

ovidiuwin commented May 6, 2022

mordechaim commented Jul 13, 2022 • edited Loading

WoLewicki commented Aug 26, 2022

mordechaim commented Aug 30, 2022

WoLewicki commented Feb 22, 2021 •

edited

Loading

alexco2 commented Jul 12, 2021 •

edited

Loading

hadnet commented Jul 19, 2021 •

edited

Loading

alexco2 commented Jul 31, 2021 •

edited

Loading

alexco2 commented Aug 27, 2021 •

edited

Loading

mordechaim commented Jul 13, 2022 •

edited

Loading