software-mansion / react-native-reanimated

React Native's Animated library reimplemented
https://docs.swmansion.com/react-native-reanimated/
MIT License
8.83k stars 1.29k forks source link

Janky first few frames of simple width/height/transformX/transformY animation #1295

Closed drewandre closed 3 years ago

drewandre commented 3 years ago

First off, reanimated2 has been a game changer for my project. Thank you to Software Mansion for all their hard work as well as the awesome seminars they've been putting on!

Description

I've been building out <PopView /> component that behaves similarly to iOS control center modules. When activated, the module's width and height expand to a given maxWidth/maxHeight, and the module's view is translated to the center of the screen. Users can tap outside the module to reset the view back to it's original position using a full screen semi-opaque view behind the module. The animation's width, height, translateX and translateY values are all derived from a single progress animated value.

My issue is that the animation jumps around a lot in the very beginning when using withTiming and its default config. Using withSpring and the default spring config is significantly worse. It's almost like 60fps isn't enough for this animation, and running at higher speeds (like when using withSpring or a shorter withTiming duration) just results in skipped frames.

I guess what I'm really asking is: hypothetically, if I animate a view from the top of the screen to the bottom using withTiming and a crazy short duration of like 5ms, frames are going to be skipped, right? The view can't possibly be moved pixel-by-pixel on it's way to the bottom, otherwise give an animation duration of 5ms and a window height of 812 (iphone Xs max) you'd need an extremely high fps?

If it's hypothetically possible, my next questions are:

Below is a screen recording below where you can see 1) the full animation run, and then 2) a slowed down version showing the jumping around I'm seeing in the first few frames. I used withSpring to really highlight the issue.

ezgif com-gif-maker

Code

Relevant code is pasted below, but a full repo demonstrating the issue can be found here (and is just a fork of the 2.0.0-alpha.7 playground repo).

export function Box({
  id,
  position: {
    maxWidth = windowWidth * 0.9,
    maxHeight = windowHeight * 0.5,
    height,
    width,
    x,
    y
  },
  progress,
  activeModuleId,
  children
}) {

  function calculateProgress(p) {
    'worklet';
    if (activeModuleId.value === id || activeModuleId.value === null) {
      return p
    } else {
      return 0
    }
  }

  function calculateOpacity(p) {
    'worklet';
    if (activeModuleId.value === id || activeModuleId.value === null) {
      return 1
    } else {
      return 1 - p
    }
  }

  function calculateZ() {
    'worklet';
    if (activeModuleId.value === id || activeModuleId.value === null) {
      return 9999
    } else {
      return 0
    }
  }

  const moduleWidthOffset = maxWidth - width
  const moduleWidth = useDerivedValue(() => width + calculateProgress(moduleWidthOffset * progress.value))

  const moduleHeightOffset = maxHeight - height
  const moduleHeight = useDerivedValue(() => height + calculateProgress(moduleHeightOffset * progress.value))

  const translateXOffset = -x + (windowWidth - maxWidth) * 0.5
  const translateX = useDerivedValue(() => calculateProgress(translateXOffset * progress.value))

  const translateYOffset = -y + (windowHeight - maxHeight) * 0.5
  const translateY = useDerivedValue(() => calculateProgress(translateYOffset * progress.value))

  const animatedBoxStyles = useAnimatedStyle(() => ({
    top: y,
    left: x,
    width: moduleWidth.value,
    height: moduleHeight.value,
    zIndex: calculateZ(progress.value),
    opacity: calculateOpacity(progress.value),
    transform: [
      { translateX: translateX.value },
      { translateY: translateY.value },
    ]
  }))

  const onModuleTapGestureHandler = useAnimatedGestureHandler({
    onStart: () => {
      activeModuleId.value = id
      progress.value = withTiming(1, withTimingConfig)
    },
  })

  return (
    <TapGestureHandler onGestureEvent={onModuleTapGestureHandler}>
      <Animated.View style={[styles.box, animatedBoxStyles]}>
        {children}
      </Animated.View>
    </TapGestureHandler>
  );
}

Package versions

terrysahaidak commented 3 years ago

Could you please test your example but with changing this line to be

animation.lastTimestamp = now - 16.66;
drewandre commented 3 years ago

@terrysahaidak I made that change but it didn't seem to make any difference. I attached a gif below. What was the intent behind that change?

ezgif com-resize

terrysahaidak commented 3 years ago

Ok, just another issue I fixed with that change but seems to be not realted.

Why do you animate both y/x and translate? I think if you rewrite it to use only either translte of position it will fix it.

drewandre commented 3 years ago

Got it, worth a shot though. It looks like animating the x/y values instead of translating the entire view is working much better - buttery smooth. This can be mark this as closed unless you think this highlights some underlying issue with the translate transforms.

Thanks @terrysahaidak!

terrysahaidak commented 3 years ago

The reason behind this is that all layout props are animated asynchronously. But UI props – synchronously. So if your view depends on width/height – then it's better to animate top/left instead of translateY/translateX, so all the changes will happen in the same frame.

terrysahaidak commented 3 years ago

Hey @Szymon20000, is #1215 only about events?

drewandre commented 3 years ago

Thanks for the explanation @terrysahaidak -- I understand layout props, but what do you mean by UI props?

terrysahaidak commented 3 years ago

UI props are props that can be updated on UI thread synchronously. Basically, it's opacity, backgroundColor, and all the transforms. All the properties currently available with useNativeDriver in Animated.

Here is the full list: https://github.com/software-mansion/react-native-reanimated/blob/master/src/ConfigHelper.js#L6-L26

drewandre commented 3 years ago

Got it - so reanimated best practices would be to avoid mixing any of these properties that pertain to layout in a useAnimatedStyle worklet? (basically just avoid mixing layout props and transforms?)

terrysahaidak commented 3 years ago

Your case is more like an exception when you should not mix them because it's noticeable that they are async - because you transform the view and change its dimensions at the same time.

In the rest of the cases, it won't be noticeable. But it's something you should be aware of.

But, for example, changing them from an event handler will be sync because of #1215.

drewandre commented 3 years ago

Understood, thanks again for your explanation. Closing this issue now and have a nice weekend!

Szymon20000 commented 3 years ago

Hey @Szymon20000, is #1215 only about events?

No, it has an impact on every non-UI prop update. However, there are still cases when we cannot update those props synchronously.