-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Operations order #2580
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
Operations order #2580
Conversation
|
Is anything holding back this PR? |
|
@hirbod I am just waiting for the review. |
kmagiera
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly we don't actually want to synchronize the access to _componentUpdateBuffer here. The only thing we want is to make some atomic flag that'd allow us to indicate there is something in that buffer such that these updates can be picked up when batch is finished processing. For that purpose it'd be safer and easier to use atomic operations and only synchronize that part. Note that the code that writes to the buffer is going to be executed more often than the code that only needs to know if there was anything added to the buffer. Moreover, if the latter reads that there is something but we already flushed it, it isn't going to have any side effects and the only issue is that we may sometimes add UI block that does nothing.
kmagiera
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, can we rename _isComponentUpdateBufferEmtpy to carry some meaning about what does setting this flag would mean. Also make that meaning "reverse", like for example "shouldRunUpdatesAfterFLush"
## Description We can't use just initial style as the default style in every render because these styles can be outdated. We can't change the default style after the first render, because after the second render we don't run mapper that's why the component can change the style to the initial value. Related: - #2580 - #2431 - #2406 - #1470 ### code Before https://user-images.githubusercontent.com/36106620/158142874-a11191e7-c0d9-4c3f-8f18-e5b540a6f17c.mov https://user-images.githubusercontent.com/36106620/158167177-81dfa334-db01-4e04-a234-e1069e8d715b.mov After https://user-images.githubusercontent.com/36106620/149799832-b0c0748d-2d9d-42b9-b9ba-f6492cc1fbf0.mov <details> <summary>code</summary> ```js import Animated, { useSharedValue, withTiming, useAnimatedStyle, Easing, } from 'react-native-reanimated'; import { View, Button } from 'react-native'; import React, { useState } from 'react'; export default function AnimatedStyleUpdateExample(props:any) { const randomWidth = useSharedValue(10); const [counter, setCounter] = useState(0); const [counter2, setCounter2] = useState(0); const [itemList, setItemList] = useState([]); const [toggleState, setToggleState] = useState(false); const config = { duration: 500, easing: Easing.bezier(0.5, 0.01, 0, 1), }; const style = useAnimatedStyle(() => { return { width: withTiming(randomWidth.value, config), }; }); const staticObject = <Animated.View style={[ { width: 100, height: 3, backgroundColor: 'black', margin: 1 }, style, ]} /> const renderItems = () => { let output = [] for(let i = 0; i < counter; i++) { output.push( <Animated.View key={i + 'a'} style={[ { width: 100, height: 3, backgroundColor: 'blue', margin: 1 }, style, ]} /> ) } return output } return ( <View style={{ flex: 1, flexDirection: 'column', marginTop: 30 }}> <Button title="animate" onPress={() => { randomWidth.value = Math.random() * 350; }} /> <Button title="increment counter" onPress={() => { setCounter(counter + 1) }} /> <Button title="add item to static lists" onPress={() => { setCounter2(counter2 + 1) setItemList([...itemList, <Animated.View key={counter2 + 'b'} style={[ { width: 100, height: 3, backgroundColor: 'green', margin: 1 }, style, ]} />]) }} /> <Button title="toggle state" onPress={() => { setToggleState(!toggleState) }} /> <Animated.View style={[ { width: 100, height: 3, backgroundColor: 'orange', margin: 1 }, style, ]} /> {toggleState && <Animated.View style={[ { width: 100, height: 3, backgroundColor: 'black', margin: 1 }, style, ]} />} {toggleState && staticObject} {renderItems()} {itemList} </View> ); } ``` </details> ### code2 Still works https://user-images.githubusercontent.com/36106620/149800303-4c4316aa-7765-4c66-a81a-74489d9f0215.mov <details> <summary>code2</summary> ```js import React from 'react'; import { View } from 'react-native'; import Animated, { useSharedValue, withSpring, useAnimatedStyle, useAnimatedGestureHandler, interpolate, Extrapolate, runOnJS, } from 'react-native-reanimated'; import { PanGestureHandler, PanGestureHandlerGestureEvent, } from 'react-native-gesture-handler'; import { useEffect, useState } from 'react'; function DragAndSnap(): React.ReactElement { const translation = { x: useSharedValue(0), y: useSharedValue(0), }; type AnimatedGHContext = { startX: number; startY: number; }; // run a couple of updates when gesture starts const [counter, setCounter] = useState(0); const makeFewUpdates = () => { let countdown = 100; const doStuff = () => { setCounter(countdown); countdown--; if (countdown > 0) { requestAnimationFrame(doStuff); } }; doStuff(); }; const gestureHandler = useAnimatedGestureHandler< PanGestureHandlerGestureEvent, AnimatedGHContext >({ onStart: (_, ctx) => { ctx.startX = translation.x.value; ctx.startY = translation.y.value; runOnJS(makeFewUpdates)(); }, onActive: (event, ctx) => { translation.x.value = ctx.startX + event.translationX; translation.y.value = ctx.startY + event.translationY; }, onEnd: (_) => { translation.x.value = withSpring(0); translation.y.value = withSpring(0); }, }); const stylez = useAnimatedStyle(() => { const H = Math.round( interpolate(translation.x.value, [0, 300], [0, 360], Extrapolate.CLAMP) ); const S = Math.round( interpolate(translation.y.value, [0, 500], [100, 50], Extrapolate.CLAMP) ); const backgroundColor = `hsl(${H},${S}%,50%)`; return { transform: [ { translateX: translation.x.value, }, { translateY: translation.y.value, }, ], backgroundColor, }; }); // make render slower let f = 0; for (var i = 0; i < 1e8; i++) { f++; } return ( <View style={{ flex: 1, margin: 50 }}> <PanGestureHandler onGestureEvent={gestureHandler}> <Animated.View style={[ { width: 40, height: 40, }, stylez, ]} /> </PanGestureHandler> </View> ); } export default DragAndSnap; ``` </details>
…OS (#3374) ## Description Hiya 👋 We stumbled on an issue where an animation was queued on an unmounted component (to fade in), and it would often either a) not fade in at all, or b) fade in after a second or two. This only occurred on iOS (simulator + device), and was introduced in 2.5.0. I narrowed the changeset to this PR: #2580 The problem is [this](https://github.com/software-mansion/react-native-reanimated/blob/03bdda7560d3fc89564df1a838fbb42c87026546/ios/REANodesManager.mm#L398) line. If the component is mounted but has a snapshot, it remains in the queue. There's no guarantee of the next time `-maybeFlushUpdateBuffer` will get called - it comes from `-[REAModule uiManagerWillPerformMounting:]`, so another component has to be mounted to flush the queue. As a result, the animation may appear halted forever, or randomly continue if you attach a component elsewhere in your tree. In a small repro video, you can see how it sometimes appears "stuck", but we can "update" to get it back on track: https://user-images.githubusercontent.com/33126/177866145-a25cad9c-047e-4755-8497-589c56c3c41b.mp4 ## Changes This PR updates the logic to separate the handling of "is the view mounted?" and "handle the snapshot". When the view is not mounted, it will continue to bail early (after updating the snapshot). However, if the view is mounted, and the snapshot exists, it will now proceed in the current update. ## Test code and steps to reproduce The repro relies on a race condition, so it's kinda tricky to reliably cause it. I use a utility called stress (`brew install stress`) and run `stress --cpu 6 --timeout 60` to increase likelihood of threading issues. The button "Start" will (re)start the animation. Press this over and over until you see the failure. When you get a failure, press the "force update" button. It's a no-op, but it causes `UIManager` (I guess?) to fire an event to clear the queue. <details> <summary>App.tsx</summary> ```typescript import React, {useCallback, useEffect, useState} from 'react'; import { Button, SafeAreaView, StatusBar, StyleSheet, Text, View, } from 'react-native'; import Animated, { Easing, interpolate, useAnimatedStyle, useSharedValue, withTiming, } from 'react-native-reanimated'; const App = () => { const [shouldStart, setShouldStart] = useState(false); const animateContent = useSharedValue(0); const start = useCallback(() => { animateContent.value = 0; animateContent.value = withTiming(1, { duration: 3000, easing: Easing.bounce, }); }, [animateContent]); useEffect(() => { if (shouldStart) { start(); setShouldStart(false); } }, [start, shouldStart, setShouldStart]); const onContentLayout = useCallback(() => {}, []); const contentStyle = useAnimatedStyle(() => ({ transform: [ { translateX: interpolate(animateContent.value, [0, 1], [50, 200]), }, ], opacity: interpolate(animateContent.value, [0, 1], [0.2, 1]), })); return ( <SafeAreaView style={{ backgroundColor: 'white', }}> <StatusBar barStyle={'light-content'} /> <Button title="Start" onPress={() => { setShouldStart(true); }} /> {!shouldStart ? ( <View style={[styles.container]}> <Button title="force update" /> <Animated.View onLayout={onContentLayout} style={[contentStyle]}> <Text>Bonjour! 👋</Text> </Animated.View> </View> ) : null} </SafeAreaView> ); }; const styles = StyleSheet.create({ container: { height: 200, width: '100%', top: 200, backgroundColor: 'rgba(127,127,127,0.5)', }, }); export default App; ``` </details> React Native 0.69.1 (also tested down to 0.66.x) Reanimated 2.9.1 (also tested 2.5.0, 2.8.0. however 2.4.1 is fine) ## Checklist - [x] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Added TS types tests - [ ] Added unit / integration tests - [ ] Updated documentation - [x] Ensured that CI passes
…OS (#3374) ## Description Hiya 👋 We stumbled on an issue where an animation was queued on an unmounted component (to fade in), and it would often either a) not fade in at all, or b) fade in after a second or two. This only occurred on iOS (simulator + device), and was introduced in 2.5.0. I narrowed the changeset to this PR: #2580 The problem is [this](https://github.com/software-mansion/react-native-reanimated/blob/03bdda7560d3fc89564df1a838fbb42c87026546/ios/REANodesManager.mm#L398) line. If the component is mounted but has a snapshot, it remains in the queue. There's no guarantee of the next time `-maybeFlushUpdateBuffer` will get called - it comes from `-[REAModule uiManagerWillPerformMounting:]`, so another component has to be mounted to flush the queue. As a result, the animation may appear halted forever, or randomly continue if you attach a component elsewhere in your tree. In a small repro video, you can see how it sometimes appears "stuck", but we can "update" to get it back on track: https://user-images.githubusercontent.com/33126/177866145-a25cad9c-047e-4755-8497-589c56c3c41b.mp4 ## Changes This PR updates the logic to separate the handling of "is the view mounted?" and "handle the snapshot". When the view is not mounted, it will continue to bail early (after updating the snapshot). However, if the view is mounted, and the snapshot exists, it will now proceed in the current update. ## Test code and steps to reproduce The repro relies on a race condition, so it's kinda tricky to reliably cause it. I use a utility called stress (`brew install stress`) and run `stress --cpu 6 --timeout 60` to increase likelihood of threading issues. The button "Start" will (re)start the animation. Press this over and over until you see the failure. When you get a failure, press the "force update" button. It's a no-op, but it causes `UIManager` (I guess?) to fire an event to clear the queue. <details> <summary>App.tsx</summary> ```typescript import React, {useCallback, useEffect, useState} from 'react'; import { Button, SafeAreaView, StatusBar, StyleSheet, Text, View, } from 'react-native'; import Animated, { Easing, interpolate, useAnimatedStyle, useSharedValue, withTiming, } from 'react-native-reanimated'; const App = () => { const [shouldStart, setShouldStart] = useState(false); const animateContent = useSharedValue(0); const start = useCallback(() => { animateContent.value = 0; animateContent.value = withTiming(1, { duration: 3000, easing: Easing.bounce, }); }, [animateContent]); useEffect(() => { if (shouldStart) { start(); setShouldStart(false); } }, [start, shouldStart, setShouldStart]); const onContentLayout = useCallback(() => {}, []); const contentStyle = useAnimatedStyle(() => ({ transform: [ { translateX: interpolate(animateContent.value, [0, 1], [50, 200]), }, ], opacity: interpolate(animateContent.value, [0, 1], [0.2, 1]), })); return ( <SafeAreaView style={{ backgroundColor: 'white', }}> <StatusBar barStyle={'light-content'} /> <Button title="Start" onPress={() => { setShouldStart(true); }} /> {!shouldStart ? ( <View style={[styles.container]}> <Button title="force update" /> <Animated.View onLayout={onContentLayout} style={[contentStyle]}> <Text>Bonjour! 👋</Text> </Animated.View> </View> ) : null} </SafeAreaView> ); }; const styles = StyleSheet.create({ container: { height: 200, width: '100%', top: 200, backgroundColor: 'rgba(127,127,127,0.5)', }, }); export default App; ``` </details> React Native 0.69.1 (also tested down to 0.66.x) Reanimated 2.9.1 (also tested 2.5.0, 2.8.0. however 2.4.1 is fine) ## Checklist - [x] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Added TS types tests - [ ] Added unit / integration tests - [ ] Updated documentation - [x] Ensured that CI passes
|
Fixes #2900 as well |
…OS (software-mansion#3374) ## Description Hiya 👋 We stumbled on an issue where an animation was queued on an unmounted component (to fade in), and it would often either a) not fade in at all, or b) fade in after a second or two. This only occurred on iOS (simulator + device), and was introduced in 2.5.0. I narrowed the changeset to this PR: software-mansion#2580 The problem is [this](https://github.com/software-mansion/react-native-reanimated/blob/03bdda7560d3fc89564df1a838fbb42c87026546/ios/REANodesManager.mm#L398) line. If the component is mounted but has a snapshot, it remains in the queue. There's no guarantee of the next time `-maybeFlushUpdateBuffer` will get called - it comes from `-[REAModule uiManagerWillPerformMounting:]`, so another component has to be mounted to flush the queue. As a result, the animation may appear halted forever, or randomly continue if you attach a component elsewhere in your tree. In a small repro video, you can see how it sometimes appears "stuck", but we can "update" to get it back on track: https://user-images.githubusercontent.com/33126/177866145-a25cad9c-047e-4755-8497-589c56c3c41b.mp4 ## Changes This PR updates the logic to separate the handling of "is the view mounted?" and "handle the snapshot". When the view is not mounted, it will continue to bail early (after updating the snapshot). However, if the view is mounted, and the snapshot exists, it will now proceed in the current update. ## Test code and steps to reproduce The repro relies on a race condition, so it's kinda tricky to reliably cause it. I use a utility called stress (`brew install stress`) and run `stress --cpu 6 --timeout 60` to increase likelihood of threading issues. The button "Start" will (re)start the animation. Press this over and over until you see the failure. When you get a failure, press the "force update" button. It's a no-op, but it causes `UIManager` (I guess?) to fire an event to clear the queue. <details> <summary>App.tsx</summary> ```typescript import React, {useCallback, useEffect, useState} from 'react'; import { Button, SafeAreaView, StatusBar, StyleSheet, Text, View, } from 'react-native'; import Animated, { Easing, interpolate, useAnimatedStyle, useSharedValue, withTiming, } from 'react-native-reanimated'; const App = () => { const [shouldStart, setShouldStart] = useState(false); const animateContent = useSharedValue(0); const start = useCallback(() => { animateContent.value = 0; animateContent.value = withTiming(1, { duration: 3000, easing: Easing.bounce, }); }, [animateContent]); useEffect(() => { if (shouldStart) { start(); setShouldStart(false); } }, [start, shouldStart, setShouldStart]); const onContentLayout = useCallback(() => {}, []); const contentStyle = useAnimatedStyle(() => ({ transform: [ { translateX: interpolate(animateContent.value, [0, 1], [50, 200]), }, ], opacity: interpolate(animateContent.value, [0, 1], [0.2, 1]), })); return ( <SafeAreaView style={{ backgroundColor: 'white', }}> <StatusBar barStyle={'light-content'} /> <Button title="Start" onPress={() => { setShouldStart(true); }} /> {!shouldStart ? ( <View style={[styles.container]}> <Button title="force update" /> <Animated.View onLayout={onContentLayout} style={[contentStyle]}> <Text>Bonjour! 👋</Text> </Animated.View> </View> ) : null} </SafeAreaView> ); }; const styles = StyleSheet.create({ container: { height: 200, width: '100%', top: 200, backgroundColor: 'rgba(127,127,127,0.5)', }, }); export default App; ``` </details> React Native 0.69.1 (also tested down to 0.66.x) Reanimated 2.9.1 (also tested 2.5.0, 2.8.0. however 2.4.1 is fine) ## Checklist - [x] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Added TS types tests - [ ] Added unit / integration tests - [ ] Updated documentation - [x] Ensured that CI passes
Description
Fixes #2345.
In some cases is possible to call
updatePropson the not yet mounted component. These updates were overridden by React Layout props. I detect this situation and save new props to buffer, and schedule updates of these props after mounting of component.Code to reproduce