From 1bc0c4d33bbde77c1c1a84545e29e1abb0891be0 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 8 Nov 2023 10:03:41 -0800 Subject: [PATCH 01/11] 1 --- impeller/geometry/path_component.cc | 11 ++++++++--- impeller/geometry/path_component.h | 5 +++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/impeller/geometry/path_component.cc b/impeller/geometry/path_component.cc index 14848ccbea9d4..cf634016b4665 100644 --- a/impeller/geometry/path_component.cc +++ b/impeller/geometry/path_component.cc @@ -106,8 +106,9 @@ std::vector QuadraticPathComponent::CreatePolyline(Scalar scale) const { return points; } -void QuadraticPathComponent::FillPointsForPolyline(std::vector& points, - Scalar scale_factor) const { +size_t QuadraticPathComponent::FillPointsForPolyline(std::vector& points, + Scalar scale_factor, + size_t prev_count) const { auto tolerance = kDefaultCurveTolerance / scale_factor; auto sqrt_tolerance = sqrt(tolerance); @@ -138,6 +139,7 @@ void QuadraticPathComponent::FillPointsForPolyline(std::vector& points, auto uscale = 1 / (u2 - u0); auto line_count = std::max(1., ceil(0.5 * val / sqrt_tolerance)); + points.reserve(line_count + 1 + prev_count); auto step = 1 / line_count; for (size_t i = 1; i < line_count; i += 1) { auto u = i * step; @@ -146,6 +148,8 @@ void QuadraticPathComponent::FillPointsForPolyline(std::vector& points, points.emplace_back(Solve(t)); } points.emplace_back(p2); + + return line_count + 1 + prev_count; } std::vector QuadraticPathComponent::Extrema() const { @@ -190,8 +194,9 @@ Point CubicPathComponent::SolveDerivative(Scalar time) const { std::vector CubicPathComponent::CreatePolyline(Scalar scale) const { auto quads = ToQuadraticPathComponents(.1); std::vector points; + size_t running_count = 0; for (const auto& quad : quads) { - quad.FillPointsForPolyline(points, scale); + running_count = quad.FillPointsForPolyline(points, scale, running_count); } return points; } diff --git a/impeller/geometry/path_component.h b/impeller/geometry/path_component.h index a8b0d4fa9bd68..69bc38eca2381 100644 --- a/impeller/geometry/path_component.h +++ b/impeller/geometry/path_component.h @@ -77,8 +77,9 @@ struct QuadraticPathComponent { // See also the implementation in kurbo: https://github.com/linebender/kurbo. std::vector CreatePolyline(Scalar scale) const; - void FillPointsForPolyline(std::vector& points, - Scalar scale_factor) const; + size_t FillPointsForPolyline(std::vector& points, + Scalar scale_factor, + size_t prev_count = 0) const; std::vector Extrema() const; From ebfaabbcdc9c6f6ef698abd357991e730d65a1c6 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 8 Nov 2023 14:36:45 -0800 Subject: [PATCH 02/11] [Impeller] Reduce allocations in polyline generation --- impeller/entity/entity_unittests.cc | 36 --------- .../entity/geometry/fill_path_geometry.cc | 14 ++-- impeller/entity/geometry/geometry.cc | 31 -------- impeller/entity/geometry/geometry.h | 5 -- .../entity/geometry/stroke_path_geometry.cc | 14 ++-- impeller/geometry/geometry_benchmarks.cc | 27 +++++-- impeller/geometry/geometry_unittests.cc | 21 ++++-- impeller/geometry/path.cc | 34 +++------ impeller/geometry/path.h | 7 +- impeller/geometry/path_component.cc | 26 ++----- impeller/geometry/path_component.h | 10 +-- impeller/renderer/renderer_unittests.cc | 5 +- impeller/tessellator/BUILD.gn | 6 +- impeller/tessellator/c/tessellator.cc | 3 +- impeller/tessellator/tessellator.cc | 48 +++++++++++- impeller/tessellator/tessellator.h | 20 +++-- impeller/tessellator/tessellator_unittests.cc | 74 +++++++++++++------ 17 files changed, 191 insertions(+), 190 deletions(-) diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index bcffed8f0f317..46311aea07395 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2354,42 +2354,6 @@ TEST_P(EntityTest, TiledTextureContentsIsOpaque) { ASSERT_FALSE(contents.IsOpaque()); } -TEST_P(EntityTest, TessellateConvex) { - { - // Sanity check simple rectangle. - auto [pts, indices] = - TessellateConvex(PathBuilder{} - .AddRect(Rect::MakeLTRB(0, 0, 10, 10)) - .TakePath() - .CreatePolyline(1.0)); - - std::vector expected = { - {0, 0}, {10, 0}, {10, 10}, {0, 10}, // - }; - std::vector expected_indices = {0, 1, 2, 0, 2, 3}; - ASSERT_EQ(pts, expected); - ASSERT_EQ(indices, expected_indices); - } - - { - auto [pts, indices] = - TessellateConvex(PathBuilder{} - .AddRect(Rect::MakeLTRB(0, 0, 10, 10)) - .AddRect(Rect::MakeLTRB(20, 20, 30, 30)) - .TakePath() - .CreatePolyline(1.0)); - - std::vector expected = { - {0, 0}, {10, 0}, {10, 10}, {0, 10}, // - {20, 20}, {30, 20}, {30, 30}, {20, 30} // - }; - std::vector expected_indices = {0, 1, 2, 0, 2, 3, - 0, 6, 7, 0, 7, 8}; - ASSERT_EQ(pts, expected); - ASSERT_EQ(indices, expected_indices); - } -} - TEST_P(EntityTest, PointFieldGeometryDivisions) { // Square always gives 4 divisions. ASSERT_EQ(PointFieldGeometry::ComputeCircleDivisions(24.0, false), 4u); diff --git a/impeller/entity/geometry/fill_path_geometry.cc b/impeller/entity/geometry/fill_path_geometry.cc index 1153572f25c08..6e6e594b25707 100644 --- a/impeller/entity/geometry/fill_path_geometry.cc +++ b/impeller/entity/geometry/fill_path_geometry.cc @@ -22,8 +22,8 @@ GeometryResult FillPathGeometry::GetPositionBuffer( if (path_.GetFillType() == FillType::kNonZero && // path_.IsConvex()) { - auto [points, indices] = TessellateConvex( - path_.CreatePolyline(entity.GetTransformation().GetMaxBasisLength())); + auto [points, indices] = renderer.GetTessellator()->TessellateConvex( + path_, entity.GetTransformation().GetMaxBasisLength()); vertex_buffer.vertex_buffer = host_buffer.Emplace( points.data(), points.size() * sizeof(Point), alignof(Point)); @@ -42,8 +42,7 @@ GeometryResult FillPathGeometry::GetPositionBuffer( } auto tesselation_result = renderer.GetTessellator()->Tessellate( - path_.GetFillType(), - path_.CreatePolyline(entity.GetTransformation().GetMaxBasisLength()), + path_, entity.GetTransformation().GetMaxBasisLength(), [&vertex_buffer, &host_buffer]( const float* vertices, size_t vertices_count, const uint16_t* indices, size_t indices_count) { @@ -84,8 +83,8 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer( if (path_.GetFillType() == FillType::kNonZero && // path_.IsConvex()) { - auto [points, indices] = TessellateConvex( - path_.CreatePolyline(entity.GetTransformation().GetMaxBasisLength())); + auto [points, indices] = renderer.GetTessellator()->TessellateConvex( + path_, entity.GetTransformation().GetMaxBasisLength()); VertexBufferBuilder vertex_builder; vertex_builder.Reserve(points.size()); @@ -114,8 +113,7 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer( VertexBufferBuilder vertex_builder; auto tesselation_result = renderer.GetTessellator()->Tessellate( - path_.GetFillType(), - path_.CreatePolyline(entity.GetTransformation().GetMaxBasisLength()), + path_, entity.GetTransformation().GetMaxBasisLength(), [&vertex_builder, &texture_coverage, &effect_transform]( const float* vertices, size_t vertices_count, const uint16_t* indices, size_t indices_count) { diff --git a/impeller/entity/geometry/geometry.cc b/impeller/entity/geometry/geometry.cc index cff1770f134fb..a48e5d1bf5e48 100644 --- a/impeller/entity/geometry/geometry.cc +++ b/impeller/entity/geometry/geometry.cc @@ -15,37 +15,6 @@ namespace impeller { -/// Given a convex polyline, create a triangle fan structure. -std::pair, std::vector> TessellateConvex( - Path::Polyline polyline) { - std::vector output; - std::vector indices; - - for (auto j = 0u; j < polyline.contours.size(); j++) { - auto [start, end] = polyline.GetContourPointBounds(j); - auto center = polyline.points[start]; - - // Some polygons will not self close and an additional triangle - // must be inserted, others will self close and we need to avoid - // inserting an extra triangle. - if (polyline.points[end - 1] == polyline.points[start]) { - end--; - } - output.emplace_back(center); - output.emplace_back(polyline.points[start + 1]); - - for (auto i = start + 2; i < end; i++) { - const auto& point_b = polyline.points[i]; - output.emplace_back(point_b); - - indices.emplace_back(0); - indices.emplace_back(i - 1); - indices.emplace_back(i); - } - } - return std::make_pair(output, indices); -} - VertexBufferBuilder ComputeUVGeometryCPU( VertexBufferBuilder& input, diff --git a/impeller/entity/geometry/geometry.h b/impeller/entity/geometry/geometry.h index c0743626953ae..f336d4717ef55 100644 --- a/impeller/entity/geometry/geometry.h +++ b/impeller/entity/geometry/geometry.h @@ -46,11 +46,6 @@ GeometryResult ComputeUVGeometryForRect(Rect source_rect, const Entity& entity, RenderPass& pass); -/// @brief Given a polyline created from a convex filled path, perform a -/// tessellation. -std::pair, std::vector> TessellateConvex( - Path::Polyline polyline); - class Geometry { public: Geometry(); diff --git a/impeller/entity/geometry/stroke_path_geometry.cc b/impeller/entity/geometry/stroke_path_geometry.cc index 9f857d5808af2..5eda19a1c98de 100644 --- a/impeller/entity/geometry/stroke_path_geometry.cc +++ b/impeller/entity/geometry/stroke_path_geometry.cc @@ -128,9 +128,9 @@ StrokePathGeometry::JoinProc StrokePathGeometry::GetJoinProc(Join stroke_join) { PathBuilder::kArcApproximationMagic * alignment * dir; - auto arc_points = CubicPathComponent(start_offset, start_handle, - middle_handle, middle) - .CreatePolyline(scale); + std::vector arc_points; + CubicPathComponent(start_offset, start_handle, middle_handle, middle) + .CreatePolyline(scale, arc_points); VS::PerVertexData vtx; for (const auto& point : arc_points) { @@ -192,7 +192,9 @@ StrokePathGeometry::CapProc StrokePathGeometry::GetCapProc(Cap stroke_cap) { vtx_builder.AppendVertex(vtx); vtx.position = position - orientation; vtx_builder.AppendVertex(vtx); - for (const auto& point : arc.CreatePolyline(scale)) { + std::vector arc_points; + arc.CreatePolyline(scale, arc_points); + for (const auto& point : arc_points) { vtx.position = position + point; vtx_builder.AppendVertex(vtx); vtx.position = position + (-point).Reflect(forward_normal); @@ -234,7 +236,9 @@ StrokePathGeometry::CreateSolidStrokeVertices( const StrokePathGeometry::CapProc& cap_proc, Scalar scale) { VertexBufferBuilder vtx_builder; - auto polyline = path.CreatePolyline(scale); + std::vector point_buffer; + point_buffer.reserve(512); + auto polyline = path.CreatePolyline(scale, point_buffer); VS::PerVertexData vtx; diff --git a/impeller/geometry/geometry_benchmarks.cc b/impeller/geometry/geometry_benchmarks.cc index 8bff21f3282bf..9e44c6d792d59 100644 --- a/impeller/geometry/geometry_benchmarks.cc +++ b/impeller/geometry/geometry_benchmarks.cc @@ -28,15 +28,28 @@ static void BM_Polyline(benchmark::State& state, Args&&... args) { size_t point_count = 0u; size_t single_point_count = 0u; + std::vector points; + points.reserve(2048); while (state.KeepRunning()) { - auto polyline = path.CreatePolyline(1.0f); - single_point_count = polyline.points.size(); - point_count += single_point_count; if (tessellate) { - tess.Tessellate( - FillType::kNonZero, polyline, - [](const float* vertices, size_t vertices_count, - const uint16_t* indices, size_t indices_count) { return true; }); + tess.Tessellate(path, 1.0f, + [&point_count, &single_point_count]( + const float* vertices, size_t vertices_count, + const uint16_t* indices, size_t indices_count) { + if (indices_count > 0) { + single_point_count = indices_count; + point_count += indices_count; + } else { + single_point_count = vertices_count; + point_count += vertices_count; + } + return true; + }); + } else { + auto polyline = path.CreatePolyline(1.0f, points); + single_point_count = polyline.points.size(); + point_count += single_point_count; + points.clear(); } } state.counters["SinglePointCount"] = single_point_count; diff --git a/impeller/geometry/geometry_unittests.cc b/impeller/geometry/geometry_unittests.cc index d5ee6d423002d..abd26f62b76e4 100644 --- a/impeller/geometry/geometry_unittests.cc +++ b/impeller/geometry/geometry_unittests.cc @@ -599,7 +599,8 @@ TEST(GeometryTest, EmptyPath) { path.GetContourComponentAtIndex(0, c); ASSERT_POINT_NEAR(c.destination, Point()); - Path::Polyline polyline = path.CreatePolyline(1.0f); + std::vector polyline_points; + Path::Polyline polyline = path.CreatePolyline(1.0f, polyline_points); ASSERT_TRUE(polyline.points.empty()); ASSERT_TRUE(polyline.contours.empty()); } @@ -2099,7 +2100,8 @@ TEST(GeometryTest, RectRoundOut) { TEST(GeometryTest, CubicPathComponentPolylineDoesNotIncludePointOne) { CubicPathComponent component({10, 10}, {20, 35}, {35, 20}, {40, 40}); - auto polyline = component.CreatePolyline(1.0f); + std::vector polyline; + component.CreatePolyline(1.0f, polyline); ASSERT_NE(polyline.front().x, 10); ASSERT_NE(polyline.front().y, 10); ASSERT_EQ(polyline.back().x, 40); @@ -2114,7 +2116,8 @@ TEST(GeometryTest, PathCreatePolyLineDoesNotDuplicatePoints) { builder.MoveTo({40, 40}); builder.LineTo({50, 50}); - auto polyline = builder.TakePath().CreatePolyline(1.0f); + std::vector points; + auto polyline = builder.TakePath().CreatePolyline(1.0f, points); ASSERT_EQ(polyline.contours.size(), 2u); ASSERT_EQ(polyline.points.size(), 5u); @@ -2196,6 +2199,7 @@ TEST(GeometryTest, PathBuilderSetsCorrectContourPropertiesForAddCommands) { } TEST(GeometryTest, PathCreatePolylineGeneratesCorrectContourData) { + std::vector polyline_points; Path::Polyline polyline = PathBuilder{} .AddLine({100, 100}, {200, 100}) .MoveTo({100, 200}) @@ -2203,7 +2207,7 @@ TEST(GeometryTest, PathCreatePolylineGeneratesCorrectContourData) { .LineTo({200, 200}) .Close() .TakePath() - .CreatePolyline(1.0f); + .CreatePolyline(1.0f, polyline_points); ASSERT_EQ(polyline.points.size(), 6u); ASSERT_EQ(polyline.contours.size(), 2u); ASSERT_EQ(polyline.contours[0].is_closed, false); @@ -2213,6 +2217,7 @@ TEST(GeometryTest, PathCreatePolylineGeneratesCorrectContourData) { } TEST(GeometryTest, PolylineGetContourPointBoundsReturnsCorrectRanges) { + std::vector polyline_points; Path::Polyline polyline = PathBuilder{} .AddLine({100, 100}, {200, 100}) .MoveTo({100, 200}) @@ -2220,7 +2225,7 @@ TEST(GeometryTest, PolylineGetContourPointBoundsReturnsCorrectRanges) { .LineTo({200, 200}) .Close() .TakePath() - .CreatePolyline(1.0f); + .CreatePolyline(1.0f, polyline_points); size_t a1, a2, b1, b2; std::tie(a1, a2) = polyline.GetContourPointBounds(0); std::tie(b1, b2) = polyline.GetContourPointBounds(1); @@ -2231,10 +2236,11 @@ TEST(GeometryTest, PolylineGetContourPointBoundsReturnsCorrectRanges) { } TEST(GeometryTest, PathAddRectPolylineHasCorrectContourData) { + std::vector polyline_points; Path::Polyline polyline = PathBuilder{} .AddRect(Rect::MakeLTRB(50, 60, 70, 80)) .TakePath() - .CreatePolyline(1.0f); + .CreatePolyline(1.0f, polyline_points); ASSERT_EQ(polyline.contours.size(), 1u); ASSERT_TRUE(polyline.contours[0].is_closed); ASSERT_EQ(polyline.contours[0].start_index, 0u); @@ -2247,6 +2253,7 @@ TEST(GeometryTest, PathAddRectPolylineHasCorrectContourData) { } TEST(GeometryTest, PathPolylineDuplicatesAreRemovedForSameContour) { + std::vector polyline_points; Path::Polyline polyline = PathBuilder{} .MoveTo({50, 50}) @@ -2259,7 +2266,7 @@ TEST(GeometryTest, PathPolylineDuplicatesAreRemovedForSameContour) { .LineTo({0, 100}) .LineTo({0, 100}) // Insert duplicate at end of contour. .TakePath() - .CreatePolyline(1.0f); + .CreatePolyline(1.0f, polyline_points); ASSERT_EQ(polyline.contours.size(), 2u); ASSERT_EQ(polyline.contours[0].start_index, 0u); ASSERT_TRUE(polyline.contours[0].is_closed); diff --git a/impeller/geometry/path.cc b/impeller/geometry/path.cc index 6158c06bd2b16..d3ee3934c209f 100644 --- a/impeller/geometry/path.cc +++ b/impeller/geometry/path.cc @@ -269,26 +269,12 @@ bool Path::UpdateContourComponentAtIndex(size_t index, return true; } -Path::Polyline Path::CreatePolyline(Scalar scale) const { - Polyline polyline; +Path::Polyline::Polyline(std::vector& point_buffer) + : points(point_buffer) {} - std::optional previous_contour_point; - auto collect_points = [&polyline, &previous_contour_point]( - const std::vector& collection) { - if (collection.empty()) { - return; - } - - for (const auto& point : collection) { - if (previous_contour_point.has_value() && - previous_contour_point.value() == point) { - // Skip over duplicate points in the same contour. - continue; - } - previous_contour_point = point; - polyline.points.push_back(point); - } - }; +Path::Polyline Path::CreatePolyline(Scalar scale, + std::vector& point_buffer) const { + Polyline polyline(point_buffer); auto get_path_component = [this](size_t component_i) -> PathComponentVariant { if (component_i >= components_.size()) { @@ -370,7 +356,7 @@ Path::Polyline Path::CreatePolyline(Scalar scale) const { .component_start_index = polyline.points.size() - 1, .is_curve = false, }); - collect_points(linears_[component.index].CreatePolyline()); + linears_[component.index].CreatePolyline(polyline.points); previous_path_component_index = component_i; break; case ComponentType::kQuadratic: @@ -378,7 +364,7 @@ Path::Polyline Path::CreatePolyline(Scalar scale) const { .component_start_index = polyline.points.size() - 1, .is_curve = true, }); - collect_points(quads_[component.index].CreatePolyline(scale)); + quads_[component.index].CreatePolyline(scale, polyline.points); previous_path_component_index = component_i; break; case ComponentType::kCubic: @@ -386,7 +372,7 @@ Path::Polyline Path::CreatePolyline(Scalar scale) const { .component_start_index = polyline.points.size() - 1, .is_curve = true, }); - collect_points(cubics_[component.index].CreatePolyline(scale)); + cubics_[component.index].CreatePolyline(scale, polyline.points); previous_path_component_index = component_i; break; case ComponentType::kContour: @@ -403,8 +389,8 @@ Path::Polyline Path::CreatePolyline(Scalar scale) const { .is_closed = contour.is_closed, .start_direction = start_direction, .components = components}); - previous_contour_point = std::nullopt; - collect_points({contour.destination}); + + polyline.points.push_back(contour.destination); break; } } diff --git a/impeller/geometry/path.h b/impeller/geometry/path.h index bbbb207793d5f..5d065ad6ab6ac 100644 --- a/impeller/geometry/path.h +++ b/impeller/geometry/path.h @@ -91,9 +91,12 @@ class Path { /// One or more contours represented as a series of points and indices in /// the point vector representing the start of a new contour. struct Polyline { + explicit Polyline(std::vector& point_buffer); + /// Points in the polyline, which may represent multiple contours specified /// by indices in |breaks|. - std::vector points; + std::vector& points; + std::vector contours; /// Convenience method to compute the start (inclusive) and end (exclusive) @@ -138,7 +141,7 @@ class Path { /// /// It is suitable to use the max basis length of the matrix used to transform /// the path. If the provided scale is 0, curves will revert to lines. - Polyline CreatePolyline(Scalar scale) const; + Polyline CreatePolyline(Scalar scale, std::vector& point_buffer) const; std::optional GetBoundingBox() const; diff --git a/impeller/geometry/path_component.cc b/impeller/geometry/path_component.cc index cf634016b4665..0170774e55cec 100644 --- a/impeller/geometry/path_component.cc +++ b/impeller/geometry/path_component.cc @@ -59,8 +59,8 @@ Point LinearPathComponent::Solve(Scalar time) const { }; } -std::vector LinearPathComponent::CreatePolyline() const { - return {p2}; +void LinearPathComponent::CreatePolyline(std::vector& points) const { + points.push_back(p2); } std::vector LinearPathComponent::Extrema() const { @@ -100,15 +100,8 @@ static Scalar ApproximateParabolaIntegral(Scalar x) { return x / (1.0 - d + sqrt(sqrt(pow(d, 4) + 0.25 * x * x))); } -std::vector QuadraticPathComponent::CreatePolyline(Scalar scale) const { - std::vector points; - FillPointsForPolyline(points, scale); - return points; -} - -size_t QuadraticPathComponent::FillPointsForPolyline(std::vector& points, - Scalar scale_factor, - size_t prev_count) const { +void QuadraticPathComponent::CreatePolyline(Scalar scale_factor, + std::vector& points) const { auto tolerance = kDefaultCurveTolerance / scale_factor; auto sqrt_tolerance = sqrt(tolerance); @@ -139,7 +132,6 @@ size_t QuadraticPathComponent::FillPointsForPolyline(std::vector& points, auto uscale = 1 / (u2 - u0); auto line_count = std::max(1., ceil(0.5 * val / sqrt_tolerance)); - points.reserve(line_count + 1 + prev_count); auto step = 1 / line_count; for (size_t i = 1; i < line_count; i += 1) { auto u = i * step; @@ -148,8 +140,6 @@ size_t QuadraticPathComponent::FillPointsForPolyline(std::vector& points, points.emplace_back(Solve(t)); } points.emplace_back(p2); - - return line_count + 1 + prev_count; } std::vector QuadraticPathComponent::Extrema() const { @@ -191,14 +181,12 @@ Point CubicPathComponent::SolveDerivative(Scalar time) const { }; } -std::vector CubicPathComponent::CreatePolyline(Scalar scale) const { +void CubicPathComponent::CreatePolyline(Scalar scale, + std::vector& points) const { auto quads = ToQuadraticPathComponents(.1); - std::vector points; - size_t running_count = 0; for (const auto& quad : quads) { - running_count = quad.FillPointsForPolyline(points, scale, running_count); + quad.CreatePolyline(scale, points); } - return points; } inline QuadraticPathComponent CubicPathComponent::Lower() const { diff --git a/impeller/geometry/path_component.h b/impeller/geometry/path_component.h index 69bc38eca2381..2c8e54465ad59 100644 --- a/impeller/geometry/path_component.h +++ b/impeller/geometry/path_component.h @@ -34,7 +34,7 @@ struct LinearPathComponent { Point Solve(Scalar time) const; - std::vector CreatePolyline() const; + void CreatePolyline(std::vector& points) const; std::vector Extrema() const; @@ -75,11 +75,7 @@ struct QuadraticPathComponent { // making it trivially parallelizable. // // See also the implementation in kurbo: https://github.com/linebender/kurbo. - std::vector CreatePolyline(Scalar scale) const; - - size_t FillPointsForPolyline(std::vector& points, - Scalar scale_factor, - size_t prev_count = 0) const; + void CreatePolyline(Scalar scale_factor, std::vector& points) const; std::vector Extrema() const; @@ -122,7 +118,7 @@ struct CubicPathComponent { // generates a polyline from those quadratics. // // See the note on QuadraticPathComponent::CreatePolyline for references. - std::vector CreatePolyline(Scalar scale) const; + void CreatePolyline(Scalar scale, std::vector& points) const; std::vector Extrema() const; diff --git a/impeller/renderer/renderer_unittests.cc b/impeller/renderer/renderer_unittests.cc index 2ecc6fab2a004..749d611bb322a 100644 --- a/impeller/renderer/renderer_unittests.cc +++ b/impeller/renderer/renderer_unittests.cc @@ -399,11 +399,10 @@ TEST_P(RendererTest, CanRenderInstanced) { ASSERT_EQ(Tessellator::Result::kSuccess, Tessellator{}.Tessellate( - FillType::kPositive, PathBuilder{} .AddRect(Rect::MakeXYWH(10, 10, 100, 100)) - .TakePath() - .CreatePolyline(1.0f), + .TakePath(FillType::kPositive), + 1.0f, [&builder](const float* vertices, size_t vertices_count, const uint16_t* indices, size_t indices_count) { for (auto i = 0u; i < vertices_count * 2; i += 2) { diff --git a/impeller/tessellator/BUILD.gn b/impeller/tessellator/BUILD.gn index d3915f9996679..25585010851aa 100644 --- a/impeller/tessellator/BUILD.gn +++ b/impeller/tessellator/BUILD.gn @@ -12,7 +12,10 @@ impeller_component("tessellator") { public_deps = [ "../geometry" ] - deps = [ "//third_party/libtess2" ] + deps = [ + "//flutter/fml", + "//third_party/libtess2", + ] } impeller_component("tessellator_shared") { @@ -32,6 +35,7 @@ impeller_component("tessellator_shared") { deps = [ "../geometry", + "//flutter/fml", "//third_party/libtess2", ] diff --git a/impeller/tessellator/c/tessellator.cc b/impeller/tessellator/c/tessellator.cc index b576d4393e809..f5300dcba1dee 100644 --- a/impeller/tessellator/c/tessellator.cc +++ b/impeller/tessellator/c/tessellator.cc @@ -41,10 +41,9 @@ struct Vertices* Tessellate(PathBuilder* builder, int fill_type, Scalar tolerance) { auto path = builder->CopyPath(static_cast(fill_type)); - auto polyline = path.CreatePolyline(tolerance); std::vector points; if (Tessellator{}.Tessellate( - path.GetFillType(), polyline, + path, tolerance, [&points](const float* vertices, size_t vertices_count, const uint16_t* indices, size_t indices_count) { // Results are expected to be re-duplicated. diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index f7c80fe747ea0..e30c56a485c6a 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -4,10 +4,13 @@ #include "impeller/tessellator/tessellator.h" +#include "flutter/fml/thread_local.h" #include "third_party/libtess2/Include/tesselator.h" namespace impeller { +FML_THREAD_LOCAL std::vector tls_point_buffer; + static void* HeapAlloc(void* userData, unsigned int size) { return malloc(size); } @@ -32,6 +35,7 @@ static const TESSalloc kAlloc = { }; Tessellator::Tessellator() : c_tessellator_(nullptr, &DestroyTessellator) { + tls_point_buffer.reserve(2048); TESSalloc alloc = kAlloc; { // libTess2 copies the TESSalloc despite the non-const argument. @@ -58,14 +62,17 @@ static int ToTessWindingRule(FillType fill_type) { return TESS_WINDING_ODD; } -Tessellator::Result Tessellator::Tessellate( - FillType fill_type, - const Path::Polyline& polyline, - const BuilderCallback& callback) const { +Tessellator::Result Tessellator::Tessellate(const Path& path, + Scalar scale, + const BuilderCallback& callback) { if (!callback) { return Result::kInputError; } + tls_point_buffer.clear(); + auto polyline = path.CreatePolyline(scale, tls_point_buffer); + auto fill_type = path.GetFillType(); + if (polyline.points.empty()) { return Result::kInputError; } @@ -218,6 +225,39 @@ Tessellator::Result Tessellator::Tessellate( return Result::kSuccess; } +std::pair, std::vector> +Tessellator::TessellateConvex(const Path& path, Scalar scale) { + std::vector output; + std::vector indices; + + tls_point_buffer.clear(); + auto polyline = path.CreatePolyline(scale, tls_point_buffer); + + for (auto j = 0u; j < polyline.contours.size(); j++) { + auto [start, end] = polyline.GetContourPointBounds(j); + auto center = polyline.points[start]; + + // Some polygons will not self close and an additional triangle + // must be inserted, others will self close and we need to avoid + // inserting an extra triangle. + if (polyline.points[end - 1] == polyline.points[start]) { + end--; + } + output.emplace_back(center); + output.emplace_back(polyline.points[start + 1]); + + for (auto i = start + 2; i < end; i++) { + const auto& point_b = polyline.points[i]; + output.emplace_back(point_b); + + indices.emplace_back(0); + indices.emplace_back(i - 1); + indices.emplace_back(i); + } + } + return std::make_pair(output, indices); +} + void DestroyTessellator(TESStesselator* tessellator) { if (tessellator != nullptr) { ::tessDeleteTess(tessellator); diff --git a/impeller/tessellator/tessellator.h b/impeller/tessellator/tessellator.h index fd86d29edfa95..b6b62756dcbc3 100644 --- a/impeller/tessellator/tessellator.h +++ b/impeller/tessellator/tessellator.h @@ -30,7 +30,8 @@ enum class WindingOrder { /// @brief A utility that generates triangles of the specified fill type /// given a polyline. This happens on the CPU. /// -/// @bug This should just be called a triangulator. +/// This object is implemented in such a way that there must be +/// only one per thread. /// class Tessellator { public: @@ -58,18 +59,23 @@ class Tessellator { size_t indices_count)>; //---------------------------------------------------------------------------- - /// @brief Generates filled triangles from the polyline. A callback is + /// @brief Generates filled triangles from the path. A callback is /// invoked once for the entire tessellation. /// - /// @param[in] fill_type The fill rule to use when filling. - /// @param[in] polyline The polyline + /// @param[in] The path to tessellate. /// @param[in] callback The callback, return false to indicate failure. /// /// @return The result status of the tessellation. /// - Tessellator::Result Tessellate(FillType fill_type, - const Path::Polyline& polyline, - const BuilderCallback& callback) const; + Tessellator::Result Tessellate(const Path& path, + Scalar scale, + const BuilderCallback& callback); + + //---------------------------------------------------------------------------- + /// @brief Given a convex path, create a triangle fan structure. + std::pair, std::vector> TessellateConvex( + const Path& path, + Scalar scale); private: CTessellator c_tessellator_; diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index 8fca675a9a9a6..a8126c009023b 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -15,42 +15,39 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { // Zero points. { Tessellator t; - auto polyline = PathBuilder{}.TakePath().CreatePolyline(1.0f); + auto path = PathBuilder{}.TakePath(FillType::kPositive); Tessellator::Result result = t.Tessellate( - FillType::kPositive, polyline, + path, 1.0f, [](const float* vertices, size_t vertices_count, const uint16_t* indices, size_t indices_count) { return true; }); - ASSERT_EQ(polyline.points.size(), 0u); ASSERT_EQ(result, Tessellator::Result::kInputError); } // One point. { Tessellator t; - auto polyline = - PathBuilder{}.LineTo({0, 0}).TakePath().CreatePolyline(1.0f); + auto path = PathBuilder{}.LineTo({0, 0}).TakePath(FillType::kPositive); Tessellator::Result result = - t.Tessellate(FillType::kPositive, polyline, + t.Tessellate(path, 1.0f, [](const float* vertices, size_t vertices_count, const uint16_t* indices_count, size_t indices_size) { return true; }); - ASSERT_EQ(polyline.points.size(), 1u); + ASSERT_EQ(result, Tessellator::Result::kSuccess); } // Two points. { Tessellator t; - auto polyline = - PathBuilder{}.AddLine({0, 0}, {0, 1}).TakePath().CreatePolyline(1.0f); + auto path = + PathBuilder{}.AddLine({0, 0}, {0, 1}).TakePath(FillType::kPositive); Tessellator::Result result = - t.Tessellate(FillType::kPositive, polyline, + t.Tessellate(path, 1.0f, [](const float* vertices, size_t vertices_count, const uint16_t* indices_count, size_t indices_size) { return true; }); - ASSERT_EQ(polyline.points.size(), 2u); ASSERT_EQ(result, Tessellator::Result::kSuccess); } @@ -62,29 +59,27 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { auto coord = i * 1.0f; builder.AddLine({coord, coord}, {coord + 1, coord + 1}); } - auto polyline = builder.TakePath().CreatePolyline(1.0f); + auto path = PathBuilder{}.TakePath(FillType::kPositive); Tessellator::Result result = - t.Tessellate(FillType::kPositive, polyline, + t.Tessellate(path, 1.0f, [](const float* vertices, size_t vertices_count, const uint16_t* indices_count, size_t indices_size) { return true; }); - ASSERT_EQ(polyline.points.size(), 2000u); ASSERT_EQ(result, Tessellator::Result::kSuccess); } // Closure fails. { Tessellator t; - auto polyline = - PathBuilder{}.AddLine({0, 0}, {0, 1}).TakePath().CreatePolyline(1.0f); + auto path = + PathBuilder{}.AddLine({0, 0}, {0, 1}).TakePath(FillType::kPositive); Tessellator::Result result = - t.Tessellate(FillType::kPositive, polyline, + t.Tessellate(path, 1.0f, [](const float* vertices, size_t vertices_count, const uint16_t* indices_count, size_t indices_size) { return false; }); - ASSERT_EQ(polyline.points.size(), 2u); ASSERT_EQ(result, Tessellator::Result::kInputError); } @@ -95,10 +90,10 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { for (auto i = 0u; i < Tessellator::kMultiContourThreshold + 1; i++) { builder.AddCircle(Point(i, i), 4); } - auto polyline = builder.TakePath().CreatePolyline(1.0f); + auto path = builder.TakePath(FillType::kNonZero); bool no_indices = false; Tessellator::Result result = t.Tessellate( - FillType::kNonZero, polyline, + path, 1.0f, [&no_indices](const float* vertices, size_t vertices_count, const uint16_t* indices, size_t indices_count) { no_indices = indices == nullptr; @@ -116,11 +111,11 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { for (auto i = 0; i < 1000; i++) { builder.AddCircle(Point(i, i), 4); } - auto polyline = builder.TakePath(FillType::kOdd).CreatePolyline(1.0f); + auto path = builder.TakePath(FillType::kOdd); bool no_indices = false; size_t count = 0u; Tessellator::Result result = t.Tessellate( - FillType::kOdd, polyline, + path, 1.0f, [&no_indices, &count](const float* vertices, size_t vertices_count, const uint16_t* indices, size_t indices_count) { no_indices = indices == nullptr; @@ -134,5 +129,40 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { } } +TEST(TessellatorTest, TessellateConvex) { + { + Tessellator t; + // Sanity check simple rectangle. + auto [pts, indices] = t.TessellateConvex( + PathBuilder{}.AddRect(Rect::MakeLTRB(0, 0, 10, 10)).TakePath(), 1.0); + + std::vector expected = { + {0, 0}, {10, 0}, {10, 10}, {0, 10}, // + }; + std::vector expected_indices = {0, 1, 2, 0, 2, 3}; + ASSERT_EQ(pts, expected); + ASSERT_EQ(indices, expected_indices); + } + + { + Tessellator t; + auto [pts, indices] = + t.TessellateConvex(PathBuilder{} + .AddRect(Rect::MakeLTRB(0, 0, 10, 10)) + .AddRect(Rect::MakeLTRB(20, 20, 30, 30)) + .TakePath(), + 1.0); + + std::vector expected = { + {0, 0}, {10, 0}, {10, 10}, {0, 10}, // + {20, 20}, {30, 20}, {30, 30}, {20, 30} // + }; + std::vector expected_indices = {0, 1, 2, 0, 2, 3, + 0, 6, 7, 0, 7, 8}; + ASSERT_EQ(pts, expected); + ASSERT_EQ(indices, expected_indices); + } +} + } // namespace testing } // namespace impeller From a92b7579adce1d518f7ec8ac76bb4e80fb719574 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 8 Nov 2023 15:37:56 -0800 Subject: [PATCH 03/11] fixes for tests, minor cleanup/rename --- .../entity/geometry/stroke_path_geometry.cc | 4 +- impeller/geometry/geometry_unittests.cc | 2 +- impeller/geometry/path.cc | 10 ++--- impeller/geometry/path_component.cc | 19 ++++++---- impeller/geometry/path_component.h | 16 ++++---- impeller/tessellator/tessellator.cc | 2 + impeller/tessellator/tessellator_unittests.cc | 38 +++++++++---------- 7 files changed, 48 insertions(+), 43 deletions(-) diff --git a/impeller/entity/geometry/stroke_path_geometry.cc b/impeller/entity/geometry/stroke_path_geometry.cc index 5eda19a1c98de..c848b517f008b 100644 --- a/impeller/entity/geometry/stroke_path_geometry.cc +++ b/impeller/entity/geometry/stroke_path_geometry.cc @@ -130,7 +130,7 @@ StrokePathGeometry::JoinProc StrokePathGeometry::GetJoinProc(Join stroke_join) { std::vector arc_points; CubicPathComponent(start_offset, start_handle, middle_handle, middle) - .CreatePolyline(scale, arc_points); + .AppendPolylinePoints(scale, arc_points); VS::PerVertexData vtx; for (const auto& point : arc_points) { @@ -193,7 +193,7 @@ StrokePathGeometry::CapProc StrokePathGeometry::GetCapProc(Cap stroke_cap) { vtx.position = position - orientation; vtx_builder.AppendVertex(vtx); std::vector arc_points; - arc.CreatePolyline(scale, arc_points); + arc.AppendPolylinePoints(scale, arc_points); for (const auto& point : arc_points) { vtx.position = position + point; vtx_builder.AppendVertex(vtx); diff --git a/impeller/geometry/geometry_unittests.cc b/impeller/geometry/geometry_unittests.cc index abd26f62b76e4..95a88f583c6a8 100644 --- a/impeller/geometry/geometry_unittests.cc +++ b/impeller/geometry/geometry_unittests.cc @@ -2101,7 +2101,7 @@ TEST(GeometryTest, RectRoundOut) { TEST(GeometryTest, CubicPathComponentPolylineDoesNotIncludePointOne) { CubicPathComponent component({10, 10}, {20, 35}, {35, 20}, {40, 40}); std::vector polyline; - component.CreatePolyline(1.0f, polyline); + component.AppendPolylinePoints(1.0f, polyline); ASSERT_NE(polyline.front().x, 10); ASSERT_NE(polyline.front().y, 10); ASSERT_EQ(polyline.back().x, 40); diff --git a/impeller/geometry/path.cc b/impeller/geometry/path.cc index d3ee3934c209f..7e45c750104d2 100644 --- a/impeller/geometry/path.cc +++ b/impeller/geometry/path.cc @@ -314,8 +314,8 @@ Path::Polyline Path::CreatePolyline(Scalar scale, std::optional previous_path_component_index; auto end_contour = [&polyline, &previous_path_component_index, &get_path_component, &components]() { - // Whenever a contour has ended, extract the exact end direction from the - // last component. + // Whenever a contour has ended, extract the exact end direction from + // the last component. if (polyline.contours.empty()) { return; } @@ -356,7 +356,7 @@ Path::Polyline Path::CreatePolyline(Scalar scale, .component_start_index = polyline.points.size() - 1, .is_curve = false, }); - linears_[component.index].CreatePolyline(polyline.points); + linears_[component.index].AppendPolylinePoints(polyline.points); previous_path_component_index = component_i; break; case ComponentType::kQuadratic: @@ -364,7 +364,7 @@ Path::Polyline Path::CreatePolyline(Scalar scale, .component_start_index = polyline.points.size() - 1, .is_curve = true, }); - quads_[component.index].CreatePolyline(scale, polyline.points); + quads_[component.index].AppendPolylinePoints(scale, polyline.points); previous_path_component_index = component_i; break; case ComponentType::kCubic: @@ -372,7 +372,7 @@ Path::Polyline Path::CreatePolyline(Scalar scale, .component_start_index = polyline.points.size() - 1, .is_curve = true, }); - cubics_[component.index].CreatePolyline(scale, polyline.points); + cubics_[component.index].AppendPolylinePoints(scale, polyline.points); previous_path_component_index = component_i; break; case ComponentType::kContour: diff --git a/impeller/geometry/path_component.cc b/impeller/geometry/path_component.cc index 0170774e55cec..32234633cd97f 100644 --- a/impeller/geometry/path_component.cc +++ b/impeller/geometry/path_component.cc @@ -59,8 +59,11 @@ Point LinearPathComponent::Solve(Scalar time) const { }; } -void LinearPathComponent::CreatePolyline(std::vector& points) const { - points.push_back(p2); +void LinearPathComponent::AppendPolylinePoints( + std::vector& points) const { + if (points.size() == 0 || points.back() != p2) { + points.push_back(p2); + } } std::vector LinearPathComponent::Extrema() const { @@ -100,8 +103,9 @@ static Scalar ApproximateParabolaIntegral(Scalar x) { return x / (1.0 - d + sqrt(sqrt(pow(d, 4) + 0.25 * x * x))); } -void QuadraticPathComponent::CreatePolyline(Scalar scale_factor, - std::vector& points) const { +void QuadraticPathComponent::AppendPolylinePoints( + Scalar scale_factor, + std::vector& points) const { auto tolerance = kDefaultCurveTolerance / scale_factor; auto sqrt_tolerance = sqrt(tolerance); @@ -181,11 +185,12 @@ Point CubicPathComponent::SolveDerivative(Scalar time) const { }; } -void CubicPathComponent::CreatePolyline(Scalar scale, - std::vector& points) const { +void CubicPathComponent::AppendPolylinePoints( + Scalar scale, + std::vector& points) const { auto quads = ToQuadraticPathComponents(.1); for (const auto& quad : quads) { - quad.CreatePolyline(scale, points); + quad.AppendPolylinePoints(scale, points); } } diff --git a/impeller/geometry/path_component.h b/impeller/geometry/path_component.h index 2c8e54465ad59..c721afd8c0826 100644 --- a/impeller/geometry/path_component.h +++ b/impeller/geometry/path_component.h @@ -14,9 +14,9 @@ namespace impeller { -// The default tolerance value for QuadraticCurveComponent::CreatePolyline and -// CubicCurveComponent::CreatePolyline. It also impacts the number of quadratics -// created when flattening a cubic curve to a polyline. +// The default tolerance value for QuadraticCurveComponent::AppendPolylinePoints +// and CubicCurveComponent::AppendPolylinePoints. It also impacts the number of +// quadratics created when flattening a cubic curve to a polyline. // // Smaller numbers mean more points. This number seems suitable for particularly // curvy curves at scales close to 1.0. As the scale increases, this number @@ -34,7 +34,7 @@ struct LinearPathComponent { Point Solve(Scalar time) const; - void CreatePolyline(std::vector& points) const; + void AppendPolylinePoints(std::vector& points) const; std::vector Extrema() const; @@ -75,7 +75,8 @@ struct QuadraticPathComponent { // making it trivially parallelizable. // // See also the implementation in kurbo: https://github.com/linebender/kurbo. - void CreatePolyline(Scalar scale_factor, std::vector& points) const; + void AppendPolylinePoints(Scalar scale_factor, + std::vector& points) const; std::vector Extrema() const; @@ -117,8 +118,9 @@ struct CubicPathComponent { // This method approximates the cubic component with quadratics, and then // generates a polyline from those quadratics. // - // See the note on QuadraticPathComponent::CreatePolyline for references. - void CreatePolyline(Scalar scale, std::vector& points) const; + // See the note on QuadraticPathComponent::AppendPolylinePoints for + // references. + void AppendPolylinePoints(Scalar scale, std::vector& points) const; std::vector Extrema() const; diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index e30c56a485c6a..478223777964a 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -206,12 +206,14 @@ Tessellator::Result Tessellator::Tessellate(const Path& path, int vertex_item_count = tessGetVertexCount(tessellator) * kVertexSize; auto vertices = tessGetVertices(tessellator); + points.reserve(vertex_item_count); for (int i = 0; i < vertex_item_count; i += 2) { points.emplace_back(vertices[i], vertices[i + 1]); } int element_item_count = tessGetElementCount(tessellator) * kPolygonSize; auto elements = tessGetElements(tessellator); + data.reserve(element_item_count); for (int i = 0; i < element_item_count; i++) { data.emplace_back(points[elements[i]].x); data.emplace_back(points[elements[i]].y); diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index a8126c009023b..3ef6bd163691a 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -28,11 +28,10 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { { Tessellator t; auto path = PathBuilder{}.LineTo({0, 0}).TakePath(FillType::kPositive); - Tessellator::Result result = - t.Tessellate(path, 1.0f, - [](const float* vertices, size_t vertices_count, - const uint16_t* indices_count, - size_t indices_size) { return true; }); + Tessellator::Result result = t.Tessellate( + path, 1.0f, + [](const float* vertices, size_t vertices_count, + const uint16_t* indices, size_t indices_count) { return true; }); ASSERT_EQ(result, Tessellator::Result::kSuccess); } @@ -42,11 +41,10 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { Tessellator t; auto path = PathBuilder{}.AddLine({0, 0}, {0, 1}).TakePath(FillType::kPositive); - Tessellator::Result result = - t.Tessellate(path, 1.0f, - [](const float* vertices, size_t vertices_count, - const uint16_t* indices_count, - size_t indices_size) { return true; }); + Tessellator::Result result = t.Tessellate( + path, 1.0f, + [](const float* vertices, size_t vertices_count, + const uint16_t* indices, size_t indices_count) { return true; }); ASSERT_EQ(result, Tessellator::Result::kSuccess); } @@ -59,12 +57,11 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { auto coord = i * 1.0f; builder.AddLine({coord, coord}, {coord + 1, coord + 1}); } - auto path = PathBuilder{}.TakePath(FillType::kPositive); - Tessellator::Result result = - t.Tessellate(path, 1.0f, - [](const float* vertices, size_t vertices_count, - const uint16_t* indices_count, - size_t indices_size) { return true; }); + auto path = builder.TakePath(FillType::kPositive); + Tessellator::Result result = t.Tessellate( + path, 1.0f, + [](const float* vertices, size_t vertices_count, + const uint16_t* indices, size_t indices_count) { return true; }); ASSERT_EQ(result, Tessellator::Result::kSuccess); } @@ -74,11 +71,10 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { Tessellator t; auto path = PathBuilder{}.AddLine({0, 0}, {0, 1}).TakePath(FillType::kPositive); - Tessellator::Result result = - t.Tessellate(path, 1.0f, - [](const float* vertices, size_t vertices_count, - const uint16_t* indices_count, - size_t indices_size) { return false; }); + Tessellator::Result result = t.Tessellate( + path, 1.0f, + [](const float* vertices, size_t vertices_count, + const uint16_t* indices, size_t indices_count) { return false; }); ASSERT_EQ(result, Tessellator::Result::kInputError); } From aa280ecb308d0d72f1d24066f7b99da27fb48804 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 8 Nov 2023 15:42:36 -0800 Subject: [PATCH 04/11] docs --- impeller/geometry/path.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/impeller/geometry/path.h b/impeller/geometry/path.h index 5d065ad6ab6ac..64e1fc52b6f29 100644 --- a/impeller/geometry/path.h +++ b/impeller/geometry/path.h @@ -90,13 +90,20 @@ class Path { /// One or more contours represented as a series of points and indices in /// the point vector representing the start of a new contour. + /// + /// Polylines are ephemeral and meant to be used by the tessellator. They do + /// not allocate their own point vectors to allow for optimizations around + /// allocation and reuse of arenas. struct Polyline { + /// The buffer must remain valid for the lifetime of this object. explicit Polyline(std::vector& point_buffer); /// Points in the polyline, which may represent multiple contours specified /// by indices in |breaks|. std::vector& points; + /// Contours are disconnected pieces of a polyline, such as when a MoveTo + /// was issued on a PathBuilder. std::vector contours; /// Convenience method to compute the start (inclusive) and end (exclusive) From cd60b9f762fa461598e61ddd997039d9cbfe007f Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 8 Nov 2023 15:51:18 -0800 Subject: [PATCH 05/11] one more reserve --- impeller/geometry/path_component.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/geometry/path_component.cc b/impeller/geometry/path_component.cc index 32234633cd97f..96cc8bc78ce40 100644 --- a/impeller/geometry/path_component.cc +++ b/impeller/geometry/path_component.cc @@ -229,7 +229,7 @@ CubicPathComponent::ToQuadraticPathComponents(Scalar accuracy) const { auto p = p2x2 - p1x2; auto err = p.Dot(p); auto quad_count = std::max(1., ceil(pow(err / max_hypot2, 1. / 6.0))); - + quads.reserve(quad_count); for (size_t i = 0; i < quad_count; i++) { auto t0 = i / quad_count; auto t1 = (i + 1) / quad_count; From 07eee60cd320b2f35eb54db63db773286c22970c Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 9 Nov 2023 13:21:13 -0800 Subject: [PATCH 06/11] No TLS, unique_ptr for vector with callback --- .../entity/geometry/stroke_path_geometry.cc | 46 +++++++------- impeller/geometry/geometry_benchmarks.cc | 13 ++-- impeller/geometry/geometry_unittests.cc | 62 +++++++++---------- impeller/geometry/path.cc | 47 +++++++++----- impeller/geometry/path.h | 28 +++++++-- impeller/tessellator/BUILD.gn | 6 +- impeller/tessellator/tessellator.cc | 52 +++++++++------- impeller/tessellator/tessellator.h | 26 ++++++-- 8 files changed, 167 insertions(+), 113 deletions(-) diff --git a/impeller/entity/geometry/stroke_path_geometry.cc b/impeller/entity/geometry/stroke_path_geometry.cc index c848b517f008b..c4a6a115ac965 100644 --- a/impeller/entity/geometry/stroke_path_geometry.cc +++ b/impeller/entity/geometry/stroke_path_geometry.cc @@ -236,9 +236,12 @@ StrokePathGeometry::CreateSolidStrokeVertices( const StrokePathGeometry::CapProc& cap_proc, Scalar scale) { VertexBufferBuilder vtx_builder; - std::vector point_buffer; - point_buffer.reserve(512); - auto polyline = path.CreatePolyline(scale, point_buffer); + auto point_buffer = std::make_unique>(); + // 512 is an arbitrary choice that should be big enough for most paths without + // needing to reallocate. If we have motivating benchmarks we should raise or + // lower this number, cause dnfield just made it up! + point_buffer->reserve(512); + auto polyline = path.CreatePolyline(scale, std::move(point_buffer)); VS::PerVertexData vtx; @@ -261,7 +264,8 @@ StrokePathGeometry::CreateSolidStrokeVertices( direction = -contour.start_direction; } else { direction = - (polyline.points[point_i] - polyline.points[point_i - 1]).Normalize(); + ((*polyline.points)[point_i] - (*polyline.points)[point_i - 1]) + .Normalize(); } previous_offset = offset; offset = Vector2{-direction.y, direction.x} * stroke_width * 0.5; @@ -280,23 +284,23 @@ StrokePathGeometry::CreateSolidStrokeVertices( for (size_t point_i = component_start_index; point_i < component_end_index; point_i++) { auto is_end_of_component = point_i == component_end_index - 1; - vtx.position = polyline.points[point_i] + offset; + vtx.position = (*polyline.points)[point_i] + offset; vtx_builder.AppendVertex(vtx); - vtx.position = polyline.points[point_i] - offset; + vtx.position = (*polyline.points)[point_i] - offset; vtx_builder.AppendVertex(vtx); // 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 + 1] + offset; + vtx.position = (*polyline.points)[point_i + 1] + offset; vtx_builder.AppendVertex(vtx); - vtx.position = polyline.points[point_i + 1] - offset; + vtx.position = (*polyline.points)[point_i + 1] - offset; vtx_builder.AppendVertex(vtx); compute_offset(point_i + 2, contour_start_point_i, contour_end_point_i, contour); if (!is_last_component && is_end_of_component) { // Generate join from the current line to the next line. - join_proc(vtx_builder, polyline.points[point_i + 1], + join_proc(vtx_builder, (*polyline.points)[point_i + 1], previous_offset, offset, scaled_miter_limit, scale); } } @@ -316,9 +320,9 @@ StrokePathGeometry::CreateSolidStrokeVertices( point_i < component_end_index; point_i++) { auto is_end_of_component = point_i == component_end_index - 1; - vtx.position = polyline.points[point_i] + offset; + vtx.position = (*polyline.points)[point_i] + offset; vtx_builder.AppendVertex(vtx); - vtx.position = polyline.points[point_i] - offset; + vtx.position = (*polyline.points)[point_i] - offset; vtx_builder.AppendVertex(vtx); compute_offset(point_i + 2, contour_start_point_i, @@ -326,13 +330,13 @@ StrokePathGeometry::CreateSolidStrokeVertices( // For curve components, the polyline is detailed enough such that // it can avoid worrying about joins altogether. if (is_end_of_component) { - vtx.position = polyline.points[point_i + 1] + offset; + vtx.position = (*polyline.points)[point_i + 1] + offset; vtx_builder.AppendVertex(vtx); - vtx.position = polyline.points[point_i + 1] - offset; + vtx.position = (*polyline.points)[point_i + 1] - offset; vtx_builder.AppendVertex(vtx); // Generate join from the current line to the next line. if (!is_last_component) { - join_proc(vtx_builder, polyline.points[point_i + 1], + join_proc(vtx_builder, (*polyline.points)[point_i + 1], previous_offset, offset, scaled_miter_limit, scale); } } @@ -348,7 +352,7 @@ StrokePathGeometry::CreateSolidStrokeVertices( switch (contour_end_point_i - contour_start_point_i) { case 1: { - Point p = polyline.points[contour_start_point_i]; + Point p = (*polyline.points)[contour_start_point_i]; cap_proc(vtx_builder, p, {-stroke_width * 0.5f, 0}, scale, false); cap_proc(vtx_builder, p, {stroke_width * 0.5f, 0}, scale, false); continue; @@ -371,14 +375,14 @@ StrokePathGeometry::CreateSolidStrokeVertices( // vertices at the start of the new contour (thus connecting the two // contours with two zero volume triangles, which will be discarded by // the rasterizer). - vtx.position = polyline.points[contour_start_point_i - 1]; + vtx.position = (*polyline.points)[contour_start_point_i - 1]; // Append two vertices when "picking up" the pen so that the triangle // drawn when moving to the beginning of the new contour will have zero // volume. vtx_builder.AppendVertex(vtx); vtx_builder.AppendVertex(vtx); - vtx.position = polyline.points[contour_start_point_i]; + vtx.position = (*polyline.points)[contour_start_point_i]; // Append two vertices at the beginning of the new contour, which // appends two triangles of zero area. vtx_builder.AppendVertex(vtx); @@ -390,8 +394,8 @@ StrokePathGeometry::CreateSolidStrokeVertices( auto cap_offset = Vector2(-contour.start_direction.y, contour.start_direction.x) * stroke_width * 0.5; // Counterclockwise normal - cap_proc(vtx_builder, polyline.points[contour_start_point_i], cap_offset, - scale, true); + cap_proc(vtx_builder, (*polyline.points)[contour_start_point_i], + cap_offset, scale, true); } for (size_t contour_component_i = 0; @@ -422,10 +426,10 @@ StrokePathGeometry::CreateSolidStrokeVertices( auto cap_offset = Vector2(-contour.end_direction.y, contour.end_direction.x) * stroke_width * 0.5; // Clockwise normal - cap_proc(vtx_builder, polyline.points[contour_end_point_i - 1], + cap_proc(vtx_builder, (*polyline.points)[contour_end_point_i - 1], cap_offset, scale, false); } else { - join_proc(vtx_builder, polyline.points[contour_start_point_i], offset, + join_proc(vtx_builder, (*polyline.points)[contour_start_point_i], offset, contour_first_offset, scaled_miter_limit, scale); } } diff --git a/impeller/geometry/geometry_benchmarks.cc b/impeller/geometry/geometry_benchmarks.cc index 9e44c6d792d59..47e056059ba12 100644 --- a/impeller/geometry/geometry_benchmarks.cc +++ b/impeller/geometry/geometry_benchmarks.cc @@ -28,8 +28,8 @@ static void BM_Polyline(benchmark::State& state, Args&&... args) { size_t point_count = 0u; size_t single_point_count = 0u; - std::vector points; - points.reserve(2048); + auto points = std::make_unique>(); + points->reserve(2048); while (state.KeepRunning()) { if (tessellate) { tess.Tessellate(path, 1.0f, @@ -46,10 +46,13 @@ static void BM_Polyline(benchmark::State& state, Args&&... args) { return true; }); } else { - auto polyline = path.CreatePolyline(1.0f, points); - single_point_count = polyline.points.size(); + auto polyline = path.CreatePolyline( + 1.0f, std::move(points), + [&points](Path::Polyline::PointBufferPointer reclaimed) { + points = std::move(reclaimed); + }); + single_point_count = polyline.points->size(); point_count += single_point_count; - points.clear(); } } state.counters["SinglePointCount"] = single_point_count; diff --git a/impeller/geometry/geometry_unittests.cc b/impeller/geometry/geometry_unittests.cc index 95a88f583c6a8..cc5a97b927b86 100644 --- a/impeller/geometry/geometry_unittests.cc +++ b/impeller/geometry/geometry_unittests.cc @@ -599,9 +599,8 @@ TEST(GeometryTest, EmptyPath) { path.GetContourComponentAtIndex(0, c); ASSERT_POINT_NEAR(c.destination, Point()); - std::vector polyline_points; - Path::Polyline polyline = path.CreatePolyline(1.0f, polyline_points); - ASSERT_TRUE(polyline.points.empty()); + Path::Polyline polyline = path.CreatePolyline(1.0f); + ASSERT_TRUE(polyline.points->empty()); ASSERT_TRUE(polyline.contours.empty()); } @@ -2116,16 +2115,15 @@ TEST(GeometryTest, PathCreatePolyLineDoesNotDuplicatePoints) { builder.MoveTo({40, 40}); builder.LineTo({50, 50}); - std::vector points; - auto polyline = builder.TakePath().CreatePolyline(1.0f, points); + auto polyline = builder.TakePath().CreatePolyline(1.0f); ASSERT_EQ(polyline.contours.size(), 2u); - ASSERT_EQ(polyline.points.size(), 5u); - ASSERT_EQ(polyline.points[0].x, 10); - ASSERT_EQ(polyline.points[1].x, 20); - ASSERT_EQ(polyline.points[2].x, 30); - ASSERT_EQ(polyline.points[3].x, 40); - ASSERT_EQ(polyline.points[4].x, 50); + ASSERT_EQ(polyline.points->size(), 5u); + ASSERT_EQ((*polyline.points)[0].x, 10); + ASSERT_EQ((*polyline.points)[1].x, 20); + ASSERT_EQ((*polyline.points)[2].x, 30); + ASSERT_EQ((*polyline.points)[3].x, 40); + ASSERT_EQ((*polyline.points)[4].x, 50); } TEST(GeometryTest, PathBuilderSetsCorrectContourPropertiesForAddCommands) { @@ -2199,7 +2197,6 @@ TEST(GeometryTest, PathBuilderSetsCorrectContourPropertiesForAddCommands) { } TEST(GeometryTest, PathCreatePolylineGeneratesCorrectContourData) { - std::vector polyline_points; Path::Polyline polyline = PathBuilder{} .AddLine({100, 100}, {200, 100}) .MoveTo({100, 200}) @@ -2207,8 +2204,8 @@ TEST(GeometryTest, PathCreatePolylineGeneratesCorrectContourData) { .LineTo({200, 200}) .Close() .TakePath() - .CreatePolyline(1.0f, polyline_points); - ASSERT_EQ(polyline.points.size(), 6u); + .CreatePolyline(1.0f); + ASSERT_EQ(polyline.points->size(), 6u); ASSERT_EQ(polyline.contours.size(), 2u); ASSERT_EQ(polyline.contours[0].is_closed, false); ASSERT_EQ(polyline.contours[0].start_index, 0u); @@ -2217,7 +2214,6 @@ TEST(GeometryTest, PathCreatePolylineGeneratesCorrectContourData) { } TEST(GeometryTest, PolylineGetContourPointBoundsReturnsCorrectRanges) { - std::vector polyline_points; Path::Polyline polyline = PathBuilder{} .AddLine({100, 100}, {200, 100}) .MoveTo({100, 200}) @@ -2225,7 +2221,7 @@ TEST(GeometryTest, PolylineGetContourPointBoundsReturnsCorrectRanges) { .LineTo({200, 200}) .Close() .TakePath() - .CreatePolyline(1.0f, polyline_points); + .CreatePolyline(1.0f); size_t a1, a2, b1, b2; std::tie(a1, a2) = polyline.GetContourPointBounds(0); std::tie(b1, b2) = polyline.GetContourPointBounds(1); @@ -2236,24 +2232,22 @@ TEST(GeometryTest, PolylineGetContourPointBoundsReturnsCorrectRanges) { } TEST(GeometryTest, PathAddRectPolylineHasCorrectContourData) { - std::vector polyline_points; Path::Polyline polyline = PathBuilder{} .AddRect(Rect::MakeLTRB(50, 60, 70, 80)) .TakePath() - .CreatePolyline(1.0f, polyline_points); + .CreatePolyline(1.0f); ASSERT_EQ(polyline.contours.size(), 1u); ASSERT_TRUE(polyline.contours[0].is_closed); ASSERT_EQ(polyline.contours[0].start_index, 0u); - ASSERT_EQ(polyline.points.size(), 5u); - ASSERT_EQ(polyline.points[0], Point(50, 60)); - ASSERT_EQ(polyline.points[1], Point(70, 60)); - ASSERT_EQ(polyline.points[2], Point(70, 80)); - ASSERT_EQ(polyline.points[3], Point(50, 80)); - ASSERT_EQ(polyline.points[4], Point(50, 60)); + ASSERT_EQ(polyline.points->size(), 5u); + ASSERT_EQ((*polyline.points)[0], Point(50, 60)); + ASSERT_EQ((*polyline.points)[1], Point(70, 60)); + ASSERT_EQ((*polyline.points)[2], Point(70, 80)); + ASSERT_EQ((*polyline.points)[3], Point(50, 80)); + ASSERT_EQ((*polyline.points)[4], Point(50, 60)); } TEST(GeometryTest, PathPolylineDuplicatesAreRemovedForSameContour) { - std::vector polyline_points; Path::Polyline polyline = PathBuilder{} .MoveTo({50, 50}) @@ -2266,20 +2260,20 @@ TEST(GeometryTest, PathPolylineDuplicatesAreRemovedForSameContour) { .LineTo({0, 100}) .LineTo({0, 100}) // Insert duplicate at end of contour. .TakePath() - .CreatePolyline(1.0f, polyline_points); + .CreatePolyline(1.0f); ASSERT_EQ(polyline.contours.size(), 2u); ASSERT_EQ(polyline.contours[0].start_index, 0u); ASSERT_TRUE(polyline.contours[0].is_closed); ASSERT_EQ(polyline.contours[1].start_index, 4u); ASSERT_FALSE(polyline.contours[1].is_closed); - ASSERT_EQ(polyline.points.size(), 7u); - ASSERT_EQ(polyline.points[0], Point(50, 50)); - ASSERT_EQ(polyline.points[1], Point(100, 50)); - ASSERT_EQ(polyline.points[2], Point(100, 100)); - ASSERT_EQ(polyline.points[3], Point(50, 50)); - ASSERT_EQ(polyline.points[4], Point(50, 50)); - ASSERT_EQ(polyline.points[5], Point(0, 50)); - ASSERT_EQ(polyline.points[6], Point(0, 100)); + ASSERT_EQ(polyline.points->size(), 7u); + ASSERT_EQ((*polyline.points)[0], Point(50, 50)); + ASSERT_EQ((*polyline.points)[1], Point(100, 50)); + ASSERT_EQ((*polyline.points)[2], Point(100, 100)); + ASSERT_EQ((*polyline.points)[3], Point(50, 50)); + ASSERT_EQ((*polyline.points)[4], Point(50, 50)); + ASSERT_EQ((*polyline.points)[5], Point(0, 50)); + ASSERT_EQ((*polyline.points)[6], Point(0, 100)); } TEST(GeometryTest, MatrixPrinting) { diff --git a/impeller/geometry/path.cc b/impeller/geometry/path.cc index 7e45c750104d2..6a939f8406287 100644 --- a/impeller/geometry/path.cc +++ b/impeller/geometry/path.cc @@ -8,6 +8,7 @@ #include #include "impeller/geometry/path_component.h" +#include "impeller/geometry/point.h" namespace impeller { @@ -20,11 +21,11 @@ Path::~Path() = default; std::tuple Path::Polyline::GetContourPointBounds( size_t contour_index) const { if (contour_index >= contours.size()) { - return {points.size(), points.size()}; + return {points->size(), points->size()}; } const size_t start_index = contours.at(contour_index).start_index; const size_t end_index = (contour_index >= contours.size() - 1) - ? points.size() + ? points->size() : contours.at(contour_index + 1).start_index; return std::make_tuple(start_index, end_index); } @@ -269,12 +270,28 @@ bool Path::UpdateContourComponentAtIndex(size_t index, return true; } -Path::Polyline::Polyline(std::vector& point_buffer) - : points(point_buffer) {} +Path::Polyline::Polyline(Path::Polyline::PointBufferPointer point_buffer, + Path::Polyline::ReclaimPointBuffer reclaim) + : points(std::move(point_buffer)), reclaim_points(reclaim) {} -Path::Polyline Path::CreatePolyline(Scalar scale, - std::vector& point_buffer) const { - Polyline polyline(point_buffer); +Path::Polyline::Polyline(Path::Polyline&& other) { + points = std::move(other.points); + reclaim_points = std::move(other.reclaim_points); + contours = std::move(other.contours); +} + +Path::Polyline::~Polyline() { + if (reclaim_points) { + points->clear(); + reclaim_points(std::move(points)); + } +} + +Path::Polyline Path::CreatePolyline( + Scalar scale, + Path::Polyline::PointBufferPointer point_buffer, + Path::Polyline::ReclaimPointBuffer reclaim) const { + Polyline polyline(std::move(point_buffer), reclaim); auto get_path_component = [this](size_t component_i) -> PathComponentVariant { if (component_i >= components_.size()) { @@ -353,26 +370,26 @@ Path::Polyline Path::CreatePolyline(Scalar scale, switch (component.type) { case ComponentType::kLinear: components.push_back({ - .component_start_index = polyline.points.size() - 1, + .component_start_index = polyline.points->size() - 1, .is_curve = false, }); - linears_[component.index].AppendPolylinePoints(polyline.points); + linears_[component.index].AppendPolylinePoints(*polyline.points); previous_path_component_index = component_i; break; case ComponentType::kQuadratic: components.push_back({ - .component_start_index = polyline.points.size() - 1, + .component_start_index = polyline.points->size() - 1, .is_curve = true, }); - quads_[component.index].AppendPolylinePoints(scale, polyline.points); + quads_[component.index].AppendPolylinePoints(scale, *polyline.points); previous_path_component_index = component_i; break; case ComponentType::kCubic: components.push_back({ - .component_start_index = polyline.points.size() - 1, + .component_start_index = polyline.points->size() - 1, .is_curve = true, }); - cubics_[component.index].AppendPolylinePoints(scale, polyline.points); + cubics_[component.index].AppendPolylinePoints(scale, *polyline.points); previous_path_component_index = component_i; break; case ComponentType::kContour: @@ -385,12 +402,12 @@ Path::Polyline Path::CreatePolyline(Scalar scale, Vector2 start_direction = compute_contour_start_direction(component_i); const auto& contour = contours_[component.index]; - polyline.contours.push_back({.start_index = polyline.points.size(), + polyline.contours.push_back({.start_index = polyline.points->size(), .is_closed = contour.is_closed, .start_direction = start_direction, .components = components}); - polyline.points.push_back(contour.destination); + polyline.points->push_back(contour.destination); break; } } diff --git a/impeller/geometry/path.h b/impeller/geometry/path.h index 64e1fc52b6f29..94b39eb357786 100644 --- a/impeller/geometry/path.h +++ b/impeller/geometry/path.h @@ -95,12 +95,21 @@ class Path { /// not allocate their own point vectors to allow for optimizations around /// allocation and reuse of arenas. struct Polyline { - /// The buffer must remain valid for the lifetime of this object. - explicit Polyline(std::vector& point_buffer); + /// The signature of a method called when it is safe to reclaim the point + /// buffer provided to the constructor of this object. + using PointBufferPointer = std::unique_ptr>; + using ReclaimPointBuffer = std::function; + + /// The buffer will be cleared and returned at the destruction of this + /// polyline. + Polyline(PointBufferPointer point_buffer, ReclaimPointBuffer reclaim); + + Polyline(Polyline&& other); + ~Polyline(); /// Points in the polyline, which may represent multiple contours specified - /// by indices in |breaks|. - std::vector& points; + /// by indices in |contours|. + PointBufferPointer points; /// Contours are disconnected pieces of a polyline, such as when a MoveTo /// was issued on a PathBuilder. @@ -112,6 +121,9 @@ class Path { /// The contour_index parameter is clamped to contours.size(). std::tuple GetContourPointBounds( size_t contour_index) const; + + private: + ReclaimPointBuffer reclaim_points; }; Path(); @@ -147,8 +159,12 @@ class Path { /// transformed. /// /// It is suitable to use the max basis length of the matrix used to transform - /// the path. If the provided scale is 0, curves will revert to lines. - Polyline CreatePolyline(Scalar scale, std::vector& point_buffer) const; + /// the path. If the provided scale is 0, curves will revert to straight + /// lines. + Polyline CreatePolyline(Scalar scale, + Polyline::PointBufferPointer point_buffer = + std::make_unique>(), + Polyline::ReclaimPointBuffer reclaim = nullptr) const; std::optional GetBoundingBox() const; diff --git a/impeller/tessellator/BUILD.gn b/impeller/tessellator/BUILD.gn index 25585010851aa..d3915f9996679 100644 --- a/impeller/tessellator/BUILD.gn +++ b/impeller/tessellator/BUILD.gn @@ -12,10 +12,7 @@ impeller_component("tessellator") { public_deps = [ "../geometry" ] - deps = [ - "//flutter/fml", - "//third_party/libtess2", - ] + deps = [ "//third_party/libtess2" ] } impeller_component("tessellator_shared") { @@ -35,7 +32,6 @@ impeller_component("tessellator_shared") { deps = [ "../geometry", - "//flutter/fml", "//third_party/libtess2", ] diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index 478223777964a..ca27c22a359f0 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -4,13 +4,10 @@ #include "impeller/tessellator/tessellator.h" -#include "flutter/fml/thread_local.h" #include "third_party/libtess2/Include/tesselator.h" namespace impeller { -FML_THREAD_LOCAL std::vector tls_point_buffer; - static void* HeapAlloc(void* userData, unsigned int size) { return malloc(size); } @@ -34,8 +31,10 @@ static const TESSalloc kAlloc = { 0 /* =extraVertices */ }; -Tessellator::Tessellator() : c_tessellator_(nullptr, &DestroyTessellator) { - tls_point_buffer.reserve(2048); +Tessellator::Tessellator() + : point_buffer_(std::make_unique>()), + c_tessellator_(nullptr, &DestroyTessellator) { + point_buffer_->reserve(2048); TESSalloc alloc = kAlloc; { // libTess2 copies the TESSalloc despite the non-const argument. @@ -63,17 +62,22 @@ static int ToTessWindingRule(FillType fill_type) { } Tessellator::Result Tessellator::Tessellate(const Path& path, - Scalar scale, + Scalar tolerance, const BuilderCallback& callback) { if (!callback) { return Result::kInputError; } - tls_point_buffer.clear(); - auto polyline = path.CreatePolyline(scale, tls_point_buffer); + point_buffer_->clear(); + auto polyline = + path.CreatePolyline(tolerance, std::move(point_buffer_), + [&](Path::Polyline::PointBufferPointer point_buffer) { + point_buffer_ = std::move(point_buffer); + }); + auto fill_type = path.GetFillType(); - if (polyline.points.empty()) { + if (polyline.points->empty()) { return Result::kInputError; } @@ -106,9 +110,9 @@ Tessellator::Result Tessellator::Tessellate(const Path& path, ::tessAddContour(tessellator, // the C tessellator kVertexSize, // - polyline.points.data() + start_point_index, // - sizeof(Point), // - end_point_index - start_point_index // + polyline.points->data() + start_point_index, // + sizeof(Point), // + end_point_index - start_point_index // ); //---------------------------------------------------------------------------- @@ -157,9 +161,9 @@ Tessellator::Result Tessellator::Tessellate(const Path& path, ::tessAddContour(tessellator, // the C tessellator kVertexSize, // - polyline.points.data() + start_point_index, // - sizeof(Point), // - end_point_index - start_point_index // + polyline.points->data() + start_point_index, // + sizeof(Point), // + end_point_index - start_point_index // ); } @@ -228,28 +232,32 @@ Tessellator::Result Tessellator::Tessellate(const Path& path, } std::pair, std::vector> -Tessellator::TessellateConvex(const Path& path, Scalar scale) { +Tessellator::TessellateConvex(const Path& path, Scalar tolerance) { std::vector output; std::vector indices; - tls_point_buffer.clear(); - auto polyline = path.CreatePolyline(scale, tls_point_buffer); + point_buffer_->clear(); + auto polyline = + path.CreatePolyline(tolerance, std::move(point_buffer_), + [&](Path::Polyline::PointBufferPointer point_buffer) { + point_buffer_ = std::move(point_buffer); + }); for (auto j = 0u; j < polyline.contours.size(); j++) { auto [start, end] = polyline.GetContourPointBounds(j); - auto center = polyline.points[start]; + auto center = (*polyline.points)[start]; // Some polygons will not self close and an additional triangle // must be inserted, others will self close and we need to avoid // inserting an extra triangle. - if (polyline.points[end - 1] == polyline.points[start]) { + if ((*polyline.points)[end - 1] == (*polyline.points)[start]) { end--; } output.emplace_back(center); - output.emplace_back(polyline.points[start + 1]); + output.emplace_back((*polyline.points)[start + 1]); for (auto i = start + 2; i < end; i++) { - const auto& point_b = polyline.points[i]; + const auto& point_b = (*polyline.points)[i]; output.emplace_back(point_b); indices.emplace_back(0); diff --git a/impeller/tessellator/tessellator.h b/impeller/tessellator/tessellator.h index b6b62756dcbc3..7733c25a5aec0 100644 --- a/impeller/tessellator/tessellator.h +++ b/impeller/tessellator/tessellator.h @@ -30,8 +30,8 @@ enum class WindingOrder { /// @brief A utility that generates triangles of the specified fill type /// given a polyline. This happens on the CPU. /// -/// This object is implemented in such a way that there must be -/// only one per thread. +/// This object is not thread safe, and its methods must not be +/// called from multiple threads. /// class Tessellator { public: @@ -62,22 +62,38 @@ class Tessellator { /// @brief Generates filled triangles from the path. A callback is /// invoked once for the entire tessellation. /// - /// @param[in] The path to tessellate. + /// @param[in] path The path to tessellate. + /// @param[in] tolerance The tolerance value for conversion of the path to + /// a polyline. This value is often derived from the + /// Matrix::GetMaxBasisLength of the CTM applied to the + /// path for rendering. /// @param[in] callback The callback, return false to indicate failure. /// /// @return The result status of the tessellation. /// Tessellator::Result Tessellate(const Path& path, - Scalar scale, + Scalar tolerance, const BuilderCallback& callback); //---------------------------------------------------------------------------- /// @brief Given a convex path, create a triangle fan structure. + /// + /// @param[in] path The path to tessellate. + /// @param[in] tolerance The tolerance value for conversion of the path to + /// a polyline. This value is often derived from the + /// Matrix::GetMaxBasisLength of the CTM applied to the + /// path for rendering. + /// + /// @return A point vector containing the vertices and a vector of indices + /// into the point vector. + /// std::pair, std::vector> TessellateConvex( const Path& path, - Scalar scale); + Scalar tolerance); private: + /// Used for polyline generation. + std::unique_ptr> point_buffer_; CTessellator c_tessellator_; Tessellator(const Tessellator&) = delete; From 761be290c8e522921a91d7f0d1db83de63e226e9 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 9 Nov 2023 14:18:49 -0800 Subject: [PATCH 07/11] more review --- .../entity/geometry/stroke_path_geometry.cc | 37 +++++++++---------- impeller/geometry/path.h | 2 + impeller/tessellator/tessellator.cc | 28 +++++++------- 3 files changed, 34 insertions(+), 33 deletions(-) diff --git a/impeller/entity/geometry/stroke_path_geometry.cc b/impeller/entity/geometry/stroke_path_geometry.cc index c4a6a115ac965..48eebb6a9286a 100644 --- a/impeller/entity/geometry/stroke_path_geometry.cc +++ b/impeller/entity/geometry/stroke_path_geometry.cc @@ -263,9 +263,8 @@ StrokePathGeometry::CreateSolidStrokeVertices( } else if (point_i <= contour_start_point_i) { direction = -contour.start_direction; } else { - direction = - ((*polyline.points)[point_i] - (*polyline.points)[point_i - 1]) - .Normalize(); + direction = (polyline.GetPoint(point_i) - polyline.GetPoint(point_i - 1)) + .Normalize(); } previous_offset = offset; offset = Vector2{-direction.y, direction.x} * stroke_width * 0.5; @@ -284,23 +283,23 @@ StrokePathGeometry::CreateSolidStrokeVertices( for (size_t point_i = component_start_index; point_i < component_end_index; point_i++) { auto is_end_of_component = point_i == component_end_index - 1; - vtx.position = (*polyline.points)[point_i] + offset; + vtx.position = polyline.GetPoint(point_i) + offset; vtx_builder.AppendVertex(vtx); - vtx.position = (*polyline.points)[point_i] - offset; + vtx.position = polyline.GetPoint(point_i) - offset; vtx_builder.AppendVertex(vtx); // 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 + 1] + offset; + vtx.position = polyline.GetPoint(point_i + 1) + offset; vtx_builder.AppendVertex(vtx); - vtx.position = (*polyline.points)[point_i + 1] - offset; + vtx.position = polyline.GetPoint(point_i + 1) - offset; vtx_builder.AppendVertex(vtx); compute_offset(point_i + 2, contour_start_point_i, contour_end_point_i, contour); if (!is_last_component && is_end_of_component) { // Generate join from the current line to the next line. - join_proc(vtx_builder, (*polyline.points)[point_i + 1], + join_proc(vtx_builder, polyline.GetPoint(point_i + 1), previous_offset, offset, scaled_miter_limit, scale); } } @@ -320,9 +319,9 @@ StrokePathGeometry::CreateSolidStrokeVertices( point_i < component_end_index; point_i++) { auto is_end_of_component = point_i == component_end_index - 1; - vtx.position = (*polyline.points)[point_i] + offset; + vtx.position = polyline.GetPoint(point_i) + offset; vtx_builder.AppendVertex(vtx); - vtx.position = (*polyline.points)[point_i] - offset; + vtx.position = polyline.GetPoint(point_i) - offset; vtx_builder.AppendVertex(vtx); compute_offset(point_i + 2, contour_start_point_i, @@ -330,13 +329,13 @@ StrokePathGeometry::CreateSolidStrokeVertices( // For curve components, the polyline is detailed enough such that // it can avoid worrying about joins altogether. if (is_end_of_component) { - vtx.position = (*polyline.points)[point_i + 1] + offset; + vtx.position = polyline.GetPoint(point_i + 1) + offset; vtx_builder.AppendVertex(vtx); - vtx.position = (*polyline.points)[point_i + 1] - offset; + vtx.position = polyline.GetPoint(point_i + 1) - offset; vtx_builder.AppendVertex(vtx); // Generate join from the current line to the next line. if (!is_last_component) { - join_proc(vtx_builder, (*polyline.points)[point_i + 1], + join_proc(vtx_builder, polyline.GetPoint(point_i + 1), previous_offset, offset, scaled_miter_limit, scale); } } @@ -352,7 +351,7 @@ StrokePathGeometry::CreateSolidStrokeVertices( switch (contour_end_point_i - contour_start_point_i) { case 1: { - Point p = (*polyline.points)[contour_start_point_i]; + Point p = polyline.GetPoint(contour_start_point_i); cap_proc(vtx_builder, p, {-stroke_width * 0.5f, 0}, scale, false); cap_proc(vtx_builder, p, {stroke_width * 0.5f, 0}, scale, false); continue; @@ -375,14 +374,14 @@ StrokePathGeometry::CreateSolidStrokeVertices( // vertices at the start of the new contour (thus connecting the two // contours with two zero volume triangles, which will be discarded by // the rasterizer). - vtx.position = (*polyline.points)[contour_start_point_i - 1]; + vtx.position = polyline.GetPoint(contour_start_point_i - 1); // Append two vertices when "picking up" the pen so that the triangle // drawn when moving to the beginning of the new contour will have zero // volume. vtx_builder.AppendVertex(vtx); vtx_builder.AppendVertex(vtx); - vtx.position = (*polyline.points)[contour_start_point_i]; + vtx.position = polyline.GetPoint(contour_start_point_i); // Append two vertices at the beginning of the new contour, which // appends two triangles of zero area. vtx_builder.AppendVertex(vtx); @@ -394,7 +393,7 @@ StrokePathGeometry::CreateSolidStrokeVertices( auto cap_offset = Vector2(-contour.start_direction.y, contour.start_direction.x) * stroke_width * 0.5; // Counterclockwise normal - cap_proc(vtx_builder, (*polyline.points)[contour_start_point_i], + cap_proc(vtx_builder, polyline.GetPoint(contour_start_point_i), cap_offset, scale, true); } @@ -426,10 +425,10 @@ StrokePathGeometry::CreateSolidStrokeVertices( auto cap_offset = Vector2(-contour.end_direction.y, contour.end_direction.x) * stroke_width * 0.5; // Clockwise normal - cap_proc(vtx_builder, (*polyline.points)[contour_end_point_i - 1], + cap_proc(vtx_builder, polyline.GetPoint(contour_end_point_i - 1), cap_offset, scale, false); } else { - join_proc(vtx_builder, (*polyline.points)[contour_start_point_i], offset, + join_proc(vtx_builder, polyline.GetPoint(contour_start_point_i), offset, contour_first_offset, scaled_miter_limit, scale); } } diff --git a/impeller/geometry/path.h b/impeller/geometry/path.h index 94b39eb357786..cedf3b44516f9 100644 --- a/impeller/geometry/path.h +++ b/impeller/geometry/path.h @@ -111,6 +111,8 @@ class Path { /// by indices in |contours|. PointBufferPointer points; + Point& GetPoint(size_t index) const { return (*points)[index]; } + /// Contours are disconnected pieces of a polyline, such as when a MoveTo /// was issued on a PathBuilder. std::vector contours; diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index ca27c22a359f0..78b824095c154 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -69,11 +69,11 @@ Tessellator::Result Tessellator::Tessellate(const Path& path, } point_buffer_->clear(); - auto polyline = - path.CreatePolyline(tolerance, std::move(point_buffer_), - [&](Path::Polyline::PointBufferPointer point_buffer) { - point_buffer_ = std::move(point_buffer); - }); + auto polyline = path.CreatePolyline( + tolerance, std::move(point_buffer_), + [this](Path::Polyline::PointBufferPointer point_buffer) { + point_buffer_ = std::move(point_buffer); + }); auto fill_type = path.GetFillType(); @@ -237,27 +237,27 @@ Tessellator::TessellateConvex(const Path& path, Scalar tolerance) { std::vector indices; point_buffer_->clear(); - auto polyline = - path.CreatePolyline(tolerance, std::move(point_buffer_), - [&](Path::Polyline::PointBufferPointer point_buffer) { - point_buffer_ = std::move(point_buffer); - }); + auto polyline = path.CreatePolyline( + tolerance, std::move(point_buffer_), + [this](Path::Polyline::PointBufferPointer point_buffer) { + point_buffer_ = std::move(point_buffer); + }); for (auto j = 0u; j < polyline.contours.size(); j++) { auto [start, end] = polyline.GetContourPointBounds(j); - auto center = (*polyline.points)[start]; + auto center = polyline.GetPoint(start); // Some polygons will not self close and an additional triangle // must be inserted, others will self close and we need to avoid // inserting an extra triangle. - if ((*polyline.points)[end - 1] == (*polyline.points)[start]) { + if (polyline.GetPoint(end - 1) == polyline.GetPoint(start)) { end--; } output.emplace_back(center); - output.emplace_back((*polyline.points)[start + 1]); + output.emplace_back(polyline.GetPoint(start + 1)); for (auto i = start + 2; i < end; i++) { - const auto& point_b = (*polyline.points)[i]; + const auto& point_b = polyline.GetPoint(i); output.emplace_back(point_b); indices.emplace_back(0); From 720a0eef1442aa679773f8f7c7c6d91e3c3adae1 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 9 Nov 2023 14:24:16 -0800 Subject: [PATCH 08/11] names --- impeller/geometry/geometry_benchmarks.cc | 2 +- impeller/geometry/path.cc | 8 ++++---- impeller/geometry/path.h | 19 ++++++++++--------- impeller/tessellator/tessellator.cc | 20 ++++++++++---------- 4 files changed, 25 insertions(+), 24 deletions(-) diff --git a/impeller/geometry/geometry_benchmarks.cc b/impeller/geometry/geometry_benchmarks.cc index 47e056059ba12..56ad1cff65506 100644 --- a/impeller/geometry/geometry_benchmarks.cc +++ b/impeller/geometry/geometry_benchmarks.cc @@ -48,7 +48,7 @@ static void BM_Polyline(benchmark::State& state, Args&&... args) { } else { auto polyline = path.CreatePolyline( 1.0f, std::move(points), - [&points](Path::Polyline::PointBufferPointer reclaimed) { + [&points](Path::Polyline::PointBufferPtr reclaimed) { points = std::move(reclaimed); }); single_point_count = polyline.points->size(); diff --git a/impeller/geometry/path.cc b/impeller/geometry/path.cc index 165a138dda46d..d00e64811ed1b 100644 --- a/impeller/geometry/path.cc +++ b/impeller/geometry/path.cc @@ -213,8 +213,8 @@ bool Path::GetContourComponentAtIndex(size_t index, return true; } -Path::Polyline::Polyline(Path::Polyline::PointBufferPointer point_buffer, - Path::Polyline::ReclaimPointBuffer reclaim) +Path::Polyline::Polyline(Path::Polyline::PointBufferPtr point_buffer, + Path::Polyline::ReclaimPointBufferCallback reclaim) : points(std::move(point_buffer)), reclaim_points(reclaim) {} Path::Polyline::Polyline(Path::Polyline&& other) { @@ -232,8 +232,8 @@ Path::Polyline::~Polyline() { Path::Polyline Path::CreatePolyline( Scalar scale, - Path::Polyline::PointBufferPointer point_buffer, - Path::Polyline::ReclaimPointBuffer reclaim) const { + Path::Polyline::PointBufferPtr point_buffer, + Path::Polyline::ReclaimPointBufferCallback reclaim) const { Polyline polyline(std::move(point_buffer), reclaim); auto get_path_component = [this](size_t component_i) -> PathComponentVariant { diff --git a/impeller/geometry/path.h b/impeller/geometry/path.h index 1c9a6f736528c..0733ad833cac9 100644 --- a/impeller/geometry/path.h +++ b/impeller/geometry/path.h @@ -97,19 +97,19 @@ class Path { struct Polyline { /// The signature of a method called when it is safe to reclaim the point /// buffer provided to the constructor of this object. - using PointBufferPointer = std::unique_ptr>; - using ReclaimPointBuffer = std::function; + using PointBufferPtr = std::unique_ptr>; + using ReclaimPointBufferCallback = std::function; /// The buffer will be cleared and returned at the destruction of this /// polyline. - Polyline(PointBufferPointer point_buffer, ReclaimPointBuffer reclaim); + Polyline(PointBufferPtr point_buffer, ReclaimPointBufferCallback reclaim); Polyline(Polyline&& other); ~Polyline(); /// Points in the polyline, which may represent multiple contours specified /// by indices in |contours|. - PointBufferPointer points; + PointBufferPtr points; Point& GetPoint(size_t index) const { return (*points)[index]; } @@ -125,7 +125,7 @@ class Path { size_t contour_index) const; private: - ReclaimPointBuffer reclaim_points; + ReclaimPointBufferCallback reclaim_points; }; Path(); @@ -163,10 +163,11 @@ class Path { /// It is suitable to use the max basis length of the matrix used to transform /// the path. If the provided scale is 0, curves will revert to straight /// lines. - Polyline CreatePolyline(Scalar scale, - Polyline::PointBufferPointer point_buffer = - std::make_unique>(), - Polyline::ReclaimPointBuffer reclaim = nullptr) const; + Polyline CreatePolyline( + Scalar scale, + Polyline::PointBufferPtr point_buffer = + std::make_unique>(), + Polyline::ReclaimPointBufferCallback reclaim = nullptr) const; std::optional GetBoundingBox() const; diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index 78b824095c154..15069a6d2cbf2 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -69,11 +69,11 @@ Tessellator::Result Tessellator::Tessellate(const Path& path, } point_buffer_->clear(); - auto polyline = path.CreatePolyline( - tolerance, std::move(point_buffer_), - [this](Path::Polyline::PointBufferPointer point_buffer) { - point_buffer_ = std::move(point_buffer); - }); + auto polyline = + path.CreatePolyline(tolerance, std::move(point_buffer_), + [this](Path::Polyline::PointBufferPtr point_buffer) { + point_buffer_ = std::move(point_buffer); + }); auto fill_type = path.GetFillType(); @@ -237,11 +237,11 @@ Tessellator::TessellateConvex(const Path& path, Scalar tolerance) { std::vector indices; point_buffer_->clear(); - auto polyline = path.CreatePolyline( - tolerance, std::move(point_buffer_), - [this](Path::Polyline::PointBufferPointer point_buffer) { - point_buffer_ = std::move(point_buffer); - }); + auto polyline = + path.CreatePolyline(tolerance, std::move(point_buffer_), + [this](Path::Polyline::PointBufferPtr point_buffer) { + point_buffer_ = std::move(point_buffer); + }); for (auto j = 0u; j < polyline.contours.size(); j++) { auto [start, end] = polyline.GetContourPointBounds(j); From 615f8fd5df69c6de2bb0d7f83894f7ace52683c3 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 9 Nov 2023 14:35:43 -0800 Subject: [PATCH 09/11] tests --- impeller/geometry/geometry_unittests.cc | 61 ++++++++++++++++++------- impeller/geometry/path.cc | 5 +- 2 files changed, 48 insertions(+), 18 deletions(-) diff --git a/impeller/geometry/geometry_unittests.cc b/impeller/geometry/geometry_unittests.cc index cc5a97b927b86..4c5ea75435e95 100644 --- a/impeller/geometry/geometry_unittests.cc +++ b/impeller/geometry/geometry_unittests.cc @@ -2119,11 +2119,11 @@ TEST(GeometryTest, PathCreatePolyLineDoesNotDuplicatePoints) { ASSERT_EQ(polyline.contours.size(), 2u); ASSERT_EQ(polyline.points->size(), 5u); - ASSERT_EQ((*polyline.points)[0].x, 10); - ASSERT_EQ((*polyline.points)[1].x, 20); - ASSERT_EQ((*polyline.points)[2].x, 30); - ASSERT_EQ((*polyline.points)[3].x, 40); - ASSERT_EQ((*polyline.points)[4].x, 50); + ASSERT_EQ(polyline.GetPoint(0).x, 10); + ASSERT_EQ(polyline.GetPoint(1).x, 20); + ASSERT_EQ(polyline.GetPoint(2).x, 30); + ASSERT_EQ(polyline.GetPoint(3).x, 40); + ASSERT_EQ(polyline.GetPoint(4).x, 50); } TEST(GeometryTest, PathBuilderSetsCorrectContourPropertiesForAddCommands) { @@ -2240,11 +2240,11 @@ TEST(GeometryTest, PathAddRectPolylineHasCorrectContourData) { ASSERT_TRUE(polyline.contours[0].is_closed); ASSERT_EQ(polyline.contours[0].start_index, 0u); ASSERT_EQ(polyline.points->size(), 5u); - ASSERT_EQ((*polyline.points)[0], Point(50, 60)); - ASSERT_EQ((*polyline.points)[1], Point(70, 60)); - ASSERT_EQ((*polyline.points)[2], Point(70, 80)); - ASSERT_EQ((*polyline.points)[3], Point(50, 80)); - ASSERT_EQ((*polyline.points)[4], Point(50, 60)); + ASSERT_EQ(polyline.GetPoint(0), Point(50, 60)); + ASSERT_EQ(polyline.GetPoint(1), Point(70, 60)); + ASSERT_EQ(polyline.GetPoint(2), Point(70, 80)); + ASSERT_EQ(polyline.GetPoint(3), Point(50, 80)); + ASSERT_EQ(polyline.GetPoint(4), Point(50, 60)); } TEST(GeometryTest, PathPolylineDuplicatesAreRemovedForSameContour) { @@ -2267,13 +2267,40 @@ TEST(GeometryTest, PathPolylineDuplicatesAreRemovedForSameContour) { ASSERT_EQ(polyline.contours[1].start_index, 4u); ASSERT_FALSE(polyline.contours[1].is_closed); ASSERT_EQ(polyline.points->size(), 7u); - ASSERT_EQ((*polyline.points)[0], Point(50, 50)); - ASSERT_EQ((*polyline.points)[1], Point(100, 50)); - ASSERT_EQ((*polyline.points)[2], Point(100, 100)); - ASSERT_EQ((*polyline.points)[3], Point(50, 50)); - ASSERT_EQ((*polyline.points)[4], Point(50, 50)); - ASSERT_EQ((*polyline.points)[5], Point(0, 50)); - ASSERT_EQ((*polyline.points)[6], Point(0, 100)); + ASSERT_EQ(polyline.GetPoint(0), Point(50, 50)); + ASSERT_EQ(polyline.GetPoint(1), Point(100, 50)); + ASSERT_EQ(polyline.GetPoint(2), Point(100, 100)); + ASSERT_EQ(polyline.GetPoint(3), Point(50, 50)); + ASSERT_EQ(polyline.GetPoint(4), Point(50, 50)); + ASSERT_EQ(polyline.GetPoint(5), Point(0, 50)); + ASSERT_EQ(polyline.GetPoint(6), Point(0, 100)); +} + +TEST(GeometryTest, PolylineBufferReuse) { + auto point_buffer = std::make_unique>(); + auto point_buffer_address = reinterpret_cast(point_buffer.get()); + Path::Polyline polyline = + PathBuilder{} + .MoveTo({50, 50}) + .LineTo({100, 100}) + .TakePath() + .CreatePolyline( + 1.0f, std::move(point_buffer), + [point_buffer_address]( + Path::Polyline::PointBufferPtr point_buffer) { + ASSERT_EQ(point_buffer->size(), 0u); + ASSERT_EQ(point_buffer_address, + reinterpret_cast(point_buffer.get())); + }); +} + +TEST(GeometryTest, PolylineFailsWithNullptrBuffer) { + EXPECT_DEATH_IF_SUPPORTED(PathBuilder{} + .MoveTo({50, 50}) + .LineTo({100, 100}) + .TakePath() + .CreatePolyline(1.0f, nullptr), + ""); } TEST(GeometryTest, MatrixPrinting) { diff --git a/impeller/geometry/path.cc b/impeller/geometry/path.cc index d00e64811ed1b..c08aa0c97cfd5 100644 --- a/impeller/geometry/path.cc +++ b/impeller/geometry/path.cc @@ -7,6 +7,7 @@ #include #include +#include "flutter/fml/logging.h" #include "impeller/geometry/path_component.h" #include "impeller/geometry/point.h" @@ -215,7 +216,9 @@ bool Path::GetContourComponentAtIndex(size_t index, Path::Polyline::Polyline(Path::Polyline::PointBufferPtr point_buffer, Path::Polyline::ReclaimPointBufferCallback reclaim) - : points(std::move(point_buffer)), reclaim_points(reclaim) {} + : points(std::move(point_buffer)), reclaim_points(reclaim) { + FML_DCHECK(points); +} Path::Polyline::Polyline(Path::Polyline&& other) { points = std::move(other.points); From 71afaa0819e27461d7225429943646c9dfe107e3 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 9 Nov 2023 15:14:12 -0800 Subject: [PATCH 10/11] lint --- impeller/geometry/geometry_benchmarks.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/impeller/geometry/geometry_benchmarks.cc b/impeller/geometry/geometry_benchmarks.cc index 56ad1cff65506..ca97d7b18177f 100644 --- a/impeller/geometry/geometry_benchmarks.cc +++ b/impeller/geometry/geometry_benchmarks.cc @@ -47,6 +47,9 @@ static void BM_Polyline(benchmark::State& state, Args&&... args) { }); } else { auto polyline = path.CreatePolyline( + // Clang-tidy doesn't know that the points get moved back before + // getting moved again in this loop. + // NOLINTNEXTLINE(clang-analyzer-cplusplus.Move) 1.0f, std::move(points), [&points](Path::Polyline::PointBufferPtr reclaimed) { points = std::move(reclaimed); From da16afee35f817abcdff4744203e7eaab0459bf8 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 9 Nov 2023 15:51:35 -0800 Subject: [PATCH 11/11] lint --- impeller/geometry/path.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/impeller/geometry/path.cc b/impeller/geometry/path.cc index c08aa0c97cfd5..5f99a5cfeb6f9 100644 --- a/impeller/geometry/path.cc +++ b/impeller/geometry/path.cc @@ -216,7 +216,7 @@ bool Path::GetContourComponentAtIndex(size_t index, Path::Polyline::Polyline(Path::Polyline::PointBufferPtr point_buffer, Path::Polyline::ReclaimPointBufferCallback reclaim) - : points(std::move(point_buffer)), reclaim_points(reclaim) { + : points(std::move(point_buffer)), reclaim_points(std::move(reclaim)) { FML_DCHECK(points); } @@ -237,7 +237,7 @@ Path::Polyline Path::CreatePolyline( Scalar scale, Path::Polyline::PointBufferPtr point_buffer, Path::Polyline::ReclaimPointBufferCallback reclaim) const { - Polyline polyline(std::move(point_buffer), reclaim); + Polyline polyline(std::move(point_buffer), std::move(reclaim)); auto get_path_component = [this](size_t component_i) -> PathComponentVariant { if (component_i >= components_.size()) {