Remove extra point from DrawRect; skip over duplicate contour points when generating polylines (flutter#114)

Author: bdero

impeller/aiks/aiks_unittests.cc

@@ -551,5 +551,19 @@ TEST_F(AiksTest, CoverageOriginShouldBeAccountedForInSubpasses) {
   ASSERT_TRUE(OpenPlaygroundHere(callback));
 }
 
+TEST_F(AiksTest, DrawRectStrokesRenderCorrectly) {
+  Canvas canvas;
+  Paint paint;
+  paint.color = Color::Red();
+  paint.style = Paint::Style::kStroke;
+  paint.stroke_width = 10;
+
+  canvas.Translate({100, 100});
+  canvas.DrawPath(PathBuilder{}.AddRect(Rect::MakeSize({100, 100})).TakePath(),
+                  paint);
+
+  ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
+}
+
 }  // namespace testing
 }  // namespace impeller

impeller/display_list/display_list_unittests.cc

@@ -127,5 +127,51 @@ TEST_F(DisplayListTest, CanDrawArc) {
   ASSERT_TRUE(OpenPlaygroundHere(callback));
 }
 
+TEST_F(DisplayListTest, StrokedPathsDrawCorrectly) {
+  flutter::DisplayListBuilder builder;
+  builder.setColor(SK_ColorRED);
+  builder.setStyle(SkPaint::Style::kStroke_Style);
+  builder.setStrokeWidth(10);
+
+  // Rectangle
+  builder.translate(100, 100);
+  builder.drawRect(SkRect::MakeSize({100, 100}));
+
+  // Rounded rectangle
+  builder.translate(150, 0);
+  builder.drawRRect(SkRRect::MakeRectXY(SkRect::MakeSize({100, 50}), 10, 10));
+
+  // Double rounded rectangle
+  builder.translate(150, 0);
+  builder.drawDRRect(
+      SkRRect::MakeRectXY(SkRect::MakeSize({100, 50}), 10, 10),
+      SkRRect::MakeRectXY(SkRect::MakeXYWH(10, 10, 80, 30), 10, 10));
+
+  // Contour with duplicate join points
+  {
+    builder.translate(150, 0);
+    SkPath path;
+    path.lineTo({100, 0});
+    path.lineTo({100, 0});
+    path.lineTo({100, 100});
+    builder.drawPath(path);
+  }
+
+  // Contour with duplicate end points
+  {
+    builder.setStrokeCap(SkPaint::Cap::kRound_Cap);
+    builder.translate(150, 0);
+    SkPath path;
+    path.moveTo(0, 0);
+    path.lineTo({0, 0});
+    path.lineTo({50, 50});
+    path.lineTo({100, 0});
+    path.lineTo({100, 0});
+    builder.drawPath(path);
+  }
+
+  ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
+}
+
 }  // namespace testing
 }  // namespace impeller

impeller/geometry/geometry_unittests.cc

@@ -3,7 +3,9 @@
 // found in the LICENSE file.
 
 #include "impeller/geometry/geometry_unittests.h"
+
 #include <limits>
+
 #include "flutter/testing/testing.h"
 #include "impeller/geometry/path.h"
 #include "impeller/geometry/path_builder.h"
@@ -982,11 +984,49 @@ TEST(GeometryTest, PolylineGetContourPointBoundsReturnsCorrectRanges) {
   ASSERT_EQ(b2, 6u);
 }
 
-TEST(GeometryTest, PolylineGetContourOutOfBoundsAborts) {
+TEST(GeometryTest, PathAddRectPolylineHasCorrectContourData) {
+  Path::Polyline polyline = PathBuilder{}
+                                .AddRect(Rect::MakeLTRB(50, 60, 70, 80))
+                                .TakePath()
+                                .CreatePolyline();
+  ASSERT_EQ(polyline.contours.size(), 1u);
+  ASSERT_TRUE(polyline.contours[0].is_closed);
+  ASSERT_EQ(polyline.contours[0].start_index, 0u);
+  ASSERT_EQ(polyline.points.size(), 5u);
+  ASSERT_EQ(polyline.points[0], Point(50, 60));
+  ASSERT_EQ(polyline.points[1], Point(70, 60));
+  ASSERT_EQ(polyline.points[2], Point(70, 80));
+  ASSERT_EQ(polyline.points[3], Point(50, 80));
+  ASSERT_EQ(polyline.points[4], Point(50, 60));
+}
+
+TEST(GeometryTest, PathPolylineDuplicatesAreRemovedForSameContour) {
   Path::Polyline polyline =
-      PathBuilder{}.AddLine({100, 100}, {200, 100}).TakePath().CreatePolyline();
-  ASSERT_EQ(polyline.GetContourPointBounds(0), std::make_tuple(0u, 2u));
-  ASSERT_EQ(polyline.GetContourPointBounds(14), std::make_tuple(2u, 2u));
+      PathBuilder{}
+          .MoveTo({50, 50})
+          .LineTo({50, 50})  // Insert duplicate at beginning of contour.
+          .LineTo({100, 50})
+          .LineTo({100, 50})  // Insert duplicate at contour join.
+          .LineTo({100, 100})
+          .Close()  // Implicitly insert duplicate {50, 50} across contours.
+          .LineTo({0, 50})
+          .LineTo({0, 100})
+          .LineTo({0, 100})  // Insert duplicate at end of contour.
+          .TakePath()
+          .CreatePolyline();
+  ASSERT_EQ(polyline.contours.size(), 2u);
+  ASSERT_EQ(polyline.contours[0].start_index, 0u);
+  ASSERT_TRUE(polyline.contours[0].is_closed);
+  ASSERT_EQ(polyline.contours[1].start_index, 4u);
+  ASSERT_FALSE(polyline.contours[1].is_closed);
+  ASSERT_EQ(polyline.points.size(), 7u);
+  ASSERT_EQ(polyline.points[0], Point(50, 50));
+  ASSERT_EQ(polyline.points[1], Point(100, 50));
+  ASSERT_EQ(polyline.points[2], Point(100, 100));
+  ASSERT_EQ(polyline.points[3], Point(50, 50));
+  ASSERT_EQ(polyline.points[4], Point(50, 50));
+  ASSERT_EQ(polyline.points[5], Point(0, 50));
+  ASSERT_EQ(polyline.points[6], Point(0, 100));
 }
 
 }  // namespace testing

impeller/geometry/path.cc

@@ -224,12 +224,27 @@ bool Path::UpdateContourComponentAtIndex(size_t index,
 Path::Polyline Path::CreatePolyline(
     const SmoothingApproximation& approximation) const {
   Polyline polyline;
-  auto collect_points = [&polyline](const std::vector<Point>& collection) {
+
+  std::optional<Point> previous_contour_point;
+  auto collect_points = [&polyline, &previous_contour_point](
+                            const std::vector<Point>& collection) {
+    if (collection.empty()) {
+      return;
+    }
+
     polyline.points.reserve(polyline.points.size() + collection.size());
-    polyline.points.insert(polyline.points.end(), collection.begin(),
-                           collection.end());
+
+    for (const auto& point : collection) {
+      if (previous_contour_point.has_value() &&
+          previous_contour_point.value() == point) {
+        // Slip over duplicate points in the same contour.
+        continue;
+      }
+      previous_contour_point = point;
+      polyline.points.push_back(point);
+    }
   };
-  // for (const auto& component : components_) {
+
   for (size_t component_i = 0; component_i < components_.size();
        component_i++) {
     const auto& component = components_[component_i];
@@ -252,6 +267,7 @@ Path::Polyline Path::CreatePolyline(
         const auto& contour = contours_[component.index];
         polyline.contours.push_back({.start_index = polyline.points.size(),
                                      .is_closed = contour.is_closed});
+        previous_contour_point = std::nullopt;
         collect_points({contour.destination});
         break;
     }

impeller/geometry/path_builder.cc

@@ -179,8 +179,7 @@ PathBuilder& PathBuilder::AddRect(Rect rect) {
   MoveTo(tl);
   prototype_.AddLinearComponent(tl, tr)
       .AddLinearComponent(tr, br)
-      .AddLinearComponent(br, bl)
-      .AddLinearComponent(bl, tl);
+      .AddLinearComponent(br, bl);
   Close();
 
   return *this;