Optionally create NativeAnimated value with native flag

[ryangomba]

The issue this addresses was originally mentioned in #9120, but here it is again:

How to reproduce:

• Check out and open the example in [NativeAnimation] Problems & solutions #9120
• Run the animation forward
• Run the animation backward
• Toggle block 2 to show it

The problem:

You will see that the first block resets to opacity 1 unexpectedly. This is because when we originally create the block, it has a non-native opacity prop (opacity = 0.25). This value gets sent to react proper. After we mark the animated value as __native, however, this prop is removed from the component. When prevProps and nextProps are diffed, react notices that opacity has been removed and calls updateView with opacity: null, thereby resetting the value to 1.

The solution

In order to avoid this behavior, I propose that native animated values be marked as __native when they are created. This would ensure that all its children would also be marked as __native, and would make the entire animation tree easier to reason about. For now, we can make this an optional config per @janicduplessis suggestion.

[janicduplessis]

Would it be possible to update views when attaching a new animated prop to it in the same frame that react update / creates the view. I'm having another issue that might be related to this when creating a new view with an native animated prop that doesn't match the view's default.

For example you have

<Animated.View style={{ transform: [{ scale: animatedValue }] }} />

if at the time the view is created the animated value is already native and has a value of 0.5 for example the view won't have 0.5 scale untill an animation starts. Even if starting an animation at the same time as creating the view there would still be a flicker for a frame.

[ryangomba]

Couple questions:
(1) are you experiencing this on iOS or Android?
(2) if on iOS, with #10663 or without?

[janicduplessis]

@ryangomba Both iOS and Android with #10663

Right now I'm doing janicduplessis@ea63ade to fix it but that can cause other issues when rerendering with setState during an animation (which I'm not really doing atm in my app so that works for me).

[janicduplessis]

I'm pretty sure this happens because we're creating views without these props so there is a time where it uses default values (no transform).

[ryangomba]

@janicduplessis any chance you could wrap this bug up in a UIExplorer example and send to me?

[janicduplessis]

class Test extends React.Component {
  state = {
    anim: new Animated.Value(0),
    views: [],
  }

  componentWillMount() {
    this.state.anim.__makeNative();
  }

  _createView = () => {
    this.setState(state => ({ views: [...state.views, {}] }));
  };

  _animate = () => {
    Animated.timing(this.state.anim, {
      toValue: 1,
      duration: 500,
      useNativeDriver: true,
    }).start();
  };

  render() {
    return (
      <View>
        <TouchableWithoutFeedback onPress={this._createView}>
          <View>
            <Text>Add view</Text>
          </View>
        </TouchableWithoutFeedback>
        <TouchableWithoutFeedback onPress={this._animate}>
          <View>
            <Text>Animate</Text>
          </View>
        </TouchableWithoutFeedback>
        {this.state.views.map((v, i) => (
          <Animated.View key={i} style={[styles.block, { opacity: this.state.anim }]} />
        ))}
      </View>
    );
  }
}

You can add this in NativeAnimationsExample.js

[janicduplessis]

The bug happens here if you click create view a few times, the first one is fine (not sure why) but the other ones after have opacity=1 instead of 0. Then if you click animate it starts an animation and fixes the values.

[janicduplessis]

Actually it works on iOS with #10663 but not on Android. I still had the issue in my app I'll try to find a better repro.

[ryangomba]

I've got another diff coming your way to tighten up android invalidation - I think it will fix

[janicduplessis]

The issue still happens but I can't reproduce it in a simple UIExplorer example. What I think happens is:

1. JS sends command to create the view without native animated props
2. View is rendered <-- Bad frame
3. JS sends command to attach native animated value to the view
4. View is rendered <-- Good frame

It is only broken for 1 frame on iOS during screen transitions.

[ryangomba]

And does it happen if you mark the animated value as native when it is created?

[janicduplessis]

Yep the animated value has to be native already otherwise it works because we pass all the props on view creation.

[janicduplessis]

Passing native animated props to the component also fixes it like in this diff but will cause other issues janicduplessis@ea63ade

[janicduplessis]

I think the issue is that we create views and animated props in 2 separate JS to Native calls so if the bridge is busy and there is some delay this can happen in 2 different render frames so you see the flicker. I think to fix this we would need a way to have view creation be aware of native animated props but I don't see a simple way to do this now.

[hramos]

What are the next steps for this PR? Any requested changes?

[janicduplessis]

@ryangomba is this still needed with the updates to invalidations? Since we are removing the prop but updating it again when connecting the native animated prop the the view.

[ryangomba]

This is still very much needed. I think you are right that we still have a race that we need to address, but I'd prefer we loop back on that issue since you're right in saying the solution won't be simple.

[janicduplessis]

Ok, anyway thinking about it it makes more sense to have useNativeDriver on the AnimatedValue anyway since JS and Native driven animations don't really work together. I will avoid having some animation use the native driver and others not causing an error.

One thing we should add to this is check if the animated value is native already when we start an animation and use the native driver in that case. That way we can specify useNativeDriver only on the animated value. For example here https://github.com/facebook/react-native/blob/master/Libraries/Animated/src/AnimatedImplementation.js#L411.

[janicduplessis]

If this seems like a better pattern we can move forward with deprecating useNativeDriver on animations and animated.event in a future diff.

[ryangomba]

@janicduplessis I've adopted your suggestion, but apply the check earlier since the animation doesn't actually store a reference to the value it is driving.

[nihgwu]

any progress on this?

[lacker]

It looks like this pull request has been abandoned so I am going to close it. If someone is still working on this then please feel free to reopen it!

[Billy-]

@ryangomba @lacker @janicduplessis What needs to be done? This PR looks good to go, no?