[Fabric] Fixes findNodeAtPoint when views were inverted #45519

zhongwuzw · 2024-07-18T15:31:41Z

Summary:

FIxes #45502 . cc @realsoelynn

Changelog:

[GENERAL] [FIXED] - Fixes findNodeAtPoint when views were inverted

Test Plan:

Demo in #45502 .

facebook-github-bot · 2024-07-18T17:06:32Z

@realsoelynn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

realsoelynn · 2024-07-18T21:12:19Z

Thank you so much for quickly looking into the fix for this

Since this is relying on scaleX and scaleY, it will have issue when rotate is involved. (I added a new test case in the reproducer app as shown in the screenshot)

I am thinking of consolidating this findNodeAtPoint with computeRelativeLayoutMetrics as that is being used in measure function to know where in the screen it's being rendered as findNodeAtPoint has been a major area for bugs due to this two different approaches.

zhongwuzw · 2024-07-23T13:48:39Z

I'm not familiar with measurement, will find some time to try to figure it out :)

zhongwuzw · 2024-07-24T15:42:52Z

@realsoelynn Hi, sorry I have no solution to consolidate them right now. But I tried to fix your rotated repro demo :) .

facebook-github-bot · 2024-07-24T17:58:03Z

@realsoelynn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

realsoelynn · 2024-07-29T21:44:21Z

C++ Test for findNodeAtPoint

Just dumping this test case i wrote a while back. Please help me add to the PR

TEST(FindNodeAtPointTest, invertedList) {
  auto builder = simpleComponentBuilder();
  
  // clang-format off
  auto element =
    Element<ScrollViewShadowNode>()
      .props([] {
        auto sharedProps = std::make_shared<ScrollViewProps>();
        sharedProps->transform = Transform::VerticalInversion();
        return sharedProps;
      })
      .tag(1)
      .finalize([](ScrollViewShadowNode &shadowNode){
        auto layoutMetrics = EmptyLayoutMetrics;
        layoutMetrics.frame.size = {100, 200};
        shadowNode.setLayoutMetrics(layoutMetrics);
      })
      .children({
        Element<ViewShadowNode>()
        .tag(2)
        .finalize([](ViewShadowNode &shadowNode){
          auto layoutMetrics = EmptyLayoutMetrics;
          layoutMetrics.frame.origin = {0, 0};
          layoutMetrics.frame.size = {100, 100};
          shadowNode.setLayoutMetrics(layoutMetrics);
        }),
        Element<ViewShadowNode>()
        .tag(3)
        .finalize([](ViewShadowNode &shadowNode){
          auto layoutMetrics = EmptyLayoutMetrics;
          layoutMetrics.frame.origin = {0, 100};
          layoutMetrics.frame.size = {100, 100};
          shadowNode.setLayoutMetrics(layoutMetrics);
        })
    });
  // clang-format on

  auto parentShadowNode = builder.build(element);

  EXPECT_EQ(
      LayoutableShadowNode::findNodeAtPoint(parentShadowNode, {10, 10})
          ->getTag(),
      3);
  EXPECT_EQ(
      LayoutableShadowNode::findNodeAtPoint(parentShadowNode, {10, 105})
          ->getTag(),
      2);
}

realsoelynn · 2024-07-29T21:46:34Z

@zhongwuzw Please take a look at this other author PR #45571

Your current solution handled perfectly for horizontal and vertical inversion.

But, we want to to be able to handle different type of rotation transformation.

#45571 I don't agree with the Inversion check in that PR. But, the added new Point transformation function has the right idea to solve rotational transform

Cc: @coado

coado · 2024-07-30T06:46:56Z

@realsoelynn Is there any way to run cpp tests on emulator locally?

coado · 2024-07-30T08:12:18Z

@zhongwuzw Please take a look at this other author PR #45571

Your current solution handled perfectly for horizontal and vertical inversion.

But, we want to to be able to handle different type of rotation transformation.

#45571 I don't agree with the Inversion check in that PR. But, the added new Point transformation function has the right idea to solve rotational transform

Cc: @coado

Do you mean this inversion check?

zhongwuzw · 2024-07-30T09:55:38Z

Maybe we can combine the two PRs: one for a quick path to handle horizontal and vertical inversion, and another for a transform fallback to matrix inversion calculation ？

realsoelynn · 2024-07-30T17:22:36Z

@zhongwuzw Please take a look at this other author PR #45571
Your current solution handled perfectly for horizontal and vertical inversion.
But, we want to to be able to handle different type of rotation transformation.
#45571 I don't agree with the Inversion check in that PR. But, the added new Point transformation function has the right idea to solve rotational transform
Cc: @coado

Do you mean this inversion check?

I mean bool isInverted = currentTransform.inv();. inv() function is not only returning bool result for if it's inverted, but also mutating the current transform.inv() function should not mutate.

realsoelynn · 2024-07-30T17:24:44Z

Maybe we can combine the two PRs: one for a quick path to handle horizontal and vertical inversion, and another for a transform fallback to matrix inversion calculation ？

@zhongwuzw yep. do you mind pulling in @coado commit to this PR?

realsoelynn · 2024-07-31T05:20:33Z

packages/react-native/ReactCommon/react/renderer/graphics/Transform.cpp

+bool Transform::inv() {
+  if (*this == Transform::Identity()) {
+    return true;
+  }
+
+  double inv[16], det;
+  int i;
+  auto m = this->matrix;
+
+  inv[0] = m[5] * m[10] * m[15] - m[5] * m[11] * m[14] - m[9] * m[6] * m[15] +
+      m[9] * m[7] * m[14] + m[13] * m[6] * m[11] - m[13] * m[7] * m[10];
+
+  inv[4] = -m[4] * m[10] * m[15] + m[4] * m[11] * m[14] + m[8] * m[6] * m[15] -
+      m[8] * m[7] * m[14] - m[12] * m[6] * m[11] + m[12] * m[7] * m[10];
+
+  inv[8] = m[4] * m[9] * m[15] - m[4] * m[11] * m[13] - m[8] * m[5] * m[15] +
+      m[8] * m[7] * m[13] + m[12] * m[5] * m[11] - m[12] * m[7] * m[9];
+
+  inv[12] = -m[4] * m[9] * m[14] + m[4] * m[10] * m[13] + m[8] * m[5] * m[14] -
+      m[8] * m[6] * m[13] - m[12] * m[5] * m[10] + m[12] * m[6] * m[9];
+
+  inv[1] = -m[1] * m[10] * m[15] + m[1] * m[11] * m[14] + m[9] * m[2] * m[15] -
+      m[9] * m[3] * m[14] - m[13] * m[2] * m[11] + m[13] * m[3] * m[10];
+
+  inv[5] = m[0] * m[10] * m[15] - m[0] * m[11] * m[14] - m[8] * m[2] * m[15] +
+      m[8] * m[3] * m[14] + m[12] * m[2] * m[11] - m[12] * m[3] * m[10];
+
+  inv[9] = -m[0] * m[9] * m[15] + m[0] * m[11] * m[13] + m[8] * m[1] * m[15] -
+      m[8] * m[3] * m[13] - m[12] * m[1] * m[11] + m[12] * m[3] * m[9];
+
+  inv[13] = m[0] * m[9] * m[14] - m[0] * m[10] * m[13] - m[8] * m[1] * m[14] +
+      m[8] * m[2] * m[13] + m[12] * m[1] * m[10] - m[12] * m[2] * m[9];
+
+  inv[2] = m[1] * m[6] * m[15] - m[1] * m[7] * m[14] - m[5] * m[2] * m[15] +
+      m[5] * m[3] * m[14] + m[13] * m[2] * m[7] - m[13] * m[3] * m[6];
+
+  inv[6] = -m[0] * m[6] * m[15] + m[0] * m[7] * m[14] + m[4] * m[2] * m[15] -
+      m[4] * m[3] * m[14] - m[12] * m[2] * m[7] + m[12] * m[3] * m[6];
+
+  inv[10] = m[0] * m[5] * m[15] - m[0] * m[7] * m[13] - m[4] * m[1] * m[15] +
+      m[4] * m[3] * m[13] + m[12] * m[1] * m[7] - m[12] * m[3] * m[5];
+
+  inv[14] = -m[0] * m[5] * m[14] + m[0] * m[6] * m[13] + m[4] * m[1] * m[14] -
+      m[4] * m[2] * m[13] - m[12] * m[1] * m[6] + m[12] * m[2] * m[5];
+
+  inv[3] = -m[1] * m[6] * m[11] + m[1] * m[7] * m[10] + m[5] * m[2] * m[11] -
+      m[5] * m[3] * m[10] - m[9] * m[2] * m[7] + m[9] * m[3] * m[6];
+
+  inv[7] = m[0] * m[6] * m[11] - m[0] * m[7] * m[10] - m[4] * m[2] * m[11] +
+      m[4] * m[3] * m[10] + m[8] * m[2] * m[7] - m[8] * m[3] * m[6];
+
+  inv[11] = -m[0] * m[5] * m[11] + m[0] * m[7] * m[9] + m[4] * m[1] * m[11] -
+      m[4] * m[3] * m[9] - m[8] * m[1] * m[7] + m[8] * m[3] * m[5];
+
+  inv[15] = m[0] * m[5] * m[10] - m[0] * m[6] * m[9] - m[4] * m[1] * m[10] +
+      m[4] * m[2] * m[9] + m[8] * m[1] * m[6] - m[8] * m[2] * m[5];
+
+  det = m[0] * inv[0] + m[1] * inv[4] + m[2] * inv[8] + m[3] * inv[12];
+
+  if (det == 0)
+    return false;
+
+  det = 1.0 / det;
+
+  for (i = 0; i < 16; i++)
+    this->matrix[i] = inv[i] * det;
+
+  return true;
+}


i don't think we need this.

coado · 2024-07-31T07:45:13Z

I think right now changes are merged in a way that omits handling rotations. I don't see my part of the code in findNodeAtPoint function, that's why the inv is not being invoked. Also, does it make sense to handle horizontal and vertical inversion separately? The inversion matrix will handle all cases.

zhongwuzw · 2024-07-31T07:50:30Z

@coado Because of conflict I just merged some parts of the code from you, after I combine them, I'll push the whole code. Also I would remove the overflow part which we can concentrate on transform things.

zhongwuzw · 2024-08-01T06:33:19Z

cc @realsoelynn @coado

facebook-github-bot · 2024-08-02T21:17:41Z

@realsoelynn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

realsoelynn · 2024-08-07T21:26:24Z

Sorry about my late reply

It's failing UT on FindNodeAtPointTest/viewIsScaled test case

Failure: Expected equality of these values:
  LayoutableShadowNode::findNodeAtPoint(parentShadowNode, {119, 119})->getTag()
    Which is: 3
  2

I am figuring out how to run C++ Unit Test like above to run without BUCK. (I will keep you folks posted about it once i learn more about it.)

realsoelynn · 2024-08-07T21:27:43Z

packages/react-native/ReactCommon/react/renderer/graphics/Transform.cpp

+  inv[0] = m[5] * m[10] * m[15] - m[5] * m[11] * m[14] - m[9] * m[6] * m[15] +
+      m[9] * m[7] * m[14] + m[13] * m[6] * m[11] - m[13] * m[7] * m[10];
+
+  inv[4] = -m[4] * m[10] * m[15] + m[4] * m[11] * m[14] + m[8] * m[6] * m[15] -
+      m[8] * m[7] * m[14] - m[12] * m[6] * m[11] + m[12] * m[7] * m[10];
+
+  inv[8] = m[4] * m[9] * m[15] - m[4] * m[11] * m[13] - m[8] * m[5] * m[15] +
+      m[8] * m[7] * m[13] + m[12] * m[5] * m[11] - m[12] * m[7] * m[9];
+
+  inv[12] = -m[4] * m[9] * m[14] + m[4] * m[10] * m[13] + m[8] * m[5] * m[14] -
+      m[8] * m[6] * m[13] - m[12] * m[5] * m[10] + m[12] * m[6] * m[9];
+
+  inv[1] = -m[1] * m[10] * m[15] + m[1] * m[11] * m[14] + m[9] * m[2] * m[15] -
+      m[9] * m[3] * m[14] - m[13] * m[2] * m[11] + m[13] * m[3] * m[10];
+
+  inv[5] = m[0] * m[10] * m[15] - m[0] * m[11] * m[14] - m[8] * m[2] * m[15] +
+      m[8] * m[3] * m[14] + m[12] * m[2] * m[11] - m[12] * m[3] * m[10];
+
+  inv[9] = -m[0] * m[9] * m[15] + m[0] * m[11] * m[13] + m[8] * m[1] * m[15] -
+      m[8] * m[3] * m[13] - m[12] * m[1] * m[11] + m[12] * m[3] * m[9];
+
+  inv[13] = m[0] * m[9] * m[14] - m[0] * m[10] * m[13] - m[8] * m[1] * m[14] +
+      m[8] * m[2] * m[13] + m[12] * m[1] * m[10] - m[12] * m[2] * m[9];
+
+  inv[2] = m[1] * m[6] * m[15] - m[1] * m[7] * m[14] - m[5] * m[2] * m[15] +
+      m[5] * m[3] * m[14] + m[13] * m[2] * m[7] - m[13] * m[3] * m[6];
+
+  inv[6] = -m[0] * m[6] * m[15] + m[0] * m[7] * m[14] + m[4] * m[2] * m[15] -
+      m[4] * m[3] * m[14] - m[12] * m[2] * m[7] + m[12] * m[3] * m[6];
+
+  inv[10] = m[0] * m[5] * m[15] - m[0] * m[7] * m[13] - m[4] * m[1] * m[15] +
+      m[4] * m[3] * m[13] + m[12] * m[1] * m[7] - m[12] * m[3] * m[5];
+
+  inv[14] = -m[0] * m[5] * m[14] + m[0] * m[6] * m[13] + m[4] * m[1] * m[14] -
+      m[4] * m[2] * m[13] - m[12] * m[1] * m[6] + m[12] * m[2] * m[5];
+
+  inv[3] = -m[1] * m[6] * m[11] + m[1] * m[7] * m[10] + m[5] * m[2] * m[11] -
+      m[5] * m[3] * m[10] - m[9] * m[2] * m[7] + m[9] * m[3] * m[6];
+
+  inv[7] = m[0] * m[6] * m[11] - m[0] * m[7] * m[10] - m[4] * m[2] * m[11] +
+      m[4] * m[3] * m[10] + m[8] * m[2] * m[7] - m[8] * m[3] * m[6];
+
+  inv[11] = -m[0] * m[5] * m[11] + m[0] * m[7] * m[9] + m[4] * m[1] * m[11] -
+      m[4] * m[3] * m[9] - m[8] * m[1] * m[7] + m[8] * m[3] * m[5];
+
+  inv[15] = m[0] * m[5] * m[10] - m[0] * m[6] * m[9] - m[4] * m[1] * m[10] +
+      m[4] * m[2] * m[9] + m[8] * m[1] * m[6] - m[8] * m[2] * m[5];
+
+  det = m[0] * inv[0] + m[1] * inv[4] + m[2] * inv[8] + m[3] * inv[12];


@coado Can i have a link to details explanation about this matrix calculation?

I would like to understand more about it.

I've opened a draft, just to check if the idea is correct. So, for now I just copied the implementation from the stackoverflow. There is a more detailed implementation of this in mesa library here.

zhongwuzw · 2024-08-08T08:59:18Z

Sorry about my late reply

It's failing UT on FindNodeAtPointTest/viewIsScaled test case
Failure: Expected equality of these values:
  LayoutableShadowNode::findNodeAtPoint(parentShadowNode, {119, 119})->getTag()
    Which is: 3
  2
I am figuring out how to run C++ Unit Test like above to run without BUCK. (I will keep you folks posted about it once i learn more about it.)

Seems this test is wrong, the tag should be 3, not 2 ?

coado · 2024-08-08T09:50:13Z

Sorry about my late reply
It's failing UT on FindNodeAtPointTest/viewIsScaled test case
Failure: Expected equality of these values:
  LayoutableShadowNode::findNodeAtPoint(parentShadowNode, {119, 119})->getTag()
    Which is: 3
  2
I am figuring out how to run C++ Unit Test like above to run without BUCK. (I will keep you folks posted about it once i learn more about it.)
Seems this test is wrong, the tag should be 3, not 2 ?

Isn't the view with tag 3 scaled down? It seems like the touch point is within the boundaries of unscaled view with tag 2, so the test is correct, but I might be missing something.

zhongwuzw · 2024-08-12T14:12:54Z

@realsoelynn I just translate C++ ut to React code like below: I hit the breakpoint and it returns child1.

import React from 'react';
import { View, StyleSheet } from 'react-native';

const FindNodeAtPointTest = () => {
  return (
    <View style={styles.parent}>
      <View style={styles.child1}>
        <View style={styles.child2} />
      </View>
    </View>
  );
};

const styles = StyleSheet.create({
  parent: {
    width: 1000,
    height: 1000,
  },
  child1: {
    position: 'absolute',
    top: 100,
    left: 100,
    width: 100,
    height: 100,
    backgroundColor: 'blue'
  },
  child2: {
    position: 'absolute',
    top: 10,
    left: 10,
    width: 10,
    height: 10,
    backgroundColor: 'red',
    transform: [{ scale: 0.5 }],
  },
});

export default FindNodeAtPointTest;

realsoelynn · 2024-08-15T05:46:58Z

@zhongwuzw sorry about my delay. Let's go with your original solution for now to do a simple inversion (without @coado changes). As 0.76 cutoff is near, we want to at least get this major usecase bugfix went in first.

@coado For your changes, let me work with you more later after this PR is merged as i need to prioritize and research into the algorithms more.

Thank you both for your patient.

packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp

facebook-github-bot · 2024-08-16T00:14:14Z

@realsoelynn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

realsoelynn · 2024-08-16T00:20:11Z

Hmm strange CircleCI failure. @zhongwuzw please help us rebase it.

…/fix_inverted_inspection

zhongwuzw · 2024-08-16T00:40:55Z

@realsoelynn Done. :)

facebook-github-bot · 2024-08-16T00:43:53Z

@realsoelynn has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-08-16T08:04:11Z

@realsoelynn merged this pull request in 1d1646a.

react-native-bot · 2024-08-16T08:04:13Z

This pull request was successfully merged by @zhongwuzw in 1d1646a

^{When will my fix make it into a release? | How to file a pick request?}

[Fabric] Fixes findNodeAtPoint when views were inverted

d24c221

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jul 18, 2024

handle rotate inversion

b064d65

Add test

3aa37d5

realsoelynn reviewed Jul 31, 2024

View reviewed changes

zhongwuzw force-pushed the features/fix_inverted_inspection branch from 5b655d3 to cec195a Compare August 1, 2024 06:29

realsoelynn reviewed Aug 7, 2024

View reviewed changes

zhongwuzw force-pushed the features/fix_inverted_inspection branch from cec195a to 3aa37d5 Compare August 15, 2024 06:19

zhongwuzw requested a review from realsoelynn August 15, 2024 06:22

realsoelynn reviewed Aug 15, 2024

View reviewed changes

packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp Show resolved Hide resolved

update

ed166f9

Merge commit '17017d2b8181acf6e1ce6543f333cd6653b6945b' into features…

60de02f

…/fix_inverted_inspection

facebook-github-bot closed this in 1d1646a Aug 16, 2024

facebook-github-bot added the Merged This PR has been merged. label Aug 16, 2024

coado mentioned this pull request Aug 19, 2024

inverting touch point #46099

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fabric] Fixes findNodeAtPoint when views were inverted #45519

[Fabric] Fixes findNodeAtPoint when views were inverted #45519

zhongwuzw commented Jul 18, 2024 •

edited

Loading

facebook-github-bot commented Jul 18, 2024

realsoelynn commented Jul 18, 2024

zhongwuzw commented Jul 23, 2024

zhongwuzw commented Jul 24, 2024

facebook-github-bot commented Jul 24, 2024

realsoelynn commented Jul 29, 2024

realsoelynn commented Jul 29, 2024 •

edited

Loading

coado commented Jul 30, 2024

coado commented Jul 30, 2024

zhongwuzw commented Jul 30, 2024

realsoelynn commented Jul 30, 2024

realsoelynn commented Jul 30, 2024

realsoelynn Jul 31, 2024

coado commented Jul 31, 2024

zhongwuzw commented Jul 31, 2024

zhongwuzw commented Aug 1, 2024

facebook-github-bot commented Aug 2, 2024

realsoelynn commented Aug 7, 2024

realsoelynn Aug 7, 2024

coado Aug 8, 2024

zhongwuzw commented Aug 8, 2024

coado commented Aug 8, 2024

zhongwuzw commented Aug 12, 2024

realsoelynn commented Aug 15, 2024

facebook-github-bot commented Aug 16, 2024

realsoelynn commented Aug 16, 2024

zhongwuzw commented Aug 16, 2024

facebook-github-bot commented Aug 16, 2024

facebook-github-bot commented Aug 16, 2024

react-native-bot commented Aug 16, 2024

[Fabric] Fixes findNodeAtPoint when views were inverted #45519

[Fabric] Fixes findNodeAtPoint when views were inverted #45519

Conversation

zhongwuzw commented Jul 18, 2024 • edited Loading

Summary:

Changelog:

Test Plan:

facebook-github-bot commented Jul 18, 2024

realsoelynn commented Jul 18, 2024

zhongwuzw commented Jul 23, 2024

zhongwuzw commented Jul 24, 2024

facebook-github-bot commented Jul 24, 2024

realsoelynn commented Jul 29, 2024

realsoelynn commented Jul 29, 2024 • edited Loading

coado commented Jul 30, 2024

coado commented Jul 30, 2024

zhongwuzw commented Jul 30, 2024

realsoelynn commented Jul 30, 2024

realsoelynn commented Jul 30, 2024

realsoelynn Jul 31, 2024

Choose a reason for hiding this comment

coado commented Jul 31, 2024

zhongwuzw commented Jul 31, 2024

zhongwuzw commented Aug 1, 2024

facebook-github-bot commented Aug 2, 2024

realsoelynn commented Aug 7, 2024

realsoelynn Aug 7, 2024

Choose a reason for hiding this comment

coado Aug 8, 2024

Choose a reason for hiding this comment

zhongwuzw commented Aug 8, 2024

coado commented Aug 8, 2024

zhongwuzw commented Aug 12, 2024

realsoelynn commented Aug 15, 2024

facebook-github-bot commented Aug 16, 2024

realsoelynn commented Aug 16, 2024

zhongwuzw commented Aug 16, 2024

facebook-github-bot commented Aug 16, 2024

facebook-github-bot commented Aug 16, 2024

react-native-bot commented Aug 16, 2024

zhongwuzw commented Jul 18, 2024 •

edited

Loading

realsoelynn commented Jul 29, 2024 •

edited

Loading