software-mansion / react-native-gesture-handler

Declarative API exposing platform native touch and gesture system to React Native.
https://docs.swmansion.com/react-native-gesture-handler/
MIT License
6.12k stars 979 forks source link

Calling a function from within `Gesture.Pan().onUpdate()` makes the app crash if not explicitly marked as "worklet". #2300

Closed ldorigo closed 1 year ago

ldorigo commented 2 years ago

Description

Not entirely sure whether this issue is with Reanimated or RNGH, but since it happens inside a RNGH method I post it here.

I needed to do some processing in the .onUpdate function and decided to put it in a separate function as it was growing too long. This caused the application to crash exactly as that function gets called. I post the code below, sorry if I don't make a reproducible env but I already spent an enormous amount of time figuring this out. Hopefully this can still be useful to you.

import React from "react";
import { StyleSheet } from "react-native";
import { Gesture, GestureDetector } from "react-native-gesture-handler";
import Animated, {
  SharedValue,
  useAnimatedStyle,
  useSharedValue,
} from "react-native-reanimated";
import { useStoreActions, useStoreState } from "../../state/store";
import { SelectedImageInfo } from "../../state/backgroundImageState";

export default function BackgroundImage() {
  console.log("Rendering BackgroundImage");
  const selectedImageInfo = useStoreState(
    (state) => state.backgroundImageState.selectedImageInfo
  );
  const screenSizes = useStoreState((state) => state.screenSizes);

  const setDisplayedImageInfo = useStoreActions(
    (actions) => actions.backgroundImageState.setDisplayedImageInfo
  );

  const imageStyle = StyleSheet.create({
    image: {
      width: selectedImageInfo?.effectiveWidth,
      height: selectedImageInfo?.effectiveHeight,
      // width: screenSizes.width,
      // height: screenSizes.height,
    },
  });

  const offset = useSharedValue({ x: 0, y: 0 });

  const startOffset = useSharedValue({ x: 0, y: 0 });
  const scale = useSharedValue(1);
  const startScale = useSharedValue(1);

  const animatedStyle = useAnimatedStyle(() => {
    // console.log("Using Animated Style");
    return {
      transform: [
        { translateX: offset.value.x },
        { translateY: offset.value.y },
        { scale: scale.value },
      ],
    };
  });

  const panGesture = Gesture.Pan()
    .onBegin(() => {
      console.log("Starting Pan gesture, start offset: ", startOffset.value);
    })
    .onUpdate((event) => {
      // return;
      console.log("Updating Pan gesture");
      if (!selectedImageInfo) {
        return;
      }
      console.log("Gesture updated", event);
      // console.log("Previous offset: ", offset.value);
      // console.log("start: ", startOffset.value);

      const maybeNewX = startOffset.value.x + event.translationX;
      const maybeNewY = startOffset.value.y + event.translationY;
      console.log("calling determineNewOffsetPan");

      //  !!! Crashes here !!!
      const { newX, newY } = determineNewOffsetPan(
        maybeNewX,
        maybeNewY,
        selectedImageInfo,
        scale,
        screenSizes
      );

      console.log("finished computing new offset");
      offset.value = { x: newX, y: newY };
      console.log("set new offset (pan)");
    })
    .onEnd((event) => {
      startOffset.value = {
        x: offset.value.x,
        y: offset.value.y,
      };
    })
    .onFinalize(() => {
      console.log("Gesture finalized");
    });

  const zoomGesture = Gesture.Pinch()
    .onBegin(() => {
      console.log("Zoom Gesture began");
    })
    .onUpdate((event) => {
      console.log("Zoom Gesture updated", event);
      console.log("Previous scale: ", scale.value);
      if (!selectedImageInfo) {
        return;
      }
      // Get current X and Y offset:
      const currentX = offset.value.x;
      const currentY = offset.value.y;
      console.log("Current X: ", currentX);
      console.log("Current Y: ", currentY);
      // Compute the with and height that the image would have if it were scaled:
      const scaledWidth =
        selectedImageInfo.width * event.scale * startScale.value;
      const scaledHeight =
        selectedImageInfo.height * event.scale * startScale.value;
      console.log("Image size: ", scaledWidth, "x", scaledHeight);
      console.log("Screen size: ", screenSizes.width, "x", screenSizes.height);

      // // only scale if both the width and height are larger than the screen:
      // if (
      //   scaledWidth > screenSizes.width &&
      //   scaledHeight > screenSizes.height
      // ) {
      scale.value = startScale.value * event.scale;
      // }
      console.log("Final scale", scale.value);
    })
    .onEnd((event) => {
      console.log("Zoom Gesture ended");
      // If the scale is less than 1, reset it to 1:
      if (scale.value < 1) {
        scale.value = 1;
      }
      startScale.value = scale.value;
    })
    .onFinalize(() => {
      console.log("Zoom Gesture finalized");
    });

  const combined = Gesture.Simultaneous(panGesture, zoomGesture);
  return (
    <GestureDetector gesture={combined}>
      <Animated.Image
        style={[imageStyle.image, animatedStyle]}
        source={{ uri: selectedImageInfo?.uri }}
        resizeMode="cover"
        resizeMethod="scale"
      />
    </GestureDetector>
  );
}
function determineNewOffsetPan(
  maybeNewX: number,
  maybeNewY: number,
  selectedImageInfo: SelectedImageInfo,
  scale: SharedValue<number>,
  screenSizes: { width: number; height: number }
) {
  "worklet";
  console.log("New candidate offset: ", maybeNewX, maybeNewY);
  // Make sure we don't go out of bounds:
  let newX = 0;
  let newY = 0;

  if (
    maybeNewX < 0 &&
    maybeNewX + selectedImageInfo.effectiveWidth * scale.value >
      screenSizes.width
  ) {
    newX = maybeNewX;
  } else if (maybeNewX > 0) {
    newX = 0;
  } else if (
    maybeNewX + selectedImageInfo.effectiveWidth * scale.value <
    screenSizes.width
  ) {
    newX = screenSizes.width - selectedImageInfo.effectiveWidth * scale.value;
  }

  console.log(
    "Right border X: ",
    maybeNewX + selectedImageInfo.effectiveWidth * scale.value
  );
  if (
    maybeNewY < 0 &&
    maybeNewY + selectedImageInfo.effectiveHeight * scale.value >
      screenSizes.height
  ) {
    newY = maybeNewY;
  } else if (maybeNewY > 0) {
    newY = 0;
  } else if (
    maybeNewY + selectedImageInfo.effectiveHeight * scale.value <
    screenSizes.height
  ) {
    newY = screenSizes.height - selectedImageInfo.effectiveHeight * scale.value;
  }
  console.log("New offset: ", newX, newY);
  return { newX, newY };
}

function determineNewOffsetZoom(
  maybeNewX: number,
  maybeNewY: number,
  maybeNewScale: number,
  selectedImageInfo: SelectedImageInfo,
  screenSizes: { width: number; height: number }
) {
  // console.log("maybeNewX: ", maybeNewX);
  // console.log("maybeNewY: ", maybeNewY);
  console.log("New candidate scale: ", maybeNewScale);
  console.log("Current x: ", maybeNewX);
}

Steps to reproduce

See code above. Sorry I don't have the time to make a reproducible example now, hope this can still help.

Snack or a link to a repository

/

Gesture Handler version

2.5.0

React Native version

0.69.6

Platforms

Android

JavaScript runtime

Hermes

Workflow

Expo managed workflow

Architecture

Paper (Old Architecture)

Build type

Debug mode

Device

Real device

Device model

Blackview bv9900 pro

Acknowledgements

Yes

github-actions[bot] commented 2 years ago

Hey! 👋

It looks like you've omitted a few important sections from the issue template.

Please complete Snack or a link to a repository section.

github-actions[bot] commented 2 years ago

Hey! 👋

The issue doesn't seem to contain a minimal reproduction.

Could you provide a snack or a link to a GitHub repository under your username that reproduces the problem?

m-bert commented 2 years ago

Hi @ldorigo 👋

First of all, could You please make a snack from the code, or remove it from <details> tag? This way it will be easier to read. Also in that code you're using relative paths import, so it is hard to reproduce the behaviour that you get.

However, I've done some tests with onUpdate in our example app and it seems to work fine, even without "worklet". In details you say that you're using Gesture Handler 2.5.0, please update it to newer version (the latest is 2.8.0) and check whether your app still crashes.

ldorigo commented 2 years ago

Sorry, I didn't see that the <details> was messing up the formatting. I will try again later with the latest version.

j-piasecki commented 2 years ago

If I understood the problem correctly that's expected behavior. If you have react-native-reanimated installed, all gesture callbacks will be run on the UI thread, unless .runOnJS(true) modifier is used. In order for a function to be run on the UI thread it needs to be marked as worklet (otherwise the app will crash, because the function will not exist on the Reanimated's JS context on UI thread). Reanimated's babel plugin automatically does it for callbacks in the gesture builder chain, but if you're using another function inside, or defining callbacks out of the chain you need to mark the relevant functions yourself.

ldorigo commented 2 years ago

Fair enough. I don't think I saw that written anywhere explicitly in the docs, but I may be wrong (then again, the docs don't say that it should work - but it definitely wasn't clear to me that I had to mark that as a worklet :-)

On the other hand, it might be useful to show some kind of error message when this happens- currently the app just crashes with no output whatsoever and it took me ages to figure out what the problem was.

joaorr3 commented 1 year ago

If I understood the problem correctly that's expected behavior. If you have react-native-reanimated installed, all gesture callbacks will be run on the UI thread, unless .runOnJS(true) modifier is used. In order for a function to be run on the UI thread it needs to be marked as worklet (otherwise the app will crash, because the function will not exist on the Reanimated's JS context on UI thread). Reanimated's babel plugin automatically does it for callbacks in the gesture builder chain, but if you're using another function inside, or defining callbacks out of the chain you need to mark the relevant functions yourself.

@j-piasecki thanks man! .runOnJS(true) is exactly what i was looking for. 👏🏻

henninghall commented 1 year ago

I also received crashes on Andriod but when using the Gesture.LongPress().onStart function. (It worked just fine without this on iOS). This too was solvable by adding .runOnJs(true). Adding the error log here for visibility of this issue thread:

 E/libc++abi: terminating with uncaught exception of type facebook::jsi::JSError: Attempting to define property on object that is not extensible.