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

Do not make a synchronous call when canceling animation. #6564

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Szymon20000
Copy link
Contributor

Summary

Screenshot 2024-10-02 at 14 00 50
I noticed in client's code that cancelAnimation called for few values takes almost a second.
That seems to be caused by the fact that it's used on every useShareValue hook on unmount and across multiple libraries build on the top of reanimated. Looks like getter is synchronous and blocks the js until we schedule a block on UI.
I suspect the idea was to make sure we use the same type as we can animate different types. I thought it can be possible to just take initial value if it's set. Please let me know if for some reason we have to make a synchronous call either way.

Test plan

  • Start animation and then cancel it with cancelAnimation function

@arasrezaei
Copy link

I noticed that on navigation backs, does that related to this?
BTW, What is the profiler name?

@Szymon20000
Copy link
Contributor Author

@tomekzaw Can I ask you for a bit of help here? :)

@joe-sam
Copy link

joe-sam commented Oct 5, 2024

It was interesting to note that an animation is cancelled by updating its corresponding SharedValue.

What happens to all the DerivedValue(s) dependent on the SharedValue(s) on CancelAnimation.

Does a batched call for cancellation with reverse tree dependency chain of responsibility of the derived animations automatically fire ?

@Amurmurmur
Copy link

@tomekzaw Please please please can we get this merged

@tomekzaw
Copy link
Member

tomekzaw commented Oct 7, 2024

Hey @Amurmurmur, right now we're in the process of releasing Reanimated 3.16.0 with support for React Native 0.76.

Once that's complete, we'll review any PRs that have been submitted in the meantime as our capacity is pretty limited.

I'd love to have this problem resolved but this PR is a significant change and we'll need to test it thoroughly prior to merging.

Copy link
Contributor

@tjzel tjzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a difficult problem, let me explain why:

There might be a situation, where:

  1. cancelAnimation is called on React runtime
  2. Slightly later another animation is started on UI runtime

This is fine with current implementation. cancelAnimation will synchronously stop the current one without interfering with the new animation. However, with your change we essentialy turn cancelAnimation to an asynchronous call.

Therefore, there might be a situation where another animation has started before previous got cancelled, resulting in the cancellation affecting the new animation. That's what I'm afraid of. Here's a video where I show this problem:

scary.mov
Reproduction snippet

import { StyleSheet, View, Button, Text } from 'react-native';

import React from 'react';

import Animated, {
useSharedValue,
useAnimatedStyle,
withTiming,
cancelAnimation,
runOnUI,
} from 'react-native-reanimated';
import { Gesture, GestureDetector } from 'react-native-gesture-handler';

export default function EmptyExample() {
const rotation = useSharedValue(0);
const opacity = useSharedValue(1);

const animatedStyle = useAnimatedStyle(() => {
return {
transform: [{ rotate: rotation.value + 'rad' }],
};
});

const touchOpacity = useAnimatedStyle(() => {
return {
opacity: opacity.value,
};
});

const tap = Gesture.Tap()
.onBegin(() => {
opacity.value = 0.5;
rotation.value = withTiming(rotation.value + Math.PI, {
duration: 2000,
});
})
.onEnd(() => {
opacity.value = 1;
});

return (

<Button
title="Start from React"
onPress={() =>
runOnUI(() => {
rotation.value = withTiming(rotation.value + Math.PI, {
duration: 2000,
});
})()
}
/>
<Button
title="Stop from React"
onPress={() => {
// Let's pretend React/UI is busy:
setTimeout(() => cancelAnimation(rotation), 500);
}}
/>

<Animated.View style={touchOpacity}>
Start from UI
</Animated.View>

<Animated.View style={[animatedStyle, styles.box]} />

);
}

const styles = StyleSheet.create({
container: {
flex: 1,
alignItems: 'center',
justifyContent: 'center',
},
box: {
marginTop: 20,
width: 100,
height: 100,
backgroundColor: 'powderblue',
},
});

This behavior isn't necessarily bad, but we can't just make such change overnight - it should be well documented and we should provide an escape hatch for the edge case I presented.

I think a reasonable solution here would be to:

  • Deprecate cancelAnimation.
  • Add new stopAnimation function. stopAnimation would be async on React runtime and sync on UI runtime. It would be the default to replace stopAnimation.
  • Add new stopAnimationSync function. It would be sync on both React and UI runtimes, therefore blocking. It would be the aforementioned escape hatch.

cc @tomekzaw @piaskowyk

Comment on lines +544 to +547
if (sharedValue._initial != null) {
sharedValue.value = sharedValue._initial;
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnacceptable, animations on cancel would restore to their initial state:

Untitled.mp4

@Szymon20000
Copy link
Contributor Author

Szymon20000 commented Oct 8, 2024

Thanks @tjzel for spending your time on it :)
We can just make runOnUI instead if you want to use the latest value to avoid setting the initial value. When I added it I assumed that people only use it on unmounting. Then we don't care much about the value.

runOnUI(() => {
'worklet' 
sv.value = sv.value;
})

I think the solution with sync and async version makes sense but to be honest I think the default one should be async. The main problem here is that we call cancelAnimation when useShareValue is being destroyed. In 99% of the cases we don't want to block the js thread. Imagine you have a component with 20 shared values (list with every item being animated). And every takes 20-100ms on unmount (in Release build).

@Szymon20000
Copy link
Contributor Author

Actually with the current approach you can also cancel new animation instead of the previous one.

  1. We synchronously read the old value
  2. On UI thread we replace the animation we wanted to cancel with a new one
  3. We schedule a async call to UI with setting a new value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants