Support Interpolation of strings when using native driver in Animated, fix Expected node to be marked as "native", optimize AnimatedNode creation and connections

[msand]

Summary

Allow interpolation of strings with useNativeDriver. This allows animating much more of react-native-svg. This PR adds support for native animation of lengths with units, path data, colors etc. Plus, fixing the redundantly created nodes and (and thus, previously incorrect) connection of native animated nodes, improving performance.

Related PRs

Docs will need to change, specifying that string interpolation works with the native driver as well.

Changelog:

[GENERAL] [Added] Add support for native driven string interpolation in Animated
[GENERAL] Fix exception: Expected node to be marked as "native"
[GENERAL] Fix connection of AnimatedNodes and creation of redundant AnimatedNodes

Test Plan

Make a set of tests, verifying that both number and string properties of svg content can be animated with the native driver. Showing that animation continues even if js thread is blocked.

[msand]

@hramos @grabbou Is it likely to get this merged? You know anyone I can ask / who can review this? Or should I change strategy?

[hramos]

cc @janicduplessis, @skevy

[grabbou]

I believe people quoted by @hramos are the best to review this change. Also @kmagiera

[janicduplessis]

Thanks for working on this! This only support interpolating a single number inside a string right? So something like "10deg" would work but not colors like "rgb(255, 255, 255)"? Also do we handle this in a similar way in the JS version of animated? It's usually a good place to get started to figure out where to go implementation wise.

I'm not a big fan of adding a stringValue prop to the AnimatedValue node but I can't really think of a better way that doesn't involve a lot more code changes. Ideally the value would be id / Object and could be accessed with asDouble or asString methods a bit like JavaOnlyMap. The problem is currently a lot of code just accesses the prop directly and assumes it's a double.

@kmagiera Any thoughts on this?

[msand]

@janicduplessis Yes, that's correct. I can add support for arbitrary number of occurrences of numbers in a string, this would be a bit more generic than just rgb, and would allow animating e.g. path data as well.

At least one difference to the js version, is that the native one could at least in theory have one interpolation output both a string and a number, such that the string can be used as an attribute, and the number can be used for further computation/interpolation.

Yeah, asDouble/String definitely seems nice, but probably a bigger change indeed.

[facebook-github-bot]

@msand I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[zachgibson]

@hramos Any idea of the status of this PR?

[msand]

@janicduplessis @hramos @skevy @kmagiera @zachgibson @shergin I've added support for native animation of rgb/rgba now as well, what do you think of merging this?

[msand]

I'm not completely satisfied with my current attempt at supporting native animation of svg. It requires adding a native view for each node, just to be able to set values on the shadow nodes. Does anyone have a better idea for how to achieve this? Or is this unavoidable with the current architecture? @sebmarkbage @sophiebits @rafeca @ss18
https://github.com/react-native-community/react-native-svg/pull/757/files#diff-50f7e7f8735df31a6ac08ae41511a2a6R34

Update: react-native-svg has been rewritten with Animated in mind. Using View on both Android and iOS in a symmetric way, and supporting length with both unitless primitive data types and with units as strings, when given as attribute/property values, in props, using setNativeProps, and in useNativeDriver: true animations. This PR would open up native animation of lengths with units, path data, and colors. Plus, fixing the redundantly created and connection of native animated nodes, improving performance.

[msand]

Here's some simple code I've used to exercise the native svg animation logic and demonstrating string animations of path data, rgba colors, and values with units:

import React, { Component } from 'react';
import { StyleSheet, View, Dimensions, Animated } from 'react-native';
import { Svg, Rect, Path } from 'react-native-svg';

const { width, height } = Dimensions.get('window');
const AnimatedRect = Animated.createAnimatedComponent(Rect);
const AnimatedPath = Animated.createAnimatedComponent(Path);

function getInitialState() {
  const anim = new Animated.Value(0);
  const fillOpacity = anim.interpolate({
    inputRange: [0, 1],
    outputRange: [0, 1],
  });
  const offset = fillOpacity.interpolate({
    inputRange: [0, 1],
    outputRange: [0, 10],
  });
  const strokeOpacity = offset.interpolate({
    inputRange: [0, 10],
    outputRange: [0, 1],
    extrapolateRight: 'clamp',
  });
  const path = anim.interpolate({
    inputRange: [0, 1],
    outputRange: ['M20,20L20,80L80,80L80,20Z', 'M40,40L33,60L60,60L65,40Z'],
  });
  const fill = strokeOpacity.interpolate({
    inputRange: [0, 1],
    outputRange: ['rgba(255, 0, 0, 0.5)', 'rgba(0, 255, 0, 0.99)'],
  });
  const oneToFivePx = offset.interpolate({
    inputRange: [0, 10],
    outputRange: ['1px', '5px'],
  });
  return { anim, fillOpacity, offset, strokeOpacity, path, fill, oneToFivePx };
}

export default class App extends Component {
  state = getInitialState();

  componentDidMount() {
    const { anim } = this.state;
    Animated.timing(anim, {
      toValue: 1,
      duration: 10000,
      useNativeDriver: true,
    }).start();
  }

  render() {
    const { fillOpacity, offset, strokeOpacity, path, fill, oneToFivePx } = this.state;
    return (
      <View style={styles.container}>
        <Svg width={width} height={height} viewBox="0 0 100 100">
          <AnimatedRect
            x="5"
            y="5"
            width="90"
            height="90"
            stroke="blue"
            fill={fill}
            strokeDasharray="1 1"
            strokeWidth={oneToFivePx}
            strokeDashoffset={offset}
            strokeOpacity={strokeOpacity}
            fillOpacity={fillOpacity}
          />
          <AnimatedPath
            d={path}
            stroke="blue"
          />
        </Svg>
      </View>
    );
  }
}

const styles = StyleSheet.create({
  container: {
    backgroundColor: '#ecf0f1',
  },
});

I'll try to rebase onto the latest master branch as well, as there's some quite useful fixes for the native animation merged recently. Should I push the rebased version or what is preferred here?

[msand]

Now with an expensive js computation to introduce some lag if useNativeDriver is false:

import React, { Component } from 'react';
import { StyleSheet, View, Dimensions, Animated } from 'react-native';
import { Svg, Rect, Path } from 'react-native-svg';

const { width, height } = Dimensions.get('window');
const AnimatedRect = Animated.createAnimatedComponent(Rect);
const AnimatedPath = Animated.createAnimatedComponent(Path);

function getInitialState() {
  const anim = new Animated.Value(0);
  const fillOpacity = anim.interpolate({
    inputRange: [0, 1],
    outputRange: [0, 1],
  });
  const offset = fillOpacity.interpolate({
    inputRange: [0, 1],
    outputRange: [0, 10],
  });
  const strokeOpacity = offset.interpolate({
    inputRange: [0, 10],
    outputRange: [0, 1],
    extrapolateRight: 'clamp',
  });
  const path = anim.interpolate({
    inputRange: [0, 1],
    outputRange: ['M20,20L20,80L80,80L80,20Z', 'M40,40L33,60L60,60L65,40Z'],
  });
  const fill = strokeOpacity.interpolate({
    inputRange: [0, 1],
    outputRange: ['rgba(255, 0, 0, 0.5)', 'rgba(0, 255, 0, 0.99)'],
  });
  const oneToFivePx = offset.interpolate({
    inputRange: [0, 10],
    outputRange: ['1px', '5px'],
  });
  return { anim, fillOpacity, offset, strokeOpacity, path, fill, oneToFivePx };
}

export default class App extends Component {
  state = getInitialState();

  expensiveComputation = () => {
    let result = 1;
    for (let i = 0; i < 100000000; i++) {
      result += i;
    }
    console.log(result);
    setTimeout(this.expensiveComputation, 200)
  };

  componentDidMount() {
    const { anim } = this.state;
    Animated.timing(anim, {
      toValue: 1,
      duration: 10000,
      useNativeDriver: true,
    }).start();
    setTimeout(this.expensiveComputation, 1000)
  }

  render() {
    const { fillOpacity, offset, strokeOpacity, path, fill, oneToFivePx } = this.state;
    return (
      <View style={styles.container}>
        <Svg width={width} height={height} viewBox="0 0 100 100">
          <AnimatedRect
            x="5"
            y="5"
            width="90"
            height="90"
            stroke="blue"
            fill={fill}
            strokeDasharray="1 1"
            strokeWidth={oneToFivePx}
            strokeDashoffset={offset}
            strokeOpacity={strokeOpacity}
            fillOpacity={fillOpacity}
          />
          <AnimatedPath
            d={path}
            stroke="blue"
          />
        </Svg>
      </View>
    );
  }
}

const styles = StyleSheet.create({
  container: {
    backgroundColor: '#ecf0f1',
  },
});

[msand]

Seems to work fine both when rebased onto v0.56 and onto v0.57.0-rc.0

[msand]

@cpojer Alright, good to know. But, this pr essentially just aligns the native animation with what the plain Animated api already does / is able to, when useNativeDriver is falsy. In a sense this could probably be considered a bugfix. But I understand if it counts as a feature and the window has closed.

[msand]

@janicduplessis Could you have another look at this? Or someone else on the team? @cpojer
The code should be relatively straightforward to follow with the comment. If there's any questions then I'm available to explain/comment further.

[todorone]

@janicduplessis @shergin @hramos @cpojer
Can we somehow push reviewing/merging this commit(it's more than 1 year old now 😸)?

Handling svg shapes animations on native side will open door to great performant vector animations which will be substantial step up for React Native. Currently only option of implementing such animations on RN is Lottie but it doesn't work well on Android and requires proprietary Adobe AfterEffects.

[msand]

@cpojer Would it be possible to get this into v0.61? useNativeDriver is currently quite broken, or at least very hard to make efficient, as you cannot use a single Animated.Value to drive multiple interpolations to set several properties on one View. For now, it requires finding very specific way to chain the interpolations together, for it not to crash and actually animate instead. That also makes the interpolations unnatural, as they need to use the range of values from the interpolation they're using as their input, rather than the simple Animated.Value which goes e.g. from 0 to 1.
To reproduce the exceptions, you can use the example I have used to test this pr, but set all the interpolations to use the Animated.Value, rather than have the very specific order in which they're chained now. So changing the getInitialState to something like this:

function getInitialState() {
  const anim = new Animated.Value(0);
  const fillOpacity = anim.interpolate({
    inputRange: [0, 1],
    outputRange: [0, 1],
  });
  const offset = anim.interpolate({
    inputRange: [0, 1],
    outputRange: [0, 10],
  });
  const strokeOpacity = anim.interpolate({
    inputRange: [0, 0.8],
    outputRange: [0, 1],
    extrapolateRight: 'clamp',
  });
  const path = anim.interpolate({
    inputRange: [0, 1],
    outputRange: ['M20,20L20,80L80,80L80,20Z', 'M40,40L33,60L60,60L65,40Z'],
  });
  const fill = anim.interpolate({
    inputRange: [0, 1],
    outputRange: ['rgba(255, 0, 0, 0.5)', 'rgba(0, 255, 0, 0.99)'],
  });
  const oneToFivePx = anim.interpolate({
    inputRange: [0, 1],
    outputRange: ['1px', '5px'],
  });
  return { anim, fillOpacity, offset, strokeOpacity, path, fill, oneToFivePx };
}

[cpojer]

Let's do it.

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @msand in 5e4a589.

^{When will my fix make it into a release? | Upcoming Releases}

[cpojer]

Unfortunately I have to unland this PR because it breaks external tests. Even worse, I cannot share that code externally but from what it looks like we have a some Android stuff that uses Animated and the element our test is looking for doesn't seem to show up any more with this diff applied :( Is there anything obvious that you could point to that may be the problem that you can fix and re-send the PR?

[msand]

Oh, I see. Well, we can probably fix it. Any chance of getting some minimal reproduction? My first main suspect would be the change of types crossing the bridge. It used to be Number in all cases, now it can also be a String. Try changing this one line:
https://github.com/facebook/react-native/pull/18187/files#diff-4d4bfac95b96bfe2f42b7eddd79418e1L275
It'll probably make it work, and would imply that some other part needs to check the type or adapt to the change some way.

[cpojer]

It seems like it broke more than just one thing and unfortunately it would take me a while to get a repro :( Your guess seems good. Is there any way you can repro elements not appearing with the existing PR and then making changes to fix it? In general, it's best if you can make sure we do not change the API between JS and native at the moment, and best not to change any JS only API unless there are flow types covering things that will make it easy to detect issues.

[msand]

@cpojer Is the issue on iOS, Android or both? Could it be related to this: #18872

[cpojer]

From what I could see, it happened on both platforms. Not sure if it's related to the other PR because that one landed in January.

[msand]

@cpojer Could you give some indication what types of components/properties/transforms are used in the failing test?

[cpojer]

Let's take this to Discord.