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

[Impeller] skip lineTo for empty contours. #52290

Merged
merged 7 commits into from
Apr 22, 2024
Merged

[Impeller] skip lineTo for empty contours. #52290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0aca046
[Impeller] skip lineTo for empty contours.
Apr 21, 2024
550c572
still close contour.
Apr 21, 2024
be07b58
update testing script.
Apr 22, 2024
ebddde6
Update aiks_unittests.cc
Apr 22, 2024
70d4d93
Merge branch 'main' into move_contours
Apr 22, 2024
bfdc1f4
add more unittests
Apr 22, 2024
ec2ba73
Merge branch 'move_contours' of github.com:jonahwilliams/engine into …
Apr 22, 2024
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
23 changes: 23 additions & 0 deletions impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3145,6 +3145,29 @@ TEST_P(AiksTest, DrawAtlasPlusWideGamut) {
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

// https://github.com/flutter/flutter/issues/146648
TEST_P(AiksTest, StrokedPathWithMoveToThenCloseDrawnCorrectly) {
Path path = PathBuilder{}
.MoveTo({0, 400})
.LineTo({0, 0})
.LineTo({400, 0})
// MoveTo implicitly adds a contour, ensure that close doesn't
// add another nearly-empty contour.
.MoveTo({0, 400})
.Close()
.TakePath();

Canvas canvas;
canvas.Translate({50, 50, 0});
canvas.DrawPath(path, {
.color = Color::Blue(),
.stroke_width = 10,
.stroke_cap = Cap::kRound,
.style = Paint::Style::kStroke,
});
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

} // namespace testing
} // namespace impeller

Expand Down
7 changes: 6 additions & 1 deletion impeller/geometry/path_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,12 @@ PathBuilder& PathBuilder::MoveTo(Point point, bool relative) {
}

PathBuilder& PathBuilder::Close() {
LineTo(subpath_start_);
// If the subpath start is the same as the current position, this
// is an empty contour and inserting a line segment will just
// confuse the tessellator.
if (subpath_start_ != current_) {
LineTo(subpath_start_);
}
SetContourClosed(true);
AddContourComponent(current_);
return *this;
Expand Down
48 changes: 38 additions & 10 deletions impeller/geometry/path_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,26 +47,26 @@ TEST(PathTest, PathBuilderSetsCorrectContourPropertiesForAddCommands) {
Path path = PathBuilder{}.AddCircle({100, 100}, 50).TakePath();
ContourComponent contour;
path.GetContourComponentAtIndex(0, contour);
ASSERT_POINT_NEAR(contour.destination, Point(100, 50));
ASSERT_TRUE(contour.is_closed);
EXPECT_POINT_NEAR(contour.destination, Point(100, 50));
EXPECT_TRUE(contour.is_closed);
}

{
Path path =
PathBuilder{}.AddOval(Rect::MakeXYWH(100, 100, 100, 100)).TakePath();
ContourComponent contour;
path.GetContourComponentAtIndex(0, contour);
ASSERT_POINT_NEAR(contour.destination, Point(150, 100));
ASSERT_TRUE(contour.is_closed);
EXPECT_POINT_NEAR(contour.destination, Point(150, 100));
EXPECT_TRUE(contour.is_closed);
}

{
Path path =
PathBuilder{}.AddRect(Rect::MakeXYWH(100, 100, 100, 100)).TakePath();
ContourComponent contour;
path.GetContourComponentAtIndex(0, contour);
ASSERT_POINT_NEAR(contour.destination, Point(100, 100));
ASSERT_TRUE(contour.is_closed);
EXPECT_POINT_NEAR(contour.destination, Point(100, 100));
EXPECT_TRUE(contour.is_closed);
}

{
Expand All @@ -75,8 +75,8 @@ TEST(PathTest, PathBuilderSetsCorrectContourPropertiesForAddCommands) {
.TakePath();
ContourComponent contour;
path.GetContourComponentAtIndex(0, contour);
ASSERT_POINT_NEAR(contour.destination, Point(110, 100));
ASSERT_TRUE(contour.is_closed);
EXPECT_POINT_NEAR(contour.destination, Point(110, 100));
EXPECT_TRUE(contour.is_closed);
}

{
Expand All @@ -86,8 +86,8 @@ TEST(PathTest, PathBuilderSetsCorrectContourPropertiesForAddCommands) {
.TakePath();
ContourComponent contour;
path.GetContourComponentAtIndex(0, contour);
ASSERT_POINT_NEAR(contour.destination, Point(110, 100));
ASSERT_TRUE(contour.is_closed);
EXPECT_POINT_NEAR(contour.destination, Point(110, 100));
EXPECT_TRUE(contour.is_closed);
}

// Open shapes.
Expand Down Expand Up @@ -454,6 +454,34 @@ TEST(PathTest, SimplePath) {
});
}

TEST(PathTest, RepeatCloseDoesNotAddNewLines) {
PathBuilder builder;
auto path = builder.LineTo({0, 10})
.LineTo({10, 10})
.Close() // Returns to (0, 0)
.Close() // No Op
.Close() // Still No op
.TakePath();

EXPECT_EQ(path.GetComponentCount(), 5u);
EXPECT_EQ(path.GetComponentCount(Path::ComponentType::kLinear), 3u);
EXPECT_EQ(path.GetComponentCount(Path::ComponentType::kContour), 2u);
}

TEST(PathTest, CloseAfterMoveDoesNotAddNewLines) {
PathBuilder builder;
auto path = builder.LineTo({0, 10})
.LineTo({10, 10})
.MoveTo({30, 30}) // Moves to (30, 30)
.Close() // No Op
.Close() // Still No op
.TakePath();

EXPECT_EQ(path.GetComponentCount(), 4u);
EXPECT_EQ(path.GetComponentCount(Path::ComponentType::kLinear), 2u);
EXPECT_EQ(path.GetComponentCount(Path::ComponentType::kContour), 2u);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the test I was looking for, namely:

MoveTo(0, 0)
LineTo(10, 0)
LineTo(10, 10)
LineTo(0, 10)
LineTo(0, 0)
Close()

should have 4 lines in it because the last LineTo returned the pen to the starting position.

Copy link
Contributor

Choose a reason for hiding this comment

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

Look at PathBuilder::AddOval for an example of this that is less degenerate. Drawing the full circle requires the path definition to return to the original point. An oval/circle cannot rely on the implicit Close() segment to finish the path, but previously it got a zero-length segment on the close. With this change that useless segment is no longer added, but we have no tests that ensure this improved behavior.

TEST(PathTest, CanBeCloned) {
PathBuilder builder;
builder.MoveTo({10, 10});
Expand Down
3 changes: 3 additions & 0 deletions testing/impeller_golden_tests_output.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,9 @@ impeller_Play_AiksTest_SrgbToLinearFilterSubpassCollapseOptimization_Vulkan.png
impeller_Play_AiksTest_StrokedCirclesRenderCorrectly_Metal.png
impeller_Play_AiksTest_StrokedCirclesRenderCorrectly_OpenGLES.png
impeller_Play_AiksTest_StrokedCirclesRenderCorrectly_Vulkan.png
impeller_Play_AiksTest_StrokedPathWithMoveToThenCloseDrawnCorrectly_Metal.png
impeller_Play_AiksTest_StrokedPathWithMoveToThenCloseDrawnCorrectly_OpenGLES.png
impeller_Play_AiksTest_StrokedPathWithMoveToThenCloseDrawnCorrectly_Vulkan.png
impeller_Play_AiksTest_SubpassWithClearColorOptimization_Metal.png
impeller_Play_AiksTest_SubpassWithClearColorOptimization_OpenGLES.png
impeller_Play_AiksTest_SubpassWithClearColorOptimization_Vulkan.png
Expand Down