Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn. #45252

Merged
merged 9 commits into from
Sep 13, 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
47 changes: 47 additions & 0 deletions impeller/aiks/aiks_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1206,6 +1206,37 @@ TEST_P(AiksTest, CanRenderRoundedRectWithNonUniformRadii) {
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, CanRenderStrokePathThatEndsAtSharpTurn) {
Canvas canvas;

Paint paint;
paint.color = Color::Red();
paint.style = Paint::Style::kStroke;
paint.stroke_width = 200;

Rect rect = {100, 100, 200, 200};
PathBuilder builder;
builder.AddArc(rect, Degrees(0), Degrees(90), false);

canvas.DrawPath(builder.TakePath(), paint);
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not too familiar with the code, let me know if this is enough to set up golden test.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, this is the correct way to add a golden. 👍

}

TEST_P(AiksTest, CanRenderStrokePathWithCubicLine) {
Canvas canvas;

Paint paint;
paint.color = Color::Red();
paint.style = Paint::Style::kStroke;
paint.stroke_width = 20;

PathBuilder builder;
builder.AddCubicCurve({0, 200}, {50, 400}, {350, 0}, {400, 200});

canvas.DrawPath(builder.TakePath(), paint);
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, CanRenderDifferencePaths) {
Canvas canvas;

Expand Down Expand Up @@ -1952,6 +1983,22 @@ TEST_P(AiksTest, DrawRectStrokesRenderCorrectly) {
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, DrawRectStrokesWithBevelJoinRenderCorrectly) {
Canvas canvas;
Paint paint;
paint.color = Color::Red();
paint.style = Paint::Style::kStroke;
paint.stroke_width = 10;
paint.stroke_join = Join::kBevel;

canvas.Translate({100, 100});
canvas.DrawPath(
PathBuilder{}.AddRect(Rect::MakeSize(Size{100, 100})).TakePath(),
{paint});

ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, SaveLayerDrawsBehindSubsequentEntities) {
// Compare with https://fiddle.skia.org/c/9e03de8567ffb49e7e83f53b64bcf636
Canvas canvas;
Expand Down
22 changes: 22 additions & 0 deletions impeller/core/formats.h
Original file line number Diff line number Diff line change
Expand Up @@ -325,11 +325,33 @@ enum class IndexType {
kNone,
};

/// Decides how backend draws pixels based on input vertices.
enum class PrimitiveType {
/// Draws a triage for each separate set of three vertices.
///
/// Vertices [A, B, C, D, E, F] will produce triages
/// [ABC, DEF].
kTriangle,

/// Draws a triage for every adjacent three vertices.
///
/// Vertices [A, B, C, D, E, F] will produce triages
/// [ABC, BCD, CDE, DEF].
kTriangleStrip,

/// Draws a line for each separate set of two vertices.
///
/// Vertices [A, B, C] will produce discontinued line
/// [AB, BC].
kLine,

/// Draws a continuous line that connect every input vertices
///
/// Vertices [A, B, C] will produce one continuous line
/// [ABC].
kLineStrip,

/// Draws a point at each input vertex.
kPoint,
// Triangle fans are implementation dependent and need extra extensions
// checks. Hence, they are not supported here.
Expand Down
49 changes: 39 additions & 10 deletions impeller/entity/geometry/stroke_path_geometry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ StrokePathGeometry::CreateSolidStrokeVertices(
for (size_t contour_i = 0; contour_i < polyline.contours.size();
contour_i++) {
auto contour = polyline.contours[contour_i];
size_t contour_component_i = 0;
size_t contour_start_point_i, contour_end_point_i;
std::tie(contour_start_point_i, contour_end_point_i) =
polyline.GetContourPointBounds(contour_i);
Expand Down Expand Up @@ -308,27 +309,55 @@ StrokePathGeometry::CreateSolidStrokeVertices(
// Generate contour geometry.
for (size_t point_i = contour_start_point_i + 1;
point_i < contour_end_point_i; point_i++) {
if ((contour_component_i + 1 >= contour.components.size()) &&
contour.components[contour_component_i + 1].component_start_index <=
point_i) {
// The point_i has entered the next component in this contour.
contour_component_i += 1;
}
// Generate line rect.
vtx.position = polyline.points[point_i - 1] + offset;
vtx_builder.AppendVertex(vtx);
vtx.position = polyline.points[point_i - 1] - offset;
vtx_builder.AppendVertex(vtx);
vtx.position = polyline.points[point_i] + offset;
Copy link
Contributor Author

@chunhtai chunhtai Aug 30, 2023

Choose a reason for hiding this comment

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

This was where the complete rect is drawn. This part is now handle by the join when then draw the first pair of inner miter point and outer

vtx_builder.AppendVertex(vtx);
vtx.position = polyline.points[point_i] - offset;
vtx_builder.AppendVertex(vtx);

if (point_i < contour_end_point_i - 1) {
compute_offset(point_i + 1);
auto is_end_of_contour = point_i == contour_end_point_i - 1;

if (!contour.components[contour_component_i].is_curve) {
// For line components, two additional points need to be appended prior
// to appending a join connecting the next component.
vtx.position = polyline.points[point_i] + offset;
vtx_builder.AppendVertex(vtx);
vtx.position = polyline.points[point_i] - offset;
vtx_builder.AppendVertex(vtx);

// Generate join from the current line to the next line.
join_proc(vtx_builder, polyline.points[point_i], previous_offset,
offset, scaled_miter_limit, scale);
if (!is_end_of_contour) {
compute_offset(point_i + 1);
// Generate join from the current line to the next line.
join_proc(vtx_builder, polyline.points[point_i], previous_offset,
offset, scaled_miter_limit, scale);
}
} else {
// For curve components, the polyline is detailed enough such that
// it can avoid worrying about joins altogether.
if (!is_end_of_contour) {
compute_offset(point_i + 1);
} else {
// If this is a curve and is the end of the contour, two end points
// need to be drawn with the contour end_direction.
auto end_offset =
Vector2(-contour.end_direction.y, contour.end_direction.x) *
stroke_width * 0.5;
vtx.position = polyline.points[contour_end_point_i - 1] + end_offset;
vtx_builder.AppendVertex(vtx);
vtx.position = polyline.points[contour_end_point_i - 1] - end_offset;
vtx_builder.AppendVertex(vtx);
}
}
}

// Generate end cap or join.
if (!polyline.contours[contour_i].is_closed) {
if (!contour.is_closed) {
auto cap_offset =
Vector2(-contour.end_direction.y, contour.end_direction.x) *
stroke_width * 0.5; // Clockwise normal
Expand Down
22 changes: 19 additions & 3 deletions impeller/geometry/path.cc
Original file line number Diff line number Diff line change
Expand Up @@ -324,9 +324,10 @@ Path::Polyline Path::CreatePolyline(Scalar scale) const {
return Vector2(0, -1);
};

std::vector<PolylineContour::Component> components;
std::optional<size_t> previous_path_component_index;
auto end_contour = [&polyline, &previous_path_component_index,
&get_path_component]() {
&get_path_component, &components]() {
// Whenever a contour has ended, extract the exact end direction from the
// last component.
if (polyline.contours.empty()) {
Expand All @@ -339,6 +340,8 @@ Path::Polyline Path::CreatePolyline(Scalar scale) const {

auto& contour = polyline.contours.back();
contour.end_direction = Vector2(0, 1);
contour.components = components;
components.clear();

size_t previous_index = previous_path_component_index.value();
while (!std::holds_alternative<std::monostate>(
Expand All @@ -363,14 +366,26 @@ Path::Polyline Path::CreatePolyline(Scalar scale) const {
const auto& component = components_[component_i];
switch (component.type) {
case ComponentType::kLinear:
components.push_back({
.component_start_index = polyline.points.size(),
.is_curve = false,
});
collect_points(linears_[component.index].CreatePolyline());
previous_path_component_index = component_i;
break;
case ComponentType::kQuadratic:
components.push_back({
.component_start_index = polyline.points.size(),
.is_curve = true,
});
collect_points(quads_[component.index].CreatePolyline(scale));
previous_path_component_index = component_i;
break;
case ComponentType::kCubic:
components.push_back({
.component_start_index = polyline.points.size(),
.is_curve = true,
});
collect_points(cubics_[component.index].CreatePolyline(scale));
previous_path_component_index = component_i;
break;
Expand All @@ -386,13 +401,14 @@ Path::Polyline Path::CreatePolyline(Scalar scale) const {
const auto& contour = contours_[component.index];
polyline.contours.push_back({.start_index = polyline.points.size(),
.is_closed = contour.is_closed,
.start_direction = start_direction});
.start_direction = start_direction,
.components = components});
previous_contour_point = std::nullopt;
collect_points({contour.destination});
break;
}
end_contour();
}
end_contour();
Copy link
Member

Choose a reason for hiding this comment

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

Nice find. :)

return polyline;
}

Expand Down
15 changes: 15 additions & 0 deletions impeller/geometry/path.h
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,17 @@ class Path {
};

struct PolylineContour {
struct Component {
size_t component_start_index;
/// Denotes whether this component is a curve.
///
/// This is set to true when this component is generated from
/// QuadraticComponent or CubicPathComponent.
bool is_curve;
};
/// Index that denotes the first point of this contour.
size_t start_index;

/// Denotes whether the last point of this contour is connected to the first
/// point of this contour or not.
bool is_closed;
Expand All @@ -71,6 +80,12 @@ class Path {
Vector2 start_direction;
/// The direction of the contour's end cap.
Vector2 end_direction;

/// Distinct components in this contour.
///
/// If this contour is generated from multiple path components, each
/// path component forms a component in this vector.
std::vector<Component> components;
};

/// One or more contours represented as a series of points and indices in
Expand Down
9 changes: 9 additions & 0 deletions impeller/geometry/path_component.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,13 @@ struct LinearPathComponent {
std::optional<Vector2> GetEndDirection() const;
};

// A component that represets a Quadratic Bézier curve.
struct QuadraticPathComponent {
// Start point.
Point p1;
// Control point.
Point cp;
// End point.
Point p2;

QuadraticPathComponent() {}
Expand Down Expand Up @@ -87,10 +91,15 @@ struct QuadraticPathComponent {
std::optional<Vector2> GetEndDirection() const;
};

// A component that represets a Cubic Bézier curve.
struct CubicPathComponent {
// Start point.
Point p1;
// The first control point.
Point cp1;
// The second control point.
Point cp2;
// End point.
Point p2;

CubicPathComponent() {}
Expand Down