Detach old animated styles and props

[tomekzaw]

Description

This pull request fixes a bug appearing when using animated styles (or props) conditionally. The root cause is that animated styles (or props) are not properly detached.

Before	After
animated_styles_before.mp4	animated_styles_after.mp4
animated_props_before.mp4	animated_props_after.mp4

Changes

• Added missing calls to detach animated styles and props

Test code and steps to reproduce

AnimatedStylesExample.tsx

import Animated, {
  useAnimatedStyle,
  useSharedValue,
  withTiming,
} from 'react-native-reanimated';
import { Button, View } from 'react-native';

import React from 'react';

export default function Example() {
  const [state, setState] = React.useState(0);

  const x = useSharedValue(0);

  const style1 = useAnimatedStyle(() => {
    return { marginLeft: 250 * x.value };
  });

  const style2 = useAnimatedStyle(() => {
    return { marginTop: 250 * x.value };
  });

  const handleToggleSharedValue = () => {
    x.value = withTiming(1 - x.value, { duration: 1000 });
  };

  const handleToggleState = () => {
    setState((state) => state + 1);
  };

  return (
    <>
      <View style={{ paddingVertical: 50 }}>
        <Button onPress={handleToggleSharedValue} title="Toggle shared value" />
        <Button onPress={handleToggleState} title="Toggle state" />
      </View>
      <Animated.View
        style={[
          {
            width: 100,
            height: 100,
            backgroundColor: state % 2 === 0 ? 'red' : 'blue',
          },
          state % 2 === 0 ? style1 : style2,
        ]}
      />
    </>
  );
}

AnimatedPropsExample.tsx

import Animated, {
  useAnimatedProps,
  useSharedValue,
  withTiming,
} from 'react-native-reanimated';
import { Button, View } from 'react-native';
import Svg, { Path } from 'react-native-svg';

import React from 'react';

const AnimatedPath = Animated.createAnimatedComponent(Path);

export default function AnimatedPropsExample() {
  const [state, setState] = React.useState(0);

  const radius = useSharedValue(50);

  const animatedProps1 = useAnimatedProps(() => {
    // draw a circle
    const path = `
    M 100, 100
    m -${radius.value}, 0
    a ${radius.value},${radius.value} 0 1,0 ${radius.value * 2},0
    a ${radius.value},${radius.value} 0 1,0 ${-radius.value * 2},0
    `;
    return {
      d: path,
    };
  });

  const animatedProps2 = useAnimatedProps(() => {
    // draw a circle
    const path = `
    M 200, 100
    m -${radius.value}, 0
    a ${radius.value},${radius.value} 0 1,0 ${radius.value * 2},0
    a ${radius.value},${radius.value} 0 1,0 ${-radius.value * 2},0
    `;
    return {
      d: path,
    };
  });

  const handleToggleSharedValue = () => {
    radius.value = withTiming(50 - radius.value, { duration: 1000 });
  };

  const handleToggleState = () => {
    setState((state) => state + 1);
  };

  return (
    <>
      <View style={{ paddingVertical: 50 }}>
        <Button onPress={handleToggleSharedValue} title="Toggle shared value" />
        <Button onPress={handleToggleState} title="Toggle state" />
      </View>
      <Svg>
        <AnimatedPath
          animatedProps={state % 2 === 0 ? animatedProps1 : animatedProps2}
          fill={state % 2 === 0 ? 'red' : 'green'}
        />
      </Svg>
    </>
  );
}

Checklist

• Included code example that can be used to test this change
• Updated TS types
• Added TS types tests
• Added unit / integration tests
• Updated documentation
• Ensured that CI passes

[kmagiera]

I'd extract this to a variable defined before the if statement to make it clear what this part of the condition means:

const hasReanimated2Props = this.props.animatedProps?.viewDescriptors || styles.length;

if (hasReanimated2Props && hostInstance?.viewConfig) {

[tomekzaw]

Done in f6b8560

[kmagiera]

document that in most of the cases views have only a single animated style and it remains unchanged (it is what useAnimatedStyle returns)

Also it'd be worth checking if we can actually use equality checks to compare useAnimatedStyle outputs

[tomekzaw]

Also it'd be worth checking if we can actually use equality checks to compare useAnimatedStyle outputs

Indeed, equality check didn't work as expected. Fixed in 21f456f. For now, comparing style.viewsRef should be enough, as this prop is assigned only once (so the reference is the same). In the future, we may introduce some auto-incremented ID field to useAnimatedStyle output.

[piaskowyk]

🚀

[mrousavy]

It looks like this PR has introduced performance issues, especially noticeable on older iPhones when animating a lot of values (e.g. in a Graph).
I'll investigate further.

[tomekzaw]

@mrousavy Thanks for letting us know. It seems like _attachAnimatedStyles is called only during render so it should not affect the performance of animations. Can you please share more details with us? In particular, please clarify if the performance issue is related to animated styles or animated props. A repro case would be really appreciated as well.

[mrousavy]

Yes, I'm investigating it! How do you guys profile performance in REA? systrace isn't very helpful on Android. Do you use the Xcode instruments on iOS?

[mrousavy]

I can definitely feel a regression in performance after upgrading from beta 3 to beta 4, not sure if this PR is the cause of that. @tomekzaw What's your discord #? I can send you the file in DM.

[tomekzaw]

@mrousavy I don't think we have any particular setup for profiling. We use Reanimated heavily in numerous apps that we develop here at Software Mansion so we constantly keep an eye on its performance in real-world production apps. Other than that, there are some heavy-duty examples (see https://twitter.com/kzzzf/status/1421104290085576710).

I can definitely feel a regression in performance after upgrading from beta 3 to beta 4, not sure if this PR is the cause of that.

Can you please undo the changes from this PR locally and check if you can still feel the regression?