Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] Make paths externally immutable, update all tests to use PathBuilder to create Path. #45393

Merged
merged 6 commits into from
Sep 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions impeller/display_list/dl_dispatcher.cc
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
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
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
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
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

/// 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
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
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