Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing style values from regular to Animated.Value removes view for several frames #219

Closed
terrysahaidak opened this issue Mar 24, 2019 · 23 comments
Assignees
Labels
Bug Important This seem to be a serious issue and we will need to take a deeper look into it some time soon

Comments

@terrysahaidak
Copy link
Contributor

Description

If you have an Animated.View with regular width: 100 style values and want to change it to an Animated.Value instance width: new Animated.Value(100), it removes the view completely from view hierarchy for several frames and mounts again.

Reproducing example

https://snack.expo.io/@terrysahaidak/reanimated-removes-view-bug

Video

https://drive.google.com/file/d/1HIfFt9vWX23lcnZukQSlw1n15-1ZWzcw/view?usp=drivesdk

Explanation

Why you ever want to change some values from animated one to regular?
The case I was playing with is lazy value initializations.
Since creating a lot of Animated.Value blocks the js thread (see #194) and I have a lot of them on the same screen, I do not initialize them until I start the animation by some event (for example, pressing the button or focusing the input).

@chrfalch
Copy link
Contributor

@terrysahaidak Did you ever find the cause of this issue in the Android source code? I'm struggling with the same issue...

@terrysahaidak
Copy link
Contributor Author

Hi @chrfalch, unfortunately not yet :( I believe @ddzirt has been able to found something.

@chrfalch
Copy link
Contributor

Ok - is @ddzirt listening in here? Would be great to try to work together to find a solution.

@ddzirt
Copy link

ddzirt commented Jun 27, 2019

@chrfalch Yes, I am investigating this when I am relatively free. From initial investigation - the issue is that view is recreated when animation starts.

@chrfalch
Copy link
Contributor

Thanks for reaching out, @ddzirt :) Yes, that is also what I've seen. Switching style from a regular style to an animated style causes the view to be recreated.

This is very visible on Android, but it is also looks like this is can be seen on iOS in some circumstances. Any idea what causes this view recreation?

@ddzirt
Copy link

ddzirt commented Jun 27, 2019

I went through node creation, and I think that Animated.Value is not used for the view before animation is actually triggered.

Regarding iOS I think there is the same issue, since I saw the same thing when I tried using new feature on iOS. But I saw that two days ago and had no time yet to confirm it in isolated project instead of work project

@chrfalch
Copy link
Contributor

Have you been able to identify the area in the source code that causes this recreation?

@ddzirt
Copy link

ddzirt commented Jun 27, 2019

Unfortunately not yet

New feature that gave me the same bug was Transition. I used the same code as in example

@chrfalch
Copy link
Contributor

I asked @osdnk about this as well. Hopefully we can find some kind of solution to this. Feel free to shout out if you want me to investigate or test out something. I'm currently working on a big rewrite of Fluid Transitions that uses Reanimated and this is what stops me at the moment.

@ddzirt
Copy link

ddzirt commented Jun 27, 2019

I am in a need as well, so I'll be investigating more and if I'll find something there will be more posts

@chrfalch
Copy link
Contributor

Ok, please don't hesitate contacting me if you find something.

@chrfalch
Copy link
Contributor

chrfalch commented Jul 9, 2019

I've investigated this issue and I think I have found the problem.

First of all - this is NOT an Android problem - this is happens on iOS as well (change title, @terrysahaidak ?), it just happens so quickly that it is hard to see it :)

The problem is evident if you attach a spy function to the message queue and is mostly visible for width/height/flex style properties (or style properties that forces a layout update).

The view is not recreated as we first assumed.

The issue is that when you update the styles the view will receive a call to update its properties through the UIManager updateView function - and the property value for the width/height/flex prop has a null value since the reanimated node has not yet been created. Creating the node happens after the call to updateView, and from there on everything is good.

I'm investigating the possibilities for a fix and will come back :)

@terrysahaidak terrysahaidak changed the title [Android] Changing style values from regular to Animated.Value removes view for several frames Changing style values from regular to Animated.Value removes view for several frames Jul 9, 2019
@ddzirt
Copy link

ddzirt commented Jul 9, 2019

@chrfalch nice, this at least gives us a proper understanding of what is happening

@chrfalch
Copy link
Contributor

chrfalch commented Jul 9, 2019

The problem seems to come from the createAnimatedComponent function's render method where animated nodes are removed from the style - which in turn causes React Native to emit a call to UIManager updateView with a null value for the width/height/flex value - ending up with a size equal to zero when recalculating sizes. The update from Reanimated arrives a little bit later, causing the view to flicker like it was removed for a frame or two.

@chrfalch
Copy link
Contributor

chrfalch commented Jul 9, 2019

Is this maybe something @osdnk or @kmagiera or someone else might have an idea on how to solve? Thanks in advance!

@ddzirt
Copy link

ddzirt commented Jul 10, 2019

@chrfalch I think we could try to investigate how original Animated with useNativeDriver works. At least that is my initial plan on trying to fix this.

@chrfalch
Copy link
Contributor

@ddzirt Another solution would be to take over all animatable properties and styles and not separate those that have animated nodes from those that doesn’t. This way reanimated would have full control over updating props on views without the synchronization issue. I’m have made a simple test doing this and it seems to be working fine.

@ddzirt
Copy link

ddzirt commented Jul 10, 2019

@chrfalch does it impact performance, by any chance? Especially if entire screen is fully covered by animations? I mean the loading/initialization time needed for reanimated to kick in. And if you have an example it would be great if you could share

@chrfalch
Copy link
Contributor

I'm traveling today so I won't be able to share anything - will try to see if I can get something together in the next few days. Don't think it will affect performance, what I'm suggesting is only to change updating props to "all from reanimated" instead of "some from react native, some from reanimated".

@ddzirt
Copy link

ddzirt commented Jul 10, 2019

No worries, I have free time only on weekends to play around the code, so I can wait.

About performance - it is just to be sure :)

@chrfalch
Copy link
Contributor

I've added a simple solution where I remove style from props in the Animated component. All styles for a component are transferred through an AnimatedStyle node - not just the style containing animated values.

The patch works on iOS - haven't got the time to fix it for Android, but a similar fix as the one I've done to the REAStyleNode.m should be implemented in the StyleNode.java (and utils.processMapping).

The only drawback I have found is that the component will be drawn once by React Native without styles before Reanimated applies styles. This should be fixable.

Here is my patch:

diff --git a/node_modules/react-native-reanimated/ios/Nodes/REAStyleNode.m b/node_modules/react-native-reanimated/ios/Nodes/REAStyleNode.m
index fbd80a7..e23bee1 100644
--- a/node_modules/react-native-reanimated/ios/Nodes/REAStyleNode.m
+++ b/node_modules/react-native-reanimated/ios/Nodes/REAStyleNode.m
@@ -21,7 +21,12 @@ - (id)evaluate
   NSMutableDictionary *styles = [NSMutableDictionary new];
   for (NSString *prop in _styleConfig) {
     REANode *propNode = [self.nodesManager findNodeByID:_styleConfig[prop]];
-    styles[prop] = [propNode value];
+      if(propNode) {
+        styles[prop] = [propNode value];
+      }
+      else {
+        styles[prop] = _styleConfig[prop];
+      }
   }
 
   return styles;
diff --git a/node_modules/react-native-reanimated/src/core/AnimatedNode.js b/node_modules/react-native-reanimated/src/core/AnimatedNode.js
index 914edcd..faefa5d 100644
--- a/node_modules/react-native-reanimated/src/core/AnimatedNode.js
+++ b/node_modules/react-native-reanimated/src/core/AnimatedNode.js
@@ -38,11 +38,11 @@ function runPropUpdates() {
   loopID += 1;
 }
 
-let nodeCount = 0;
+let nodeCount = -100000;
 
 export default class AnimatedNode {
   constructor(nodeConfig, inputNodes) {
-    this.__nodeID = ++nodeCount;
+    this.__nodeID = --nodeCount;
     this.__nodeConfig = sanitizeConfig(nodeConfig);
     this.__initialized = false;
     this.__inputNodes =
diff --git a/node_modules/react-native-reanimated/src/core/AnimatedStyle.js b/node_modules/react-native-reanimated/src/core/AnimatedStyle.js
index fefc318..000acf7 100644
--- a/node_modules/react-native-reanimated/src/core/AnimatedStyle.js
+++ b/node_modules/react-native-reanimated/src/core/AnimatedStyle.js
@@ -11,6 +11,12 @@ function sanitizeStyle(inputStyle) {
     const value = inputStyle[key];
     if (value instanceof AnimatedNode) {
       style[key] = value.__nodeID;
+    } else {
+      if (key.toLowerCase().includes("color")) {
+        style[key] = processColor(inputStyle[key]);
+      } else {
+        style[key] = inputStyle[key];
+      }
     }
   }
   return style;

@@ -51,7 +57,7 @@ export default class AnimatedStyle extends AnimatedNode {
       const value = style[key];
       if (value instanceof AnimatedNode) {
         updatedStyle[key] = value.__getValue();
-      } else if (value && !Array.isArray(value) && typeof value === 'object') {
+      } else if (value && !Array.isArray(value) && typeof value === "object") {
         // Support animating nested values (for example: shadowOffset.height)
         updatedStyle[key] = this._walkStyleAndGetAnimatedValues(value);
       }
diff --git a/node_modules/react-native-reanimated/src/createAnimatedComponent.js b/node_modules/react-native-reanimated/src/createAnimatedComponent.js
index bca82bc..96dabf6 100644
--- a/node_modules/react-native-reanimated/src/createAnimatedComponent.js
+++ b/node_modules/react-native-reanimated/src/createAnimatedComponent.js
@@ -202,8 +202,8 @@ export default function createAnimatedComponent(Component) {
       const props = {};
       for (const key in inputProps) {
         const value = inputProps[key];
-        if (key === 'style') {
-          props[key] = this._filterNonAnimatedStyle(StyleSheet.flatten(value));
+        if (key === "style") {
+          props[key] = null; //this._filterNonAnimatedStyle(StyleSheet.flatten(value));
         } else if (!(value instanceof AnimatedNode)) {
           props[key] = value;
         }

@MasterLambaster
Copy link

Is there any updates on this? This is still an issue and makes impossible to have any components that have any animated initial state along side with regular styling.

@jakub-gonet jakub-gonet added the Important This seem to be a serious issue and we will need to take a deeper look into it some time soon label Jul 24, 2020
@jakub-gonet
Copy link
Member

Can you guys check if #1027 fixes that issue for you?

@jakub-gonet jakub-gonet self-assigned this Jul 26, 2020
jakub-gonet added a commit that referenced this issue Jul 27, 2020
## Description

Fixes #219 
Fixes #762

Previously components were flickering when mounted due to asynchronously updating component styles.

In `createAnimatedComponent()` we filtered out styles with Animated nodes, then component rendered with default values,
then nodes were initialized on the native side and applied their styles.
This issue was mostly observed when layout styles were changed, for example when setting height like in the example below.

This fix relies on updating the value of `Value` when nodes are detached (`__detach()` in `InternalAnimatedValue`) because it updates its value on the JS side. Without that this fix would only work on the first mount (when initial values are used).

## Example

```js
import * as React from 'react';
import {Button, Text, View} from 'react-native';
import Animated from 'react-native-reanimated';

export default function Flickering() {
  const value = Animated.useValue(80);
  const [i, setI] = React.useState(0);
  const triggerRemount = () => {
    setI(i => i + 1);
  };
  return (
    <View
      key={i}
      style={{
        height: '100%',
        backgroundColor: 'white',
        position: 'relative',
        paddingTop: 100,
      }}>
      <Animated.View
        style={{
          height: value,
          position: 'absolute',
          width: '100%',
          backgroundColor: 'green',
        }}>
        <Text style={{color: 'white', fontSize: 15}}>Some footer</Text>
      </Animated.View>
      <Button
        onPress={() => {
          triggerRemount();
        }}
        style={{marginTop: 100}}
        title="Trigger remount!"
      />
    </View>
  );
}
```
jakub-gonet added a commit that referenced this issue Aug 12, 2020
Fixes #219
Fixes #762

Previously components were flickering when mounted due to asynchronously updating component styles.

In `createAnimatedComponent()` we filtered out styles with Animated nodes, then component rendered with default values,
then nodes were initialized on the native side and applied their styles.
This issue was mostly observed when layout styles were changed, for example when setting height like in the example below.

This fix relies on updating the value of `Value` when nodes are detached (`__detach()` in `InternalAnimatedValue`) because it updates its value on the JS side. Without that this fix would only work on the first mount (when initial values are used). It also doesn't support any nested properties (like transform).

```js
import * as React from 'react';
import {Button, Text, View} from 'react-native';
import Animated from 'react-native-reanimated';

export default function Flickering() {
  const value = Animated.useValue(80);
  const [i, setI] = React.useState(0);
  const triggerRemount = () => {
    setI(i => i + 1);
  };
  return (
    <View
      key={i}
      style={{
        height: '100%',
        backgroundColor: 'white',
        position: 'relative',
        paddingTop: 100,
      }}>
      <Animated.View
        style={{
          height: value,
          position: 'absolute',
          width: '100%',
          backgroundColor: 'green',
        }}>
        <Text style={{color: 'white', fontSize: 15}}>Some footer</Text>
      </Animated.View>
      <Button
        onPress={() => {
          triggerRemount();
        }}
        style={{marginTop: 100}}
        title="Trigger remount!"
      />
    </View>
  );
}
```
kmagiera pushed a commit that referenced this issue Aug 13, 2020
Fixes #219
Fixes #762

Previously components were flickering when mounted due to asynchronously updating component styles.

In `createAnimatedComponent()` we filtered out styles with Animated nodes, then component rendered with default values,
then nodes were initialized on the native side and applied their styles.
This issue was mostly observed when layout styles were changed, for example when setting height like in the example below.

This fix relies on updating the value of `Value` when nodes are detached (`__detach()` in `InternalAnimatedValue`) because it updates its value on the JS side. Without that this fix would only work on the first mount (when initial values are used). It also doesn't support any nested properties (like transform).

```js
import * as React from 'react';
import {Button, Text, View} from 'react-native';
import Animated from 'react-native-reanimated';

export default function Flickering() {
  const value = Animated.useValue(80);
  const [i, setI] = React.useState(0);
  const triggerRemount = () => {
    setI(i => i + 1);
  };
  return (
    <View
      key={i}
      style={{
        height: '100%',
        backgroundColor: 'white',
        position: 'relative',
        paddingTop: 100,
      }}>
      <Animated.View
        style={{
          height: value,
          position: 'absolute',
          width: '100%',
          backgroundColor: 'green',
        }}>
        <Text style={{color: 'white', fontSize: 15}}>Some footer</Text>
      </Animated.View>
      <Button
        onPress={() => {
          triggerRemount();
        }}
        style={{marginTop: 100}}
        title="Trigger remount!"
      />
    </View>
  );
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Important This seem to be a serious issue and we will need to take a deeper look into it some time soon
Projects
None yet
Development

No branches or pull requests

5 participants