[Impeller] Add Rect::NormalizePoint method #47672

flar · 2023-11-03T21:50:03Z

Adds a utility method to perform work that is used in a number of locations within Impeller. Follow-on PRs will move those usages to the new utility methods in batches.

gaaclarke

lgtm, just switch to useing EXPECT

gaaclarke · 2023-11-03T21:56:10Z

impeller/geometry/rect_unittests.cc

+    ASSERT_EQ(Rect::MakeLTRB(l, t, r, b).NormalizePoint(nan_x), Point())
+        << "Point(NaN x) in " << rect_desc << " Rect";
+    ASSERT_EQ(Rect::MakeLTRB(l, t, r, b).NormalizePoint(nan_y), Point())
+        << "Point(NaN y) in " << rect_desc << " Rect";
+    ASSERT_EQ(Rect::MakeLTRB(l, t, r, b).NormalizePoint(nan_p), Point())
+        << "Point(NaN x&y) in " << rect_desc << " Rect";
+    ASSERT_EQ(IRect::MakeLTRB(l, t, r, b).NormalizePoint(nan_x), Point())
+        << "Point(NaN x) in " << rect_desc << " Rect";
+    ASSERT_EQ(IRect::MakeLTRB(l, t, r, b).NormalizePoint(nan_y), Point())
+        << "Point(NaN y) in " << rect_desc << " Rect";
+    ASSERT_EQ(IRect::MakeLTRB(l, t, r, b).NormalizePoint(nan_p), Point())
+        << "Point(NaN x&y) in " << rect_desc << " Rect";


You should change this to EXPECT_EQ so the test always runs to completion. Probably all of them I'd have to double check.

I find a lot of inconsistency on that in the various tests. I can see your point about running to completion, but in the end all of the errors have to be fixed anyway and they are often due to the same mistake. So, is it spam or is it a full roster of errors?

I suppose given that none of these ASSERT statements can actually terminate the test (since they are inside local closures), EXPECT would be consistent with the fact that the test will complete its various calls to test() regardless of whether an individual call to test() terminates early.

They are strict about this internally and I ran into it the other day which reminded me not to sleep on it.

In doing a global replace I discovered that the NEAR macros don't have EXPECT equivalents. I'll file an issue so that we can apply the same logic to those kinds of tests...

(Filed: flutter/flutter#137873)

googltest has EXPECT_NEAR

For single values, but the Impeller macros are for whole objects.

gaaclarke

lgtm! i'm curious if this autovectorizes.

flar · 2023-11-03T22:12:54Z

lgtm! i'm curious if this autovectorizes.

I'm planning on switching to LTRB fields after all of this refactoring work which will change the implementation here. I'll run some experiments on godbolt when I do that to see how things vectorize.

matanlurey · 2023-11-03T22:17:46Z

impeller/geometry/rect_unittests.cc

  }
 }

+TEST(RectTest, NormalizePoint) {


I'd rather see individual TEST cases versus a single case with a list of things it tests (you can pull up/re-use the test_one closure I suppose). Basically any place you have to explain what different things are being tested.

There are 630+ cases tested there. Do you want 630 TEST macros? If we require a TEST invocation for each and every condition tested we are incentivizing spot testing rather than rigorous testing.

I'm just communicating I wouldn't understand what "failure: NormalizePoint line 969" means in a test log. Maybe that means a few (not 630, but not 3) test macros, and maybe it doesn't.

I guess I'm curious as to how much factorization you see being appropriate here. In the long view they are testing just a single Rect method. In the near view there's a lot of combinatorics and breaking up each combinatoric case would be unwieldy. But, maybe there is a slice in all of the cases that can be broken out? I'd want to avoid:

TEST(RectTest, NormalizePointWhereRectHasNaNXOriginAndPointHasInfiniteYOnRectAndIPoint) { ... }

I'm just communicating I wouldn't understand what "failure: NormalizePoint line 969" means in a test log. Maybe that means a few (not 630, but not 3) test macros, and maybe it doesn't.

Which is why I use the labeling feature. The output contains the cases being tested.

I thought of a better way to use the labeling feature. Right now I'm using those strings to try to print out an "english description" of what the test case was, but perhaps I should just output Test Case: [I]Rect::MakeLTRB(<the values>).NormalizePoint([I]Point(<the values>)? No need to explain that this is the "UL" or "Upper Left Corner of the Source Rectangle" - just let the values do the explaining...

Basically, use this to run every test case so that the failures are self-documenting:

// Runs a single test case and outputs the parameters used on failure // to make it easier to diagnose which test case failed. auto test_case = [](Rect r, Point p, Point expected) { EXPECT_EQ(r.NormalizePoint(p), expected) << "Rect" << r << ".NormalizePoint(Point" << p << ")"; };

I ran with a variant of this using a macro that will perform the test and then print out the data on failure which helps identify which values are not compliant with the test/spec.

I didn't factor out the 2 cases any more than they had been, though, but they should be easier to read and the error output will be self-revealing.

I sabotaged one of the tests to get some error output to show what it looks like:

../../flutter/impeller/geometry/rect_unittests.cc:80: Failure Expected equality of these values: Rect::MakeLTRB(l, t, r, b).NormalizePoint(Point(px, px)) Which is: (1, 1) expected Which is: (1, 0) Rect::MakeLTRB(100, 100, 200, 200).NormalizePoint(Point(200, 200)) ../../flutter/impeller/geometry/rect_unittests.cc:80: Failure Expected equality of these values: Rect::MakeLTRB(l, t, r, b).NormalizePoint(Point(px, px)) Which is: (0, 0) expected Which is: (0, 1) Rect::MakeLTRB(100, 100, 200, 200).NormalizePoint(Point(100, 100)) ../../flutter/impeller/geometry/rect_unittests.cc:80: Failure Expected equality of these values: Rect::MakeLTRB(l, t, r, b).NormalizePoint(Point(px, px)) Which is: (2, 2) expected Which is: (2, -1) Rect::MakeLTRB(100, 100, 200, 200).NormalizePoint(Point(300, 300))

matanlurey · 2023-11-03T22:19:07Z

impeller/geometry/rect.h

Could you add in the PR description what you imagine this being used for? Like even a link to a single existing method would be helpful to understand the context.

A couple of examples:

engine/impeller/entity/geometry/geometry.cc

Line 82 in b66a876

data[i + 1] = effect_transform * (points[j] - texture_coverage.origin) /

engine/impeller/entity/geometry/fill_path_geometry.cc

Line 96 in b66a876

data.texture_coords = effect_transform *

I don't think these references make sense in the PR description as these are examples of "why" and the PR description, which gets copied into the final commit comment, should be more about the "what".

It's hard for me to review a PR that will be used later without knowing for what.

The PR description (versus in the review) helps context for the next person "oh why did Jim add this method, oh its because of X").

It's used to normalize points. I believe it is appropriate to assume some familiarity with computer graphics and where normalization of points is used to review PRs for Impeller. I don't see how adding references in the description of the PR really enhances anything. The new method has an API comment to help explain what it does.

Should we explain which specific parts of the code would use Union, Intersection, Contains when we add those?

I think it is reasonable to ask if the new methods will be used in a review, but I don't think the answer to that belongs in the PR description or the github commit comments. Those shouldn't be written to assuage suspicions of uselessness.

impeller/geometry/rect_unittests.cc

auto-submit · 2023-11-03T22:22:05Z

auto label is removed for flutter/engine/47672, due to This PR has not met approval requirements for merging. Changes were requested by {matanlurey}, please make the needed changes and resubmit this PR.
You are a member of flutter-hackers and need 0 more review(s) in order to merge this PR.

Merge guidelines: You need at least one approved review if you are already part of flutter-hackers or two member reviews if you are not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

…ures

flar · 2023-11-06T20:30:33Z

(TLDR - jedi hand wave - this may not be the method that we are looking for)

@gaaclarke in reference to your comment about vectorization I was mulling the issue a bit over the weekend.

At first I thought that it isn't likely to have a lot of impact because in the places this code is used, there isn't a tight loop around applying this method to some coordinates. Instead the loop always seems to contain at least a coordinate transform on the point in addition to the normalization, and in the case that is likely to see the most points applied in a single operation, the data is also being packed into a data structure for passing along.

But then I realized that the normalization procedure itself is just a coordinate transform, equivalent to Translate(-origin) * Scale(1 / size) (give or take a commutative swap) and since the code is already applying a matrix transform anyway, then the best solution would be to bake this normalization into the transform. So, instead we should be looking for:

  auto matrix = incoming_transform * Matrix::MakeNormalizing(texture_bounds);
  // OR
  auto matrix = incoming_transform.NormalizeTo(texture_bounds);

and then use this matrix in the inner loops instead of the incoming_transform and a separate normalization step...

flar · 2023-11-08T04:38:48Z

Closing in favor of #47775

Will reopen/reconsider if we ever need to normalize coordinates while also not transforming them at the same time.

github-actions bot added the e: impeller label Nov 3, 2023

flar requested review from matanlurey, gaaclarke and bdero November 3, 2023 21:52

gaaclarke suggested changes Nov 3, 2023

View reviewed changes

flar requested a review from gaaclarke November 3, 2023 22:09

gaaclarke approved these changes Nov 3, 2023

View reviewed changes

flar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 3, 2023

matanlurey suggested changes Nov 3, 2023

View reviewed changes

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 3, 2023

flar added 3 commits November 3, 2023 16:45

Add Rect::NormalizePoint method

d8d8043

switch to EXPECT for more complete test runs on failures

79bfa41

better comments in the unit test

a112dfa

flar force-pushed the impeller-Rect-NormalizePoint branch from fb44c71 to a112dfa Compare November 3, 2023 23:46

flar requested a review from matanlurey November 3, 2023 23:47

flar added 2 commits November 3, 2023 18:42

make the test cases more concise and show the exact test data on fail…

5b7d69a

…ures

add newline on failed tests so output is easier to scan

0659b6a

flar closed this Nov 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Add Rect::NormalizePoint method #47672

[Impeller] Add Rect::NormalizePoint method #47672

flar commented Nov 3, 2023

gaaclarke left a comment

gaaclarke Nov 3, 2023

flar Nov 3, 2023

gaaclarke Nov 3, 2023

flar Nov 3, 2023

gaaclarke Nov 3, 2023

flar Nov 3, 2023

gaaclarke left a comment

flar commented Nov 3, 2023

matanlurey Nov 3, 2023

flar Nov 3, 2023

matanlurey Nov 3, 2023

flar Nov 3, 2023

flar Nov 4, 2023

flar Nov 4, 2023

flar Nov 4, 2023

flar Nov 4, 2023

flar Nov 4, 2023

matanlurey Nov 3, 2023

flar Nov 3, 2023

matanlurey Nov 3, 2023

flar Nov 4, 2023

flar Nov 4, 2023

auto-submit bot commented Nov 3, 2023

flar commented Nov 6, 2023 •

edited

Loading

flar commented Nov 8, 2023

[Impeller] Add Rect::NormalizePoint method #47672

[Impeller] Add Rect::NormalizePoint method #47672

Conversation

flar commented Nov 3, 2023

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

flar commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auto-submit bot commented Nov 3, 2023

flar commented Nov 6, 2023 • edited Loading

flar commented Nov 8, 2023

flar commented Nov 6, 2023 •

edited

Loading