From db62ff228450525af95fbfee3d78b6a493bfc3fb Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 1 Sep 2023 12:12:33 -0700 Subject: [PATCH 1/6] [Impeller] make Paths externally immutable, always use PathBuilder. --- impeller/display_list/skia_conversions.cc | 7 +-- impeller/display_list/skia_conversions.h | 4 +- impeller/geometry/geometry_unittests.cc | 29 ++++++------ impeller/geometry/path.h | 55 +++++++++++------------ impeller/geometry/path_builder.cc | 5 +++ impeller/geometry/path_builder.h | 3 ++ 6 files changed, 56 insertions(+), 47 deletions(-) diff --git a/impeller/display_list/skia_conversions.cc b/impeller/display_list/skia_conversions.cc index 0fdeeab6adbd3..92db8d8034ed4 100644 --- a/impeller/display_list/skia_conversions.cc +++ b/impeller/display_list/skia_conversions.cc @@ -46,7 +46,7 @@ PathBuilder::RoundingRadii ToRoundingRadii(const SkRRect& rrect) { return radii; } -Path ToPath(const SkPath& path) { +Path ToPath(const SkPath& path, Point shift) { auto iterator = SkPath::Iter(path, false); struct PathData { @@ -121,6 +121,7 @@ Path ToPath(const SkPath& path) { } builder.SetConvexity(path.isConvex() ? Convexity::kConvex : Convexity::kUnknown); + builder.Shift(shift); return builder.TakePath(fill_type); } @@ -161,12 +162,12 @@ std::vector ToRSXForms(const SkRSXform xform[], int count) { return result; } -Path PathDataFromTextBlob(const sk_sp& blob) { +Path PathDataFromTextBlob(const sk_sp& blob, Point shift) { if (!blob) { return {}; } - return ToPath(skia::textlayout::Paragraph::GetPath(blob.get())); + return ToPath(skia::textlayout::Paragraph::GetPath(blob.get()), shift); } std::optional ToPixelFormat(SkColorType type) { diff --git a/impeller/display_list/skia_conversions.h b/impeller/display_list/skia_conversions.h index c56cf298a932f..f6b8f9f5d45e7 100644 --- a/impeller/display_list/skia_conversions.h +++ b/impeller/display_list/skia_conversions.h @@ -38,11 +38,11 @@ std::vector ToRSXForms(const SkRSXform xform[], int count); PathBuilder::RoundingRadii ToRoundingRadii(const SkRRect& rrect); -Path ToPath(const SkPath& path); +Path ToPath(const SkPath& path, Point shift = Point(0, 0)); Path ToPath(const SkRRect& rrect); -Path PathDataFromTextBlob(const sk_sp& blob); +Path PathDataFromTextBlob(const sk_sp& blob, Point shift = Point(0, 0)); std::optional ToPixelFormat(SkColorType type); diff --git a/impeller/geometry/geometry_unittests.cc b/impeller/geometry/geometry_unittests.cc index 1a43f65cba1e5..6ceb49ee34d95 100644 --- a/impeller/geometry/geometry_unittests.cc +++ b/impeller/geometry/geometry_unittests.cc @@ -605,11 +605,12 @@ TEST(GeometryTest, EmptyPath) { } TEST(GeometryTest, SimplePath) { - Path path; + PathBuilder builder; - path.AddLinearComponent({0, 0}, {100, 100}) - .AddQuadraticComponent({100, 100}, {200, 200}, {300, 300}) - .AddCubicComponent({300, 300}, {400, 400}, {500, 500}, {600, 600}); + auto path = builder.AddLine({0, 0}, {100, 100}) + .AddQuadraticCurve({100, 100}, {200, 200}, {300, 300}) + .AddCubicCurve({300, 300}, {400, 400}, {500, 500}, {600, 600}) + .TakePath(); ASSERT_EQ(path.GetComponentCount(), 4u); ASSERT_EQ(path.GetComponentCount(Path::ComponentType::kLinear), 1u); @@ -654,8 +655,8 @@ TEST(GeometryTest, SimplePath) { } TEST(GeometryTest, BoundingBoxCubic) { - Path path; - path.AddCubicComponent({120, 160}, {25, 200}, {220, 260}, {220, 40}); + PathBuilder builder; + auto path = builder.AddCubicCurve({120, 160}, {25, 200}, {220, 260}, {220, 40}).TakePath(); auto box = path.GetBoundingBox(); Rect expected(93.9101, 40, 126.09, 158.862); ASSERT_TRUE(box.has_value()); @@ -2007,14 +2008,14 @@ TEST(GeometryTest, CubicPathComponentPolylineDoesNotIncludePointOne) { } TEST(GeometryTest, PathCreatePolyLineDoesNotDuplicatePoints) { - Path path; - path.AddContourComponent({10, 10}); - path.AddLinearComponent({10, 10}, {20, 20}); - path.AddLinearComponent({20, 20}, {30, 30}); - path.AddContourComponent({40, 40}); - path.AddLinearComponent({40, 40}, {50, 50}); + PathBuilder builder; + builder.MoveTo({10, 10}); + builder.AddLine({10, 10}, {20, 20}); + builder.AddLine({20, 20}, {30, 30}); + builder.MoveTo({40, 40}); + builder.AddLine({40, 40}, {50, 50}); - auto polyline = path.CreatePolyline(1.0f); + auto polyline = builder.TakePath().CreatePolyline(1.0f); ASSERT_EQ(polyline.contours.size(), 2u); ASSERT_EQ(polyline.points.size(), 5u); @@ -2381,8 +2382,8 @@ TEST(GeometryTest, PathShifting) { .AddCubicCurve(Point(20, 20), Point(25, 25), Point(-5, -5), Point(30, 30)) .Close() + .Shift(Point(1, 1)) .TakePath(); - path.Shift(Point(1, 1)); ContourComponent contour; LinearPathComponent linear; diff --git a/impeller/geometry/path.h b/impeller/geometry/path.h index dbed30bfae7d1..921cd1715c42d 100644 --- a/impeller/geometry/path.h +++ b/impeller/geometry/path.h @@ -48,8 +48,8 @@ enum class Convexity { /// All shapes supported by Impeller are paths either directly or /// via approximation (in the case of circles). /// -/// Creating paths that describe complex shapes is usually done by a -/// path builder. +/// Paths are externally immutable once created, Creating paths must be +/// done using a path builder. /// class Path { public: @@ -95,25 +95,10 @@ class Path { size_t GetComponentCount(std::optional type = {}) const; - void SetFillType(FillType fill); - FillType GetFillType() const; bool IsConvex() const; - Path& AddLinearComponent(Point p1, Point p2); - - Path& AddQuadraticComponent(Point p1, Point cp, Point p2); - - Path& AddCubicComponent(Point p1, Point cp1, Point cp2, Point p2); - - Path& AddContourComponent(Point destination, bool is_closed = false); - - void SetContourClosed(bool is_closed); - - /// @brief Transform the path by the given offset in-place. - void Shift(Point shift); - template using Applier = std::function; void EnumerateComponents( @@ -133,17 +118,6 @@ class Path { bool GetContourComponentAtIndex(size_t index, ContourComponent& contour) const; - bool UpdateLinearComponentAtIndex(size_t index, - const LinearPathComponent& linear); - - bool UpdateQuadraticComponentAtIndex(size_t index, - const QuadraticPathComponent& quadratic); - - bool UpdateCubicComponentAtIndex(size_t index, CubicPathComponent& cubic); - - bool UpdateContourComponentAtIndex(size_t index, - const ContourComponent& contour); - /// Callers must provide the scale factor for how this path will be /// transformed. /// @@ -162,6 +136,31 @@ class Path { void SetConvexity(Convexity value); + void SetFillType(FillType fill); + + Path& AddLinearComponent(Point p1, Point p2); + + Path& AddQuadraticComponent(Point p1, Point cp, Point p2); + + Path& AddCubicComponent(Point p1, Point cp1, Point cp2, Point p2); + + Path& AddContourComponent(Point destination, bool is_closed = false); + + void SetContourClosed(bool is_closed); + + void Shift(Point shift); + + bool UpdateLinearComponentAtIndex(size_t index, + const LinearPathComponent& linear); + + bool UpdateQuadraticComponentAtIndex(size_t index, + const QuadraticPathComponent& quadratic); + + bool UpdateCubicComponentAtIndex(size_t index, CubicPathComponent& cubic); + + bool UpdateContourComponentAtIndex(size_t index, + const ContourComponent& contour); + struct ComponentIndexPair { ComponentType type = ComponentType::kLinear; size_t index = 0; diff --git a/impeller/geometry/path_builder.cc b/impeller/geometry/path_builder.cc index 93855dadd488d..f7876a9f50416 100644 --- a/impeller/geometry/path_builder.cc +++ b/impeller/geometry/path_builder.cc @@ -453,4 +453,9 @@ PathBuilder& PathBuilder::AddPath(const Path& path) { return *this; } +PathBuilder& PathBuilder::Shift(Point offset) { + prototype_.Shift(offset); + return *this; +} + } // namespace impeller diff --git a/impeller/geometry/path_builder.h b/impeller/geometry/path_builder.h index 52a9139f94f09..62f84cf87f63a 100644 --- a/impeller/geometry/path_builder.h +++ b/impeller/geometry/path_builder.h @@ -75,6 +75,9 @@ class PathBuilder { PathBuilder& AddCubicCurve(Point p1, Point cp1, Point cp2, Point p2); + /// @brief Transform the path by the given offset in-place. + PathBuilder& Shift(Point offset); + struct RoundingRadii { Point top_left; Point bottom_left; From 34a516163116f20ab74cc0768f4c7103139a7552 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 1 Sep 2023 12:13:45 -0700 Subject: [PATCH 2/6] [Impeller] make Paths externally immutable, always use PathBuilder. --- impeller/display_list/skia_conversions.h | 3 ++- impeller/geometry/geometry_unittests.cc | 10 ++++++---- impeller/geometry/path.h | 4 ++-- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/impeller/display_list/skia_conversions.h b/impeller/display_list/skia_conversions.h index f6b8f9f5d45e7..4b535db6227c3 100644 --- a/impeller/display_list/skia_conversions.h +++ b/impeller/display_list/skia_conversions.h @@ -42,7 +42,8 @@ Path ToPath(const SkPath& path, Point shift = Point(0, 0)); Path ToPath(const SkRRect& rrect); -Path PathDataFromTextBlob(const sk_sp& blob, Point shift = Point(0, 0)); +Path PathDataFromTextBlob(const sk_sp& blob, + Point shift = Point(0, 0)); std::optional ToPixelFormat(SkColorType type); diff --git a/impeller/geometry/geometry_unittests.cc b/impeller/geometry/geometry_unittests.cc index 6ceb49ee34d95..c557397271481 100644 --- a/impeller/geometry/geometry_unittests.cc +++ b/impeller/geometry/geometry_unittests.cc @@ -608,9 +608,9 @@ TEST(GeometryTest, SimplePath) { PathBuilder builder; auto path = builder.AddLine({0, 0}, {100, 100}) - .AddQuadraticCurve({100, 100}, {200, 200}, {300, 300}) - .AddCubicCurve({300, 300}, {400, 400}, {500, 500}, {600, 600}) - .TakePath(); + .AddQuadraticCurve({100, 100}, {200, 200}, {300, 300}) + .AddCubicCurve({300, 300}, {400, 400}, {500, 500}, {600, 600}) + .TakePath(); ASSERT_EQ(path.GetComponentCount(), 4u); ASSERT_EQ(path.GetComponentCount(Path::ComponentType::kLinear), 1u); @@ -656,7 +656,9 @@ TEST(GeometryTest, SimplePath) { TEST(GeometryTest, BoundingBoxCubic) { PathBuilder builder; - auto path = builder.AddCubicCurve({120, 160}, {25, 200}, {220, 260}, {220, 40}).TakePath(); + auto path = + builder.AddCubicCurve({120, 160}, {25, 200}, {220, 260}, {220, 40}) + .TakePath(); auto box = path.GetBoundingBox(); Rect expected(93.9101, 40, 126.09, 158.862); ASSERT_TRUE(box.has_value()); diff --git a/impeller/geometry/path.h b/impeller/geometry/path.h index 921cd1715c42d..be3c9cce4e50d 100644 --- a/impeller/geometry/path.h +++ b/impeller/geometry/path.h @@ -48,8 +48,8 @@ enum class Convexity { /// All shapes supported by Impeller are paths either directly or /// via approximation (in the case of circles). /// -/// Paths are externally immutable once created, Creating paths must be -/// done using a path builder. +/// Paths are externally immutable once created, Creating paths must +/// be done using a path builder. /// class Path { public: From 092414ef0c93d3e549d39a7381445bd8bdf745bb Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 1 Sep 2023 12:22:46 -0700 Subject: [PATCH 3/6] ++ --- impeller/display_list/dl_dispatcher.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/impeller/display_list/dl_dispatcher.cc b/impeller/display_list/dl_dispatcher.cc index 5e4205465850d..1ae6e4920625d 100644 --- a/impeller/display_list/dl_dispatcher.cc +++ b/impeller/display_list/dl_dispatcher.cc @@ -1118,8 +1118,8 @@ void DlDispatcher::drawTextBlob(const sk_sp blob, if (paint_.style == Paint::Style::kStroke || paint_.color_source.GetType() != ColorSource::Type::kColor) { auto bounds = blob->bounds(); - auto path = skia_conversions::PathDataFromTextBlob(blob); - path.Shift(Point(x + bounds.left(), y + bounds.top())); + auto path = skia_conversions::PathDataFromTextBlob( + blob, Point(x + bounds.left(), y + bounds.top())); canvas_.DrawPath(path, paint_); return; } From 47a55fdd066ed775f2bb0fa1487b4009791ccbd9 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 1 Sep 2023 14:15:03 -0700 Subject: [PATCH 4/6] ++ --- impeller/geometry/geometry_unittests.cc | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/impeller/geometry/geometry_unittests.cc b/impeller/geometry/geometry_unittests.cc index c557397271481..439e1deeef433 100644 --- a/impeller/geometry/geometry_unittests.cc +++ b/impeller/geometry/geometry_unittests.cc @@ -612,11 +612,11 @@ TEST(GeometryTest, SimplePath) { .AddCubicCurve({300, 300}, {400, 400}, {500, 500}, {600, 600}) .TakePath(); - ASSERT_EQ(path.GetComponentCount(), 4u); + ASSERT_EQ(path.GetComponentCount(), 6u); ASSERT_EQ(path.GetComponentCount(Path::ComponentType::kLinear), 1u); ASSERT_EQ(path.GetComponentCount(Path::ComponentType::kQuadratic), 1u); ASSERT_EQ(path.GetComponentCount(Path::ComponentType::kCubic), 1u); - ASSERT_EQ(path.GetComponentCount(Path::ComponentType::kContour), 1u); + ASSERT_EQ(path.GetComponentCount(Path::ComponentType::kContour), 3u); path.EnumerateComponents( [](size_t index, const LinearPathComponent& linear) { @@ -630,7 +630,7 @@ TEST(GeometryTest, SimplePath) { Point p1(100, 100); Point cp(200, 200); Point p2(300, 300); - ASSERT_EQ(index, 2u); + ASSERT_EQ(index, 3u); ASSERT_EQ(quad.p1, p1); ASSERT_EQ(quad.cp, cp); ASSERT_EQ(quad.p2, p2); @@ -640,16 +640,26 @@ TEST(GeometryTest, SimplePath) { Point cp1(400, 400); Point cp2(500, 500); Point p2(600, 600); - ASSERT_EQ(index, 3u); + ASSERT_EQ(index, 5u); ASSERT_EQ(cubic.p1, p1); ASSERT_EQ(cubic.cp1, cp1); ASSERT_EQ(cubic.cp2, cp2); ASSERT_EQ(cubic.p2, p2); }, [](size_t index, const ContourComponent& contour) { - Point p1(0, 0); - ASSERT_EQ(index, 0u); - ASSERT_EQ(contour.destination, p1); + // There is an initial countour added for each curve. + if (index == 0u) { + Point p1(0, 0); + ASSERT_EQ(contour.destination, p1); + } else if (index == 2u) { + Point p1(100, 100); + ASSERT_EQ(contour.destination, p1); + } else if (index == 4u) { + Point p1(300, 300); + ASSERT_EQ(contour.destination, p1); + } else { + ASSERT_FALSE(true); + } ASSERT_FALSE(contour.is_closed); }); } From 49af6b94be0f19f46e616488ed94b4a7886338cc Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 1 Sep 2023 17:09:05 -0700 Subject: [PATCH 5/6] add some docs and fix test. --- impeller/geometry/geometry_unittests.cc | 6 +++--- impeller/geometry/path_builder.h | 19 ++++++++++++++++++- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/impeller/geometry/geometry_unittests.cc b/impeller/geometry/geometry_unittests.cc index 439e1deeef433..d5780fb70bb77 100644 --- a/impeller/geometry/geometry_unittests.cc +++ b/impeller/geometry/geometry_unittests.cc @@ -2022,10 +2022,10 @@ TEST(GeometryTest, CubicPathComponentPolylineDoesNotIncludePointOne) { TEST(GeometryTest, PathCreatePolyLineDoesNotDuplicatePoints) { PathBuilder builder; builder.MoveTo({10, 10}); - builder.AddLine({10, 10}, {20, 20}); - builder.AddLine({20, 20}, {30, 30}); + builder.LineTo({20, 20}); + builder.LineTo({30, 30}); builder.MoveTo({40, 40}); - builder.AddLine({40, 40}, {50, 50}); + builder.LineTo({50, 50}); auto polyline = builder.TakePath().CreatePolyline(1.0f); diff --git a/impeller/geometry/path_builder.h b/impeller/geometry/path_builder.h index 62f84cf87f63a..920e432ab4b8b 100644 --- a/impeller/geometry/path_builder.h +++ b/impeller/geometry/path_builder.h @@ -37,18 +37,29 @@ class PathBuilder { PathBuilder& Close(); + /// @brief Insert a line from the current position to `point`. + /// + /// If `relative` is true, then `point` is relative to the current location. PathBuilder& LineTo(Point point, bool relative = false); PathBuilder& HorizontalLineTo(Scalar x, bool relative = false); PathBuilder& VerticalLineTo(Scalar y, bool relative = false); + /// @brief Insert a quadradic curve from the current position to `point` using + /// the control point `controlPoint`. + /// + /// If `relative` is true the `point` and `controlPoint` are relative to current location. PathBuilder& QuadraticCurveTo(Point controlPoint, Point point, bool relative = false); PathBuilder& SmoothQuadraticCurveTo(Point point, bool relative = false); + /// @brief Insert a cubic curve from the curren position to `point` using the + /// control points `controlPoint1` and `controlPoint2`. + /// + /// If `relative` is true the `point`, `controlPoint1`, and `controlPoint2` are relative to current location. PathBuilder& CubicCurveTo(Point controlPoint1, Point controlPoint2, Point point, @@ -69,13 +80,19 @@ class PathBuilder { PathBuilder& AddOval(const Rect& rect); + /// @brief Move to point `p1`, then insert a line from `p1` to `p2`. PathBuilder& AddLine(const Point& p1, const Point& p2); + /// @brief Move to point `p1`, then insert a quadradic curve from `p1` to `p2` + /// with the control point `cp`. PathBuilder& AddQuadraticCurve(Point p1, Point cp, Point p2); + /// @brief Move to point `p1`, then insert a cubic curve from `p1` to `p2` + /// with control points `cp1` and `cp2`. PathBuilder& AddCubicCurve(Point p1, Point cp1, Point cp2, Point p2); - /// @brief Transform the path by the given offset in-place. + /// @brief Transform the existing path segments and contours by the given + /// `offset`. PathBuilder& Shift(Point offset); struct RoundingRadii { From 923f7b7d61322790b6e70d26b900b5e7b4242526 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 1 Sep 2023 17:10:53 -0700 Subject: [PATCH 6/6] ++ --- impeller/geometry/path_builder.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/impeller/geometry/path_builder.h b/impeller/geometry/path_builder.h index 920e432ab4b8b..2361da56a55f4 100644 --- a/impeller/geometry/path_builder.h +++ b/impeller/geometry/path_builder.h @@ -49,7 +49,8 @@ class PathBuilder { /// @brief Insert a quadradic curve from the current position to `point` using /// the control point `controlPoint`. /// - /// If `relative` is true the `point` and `controlPoint` are relative to current location. + /// If `relative` is true the `point` and `controlPoint` are relative to + /// current location. PathBuilder& QuadraticCurveTo(Point controlPoint, Point point, bool relative = false); @@ -59,7 +60,8 @@ class PathBuilder { /// @brief Insert a cubic curve from the curren position to `point` using the /// control points `controlPoint1` and `controlPoint2`. /// - /// If `relative` is true the `point`, `controlPoint1`, and `controlPoint2` are relative to current location. + /// If `relative` is true the `point`, `controlPoint1`, and `controlPoint2` + /// are relative to current location. PathBuilder& CubicCurveTo(Point controlPoint1, Point controlPoint2, Point point,