[Impeller] skip lineTo for empty contours. (flutter#52290)

If we close an empty contour, don't insert a lineTo Fixes flutter/flutter#146648
mnordine · Apr 22, 2024 · 62c9f17 · 62c9f17
1 parent 20d155f
commit 62c9f17
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 11 deletions.
diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc
@@ -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
 

diff --git a/impeller/geometry/path_builder.cc b/impeller/geometry/path_builder.cc
@@ -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;

diff --git a/impeller/geometry/path_unittests.cc b/impeller/geometry/path_unittests.cc
@@ -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);
   }
 
   {
@@ -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.
@@ -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});

diff --git a/testing/impeller_golden_tests_output.txt b/testing/impeller_golden_tests_output.txt
@@ -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