From 5238bbae830c11247ae6cc5989e8ce8ced59c24d Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Wed, 13 Sep 2023 10:27:04 -0700 Subject: [PATCH] [Impeller] Fixes stroke path geometry that can draw outside of path if the path ends at sharp turn. (flutter/engine#45252) Fixes https://github.com/flutter/flutter/issues/133032 The issue is that when we draw a line we always try to draw a complete rectangle from point 1 to point2 before drawing the join and the next line ![Untitled drawing (7)](https://github.com/flutter/engine/assets/47866232/c51aa4a8-bd8e-4764-9941-4e2968b4fb8e) This becomes a problem if there is a sharp turn ![Untitled drawing (6)](https://github.com/flutter/engine/assets/47866232/2b743792-52b7-402d-b2e8-f08ed47c0943) ![Untitled drawing (5)](https://github.com/flutter/engine/assets/47866232/794a75c9-5e87-4548-a264-7dd9cf088ae7) Notice there will be a slight over drawing at the bottom. The solution then is to not draw the rect first before drawing the join. It will be the join's responsibility to draw the complete the rect from the line start to the join. This should also save a lot of repeatedly drawing during the join ![rect](https://github.com/flutter/engine/assets/47866232/24802df6-69b5-4543-8d09-fcfbff4cedbb) [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../flutter/impeller/aiks/aiks_unittests.cc | 47 ++++++++++++++++++ engine/src/flutter/impeller/core/formats.h | 22 +++++++++ .../entity/geometry/stroke_path_geometry.cc | 49 +++++++++++++++---- engine/src/flutter/impeller/geometry/path.cc | 22 +++++++-- engine/src/flutter/impeller/geometry/path.h | 15 ++++++ .../impeller/geometry/path_component.h | 9 ++++ 6 files changed, 151 insertions(+), 13 deletions(-) diff --git a/engine/src/flutter/impeller/aiks/aiks_unittests.cc b/engine/src/flutter/impeller/aiks/aiks_unittests.cc index 3772a2b77a2..b813f428566 100644 --- a/engine/src/flutter/impeller/aiks/aiks_unittests.cc +++ b/engine/src/flutter/impeller/aiks/aiks_unittests.cc @@ -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())); +} + +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; @@ -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; diff --git a/engine/src/flutter/impeller/core/formats.h b/engine/src/flutter/impeller/core/formats.h index 6cc151e8193..0e8d0d7d86a 100644 --- a/engine/src/flutter/impeller/core/formats.h +++ b/engine/src/flutter/impeller/core/formats.h @@ -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. diff --git a/engine/src/flutter/impeller/entity/geometry/stroke_path_geometry.cc b/engine/src/flutter/impeller/entity/geometry/stroke_path_geometry.cc index 43648a548e2..9b5ab6877cd 100644 --- a/engine/src/flutter/impeller/entity/geometry/stroke_path_geometry.cc +++ b/engine/src/flutter/impeller/entity/geometry/stroke_path_geometry.cc @@ -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); @@ -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; - 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; - // 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 (!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); + + 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 diff --git a/engine/src/flutter/impeller/geometry/path.cc b/engine/src/flutter/impeller/geometry/path.cc index ff2c39d6eca..863b2204553 100644 --- a/engine/src/flutter/impeller/geometry/path.cc +++ b/engine/src/flutter/impeller/geometry/path.cc @@ -324,9 +324,10 @@ Path::Polyline Path::CreatePolyline(Scalar scale) const { return Vector2(0, -1); }; + std::vector components; std::optional 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()) { @@ -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( @@ -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; @@ -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(); return polyline; } diff --git a/engine/src/flutter/impeller/geometry/path.h b/engine/src/flutter/impeller/geometry/path.h index 757c5c03d95..bbbb207793d 100644 --- a/engine/src/flutter/impeller/geometry/path.h +++ b/engine/src/flutter/impeller/geometry/path.h @@ -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; @@ -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 components; }; /// One or more contours represented as a series of points and indices in diff --git a/engine/src/flutter/impeller/geometry/path_component.h b/engine/src/flutter/impeller/geometry/path_component.h index c2a808cf18d..a8b0d4fa9bd 100644 --- a/engine/src/flutter/impeller/geometry/path_component.h +++ b/engine/src/flutter/impeller/geometry/path_component.h @@ -47,9 +47,13 @@ struct LinearPathComponent { std::optional 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() {} @@ -87,10 +91,15 @@ struct QuadraticPathComponent { std::optional 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() {}