Skip to content

Commit

Permalink
[Impeller] Make paths externally immutable, update all tests to use P…
Browse files Browse the repository at this point in the history 
…athBuilder to create Path. (#45393)

This is a design problem in the Flutter API, that in general you can't assume a Path hasn't changed from the last time you used it. We're so close, so just move the mutation methods to PathBuilder :)

Fixes flutter/flutter#133880
  • Loading branch information
@jonahwilliams
jonahwilliams authored Sep 2, 2023
1 parent 0b40c44 commit eabe8fb
Show file tree
Hide file tree
Showing 7 changed files with 97 additions and 56 deletions.
4 changes: 2 additions & 2 deletions impeller/display_list/dl_dispatcher.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1118,8 +1118,8 @@ void DlDispatcher::drawTextBlob(const sk_sp<SkTextBlob> 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;
}
Expand Down
7 changes: 4 additions & 3 deletions impeller/display_list/skia_conversions.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -161,12 +162,12 @@ std::vector<Matrix> ToRSXForms(const SkRSXform xform[], int count) {
return result;
}

Path PathDataFromTextBlob(const sk_sp<SkTextBlob>& blob) {
Path PathDataFromTextBlob(const sk_sp<SkTextBlob>& 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<impeller::PixelFormat> ToPixelFormat(SkColorType type) {
Expand Down
5 changes: 3 additions & 2 deletions impeller/display_list/skia_conversions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,12 @@ std::vector<Matrix> 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<SkTextBlob>& blob);
Path PathDataFromTextBlob(const sk_sp<SkTextBlob>& blob,
Point shift = Point(0, 0));

std::optional<impeller::PixelFormat> ToPixelFormat(SkColorType type);

Expand Down
55 changes: 34 additions & 21 deletions impeller/geometry/geometry_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -605,17 +605,18 @@ 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(), 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) {
Expand All @@ -629,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);
Expand All @@ -639,23 +640,35 @@ 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);
});
}

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());
Expand Down Expand Up @@ -2007,14 +2020,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.LineTo({20, 20});
builder.LineTo({30, 30});
builder.MoveTo({40, 40});
builder.LineTo({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);
Expand Down Expand Up @@ -2381,8 +2394,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;
Expand Down
55 changes: 27 additions & 28 deletions impeller/geometry/path.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -95,25 +95,10 @@ class Path {

size_t GetComponentCount(std::optional<ComponentType> 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 <class T>
using Applier = std::function<void(size_t index, const T& component)>;
void EnumerateComponents(
Expand All @@ -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.
///
Expand All @@ -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;
Expand Down
5 changes: 5 additions & 0 deletions impeller/geometry/path_builder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -453,4 +453,9 @@ PathBuilder& PathBuilder::AddPath(const Path& path) {
return *this;
}

PathBuilder& PathBuilder::Shift(Point offset) {
prototype_.Shift(offset);
return *this;
}

} // namespace impeller
22 changes: 22 additions & 0 deletions impeller/geometry/path_builder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,31 @@ 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,
Expand All @@ -69,12 +82,21 @@ 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 existing path segments and contours by the given
/// `offset`.
PathBuilder& Shift(Point offset);

struct RoundingRadii {
Point top_left;
Point bottom_left;
Expand Down

0 comments on commit eabe8fb

Please sign in to comment.