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
5.85k stars 954 forks source link

focalX and focalY are wrong on android only #2138

Closed wood1986 closed 1 year ago

wood1986 commented 1 year ago

Description

I am trying to implement pinch to zoom and I pretty sure my math is correct. But the focalX and focalY are wrong on android only

Android android

iOS ios

On android, the first pinch-to-zoom behave "correct". The last pinch-to-zoom must be wrong. I tap two points and the coordinates are x = 194.86 y = 351.61 and x = 194.32 y = 396.36 . Then I pinch and the focal point is x = 195.93 y = 464.81. I expect the y-coord should be between two taps point. Now it is outside. 351.61 <= 464.81 <= 396.36 ❌ ❌ ❌ ❌. x-coord has the same issue.

When you take a look iOS, it behaves correctly 313 <= 351 <= 374 ✅✅✅✅

I think the bug is on either react-native or react-native-gesture-handler. I do not think it is react or react-native-reanimated bug

Platforms

Screenshots

Steps To Reproduce

  1. git clone https://github.com/wood1986/pinch-bug.git
  2. yarn android

Expected behavior

tap[0].x <= focalX <= tap[1].x tap[0].y <= focalY <= tap[1].y

Actual behavior

focalX <= tap[0].x or tap[1].x <= focalX focalY <= tap[0].y or tap[1].y <= focalY

Snack or minimal code example

Package versions

github-actions[bot] commented 1 year ago

Hey! 👋

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

Please complete Snack or minimal code example section.

j-piasecki commented 1 year ago

Hi! I tried to reproduce it on a simple view but it seems to be working fine in that case. Would you mind sharing the repository with the application you used to make a recordings?

wood1986 commented 1 year ago

In the repro step, I have included my sample code.

https://github.com/wood1986/pinch-bug.git

wood1986 commented 1 year ago

Thanks @j-piasecki

Here are my update

Point A from onManualTouchesDown callback:     {"allTouches": [{"id": 0, "x": 206.6055450439453, "y": 512.3021850585938}], "changedTouches": [{"id": 0, "x": 206.6055450439453, "y": 512.3021850585938}], "eventName": "5onGestureHandlerEvent", "eventType": 1, "handlerTag": 3, "numberOfTouches": 1, "state": 2}
Point A from onTapEnd callback:                {"handlerTag": 2, "numberOfPointers": 1, "oldState": 4, "state": 5, "x": 206.6055450439453, "y": 512.3021850585938}
Point B from onManualTouchesDown callback:     {"allTouches": [{"id": 0, "x": 237.4179229736328, "y": 508.9383850097656}], "changedTouches": [{"id": 0, "x": 237.4179229736328, "y": 508.9383850097656}], "eventName": "5onGestureHandlerEvent", "eventType": 1, "handlerTag": 3, "numberOfTouches": 1, "state": 2}
Point B from onTapEnd callback:                {"handlerTag": 2, "numberOfPointers": 1, "oldState": 4, "state": 5, "x": 237.4179229736328, "y": 508.9383850097656}
Point A + B from onManualTouchesDown callback: {"allTouches": [{"id": 0, "x": 210.2384490966797, "y": 512.436767578125}, {"id": 1, "x": 285.6670227050781, "y": 503.67486572265625}]}
Point A + B from onPinchStart callback:        {"focalX": 170.48695373535156, "focalY": 321.1535949707031, "handlerTag": 1, "numberOfPointers": 2, "oldState": 2, "scale": 1, "state": 4, "velocity": 0}

I tried to use Gesture.manual.onTouchesDown to keep track of the focal point. If you take a look the coordinate, there is a huge gap. pinch's focalY gives 321.15 but manual gives (503.67486572265625 + 512.436767578125) / 2 = 508.55

Not only this, I had a deep dive into this file ScaleGestureDetector.java. As you copy this from AOSP and it does not have a complex calculation for the focal point, it is hard for me to believe it is the bug on android side. But I have to say it is because motionEvent getX and getY return this value.

Does it make sense to you?

j-piasecki commented 1 year ago

I found the problem: https://github.com/software-mansion/react-native-gesture-handler/blob/3ff7674fe67041d5ca6e80caf768fb62908c11cc/android/lib/src/main/java/com/swmansion/gesturehandler/GestureHandlerOrchestrator.kt#L248-L254 Only the first point will have correct position, while others will still be scaled with respect to the upper-left corner of the view. I'll try to figure out a way to fix it without making breaking changes.

j-piasecki commented 1 year ago

@wood1986 As a workaround for now you can wrap the transformed Animated.View with a not transformed one, i.e.:

<GestureDetector gesture={Gesture.Race(tap, pinch)}>
  <Animated.View>
    <Animated.View collapsable={false} style={animatedStyle}>
      ...
    </Animated.View>
  </Animated.View>
</GestureDetector>

This way the coordinates will be consistent between tap and pinch.

wood1986 commented 1 year ago

Thanks @j-piasecki. Your workaround does not work because the coordinate of that workaround is not based on scaled view space. it is based on the parent view space. If I tap on the 4 corners on a scaled view, I expect it should not be [0 - width] or [0 - height]

wood1986 commented 1 year ago

I have just checked the history. This issue has been there for more than 3 years. Not sure if you can fix it quickly

j-piasecki commented 1 year ago

Yeah, unfortunately this is bigger than it looked like 😞. Fixing this properly would mean that all gestures would be calculated in the transformed coordinate space of the view, this would mean the speed of pan would depend on the scale, pan translation and the tap/long press coordinates affected by the rotation etc. That would also mean a reimplementing how the events are processed by the Gesture Handler. It goes without saying that it would be a huge breaking change and I'm not sure when (or even if) that would be done.

As to the workaround, you could make use of touch events the fact that the coordinates of the first pointer are correct - scaling the rest of the pointers with respect to the first one should allow you to calculate the correct focal point.

wood1986 commented 1 year ago

Thanks. BTW how come iOS does not this issue?

j-piasecki commented 1 year ago

Gesture Handler on iOS uses UIGestureRecognizers under the hood, they allow us to calculate the location with respect to any view thanks to locationInView method.

wood1986 commented 1 year ago

Hey @j-piasecki, as I really need this bug to be fixed otherwise android will never be able to have a perfect pinch to zoom for all react-native app, my question is if I can get the exact same locationInView functionality on android, will that help?

I also try to scaling the rest of pointer and it does not work. I took the transformation matrix and then apply it to second point. the value is the not correct. maybe my math was wrong.

j-piasecki commented 1 year ago

It's hard to say right now, I'll look more into this and come back to you. Meanwhile, if you want to try one more thing, you could use the additional Animated.View like in the snippet I sent earlier. The point will be in the parent coordinate system, so it should be possible to calculate the correct focal point using the same transformation as for the child view, although it may be a little tricky because of how transforms are applied.

wood1986 commented 1 year ago

Thanks @j-piasecki, just let you know I want to help and fix it fundamentally. I have been looking at the place setLocation and trying to apply matrix transformation to the second point to see if I can get the right coordinates. But no progress.

wood1986 commented 1 year ago

https://github.com/software-mansion/react-native-gesture-handler/blob/3ff7674fe67041d5ca6e80caf768fb62908c11cc/android/lib/src/main/java/com/swmansion/gesturehandler/GestureHandlerOrchestrator.kt#L248-L254

Based on the comment, I changed to this

// TODO: we may consider scaling events if necessary using MotionEvent.transform
// for now the events are only offset to the top left corner of the view but if
// view or any ot the parents is scaled the other pointers position will not reflect
// their actual place in the view. On the other hand not scaling seems like a better
// approach when we want to use pointer coordinates to calculate velocity or distance
// for pinch so I don't know yet if we should transform or not...

handler.view?.let {
  it.matrix.invert(inverseMatrix)
  event.transform(inverseMatrix)
}

It can return the touch position correctly. But it is flicking

Kapture 2022-08-04 at 19 13 11

Do you know why?

j-piasecki commented 1 year ago

Unfortunately not, I've also tried it (and got the same result) but I haven't investigated it yet.

j-piasecki commented 1 year ago

@wood1986 Could you check if https://github.com/software-mansion/react-native-gesture-handler/pull/2156 solves the problem for you? Just keep in mind that it's not production-ready and I haven't tested it that much, so it may be breaking other gestures.

wood1986 commented 1 year ago

Thanks @j-piasecki

With your fix, the pinch to zoom behaviour is definitely better than the current. Kapture 2022-08-08 at 12 12 10

I found an issue when zoom out Kapture 2022-08-08 at 12 13 59

I have a few questions.

  1. I have been looking at the flickering issue last weekend and I still cannot figure out. How did you fix flickering?
  2. May I know what your plan is for the draft? When do you think it will go production? How I can help you for test?
wood1986 commented 1 year ago

FYI: I do not know if you are using my repo for testing the fix. I did a clean up with a force push. you may need to clone again

j-piasecki commented 1 year ago

The flickering was caused because the scale factor was being calculated in the coordinate space of the view being scaled. Because of that, when the pointers moved the scale of the view would change, which in turn would change the position of the pointers relative to the view. I worked around it by calculating the scale factor in the coordinate view of the GestureHandlerRootView.

I will continue to work on the draft, trying to figure out a way to solve the issues and, hopefully, find a way to integrate it without (or as little as possible) breaking changes. As it's a relatively large change to the core of the library I cannot guarantee when it will be merged. As for the help, pointing out all the issues you find would be greatly appreciated 🙂.

j-piasecki commented 1 year ago

Also, the same thing happens on iOS:

https://user-images.githubusercontent.com/21055725/183613590-10a9c702-6b14-453f-91a1-08353452e259.mov

Since, we're not using any custom logic for scaling and transforming events there, this suggests that something may be wrong in your code.

wood1986 commented 1 year ago

Hey @j-piasecki, your PR fixes the major issue. But it still have some issue. Let me give you some context. I am trying to have the same pinch-to-zoom experience as the iOS built-in Photos app. And I am not sure if it is achievable

See Kapture 2022-08-22 at 23 08 34

When I try to pan with using 2 fingers and the existing scale, the image does not move. in iOS Photos, it moves

When I pinch and then pan using 2 fingers, the image moves but I move a opposite direction. in iOS Photos, it moves the right direction

As the event in Gesture.Pinch().onChange((event) => { }) does not have the touches information and it only has focal information, I try to put my matrix transformation logic inside onTouchesMove but it has the flickering issue. I want to know if onTouchesMove is supposed to handle the matrix transformation.

Kapture 2022-08-22 at 23 38 01

Here is the https://github.com/wood1986/pinch-bug/tree/p2z

j-piasecki commented 1 year ago

This should do the trick:

import React from 'react';
import { StyleSheet, SafeAreaView, View, Button } from 'react-native';
import {
  Gesture,
  GestureDetector,
  GestureHandlerRootView,
} from 'react-native-gesture-handler';
import Animated, {
  useAnimatedStyle,
  useSharedValue,
  useAnimatedRef,
  measure,
} from 'react-native-reanimated';
import { identity3, Matrix3, multiply3 } from 'react-native-redash';

function translateMatrix(matrix: Matrix3, x: number, y: number) {
  'worklet';
  return multiply3(matrix, [1, 0, x, 0, 1, y, 0, 0, 1]);
}

function scaleMatrix(matrox: Matrix3, value: number) {
  'worklet';
  return multiply3(matrox, [value, 0, 0, 0, value, 0, 0, 0, 1]);
}

const ImageViewer = () => {
  const ref = useAnimatedRef();
  const origin = useSharedValue({ x: 0, y: 0 });
  const transform = useSharedValue(identity3);
  const scale = useSharedValue(1);
  const translation = useSharedValue({ x: 0, y: 0 });

  const pinch = Gesture.Pinch()
    .onStart((event) => {
      const measured = measure(ref);
      origin.value = {
        x: event.focalX - measured.width / 2,
        y: event.focalY - measured.height / 2,
      };
    })
    .onChange((event) => {
      scale.value = event.scale;
    })
    .onEnd(() => {
      let matrix = identity3;
      matrix = translateMatrix(matrix, origin.value.x, origin.value.y);
      matrix = scaleMatrix(matrix, scale.value);
      matrix = translateMatrix(matrix, -origin.value.x, -origin.value.y);
      transform.value = multiply3(matrix, transform.value);
      scale.value = 1;
    });

  const pan = Gesture.Pan()
    .averageTouches(true)
    .onChange((event) => {
      translation.value = {
        x: event.translationX,
        y: event.translationY,
      };
    })
    .onEnd(() => {
      let matrix = identity3;
      matrix = translateMatrix(
        matrix,
        translation.value.x,
        translation.value.y
      );
      transform.value = multiply3(matrix, transform.value);
      translation.value = { x: 0, y: 0 };
    });

  const animatedStyle = useAnimatedStyle(() => {
    let matrix = identity3;

    if (translation.value.x !== 0 || translation.value.y !== 0) {
      matrix = translateMatrix(
        matrix,
        translation.value.x,
        translation.value.y
      );
    }

    if (scale.value !== 1) {
      matrix = translateMatrix(matrix, origin.value.x, origin.value.y);
      matrix = scaleMatrix(matrix, scale.value);
      matrix = translateMatrix(matrix, -origin.value.x, -origin.value.y);
    }

    matrix = multiply3(matrix, transform.value);

    return {
      transform: [
        { translateX: matrix[2] },
        { translateY: matrix[5] },
        { scaleX: matrix[0] },
        { scaleY: matrix[4] },
      ],
    };
  });

  return (
    <>
      <GestureDetector gesture={Gesture.Simultaneous(pinch, pan)}>
        <Animated.View
          ref={ref}
          collapsable={false}
          style={[styles.fullscreen]}>
          <Animated.Image
            source={require('./1.png')}
            resizeMode={'contain'}
            style={[styles.fullscreen, animatedStyle]}
          />
        </Animated.View>
      </GestureDetector>

      <View style={{ position: 'absolute', end: 0, backgroundColor: 'black' }}>
        <Button
          title="RESET"
          onPress={() => {
            transform.value = identity3;
          }}
        />
      </View>
    </>
  );
};

const styles = StyleSheet.create({
  fullscreen: {
    ...StyleSheet.absoluteFillObject,
    flex: 1,
    width: '100%',
    height: '100%',
    resizeMode: 'contain',
  },

  pointer: {
    width: 60,
    height: 60,
    borderRadius: 30,
    backgroundColor: 'red',
    position: 'absolute',
    marginStart: -30,
    marginTop: -30,
  },
});

const App = () => {
  return (
    <GestureHandlerRootView style={{ flex: 1 }}>
      <SafeAreaView style={{ flex: 1, backgroundColor: 'black' }}>
        <ImageViewer />
      </SafeAreaView>
    </GestureHandlerRootView>
  );
};

export default App;
wood1986 commented 1 year ago

Your code is perfect!!!!! Thank you so much

wood1986 commented 1 year ago

Originally, I thought 2 fingers pan were done in the pinch gesture. In fact, it is handled by pan gesture. I was wrong at the beginning. Without you I will never be able to implement that. Once you merge this code, I plan to add this example to this repo.

Maybe make a component for people to use

gaearon commented 7 months ago

Thanks a ton for https://github.com/software-mansion/react-native-gesture-handler/issues/2138#issuecomment-1231634779, this was the only example I could find that handles focal pinching/panning correctly. For future readers who also want to have pan constrained by the bounds (like in stock Android apps), I've extended this code to do that in https://github.com/bluesky-social/social-app/pull/1563 — feel free to have a look.