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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix spring bug with extra type verification #4512

Merged
merged 6 commits into from
Jun 12, 2023
Merged

Conversation

Latropos
Copy link
Contributor

@Latropos Latropos commented May 31, 2023

Summary

There is a need to check if velocity from previous animation is undefined.
(Hopefully such a bugs will be soon reported by typescript 馃槉)

Closes #4510

BEFOREAFTER
Screen.Recording.2023-05-31.at.14.58.58.mov
Screen.Recording.2023-05-31.at.14.55.20.mov
## Test plan Tested on added to our bouncing box animation sequence from this issue https://github.com//issues/4510:
    .onFinalize(() => {
+      offset.value = withSequence(
+        withTiming(20, { duration: 75 }),
+        withTiming(-20, { duration: 75 }),
+        withTiming(20, { duration: 75 }),
+        withSpring(0, { damping: 50, stiffness: 500, mass: 2.8 })
+      );
    });

@Latropos Latropos requested a review from tomekzaw May 31, 2023 12:59
@Latropos Latropos marked this pull request as ready for review May 31, 2023 13:02
@erie-e9
Copy link

erie-e9 commented Jun 2, 2023

I need this change, please 馃憤

src/reanimated2/animation/spring.ts Outdated Show resolved Hide resolved
src/reanimated2/animation/spring.ts Outdated Show resolved Hide resolved
@piaskowyk
Copy link
Member

piaskowyk commented Jun 6, 2023

I have found a issue when we set stiffness to 0:

Screen.Recording.2023-06-06.at.12.07.51.mov

Repro

code
import Animated, {
  useSharedValue,
  withTiming,
  withSpring,
  useAnimatedStyle,
  Easing,
} from 'react-native-reanimated';
import { View, Button } from 'react-native';
import React from 'react';

export default function AnimatedStyleUpdateExample(): React.ReactElement {
  const randomWidth = useSharedValue(10);
  const style = useAnimatedStyle(() => {
    return {
      width: withSpring(randomWidth.value, { duration: 1000, stiffness: 0 }),
    };
  });

  return (
    <View
      style={{
        flex: 1,
        flexDirection: 'column',
      }}>
      <Animated.View
        style={[
          { width: 100, height: 80, backgroundColor: 'black', margin: 30 },
          style,
        ]}
      />
      <Button
        title="toggle"
        onPress={() => {
          randomWidth.value = Math.random() * 300 + 50;
        }}
      />
    </View>
  );
}

@Latropos Latropos requested a review from piaskowyk June 6, 2023 13:11
@@ -112,6 +136,7 @@ export function withSpring(
animation: SpringAnimation
) {
return (
previousAnimation?.startTimestamp &&
Copy link
Member

Choose a reason for hiding this comment

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

I ask just in case 馃槄 Do you know that, when startTimestamp will be equal 0 then previousAnimation?.startTimestamp = false ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I do, we have already identical assertion in timing.ts

else {
animation.current = toValue;
animation.lastTimestamp = 0;
console.log('STOP');
Copy link
Member

Choose a reason for hiding this comment

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

we need to remove this console.log

Copy link
Contributor Author

@Latropos Latropos Jun 7, 2023

Choose a reason for hiding this comment

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

Can we add the line below to eslint config? I've tested it and it works good

 'no-console': ['error', { allow: ['warn', 'error'] }],

@@ -56,6 +73,16 @@ export function withSpring(
return true;
}

if (config.isWrongConfig) {
// We don't animate wrong config
if (config.useDuration) return false;
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to have invalid configuration when we computing duration? 馃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so I've added conditional to instantly return 0 in such a case.

src/reanimated2/animation/spring.ts Outdated Show resolved Hide resolved
@@ -56,6 +78,15 @@ export function withSpring(
return true;
}

if (config.configIsInvalid) {
// We don't animate wrong config
if (config.useDuration) return false;
Copy link
Member

Choose a reason for hiding this comment

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

I am afraid if this condition could be a cause of infinity loop. I think that we should end the animation here, why you want to continue animation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To wait the duration time.

@Latropos Latropos added this pull request to the merge queue Jun 12, 2023
Merged via the queue into main with commit 9b790a0 Jun 12, 2023
1 check passed
@Latropos Latropos deleted the @acynk/spring_fix branch June 12, 2023 08:03
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.

Reanimated v 3.2.0 has broken withSpring animations
3 participants