Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Add Rect::NormalizePoint method #47672

Closed
wants to merge 5 commits into from
Closed

[Impeller] Add Rect::NormalizePoint method #47672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d8d8043
Add Rect::NormalizePoint method
flar Nov 3, 2023
79bfa41
switch to EXPECT for more complete test runs on failures
flar Nov 3, 2023
a112dfa
better comments in the unit test
flar Nov 3, 2023
5b7d69a
make the test cases more concise and show the exact test data on fail…
flar Nov 4, 2023
0659b6a
add newline on failed tests so output is easier to scan
flar Nov 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions impeller/geometry/rect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,35 @@ struct TRect {
return {GetRight(), GetBottom()};
}

/// @brief Computes the normalized location of an absolute point relative
/// to this rectangle and returns it as a relative Scalar Point
/// where (0, 0) represents the origin and (1, 1) represents the
/// lower right corner of the rectangle.
///
/// Empty rectangles produce (0, 0) for all input values as well
/// as infinite rectangles in the case where the computation is
/// mathematically impossible.
template <typename U>
constexpr TPoint<Scalar> NormalizePoint(TPoint<U> absolute) {
if (size.IsEmpty()) {
// empty rects have no interior so the only point that maps correctly
// via the calculations is the origin and the rest produce infinities
// or NaN. To avoid polluting the downstream calculations with values
// that are not finite, all points will be the origin relative to an
// empty rectangle. The checks below would catch this case for zero
// sized empty rects, but not for negative sizes.
return {};
}
Scalar relativeX =
(static_cast<Scalar>(absolute.x) - origin.x) / size.width;
Scalar relativeY =
(static_cast<Scalar>(absolute.y) - origin.y) / size.height;
// An infinite rect can still produce NaN for (infinity / infinity)
return (std::isfinite(relativeX) && std::isfinite(relativeY))
? Point(relativeX, relativeY)
: Point();
}

constexpr std::array<T, 4> GetLTRB() const {
return {GetLeft(), GetTop(), GetRight(), GetBottom()};
}
Expand Down
179 changes: 173 additions & 6 deletions impeller/geometry/rect_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@ namespace testing {
TEST(RectTest, RectOriginSizeGetters) {
{
Rect r = Rect::MakeOriginSize({10, 20}, {50, 40});
ASSERT_EQ(r.GetOrigin(), Point(10, 20));
ASSERT_EQ(r.GetSize(), Size(50, 40));
EXPECT_EQ(r.GetOrigin(), Point(10, 20));
EXPECT_EQ(r.GetSize(), Size(50, 40));
}

{
Rect r = Rect::MakeLTRB(10, 20, 50, 40);
ASSERT_EQ(r.GetOrigin(), Point(10, 20));
ASSERT_EQ(r.GetSize(), Size(40, 20));
EXPECT_EQ(r.GetOrigin(), Point(10, 20));
EXPECT_EQ(r.GetSize(), Size(40, 20));
}
}

Expand All @@ -44,16 +44,183 @@ TEST(RectTest, RectMakeSize) {
Size s(100, 200);
IRect r = IRect::MakeSize(s);
IRect expected = IRect::MakeLTRB(0, 0, 100, 200);
ASSERT_EQ(r, expected);
EXPECT_EQ(r, expected);
}

{
ISize s(100, 200);
IRect r = IRect::MakeSize(s);
IRect expected = IRect::MakeLTRB(0, 0, 100, 200);
ASSERT_EQ(r, expected);
EXPECT_EQ(r, expected);
}
}

// Performs an EXPECT_EQ on the operation of Rect::NormalizePoint and
// outputs the raw data for the test case on failure to help with diagnosis
#define TEST_NORMALIZE_POINT(RT, PT, l, t, r, b, px, py, expected) \
EXPECT_EQ(RT::MakeLTRB(l, t, r, b).NormalizePoint(PT(px, py)), expected) \
<< #RT "::MakeLTRB(" << l << ", " << t << ", " << r << ", " << b << ")" \
<< ".NormalizePoint(" << #PT "(" << px << ", " << py << "))" \
<< std::endl

TEST(RectTest, NormalizePoint) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 << ")";
  };

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

// Tests finite rects including:
// - points at the corners, inside, and outside
// - rects that are non-empty or empty through either zero or
// negative width and/or height
// - all combinations of integer and scalar rects and points.

// Tests one rectangle against one point in all 4 combinations of integer
// and scalar data types and with NaN and infinity point values
auto test_one = [](int64_t l, int64_t t, int64_t r, int64_t b, //
int64_t px, int64_t py, //
Point expected) {
// First test with all combinations of [I]Rect and [I]Point
// clang-format off
TEST_NORMALIZE_POINT( Rect, Point, l, t, r, b, px, py, expected);
TEST_NORMALIZE_POINT(IRect, Point, l, t, r, b, px, py, expected);
TEST_NORMALIZE_POINT( Rect, IPoint, l, t, r, b, px, py, expected);
TEST_NORMALIZE_POINT(IRect, IPoint, l, t, r, b, px, py, expected);
// clang-format on

// Now test nan handling by substituting a nan for the X and/or Y of
// the point.
auto nan = std::numeric_limits<Scalar>::quiet_NaN();
// Test with Rect and IRect, but we have to use Point to hold the NaN
// clang-format off
TEST_NORMALIZE_POINT( Rect, Point, l, t, r, b, nan, py, Point());
TEST_NORMALIZE_POINT( Rect, Point, l, t, r, b, px, nan, Point());
TEST_NORMALIZE_POINT( Rect, Point, l, t, r, b, nan, nan, Point());
TEST_NORMALIZE_POINT(IRect, Point, l, t, r, b, nan, py, Point());
TEST_NORMALIZE_POINT(IRect, Point, l, t, r, b, px, nan, Point());
TEST_NORMALIZE_POINT(IRect, Point, l, t, r, b, nan, nan, Point());
// clang-format on

// Now test +/- infinity handling by substituting an infinity for the
// X and/or Y of the point.
auto inf = std::numeric_limits<Scalar>::infinity();

// Test with Rect, but we have to use Point to hold the infinity
// clang-format off
TEST_NORMALIZE_POINT( Rect, Point, l, t, r, b, inf, py, Point());
TEST_NORMALIZE_POINT( Rect, Point, l, t, r, b, px, inf, Point());
TEST_NORMALIZE_POINT( Rect, Point, l, t, r, b, inf, inf, Point());
TEST_NORMALIZE_POINT( Rect, Point, l, t, r, b, -inf, py, Point());
TEST_NORMALIZE_POINT( Rect, Point, l, t, r, b, px, -inf, Point());
TEST_NORMALIZE_POINT( Rect, Point, l, t, r, b, -inf, -inf, Point());
// clang-format on

// Test with IRect, but we have to use Point to hold the infinity
// clang-format off
TEST_NORMALIZE_POINT(IRect, Point, l, t, r, b, inf, py, Point());
TEST_NORMALIZE_POINT(IRect, Point, l, t, r, b, px, inf, Point());
TEST_NORMALIZE_POINT(IRect, Point, l, t, r, b, inf, inf, Point());
TEST_NORMALIZE_POINT(IRect, Point, l, t, r, b, -inf, py, Point());
TEST_NORMALIZE_POINT(IRect, Point, l, t, r, b, px, -inf, Point());
TEST_NORMALIZE_POINT(IRect, Point, l, t, r, b, -inf, -inf, Point());
// clang-format on
};

// Tests the rectangle as specified, which is assumed to be non-empty
// and then tests it in various states of emptiness in both dimensions
// by reversing left/right and top/bottom or using the same value for
// both.
auto test = [&test_one](int64_t l, int64_t t, int64_t r, int64_t b, //
int64_t px, int64_t py, //
Point expected) {
// clang-format off
// expected
// rect point result
test_one(l, t, r, b, px, py, expected); // normal
test_one(l, t, l, b, px, py, Point()); // empty horizontally
test_one(r, t, l, b, px, py, Point()); // reversed horizontally
test_one(l, t, r, t, px, py, Point()); // empty vertically
test_one(l, b, r, t, px, py, Point()); // reversed vertically
test_one(l, t, l, t, px, py, Point()); // empty in both axes
test_one(r, b, l, t, px, py, Point()); // reversed in both axes
// clang-format on
};

// clang-format off
// rect point expected
// [ l t r b ] [ px py ] result
test(100, 100, 200, 200, 100, 100, Point(0, 0)); // Upper Left (UL)
test(100, 100, 200, 200, 200, 100, Point(1, 0)); // Upper Right (UR)
test(100, 100, 200, 200, 200, 200, Point(1, 1)); // Lower Right (LR)
test(100, 100, 200, 200, 100, 200, Point(0, 1)); // Lower Left (LL)
test(100, 100, 200, 200, 150, 150, Point(0.5, 0.5)); // Center
test(100, 100, 200, 200, 0, 0, Point(-1, -1)); // Outside UL
test(100, 100, 200, 200, 300, 0, Point(2, -1)); // Outside UR
test(100, 100, 200, 200, 300, 300, Point(2, 2)); // Outside LR
test(100, 100, 200, 200, 0, 300, Point(-1, 2)); // Outside LL
// clang-format on

// We can't test the true min and max 64-bit values due to overflow of
// the xywh internal representation, but we can test with half their
// values.
//
// When TRect is converted to ltrb notation, we can relax this
// restriction.
int64_t min_int = std::numeric_limits<int64_t>::min() / 2;
int64_t max_int = std::numeric_limits<int64_t>::max() / 2;

// clang-format off
test(min_int, 100, max_int, 200, 0, 150, Point(0.5, 0.5));
test( 100, min_int, 200, max_int, 150, 0, Point(0.5, 0.5));
test(min_int, min_int, max_int, max_int, 150, 150, Point(0.5, 0.5));
// clang-format on
}

TEST(RectTest, NormalizePointToNonFiniteRects) {
// Tests non-finite Scalar rects including:
// - points at the corners, inside, and outside
// - rects that are non-empty or empty through either zero or
// negative width and/or height

// Tests one rectangle against supplied point values and NaN replacements.
auto test = [](Scalar l, Scalar t, Scalar r, Scalar b, //
Scalar px, Scalar py, //
Point expected) {
auto nan = std::numeric_limits<Scalar>::quiet_NaN();
auto inf = std::numeric_limits<Scalar>::infinity();

TEST_NORMALIZE_POINT(Rect, Point, l, t, r, b, px, py, expected);

// Now retest with NaN replacing each rect parameter - producing (0, 0)
TEST_NORMALIZE_POINT(Rect, Point, nan, t, r, b, px, py, Point());
TEST_NORMALIZE_POINT(Rect, Point, l, nan, r, b, px, py, Point());
TEST_NORMALIZE_POINT(Rect, Point, l, t, nan, b, px, py, Point());
TEST_NORMALIZE_POINT(Rect, Point, l, t, r, nan, px, py, Point());

// Now retest with -inf replacing the left/top values - producing (0, 0)
TEST_NORMALIZE_POINT(Rect, Point, -inf, t, r, b, px, py, Point());
TEST_NORMALIZE_POINT(Rect, Point, l, -inf, r, b, px, py, Point());

// Now retest with +inf replacing the right/bottom values which
// produces a valid result, but the associated normalized coordinate
// will always be 0.
TEST_NORMALIZE_POINT(Rect, Point, l, t, inf, b, px, py,
Point(0, expected.y));
TEST_NORMALIZE_POINT(Rect, Point, l, t, r, inf, px, py,
Point(expected.x, 0));
};

// clang-format off
// rect point expected
// [ l t r b ] [ px py ] result
test(100, 100, 200, 200, 100, 100, Point(0, 0)); // Upper Left (UL)
test(100, 100, 200, 200, 200, 100, Point(1, 0)); // Upper Right (UR)
test(100, 100, 200, 200, 200, 200, Point(1, 1)); // Lower Right (LR)
test(100, 100, 200, 200, 100, 200, Point(0, 1)); // Lower Left (LL)
test(100, 100, 200, 200, 150, 150, Point(0.5, 0.5)); // Center
test(100, 100, 200, 200, 0, 0, Point(-1, -1)); // Outside UL
test(100, 100, 200, 200, 300, 0, Point(2, -1)); // Outside UR
test(100, 100, 200, 200, 300, 300, Point(2, 2)); // Outside LR
test(100, 100, 200, 200, 0, 300, Point(-1, 2)); // Outside LL
// clang-format on
}

#undef TEST_NORMALIZE_POINT

} // namespace testing
} // namespace impeller