From 0aca0462f507475d63a75d419817bbb37705d19a Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 21 Apr 2024 13:13:06 -0700 Subject: [PATCH 1/5] [Impeller] skip lineTo for empty contours. --- impeller/aiks/aiks_unittests.cc | 24 ++++++++++++++++++++++++ impeller/geometry/path_builder.cc | 11 ++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index fc0863fa9ba0c..7ad83ee0a93ee 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -3136,6 +3136,30 @@ 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}) + // .CubicCurveTo({10, 10}, {-10, -10}, {4, 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::Red(), + .stroke_width = 10, + .stroke_cap = Cap::kRound, + .style = Paint::Style::kStroke, + }); + ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); +} + } // namespace testing } // namespace impeller diff --git a/impeller/geometry/path_builder.cc b/impeller/geometry/path_builder.cc index 5cc9e28127cb3..c0c7f94823919 100644 --- a/impeller/geometry/path_builder.cc +++ b/impeller/geometry/path_builder.cc @@ -38,9 +38,14 @@ PathBuilder& PathBuilder::MoveTo(Point point, bool relative) { } PathBuilder& PathBuilder::Close() { - LineTo(subpath_start_); - SetContourClosed(true); - AddContourComponent(current_); + // 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; } From 550c572169a16f6eb3210575e36d8c1cf8b51ce3 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 21 Apr 2024 14:08:12 -0700 Subject: [PATCH 2/5] still close contour. --- impeller/geometry/path_builder.cc | 4 ++-- impeller/geometry/path_unittests.cc | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/impeller/geometry/path_builder.cc b/impeller/geometry/path_builder.cc index c0c7f94823919..be591d92884c7 100644 --- a/impeller/geometry/path_builder.cc +++ b/impeller/geometry/path_builder.cc @@ -43,9 +43,9 @@ PathBuilder& PathBuilder::Close() { // confuse the tessellator. if (subpath_start_ != current_) { LineTo(subpath_start_); - SetContourClosed(true); - AddContourComponent(current_); } + SetContourClosed(true); + AddContourComponent(current_); return *this; } diff --git a/impeller/geometry/path_unittests.cc b/impeller/geometry/path_unittests.cc index 6ffd69a1de56a..cfcb237d8c036 100644 --- a/impeller/geometry/path_unittests.cc +++ b/impeller/geometry/path_unittests.cc @@ -47,8 +47,8 @@ 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); } { @@ -56,8 +56,8 @@ TEST(PathTest, PathBuilderSetsCorrectContourPropertiesForAddCommands) { 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); } { @@ -65,8 +65,8 @@ TEST(PathTest, PathBuilderSetsCorrectContourPropertiesForAddCommands) { 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); } { @@ -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); } { @@ -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. From be07b584321209fa78a2d40eedd01c2db99ed037 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 22 Apr 2024 09:18:57 -0700 Subject: [PATCH 3/5] update testing script. --- testing/impeller_golden_tests_output.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/testing/impeller_golden_tests_output.txt b/testing/impeller_golden_tests_output.txt index d0796e370311f..6cb415ad4f0b3 100644 --- a/testing/impeller_golden_tests_output.txt +++ b/testing/impeller_golden_tests_output.txt @@ -720,6 +720,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 From ebddde61ec52941019d75c98e900f276a59972ae Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 22 Apr 2024 10:19:50 -0700 Subject: [PATCH 4/5] Update aiks_unittests.cc --- impeller/aiks/aiks_unittests.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 7ad83ee0a93ee..254449d0ee53b 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -3141,7 +3141,6 @@ TEST_P(AiksTest, StrokedPathWithMoveToThenCloseDrawnCorrectly) { Path path = PathBuilder{} .MoveTo({0, 400}) .LineTo({0, 0}) - // .CubicCurveTo({10, 10}, {-10, -10}, {4, 0}) .LineTo({400, 0}) // MoveTo implicitly adds a contour, ensure that close doesn't // add another nearly-empty contour. @@ -3152,7 +3151,7 @@ TEST_P(AiksTest, StrokedPathWithMoveToThenCloseDrawnCorrectly) { Canvas canvas; canvas.Translate({50, 50, 0}); canvas.DrawPath(path, { - .color = Color::Red(), + .color = Color::Blue(), .stroke_width = 10, .stroke_cap = Cap::kRound, .style = Paint::Style::kStroke, From bfdc1f44a663e92ce1a0497882c5070678fa5aa2 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 22 Apr 2024 15:21:05 -0700 Subject: [PATCH 5/5] add more unittests --- impeller/geometry/path_unittests.cc | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/impeller/geometry/path_unittests.cc b/impeller/geometry/path_unittests.cc index cfcb237d8c036..6015bb264a176 100644 --- a/impeller/geometry/path_unittests.cc +++ b/impeller/geometry/path_unittests.cc @@ -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); +} + TEST(PathTest, CanBeCloned) { PathBuilder builder; builder.MoveTo({10, 10});