react-native-gesture-handler icon indicating copy to clipboard operation
react-native-gesture-handler copied to clipboard

focalX and focalY are wrong on android only

Open wood1986 opened this issue 2 years ago • 21 comments

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

  • [ ] iOS
  • [x] Android
  • [ ] Web

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

  • React: 18.0.0
  • React Native: 0.69.2
  • React Native Gesture Handler: 2.5.0
  • React Native Reanimated: 2.9.1

wood1986 avatar Jul 24 '22 08:07 wood1986

Hey! 👋

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

Please complete Snack or minimal code example section.

github-actions[bot] avatar Jul 24 '22 08:07 github-actions[bot]

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?

j-piasecki avatar Jul 25 '22 13:07 j-piasecki

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

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

wood1986 avatar Jul 25 '22 16:07 wood1986

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?

wood1986 avatar Jul 25 '22 18:07 wood1986

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 avatar Jul 26 '22 12:07 j-piasecki

@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.

j-piasecki avatar Jul 26 '22 13:07 j-piasecki

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 avatar Jul 26 '22 17:07 wood1986

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

wood1986 avatar Jul 26 '22 19:07 wood1986

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.

j-piasecki avatar Jul 27 '22 08:07 j-piasecki

Thanks. BTW how come iOS does not this issue?

wood1986 avatar Jul 27 '22 08:07 wood1986

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.

j-piasecki avatar Jul 27 '22 09:07 j-piasecki

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.

wood1986 avatar Jul 28 '22 19:07 wood1986

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.

j-piasecki avatar Aug 01 '22 10:08 j-piasecki

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 avatar Aug 01 '22 21:08 wood1986

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?

wood1986 avatar Aug 05 '22 02:08 wood1986

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

j-piasecki avatar Aug 05 '22 09:08 j-piasecki

@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.

j-piasecki avatar Aug 08 '22 13:08 j-piasecki

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 avatar Aug 08 '22 19:08 wood1986

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

wood1986 avatar Aug 08 '22 19:08 wood1986

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 avatar Aug 09 '22 07:08 j-piasecki

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.

j-piasecki avatar Aug 09 '22 09:08 j-piasecki

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

wood1986 avatar Aug 23 '22 06:08 wood1986

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;

j-piasecki avatar Aug 30 '22 13:08 j-piasecki

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

wood1986 avatar Sep 02 '22 06:09 wood1986

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

wood1986 avatar Sep 02 '22 06:09 wood1986

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.

gaearon avatar Sep 28 '23 17:09 gaearon