Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] fixed Rect::Contains #49294

Merged
merged 1 commit into from
Dec 20, 2023
Merged

[Impeller] fixed Rect::Contains #49294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion impeller/geometry/rect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,8 @@ struct TRect {
}

[[nodiscard]] constexpr bool Contains(const TRect& o) const {
Copy link
Member Author

Choose a reason for hiding this comment

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

We want a.Contains(a) == true, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes indeed, we assume this inclusivity in our current usages

Copy link
Member Author

Choose a reason for hiding this comment

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

I only ask because the Contains(Point) uses < operators for the right and bottom axises.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another issue with these methods is that empty can contain another rectangle and vice versa. I'm planning to address all of these in a rewrite now that the fields are hidden, but I'm just starting on that now and if this PR fixes an existing practical issue, then we should proceed with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The left and top edges are considered "inside", the right and bottom edges are considered "outside".

For points you want >= left/top and < right/bottom to match that rule.

For rects, both have the same insideness and so matching edges always have the same containment as each other. In the case of right and bottom, those edges are the non-inclusive limit of what each rectangle considers "inside" and so if they are equal then all of the points inside one of them are also inside the other (wrt that one edge).

return Union(o).size == size;
return o.GetLeft() >= GetLeft() && o.GetTop() >= GetTop() &&
o.GetRight() <= GetRight() && o.GetBottom() <= GetBottom();
}

/// Returns true if either of the width or height are 0, negative, or NaN.
Expand Down
7 changes: 7 additions & 0 deletions impeller/geometry/rect_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,5 +492,12 @@ TEST(RectTest, IRectExpand) {
EXPECT_EQ(rect.Expand(ISize{-10, -10}), IRect::MakeLTRB(110, 110, 190, 190));
}

TEST(RectTest, ContainsFloatingPoint) {
auto rect1 =
Rect::MakeXYWH(472.599945f, 440.999969f, 1102.80005f, 654.000061f);
auto rect2 = Rect::MakeXYWH(724.f, 618.f, 600.f, 300.f);
EXPECT_TRUE(rect1.Contains(rect2));
}

} // namespace testing
} // namespace impeller