From 031fd40248097c920fcd52c43e1eddc47792c836 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 15 Apr 2024 12:58:15 -0700 Subject: [PATCH 01/18] [Impeller] remove most temporary allocation during polyline generation. --- .../entity/geometry/fill_path_geometry.cc | 30 +---- impeller/geometry/geometry_benchmarks.cc | 8 +- impeller/geometry/path.cc | 41 +++++++ impeller/geometry/path.h | 8 ++ impeller/geometry/path_component.cc | 79 ++++++++++++++ impeller/geometry/path_component.h | 28 ++++- impeller/tessellator/tessellator.cc | 103 ++++++++---------- impeller/tessellator/tessellator.h | 25 ++++- impeller/tessellator/tessellator_unittests.cc | 28 +++-- .../plugin/platform/PlatformViewWrapper.java | 2 + 10 files changed, 256 insertions(+), 96 deletions(-) diff --git a/impeller/entity/geometry/fill_path_geometry.cc b/impeller/entity/geometry/fill_path_geometry.cc index 1b3dc51813266..89088c86e8e3f 100644 --- a/impeller/entity/geometry/fill_path_geometry.cc +++ b/impeller/entity/geometry/fill_path_geometry.cc @@ -36,15 +36,8 @@ GeometryResult FillPathGeometry::GetPositionBuffer( }; } - VertexBuffer vertex_buffer; - - auto points = renderer.GetTessellator()->TessellateConvex( - path_, entity.GetTransform().GetMaxBasisLength()); - - vertex_buffer.vertex_buffer = host_buffer.Emplace( - points.data(), points.size() * sizeof(Point), alignof(Point)); - vertex_buffer.index_buffer = {}, vertex_buffer.vertex_count = points.size(); - vertex_buffer.index_type = IndexType::kNone; + VertexBuffer vertex_buffer = renderer.GetTessellator()->TessellateConvex( + path_, host_buffer, entity.GetTransform().GetMaxBasisLength()); return GeometryResult{ .type = PrimitiveType::kTriangleStrip, @@ -61,8 +54,6 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer( const ContentContext& renderer, const Entity& entity, RenderPass& pass) const { - using VS = TextureFillVertexShader; - const auto& bounding_box = path_.GetBoundingBox(); if (bounding_box.has_value() && bounding_box->IsEmpty()) { return GeometryResult{ @@ -80,22 +71,13 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer( auto uv_transform = texture_coverage.GetNormalizingTransform() * effect_transform; - auto points = renderer.GetTessellator()->TessellateConvex( - path_, entity.GetTransform().GetMaxBasisLength()); - - VertexBufferBuilder vertex_builder; - vertex_builder.Reserve(points.size()); - for (auto i = 0u; i < points.size(); i++) { - VS::PerVertexData data; - data.position = points[i]; - data.texture_coords = uv_transform * points[i]; - vertex_builder.AppendVertex(data); - } + VertexBuffer vertex_buffer = renderer.GetTessellator()->TessellateConvex( + path_, renderer.GetTransientsBuffer(), + entity.GetTransform().GetMaxBasisLength(), uv_transform); return GeometryResult{ .type = PrimitiveType::kTriangleStrip, - .vertex_buffer = - vertex_builder.CreateVertexBuffer(renderer.GetTransientsBuffer()), + .vertex_buffer = vertex_buffer, .transform = entity.GetShaderTransform(pass), .mode = GetResultMode(), }; diff --git a/impeller/geometry/geometry_benchmarks.cc b/impeller/geometry/geometry_benchmarks.cc index 98d17cf7d8c15..a384af9b3319b 100644 --- a/impeller/geometry/geometry_benchmarks.cc +++ b/impeller/geometry/geometry_benchmarks.cc @@ -155,11 +155,13 @@ static void BM_Convex(benchmark::State& state, Args&&... args) { size_t point_count = 0u; size_t single_point_count = 0u; auto points = std::make_unique>(); + auto indices = std::make_unique>(); points->reserve(2048); + indices->reserve(2048); while (state.KeepRunning()) { - auto points = tess.TessellateConvex(path, 1.0f); - single_point_count = points.size(); - point_count += points.size(); + tess.TessellateConvexInternal(path, *points, *indices, 1.0f); + single_point_count = indices->size(); + point_count += indices->size(); } state.counters["SinglePointCount"] = single_point_count; state.counters["TotalPointCount"] = point_count; diff --git a/impeller/geometry/path.cc b/impeller/geometry/path.cc index 2eb682f5ff722..73daedaab3b97 100644 --- a/impeller/geometry/path.cc +++ b/impeller/geometry/path.cc @@ -349,4 +349,45 @@ std::optional Path::GetTransformedBoundingBox( return bounds->TransformBounds(transform); } +void Path::WritePolyline(Scalar scale, VertexWriter& writer) const { + auto& path_components = data_->components; + auto& path_points = data_->points; + + for (size_t component_i = 0; component_i < path_components.size(); + component_i++) { + const auto& path_component = path_components[component_i]; + switch (path_component.type) { + case ComponentType::kLinear: { + const LinearPathComponent* linear = + reinterpret_cast( + &path_points[path_component.index]); + writer.Write(linear->p2); + break; + } + case ComponentType::kQuadratic: { + const QuadraticPathComponent* quad = + reinterpret_cast( + &path_points[path_component.index]); + quad->ToLinearPathComponents(scale, writer); + break; + } + case ComponentType::kCubic: { + const CubicPathComponent* cubic = + reinterpret_cast( + &path_points[path_component.index]); + cubic->ToLinearPathComponents(scale, writer); + break; + } + case ComponentType::kContour: + if (component_i == path_components.size() - 1) { + // If the last component is a contour, that means it's an empty + // contour, so skip it. + continue; + } + writer.EndContour(); + break; + } + } +} + } // namespace impeller diff --git a/impeller/geometry/path.h b/impeller/geometry/path.h index ba56a4101c1bd..3ba9ff98d1cd8 100644 --- a/impeller/geometry/path.h +++ b/impeller/geometry/path.h @@ -11,6 +11,7 @@ #include #include "impeller/geometry/path_component.h" +#include "impeller/geometry/rect.h" namespace impeller { @@ -168,6 +169,13 @@ class Path { std::make_unique>(), Polyline::ReclaimPointBufferCallback reclaim = nullptr) const; + /// Generate a polyline into the temporary storage held by the [writer]. + /// + /// 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. + void WritePolyline(Scalar scale, VertexWriter& writer) const; + std::optional GetBoundingBox() const; std::optional GetTransformedBoundingBox(const Matrix& transform) const; diff --git a/impeller/geometry/path_component.cc b/impeller/geometry/path_component.cc index 3a00414dfa804..7e1b558a2e49f 100644 --- a/impeller/geometry/path_component.cc +++ b/impeller/geometry/path_component.cc @@ -10,6 +10,66 @@ namespace impeller { +VertexWriter::VertexWriter(std::vector& points, + std::vector& indices, + std::optional uv_transform) + : points_(points), indices_(indices), uv_transform_(uv_transform) {} + +void VertexWriter::EndContour() { + if (points_.size() == 0u || contour_start_ == points_.size() - 1) { + // Empty or first contour. + return; + } + + auto start = contour_start_; + auto end = points_.size() - 1; + // 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 (points_[end] == points_[start]) { + end--; + } + + if (contour_start_ > 0) { + // Triangle strip break. + indices_.emplace_back(indices_.back()); + indices_.emplace_back(start); + indices_.emplace_back(start); + + // If the contour has an odd number of points, insert an extra point when + // bridging to the next contour to preserve the correct triangle winding + // order. + if (previous_contour_odd_points_) { + indices_.emplace_back(start); + } + } else { + indices_.emplace_back(start); + } + + size_t a = start + 1; + size_t b = end; + while (a < b) { + indices_.emplace_back(a); + indices_.emplace_back(b); + a++; + b--; + } + if (a == b) { + indices_.emplace_back(a); + previous_contour_odd_points_ = false; + } else { + previous_contour_odd_points_ = true; + } + contour_start_ = points_.size(); +} + +void VertexWriter::Write(Point point) { + points_.emplace_back(point); + if (uv_transform_.has_value()) { + points_.emplace_back(*uv_transform_ * point); + } +} + /* * Based on: https://en.wikipedia.org/wiki/B%C3%A9zier_curve#Specific_cases */ @@ -119,6 +179,16 @@ void QuadraticPathComponent::ToLinearPathComponents( proc(p2); } +void QuadraticPathComponent::ToLinearPathComponents( + Scalar scale, + VertexWriter& writer) const { + Scalar line_count = std::ceilf(ComputeQuadradicSubdivisions(scale, *this)); + for (size_t i = 1; i < line_count; i += 1) { + writer.Write(Solve(i / line_count)); + } + writer.Write(p2); +} + std::vector QuadraticPathComponent::Extrema() const { CubicPathComponent elevated(*this); return elevated.Extrema(); @@ -189,6 +259,15 @@ void CubicPathComponent::ToLinearPathComponents(Scalar scale, proc(p2); } +void CubicPathComponent::ToLinearPathComponents(Scalar scale, + VertexWriter& writer) const { + Scalar line_count = std::ceilf(ComputeCubicSubdivisions(scale, *this)); + for (size_t i = 1; i < line_count; i++) { + writer.Write(Solve(i / line_count)); + } + writer.Write(p2); +} + static inline bool NearEqual(Scalar a, Scalar b, Scalar epsilon) { return (a > (b - epsilon)) && (a < (b + epsilon)); } diff --git a/impeller/geometry/path_component.h b/impeller/geometry/path_component.h index c310052a893dc..5e5e2106b7682 100644 --- a/impeller/geometry/path_component.h +++ b/impeller/geometry/path_component.h @@ -10,12 +10,34 @@ #include #include +#include "impeller/geometry/matrix.h" #include "impeller/geometry/point.h" -#include "impeller/geometry/rect.h" #include "impeller/geometry/scalar.h" namespace impeller { +/// @brief An interface for generating a multi contour polyline as a triangle +/// strip. +class VertexWriter { + public: + explicit VertexWriter(std::vector& points, + std::vector& indices, + std::optional uv_transform); + + ~VertexWriter() = default; + + void EndContour(); + + void Write(Point point); + + private: + bool previous_contour_odd_points_ = false; + size_t contour_start_ = 0u; + std::vector& points_; + std::vector& indices_; + std::optional uv_transform_; +}; + struct LinearPathComponent { Point p1; Point p2; @@ -64,6 +86,8 @@ struct QuadraticPathComponent { void ToLinearPathComponents(Scalar scale_factor, const PointProc& proc) const; + void ToLinearPathComponents(Scalar scale, VertexWriter& writer) const; + std::vector Extrema() const; bool operator==(const QuadraticPathComponent& other) const { @@ -109,6 +133,8 @@ struct CubicPathComponent { void ToLinearPathComponents(Scalar scale, const PointProc& proc) const; + void ToLinearPathComponents(Scalar scale, VertexWriter& writer) const; + CubicPathComponent Subsegment(Scalar t0, Scalar t1) const; bool operator==(const CubicPathComponent& other) const { diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index afbf01da90f83..b4e0d973477d9 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -4,6 +4,10 @@ #include "impeller/tessellator/tessellator.h" +#include "impeller/core/buffer_view.h" +#include "impeller/core/formats.h" +#include "impeller/core/vertex_buffer.h" +#include "impeller/geometry/path_component.h" #include "third_party/libtess2/Include/tesselator.h" namespace impeller { @@ -33,8 +37,10 @@ static const TESSalloc kAlloc = { Tessellator::Tessellator() : point_buffer_(std::make_unique>()), + index_buffer_(std::make_unique>()), c_tessellator_(nullptr, &DestroyTessellator) { point_buffer_->reserve(2048); + index_buffer_->reserve(2048); TESSalloc alloc = kAlloc; { // libTess2 copies the TESSalloc despite the non-const argument. @@ -176,67 +182,52 @@ Path::Polyline Tessellator::CreateTempPolyline(const Path& path, return polyline; } -std::vector Tessellator::TessellateConvex(const Path& path, - Scalar tolerance) { +VertexBuffer Tessellator::TessellateConvex(const Path& path, + HostBuffer& host_buffer, + Scalar tolerance, + std::optional uv_transform) { FML_DCHECK(point_buffer_); - - std::vector output; - point_buffer_->clear(); - auto polyline = - path.CreatePolyline(tolerance, std::move(point_buffer_), - [this](Path::Polyline::PointBufferPtr point_buffer) { - point_buffer_ = std::move(point_buffer); - }); - if (polyline.points->size() == 0) { - return output; + FML_DCHECK(index_buffer_); + TessellateConvexInternal(path, *point_buffer_, *index_buffer_, tolerance, + uv_transform); + + if (point_buffer_->empty()) { + return VertexBuffer{ + .vertex_buffer = {}, + .index_buffer = {}, + .vertex_count = 0u, + .index_type = IndexType::k16bit, + }; } - output.reserve(polyline.points->size() + - (4 * (polyline.contours.size() - 1))); - bool previous_contour_odd_points = false; - for (auto j = 0u; j < polyline.contours.size(); j++) { - auto [start, end] = polyline.GetContourPointBounds(j); - auto first_point = 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.GetPoint(end - 1) == first_point) { - end--; - } + BufferView vertex_buffer = host_buffer.Emplace( + point_buffer_->data(), sizeof(Point) * point_buffer_->size(), + alignof(Point)); - if (j > 0) { - // Triangle strip break. - output.emplace_back(output.back()); - output.emplace_back(first_point); - output.emplace_back(first_point); - - // If the contour has an odd number of points, insert an extra point when - // bridging to the next contour to preserve the correct triangle winding - // order. - if (previous_contour_odd_points) { - output.emplace_back(first_point); - } - } else { - output.emplace_back(first_point); - } + BufferView index_buffer = host_buffer.Emplace( + index_buffer_->data(), sizeof(uint16_t) * index_buffer_->size(), + alignof(uint16_t)); - size_t a = start + 1; - size_t b = end - 1; - while (a < b) { - output.emplace_back(polyline.GetPoint(a)); - output.emplace_back(polyline.GetPoint(b)); - a++; - b--; - } - if (a == b) { - previous_contour_odd_points = false; - output.emplace_back(polyline.GetPoint(a)); - } else { - previous_contour_odd_points = true; - } - } - return output; + return VertexBuffer{ + .vertex_buffer = std::move(vertex_buffer), + .index_buffer = std::move(index_buffer), + .vertex_count = index_buffer_->size(), + .index_type = IndexType::k16bit, + }; +} + +void Tessellator::TessellateConvexInternal(const Path& path, + std::vector& point_buffer, + std::vector& index_buffer, + Scalar tolerance, + std::optional uv_transform) { + index_buffer_->clear(); + point_buffer_->clear(); + + VertexWriter writer(*point_buffer_, *index_buffer_, uv_transform); + + path.WritePolyline(tolerance, writer); + writer.EndContour(); } void DestroyTessellator(TESStesselator* tessellator) { diff --git a/impeller/tessellator/tessellator.h b/impeller/tessellator/tessellator.h index 6251e814aa764..31cabd9ab7d39 100644 --- a/impeller/tessellator/tessellator.h +++ b/impeller/tessellator/tessellator.h @@ -10,6 +10,8 @@ #include #include "impeller/core/formats.h" +#include "impeller/core/host_buffer.h" +#include "impeller/core/vertex_buffer.h" #include "impeller/geometry/path.h" #include "impeller/geometry/point.h" #include "impeller/geometry/trig.h" @@ -207,10 +209,28 @@ class Tessellator { /// a polyline. This value is often derived from the /// Matrix::GetMaxBasisLength of the CTM applied to the /// path for rendering. + /// @param[in] host_buffer The host buffer for allocation of vertices/index + /// data. + /// @param[in] uv_transform If provided, then uvs are also generated into the + /// point buffer. Defaults to std::nullopt. /// - /// @return A point vector containing the vertices in triangle strip format. + /// @return A vertex buffer containing all data from the provided curve. + VertexBuffer TessellateConvex( + const Path& path, + HostBuffer& host_buffer, + Scalar tolerance, + std::optional uv_transform = std::nullopt); + + /// Visible for testing. /// - std::vector TessellateConvex(const Path& path, Scalar tolerance); + /// This method only exists for the ease of benchmarking without using the + /// real allocator needed by the [host_buffer]. + void TessellateConvexInternal( + const Path& path, + std::vector& point_buffer, + std::vector& index_buffer, + Scalar tolerance, + std::optional uv_transform = std::nullopt); //---------------------------------------------------------------------------- /// @brief Create a temporary polyline. Only one per-process can exist at @@ -299,6 +319,7 @@ class Tessellator { private: /// Used for polyline generation. std::unique_ptr> point_buffer_; + std::unique_ptr> index_buffer_; CTessellator c_tessellator_; // Data for variouos Circle/EllipseGenerator classes, cached per diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index 2c54d4282db01..0aa227efc5c31 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -83,26 +83,31 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { TEST(TessellatorTest, TessellateConvex) { { Tessellator t; + std::vector points; + std::vector indices; // Sanity check simple rectangle. - auto pts = t.TessellateConvex( - PathBuilder{}.AddRect(Rect::MakeLTRB(0, 0, 10, 10)).TakePath(), 1.0); + t.TessellateConvexInternal( + PathBuilder{}.AddRect(Rect::MakeLTRB(0, 0, 10, 10)).TakePath(), points, + indices, 1.0); std::vector expected = {{0, 0}, {10, 0}, {0, 10}, {10, 10}}; - EXPECT_EQ(pts, expected); + EXPECT_EQ(points, expected); } { Tessellator t; - auto pts = t.TessellateConvex(PathBuilder{} - .AddRect(Rect::MakeLTRB(0, 0, 10, 10)) - .AddRect(Rect::MakeLTRB(20, 20, 30, 30)) - .TakePath(), - 1.0); + std::vector points; + std::vector indices; + t.TessellateConvexInternal(PathBuilder{} + .AddRect(Rect::MakeLTRB(0, 0, 10, 10)) + .AddRect(Rect::MakeLTRB(20, 20, 30, 30)) + .TakePath(), + points, indices, 1.0); std::vector expected = {{0, 0}, {10, 0}, {0, 10}, {10, 10}, {10, 10}, {20, 20}, {20, 20}, {30, 20}, {20, 30}, {30, 30}}; - EXPECT_EQ(pts, expected); + EXPECT_EQ(points, expected); } } @@ -474,7 +479,10 @@ TEST(TessellatorTest, EarlyReturnEmptyConvexShape) { builder.MoveTo({0, 0}); builder.MoveTo({10, 10}, /*relative=*/true); - auto points = tessellator->TessellateConvex(builder.TakePath(), 3.0); + std::vector points; + std::vector indices; + tessellator->TessellateConvexInternal(builder.TakePath(), points, indices, + 3.0); EXPECT_TRUE(points.empty()); } diff --git a/shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java b/shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java index 1b202c14cb535..2d5f25c531b13 100644 --- a/shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java +++ b/shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java @@ -71,6 +71,8 @@ public PlatformViewWrapper( } finally { surface.unlockCanvasAndPost(canvas); } + } else { + Log.e(TAG, "DISABLED SURFACE CLEAR"); } } From 206abba9e0f34990fe06654effa2f2b5c05eeb43 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 15 Apr 2024 13:00:06 -0700 Subject: [PATCH 02/18] revert unrelated change --- .../android/io/flutter/plugin/platform/PlatformViewWrapper.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java b/shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java index 2d5f25c531b13..1b202c14cb535 100644 --- a/shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java +++ b/shell/platform/android/io/flutter/plugin/platform/PlatformViewWrapper.java @@ -71,8 +71,6 @@ public PlatformViewWrapper( } finally { surface.unlockCanvasAndPost(canvas); } - } else { - Log.e(TAG, "DISABLED SURFACE CLEAR"); } } From a9af303070aeed615d32f2c0b80320002922037d Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 15 Apr 2024 13:43:32 -0700 Subject: [PATCH 03/18] update tessellator unittests. --- impeller/tessellator/tessellator.cc | 2 +- impeller/tessellator/tessellator_unittests.cc | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index b4e0d973477d9..89fad9c084c6b 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -224,7 +224,7 @@ void Tessellator::TessellateConvexInternal(const Path& path, index_buffer_->clear(); point_buffer_->clear(); - VertexWriter writer(*point_buffer_, *index_buffer_, uv_transform); + VertexWriter writer(point_buffer, index_buffer, uv_transform); path.WritePolyline(tolerance, writer); writer.EndContour(); diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index 0aa227efc5c31..3c6ee2e340607 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -90,8 +90,10 @@ TEST(TessellatorTest, TessellateConvex) { PathBuilder{}.AddRect(Rect::MakeLTRB(0, 0, 10, 10)).TakePath(), points, indices, 1.0); - std::vector expected = {{0, 0}, {10, 0}, {0, 10}, {10, 10}}; + std::vector expected = {{10, 0}, {10, 10}, {0, 10}, {0, 0}}; + std::vector expected_indices = {0, 1, 3, 2}; EXPECT_EQ(points, expected); + EXPECT_EQ(indices, expected_indices); } { @@ -104,10 +106,11 @@ TEST(TessellatorTest, TessellateConvex) { .TakePath(), points, indices, 1.0); - std::vector expected = {{0, 0}, {10, 0}, {0, 10}, {10, 10}, - {10, 10}, {20, 20}, {20, 20}, {30, 20}, - {20, 30}, {30, 30}}; + std::vector expected = {{10, 0}, {10, 10}, {0, 10}, {0, 0}, + {30, 20}, {30, 30}, {20, 30}, {20, 20}}; + std::vector expected_indices = {0, 1, 3, 2, 2, 4, 4, 5, 7, 6}; EXPECT_EQ(points, expected); + EXPECT_EQ(indices, expected_indices); } } From 2819e40ca25651e538ae8712cc230d7e7f0937d2 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 15 Apr 2024 17:48:46 -0700 Subject: [PATCH 04/18] Update tessellator.cc --- impeller/tessellator/tessellator.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index 89fad9c084c6b..929372970e59f 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -7,7 +7,6 @@ #include "impeller/core/buffer_view.h" #include "impeller/core/formats.h" #include "impeller/core/vertex_buffer.h" -#include "impeller/geometry/path_component.h" #include "third_party/libtess2/Include/tesselator.h" namespace impeller { From d11a3a07ee98c2f1f5fa0a0c88d35107c846dddd Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 15 Apr 2024 20:29:25 -0700 Subject: [PATCH 05/18] try moving include of tessellator. --- impeller/core/range.h | 2 -- impeller/tessellator/c/tessellator.cc | 2 ++ impeller/tessellator/c/tessellator.h | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/impeller/core/range.h b/impeller/core/range.h index d0971725c6928..94c61baf3bb3a 100644 --- a/impeller/core/range.h +++ b/impeller/core/range.h @@ -7,8 +7,6 @@ #include -#include "flutter/fml/macros.h" - namespace impeller { struct Range { diff --git a/impeller/tessellator/c/tessellator.cc b/impeller/tessellator/c/tessellator.cc index f5300dcba1dee..b1e7ca4068485 100644 --- a/impeller/tessellator/c/tessellator.cc +++ b/impeller/tessellator/c/tessellator.cc @@ -6,6 +6,8 @@ #include +#include "impeller/tessellator/tessellator.h" + namespace impeller { PathBuilder* CreatePathBuilder() { return new PathBuilder(); diff --git a/impeller/tessellator/c/tessellator.h b/impeller/tessellator/c/tessellator.h index ef5bfc950f107..c8e19b0e4f248 100644 --- a/impeller/tessellator/c/tessellator.h +++ b/impeller/tessellator/c/tessellator.h @@ -8,7 +8,6 @@ #include #include "impeller/geometry/path_builder.h" -#include "impeller/tessellator/tessellator.h" #ifdef _WIN32 #define IMPELLER_API __declspec(dllexport) From 63b69f7f93e66a1dbf88d0a55db01bcef3501eba Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 15 Apr 2024 21:18:15 -0700 Subject: [PATCH 06/18] move libtess out of tessellation. --- impeller/geometry/geometry_benchmarks.cc | 47 +--- impeller/renderer/renderer_unittests.cc | 30 +-- impeller/tessellator/BUILD.gn | 3 - impeller/tessellator/c/tessellator.cc | 200 +++++++++++++++++- impeller/tessellator/tessellator.cc | 158 +------------- impeller/tessellator/tessellator.h | 40 +--- impeller/tessellator/tessellator_unittests.cc | 67 ------ 7 files changed, 219 insertions(+), 326 deletions(-) diff --git a/impeller/geometry/geometry_benchmarks.cc b/impeller/geometry/geometry_benchmarks.cc index a384af9b3319b..3ea82c5ba1970 100644 --- a/impeller/geometry/geometry_benchmarks.cc +++ b/impeller/geometry/geometry_benchmarks.cc @@ -59,39 +59,22 @@ template static void BM_Polyline(benchmark::State& state, Args&&... args) { auto args_tuple = std::make_tuple(std::move(args)...); auto path = std::get(args_tuple); - bool tessellate = std::get(args_tuple); size_t point_count = 0u; size_t single_point_count = 0u; auto points = std::make_unique>(); points->reserve(2048); while (state.KeepRunning()) { - if (tessellate) { - 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( - // 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); - }); - single_point_count = polyline.points->size(); - point_count += single_point_count; - } + 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); + }); + single_point_count = polyline.points->size(); + point_count += single_point_count; } state.counters["SinglePointCount"] = single_point_count; state.counters["TotalPointCount"] = point_count; @@ -184,27 +167,17 @@ static void BM_Convex(benchmark::State& state, Args&&... args) { MAKE_STROKE_BENCHMARK_CAPTURE_CAPS_JOINS(path, _uvNoTx, UVMode::kUVRect) BENCHMARK_CAPTURE(BM_Polyline, cubic_polyline, CreateCubic(true), false); -BENCHMARK_CAPTURE(BM_Polyline, cubic_polyline_tess, CreateCubic(true), true); BENCHMARK_CAPTURE(BM_Polyline, unclosed_cubic_polyline, CreateCubic(false), false); -BENCHMARK_CAPTURE(BM_Polyline, - unclosed_cubic_polyline_tess, - CreateCubic(false), - true); MAKE_STROKE_BENCHMARK_CAPTURE_UVS(Cubic); BENCHMARK_CAPTURE(BM_Polyline, quad_polyline, CreateQuadratic(true), false); -BENCHMARK_CAPTURE(BM_Polyline, quad_polyline_tess, CreateQuadratic(true), true); BENCHMARK_CAPTURE(BM_Polyline, unclosed_quad_polyline, CreateQuadratic(false), false); -BENCHMARK_CAPTURE(BM_Polyline, - unclosed_quad_polyline_tess, - CreateQuadratic(false), - true); MAKE_STROKE_BENCHMARK_CAPTURE_UVS(Quadratic); BENCHMARK_CAPTURE(BM_Convex, rrect_convex, CreateRRect(), true); diff --git a/impeller/renderer/renderer_unittests.cc b/impeller/renderer/renderer_unittests.cc index 0024e42c9d861..591de6c2262d9 100644 --- a/impeller/renderer/renderer_unittests.cc +++ b/impeller/renderer/renderer_unittests.cc @@ -7,6 +7,7 @@ #include "impeller/core/formats.h" #include "impeller/core/host_buffer.h" #include "impeller/core/sampler_descriptor.h" +#include "impeller/entity/geometry/geometry.h" #include "impeller/fixtures/array.frag.h" #include "impeller/fixtures/array.vert.h" #include "impeller/fixtures/box_fade.frag.h" @@ -38,7 +39,6 @@ #include "impeller/renderer/render_target.h" #include "impeller/renderer/renderer.h" #include "impeller/renderer/vertex_buffer_builder.h" -#include "impeller/tessellator/tessellator.h" #include "third_party/imgui/imgui.h" // TODO(zanderso): https://github.com/flutter/flutter/issues/127701 @@ -392,25 +392,15 @@ TEST_P(RendererTest, CanRenderInstanced) { using FS = InstancedDrawFragmentShader; VertexBufferBuilder builder; - - ASSERT_EQ(Tessellator::Result::kSuccess, - Tessellator{}.Tessellate( - PathBuilder{} - .AddRect(Rect::MakeXYWH(10, 10, 100, 100)) - .TakePath(FillType::kOdd), - 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) { - VS::PerVertexData data; - data.vtx = {vertices[i], vertices[i + 1]}; - builder.AppendVertex(data); - } - for (auto i = 0u; i < indices_count; i++) { - builder.AppendIndex(indices[i]); - } - return true; - })); + builder.AddVertices({ + VS::PerVertexData{.vtx = Point{10, 10}}, + VS::PerVertexData{.vtx = Point{110, 10}}, + VS::PerVertexData{.vtx = Point{10, 110}}, + + VS::PerVertexData{.vtx = Point{110, 10}}, + VS::PerVertexData{.vtx = Point{10, 110}}, + VS::PerVertexData{.vtx = Point{110, 100}}, + }); ASSERT_NE(GetContext(), nullptr); auto pipeline = diff --git a/impeller/tessellator/BUILD.gn b/impeller/tessellator/BUILD.gn index bc56fb770f069..9bd2f853c75c7 100644 --- a/impeller/tessellator/BUILD.gn +++ b/impeller/tessellator/BUILD.gn @@ -15,7 +15,6 @@ impeller_component("tessellator") { deps = [ "../core", "//flutter/fml", - "//third_party/libtess2", ] } @@ -30,8 +29,6 @@ impeller_component("tessellator_shared") { sources = [ "c/tessellator.cc", "c/tessellator.h", - "tessellator.cc", - "tessellator.h", ] deps = [ diff --git a/impeller/tessellator/c/tessellator.cc b/impeller/tessellator/c/tessellator.cc index b1e7ca4068485..0f2c9734fc3b6 100644 --- a/impeller/tessellator/c/tessellator.cc +++ b/impeller/tessellator/c/tessellator.cc @@ -4,11 +4,205 @@ #include "tessellator.h" +#include #include -#include "impeller/tessellator/tessellator.h" +#include "third_party/libtess2/Include/tesselator.h" namespace impeller { + +void DestroyTessellator(TESStesselator* tessellator) { + if (tessellator != nullptr) { + ::tessDeleteTess(tessellator); + } +} + +using CTessellator = + std::unique_ptr; + +static int ToTessWindingRule(FillType fill_type) { + switch (fill_type) { + case FillType::kOdd: + return TESS_WINDING_ODD; + case FillType::kNonZero: + return TESS_WINDING_NONZERO; + } + return TESS_WINDING_ODD; +} + +static void* HeapAlloc(void* userData, unsigned int size) { + return malloc(size); +} + +static void* HeapRealloc(void* userData, void* ptr, unsigned int size) { + return realloc(ptr, size); +} + +static void HeapFree(void* userData, void* ptr) { + free(ptr); +} + +// Note: these units are "number of entities" for bucket size and not in KB. +static const TESSalloc kAlloc = { + HeapAlloc, HeapRealloc, HeapFree, 0, /* =userData */ + 16, /* =meshEdgeBucketSize */ + 16, /* =meshVertexBucketSize */ + 16, /* =meshFaceBucketSize */ + 16, /* =dictNodeBucketSize */ + 16, /* =regionBucketSize */ + 0 /* =extraVertices */ +}; + +class LibtessTessellator { + public: + LibtessTessellator() : c_tessellator_(nullptr, &DestroyTessellator) { + TESSalloc alloc = kAlloc; + { + // libTess2 copies the TESSalloc despite the non-const argument. + CTessellator tessellator(::tessNewTess(&alloc), &DestroyTessellator); + c_tessellator_ = std::move(tessellator); + } + } + + ~LibtessTessellator() {} + + enum class Result { + kSuccess, + kInputError, + kTessellationError, + }; + + /// @brief A callback that returns the results of the tessellation. + /// + /// The index buffer may not be populated, in which case [indices] will + /// be nullptr and indices_count will be 0. + using BuilderCallback = std::function; + + //---------------------------------------------------------------------------- + /// @brief Generates filled triangles from the path. A callback is + /// invoked once for the entire tessellation. + /// + /// @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. + /// + Result Tessellate(const Path& path, + Scalar tolerance, + const BuilderCallback& callback) { + if (!callback) { + return Result::kInputError; + } + + std::unique_ptr> buffer = + std::make_unique>(); + auto polyline = path.CreatePolyline(tolerance, std::move(buffer)); + + auto fill_type = path.GetFillType(); + + if (polyline.points->empty()) { + return Result::kInputError; + } + + auto tessellator = c_tessellator_.get(); + if (!tessellator) { + return Result::kTessellationError; + } + + constexpr int kVertexSize = 2; + constexpr int kPolygonSize = 3; + + //---------------------------------------------------------------------------- + /// Feed contour information to the tessellator. + /// + static_assert(sizeof(Point) == 2 * sizeof(float)); + for (size_t contour_i = 0; contour_i < polyline.contours.size(); + contour_i++) { + size_t start_point_index, end_point_index; + std::tie(start_point_index, end_point_index) = + polyline.GetContourPointBounds(contour_i); + + ::tessAddContour(tessellator, // the C tessellator + kVertexSize, // + polyline.points->data() + start_point_index, // + sizeof(Point), // + end_point_index - start_point_index // + ); + } + + //---------------------------------------------------------------------------- + /// Let's tessellate. + /// + auto result = ::tessTesselate(tessellator, // tessellator + ToTessWindingRule(fill_type), // winding + TESS_POLYGONS, // element type + kPolygonSize, // polygon size + kVertexSize, // vertex size + nullptr // normal (null is automatic) + ); + + if (result != 1) { + return Result::kTessellationError; + } + + int element_item_count = tessGetElementCount(tessellator) * kPolygonSize; + + // We default to using a 16bit index buffer, but in cases where we generate + // more tessellated data than this can contain we need to fall back to + // dropping the index buffer entirely. Instead code could instead switch to + // a uint32 index buffer, but this is done for simplicity with the other + // fast path above. + if (element_item_count < USHRT_MAX) { + int vertex_item_count = tessGetVertexCount(tessellator); + auto vertices = tessGetVertices(tessellator); + auto elements = tessGetElements(tessellator); + + // libtess uses an int index internally due to usage of -1 as a sentinel + // value. + std::vector indices(element_item_count); + for (int i = 0; i < element_item_count; i++) { + indices[i] = static_cast(elements[i]); + } + if (!callback(vertices, vertex_item_count, indices.data(), + element_item_count)) { + return Result::kInputError; + } + } else { + std::vector points; + std::vector data; + + 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); + } + if (!callback(data.data(), element_item_count, nullptr, 0u)) { + return Result::kInputError; + } + } + + return Result::kSuccess; + } + + CTessellator c_tessellator_; +}; + PathBuilder* CreatePathBuilder() { return new PathBuilder(); } @@ -44,7 +238,7 @@ struct Vertices* Tessellate(PathBuilder* builder, Scalar tolerance) { auto path = builder->CopyPath(static_cast(fill_type)); std::vector points; - if (Tessellator{}.Tessellate( + if (LibtessTessellator{}.Tessellate( path, tolerance, [&points](const float* vertices, size_t vertices_count, const uint16_t* indices, size_t indices_count) { @@ -59,7 +253,7 @@ struct Vertices* Tessellate(PathBuilder* builder, points.push_back(point.y); } return true; - }) != Tessellator::Result::kSuccess) { + }) != LibtessTessellator::Result::kSuccess) { return nullptr; } diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index 929372970e59f..ac63c66078fb9 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -7,168 +7,18 @@ #include "impeller/core/buffer_view.h" #include "impeller/core/formats.h" #include "impeller/core/vertex_buffer.h" -#include "third_party/libtess2/Include/tesselator.h" namespace impeller { -static void* HeapAlloc(void* userData, unsigned int size) { - return malloc(size); -} - -static void* HeapRealloc(void* userData, void* ptr, unsigned int size) { - return realloc(ptr, size); -} - -static void HeapFree(void* userData, void* ptr) { - free(ptr); -} - -// Note: these units are "number of entities" for bucket size and not in KB. -static const TESSalloc kAlloc = { - HeapAlloc, HeapRealloc, HeapFree, 0, /* =userData */ - 16, /* =meshEdgeBucketSize */ - 16, /* =meshVertexBucketSize */ - 16, /* =meshFaceBucketSize */ - 16, /* =dictNodeBucketSize */ - 16, /* =regionBucketSize */ - 0 /* =extraVertices */ -}; - Tessellator::Tessellator() : point_buffer_(std::make_unique>()), - index_buffer_(std::make_unique>()), - c_tessellator_(nullptr, &DestroyTessellator) { + index_buffer_(std::make_unique>()) { point_buffer_->reserve(2048); index_buffer_->reserve(2048); - TESSalloc alloc = kAlloc; - { - // libTess2 copies the TESSalloc despite the non-const argument. - CTessellator tessellator(::tessNewTess(&alloc), &DestroyTessellator); - c_tessellator_ = std::move(tessellator); - } } Tessellator::~Tessellator() = default; -static int ToTessWindingRule(FillType fill_type) { - switch (fill_type) { - case FillType::kOdd: - return TESS_WINDING_ODD; - case FillType::kNonZero: - return TESS_WINDING_NONZERO; - } - return TESS_WINDING_ODD; -} - -Tessellator::Result Tessellator::Tessellate(const Path& path, - Scalar tolerance, - const BuilderCallback& callback) { - if (!callback) { - return Result::kInputError; - } - - point_buffer_->clear(); - 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(); - - if (polyline.points->empty()) { - return Result::kInputError; - } - - auto tessellator = c_tessellator_.get(); - if (!tessellator) { - return Result::kTessellationError; - } - - constexpr int kVertexSize = 2; - constexpr int kPolygonSize = 3; - - //---------------------------------------------------------------------------- - /// Feed contour information to the tessellator. - /// - static_assert(sizeof(Point) == 2 * sizeof(float)); - for (size_t contour_i = 0; contour_i < polyline.contours.size(); - contour_i++) { - size_t start_point_index, end_point_index; - std::tie(start_point_index, end_point_index) = - polyline.GetContourPointBounds(contour_i); - - ::tessAddContour(tessellator, // the C tessellator - kVertexSize, // - polyline.points->data() + start_point_index, // - sizeof(Point), // - end_point_index - start_point_index // - ); - } - - //---------------------------------------------------------------------------- - /// Let's tessellate. - /// - auto result = ::tessTesselate(tessellator, // tessellator - ToTessWindingRule(fill_type), // winding - TESS_POLYGONS, // element type - kPolygonSize, // polygon size - kVertexSize, // vertex size - nullptr // normal (null is automatic) - ); - - if (result != 1) { - return Result::kTessellationError; - } - - int element_item_count = tessGetElementCount(tessellator) * kPolygonSize; - - // We default to using a 16bit index buffer, but in cases where we generate - // more tessellated data than this can contain we need to fall back to - // dropping the index buffer entirely. Instead code could instead switch to - // a uint32 index buffer, but this is done for simplicity with the other - // fast path above. - if (element_item_count < USHRT_MAX) { - int vertex_item_count = tessGetVertexCount(tessellator); - auto vertices = tessGetVertices(tessellator); - auto elements = tessGetElements(tessellator); - - // libtess uses an int index internally due to usage of -1 as a sentinel - // value. - std::vector indices(element_item_count); - for (int i = 0; i < element_item_count; i++) { - indices[i] = static_cast(elements[i]); - } - if (!callback(vertices, vertex_item_count, indices.data(), - element_item_count)) { - return Result::kInputError; - } - } else { - std::vector points; - std::vector data; - - 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); - } - if (!callback(data.data(), element_item_count, nullptr, 0u)) { - return Result::kInputError; - } - } - - return Result::kSuccess; -} - Path::Polyline Tessellator::CreateTempPolyline(const Path& path, Scalar tolerance) { FML_DCHECK(point_buffer_); @@ -229,12 +79,6 @@ void Tessellator::TessellateConvexInternal(const Path& path, writer.EndContour(); } -void DestroyTessellator(TESStesselator* tessellator) { - if (tessellator != nullptr) { - ::tessDeleteTess(tessellator); - } -} - static constexpr int kPrecomputedDivisionCount = 1024; static int kPrecomputedDivisions[kPrecomputedDivisionCount] = { // clang-format off diff --git a/impeller/tessellator/tessellator.h b/impeller/tessellator/tessellator.h index 31cabd9ab7d39..013bf4dff044b 100644 --- a/impeller/tessellator/tessellator.h +++ b/impeller/tessellator/tessellator.h @@ -20,11 +20,6 @@ struct TESStesselator; namespace impeller { -void DestroyTessellator(TESStesselator* tessellator); - -using CTessellator = - std::unique_ptr; - //------------------------------------------------------------------------------ /// @brief A utility that generates triangles of the specified fill type /// given a polyline. This happens on the CPU. @@ -71,12 +66,6 @@ class Tessellator { }; public: - enum class Result { - kSuccess, - kInputError, - kTessellationError, - }; - /// @brief A callback function for a |VertexGenerator| to deliver /// the vertices it computes as |Point| objects. using TessellatedVertexProc = std::function; @@ -175,32 +164,6 @@ class Tessellator { ~Tessellator(); - /// @brief A callback that returns the results of the tessellation. - /// - /// The index buffer may not be populated, in which case [indices] will - /// be nullptr and indices_count will be 0. - using BuilderCallback = std::function; - - //---------------------------------------------------------------------------- - /// @brief Generates filled triangles from the path. A callback is - /// invoked once for the entire tessellation. - /// - /// @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 tolerance, - const BuilderCallback& callback); - //---------------------------------------------------------------------------- /// @brief Given a convex path, create a triangle fan structure. /// @@ -320,9 +283,8 @@ class Tessellator { /// Used for polyline generation. std::unique_ptr> point_buffer_; std::unique_ptr> index_buffer_; - CTessellator c_tessellator_; - // Data for variouos Circle/EllipseGenerator classes, cached per + // Data for various Circle/EllipseGenerator classes, cached per // Tessellator instance which is usually the foreground life of an app // if not longer. static constexpr size_t kCachedTrigCount = 300; diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index 3c6ee2e340607..b967ee6b20467 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -13,73 +13,6 @@ namespace impeller { namespace testing { -TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { - // Zero points. - { - Tessellator t; - auto path = PathBuilder{}.TakePath(FillType::kOdd); - 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::kInputError); - } - - // One point. - { - Tessellator t; - auto path = PathBuilder{}.LineTo({0, 0}).TakePath(FillType::kOdd); - 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); - } - - // Two points. - { - Tessellator t; - auto path = PathBuilder{}.AddLine({0, 0}, {0, 1}).TakePath(FillType::kOdd); - 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); - } - - // Many points. - { - Tessellator t; - PathBuilder builder; - for (int i = 0; i < 1000; i++) { - auto coord = i * 1.0f; - builder.AddLine({coord, coord}, {coord + 1, coord + 1}); - } - auto path = builder.TakePath(FillType::kOdd); - 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); - } - - // Closure fails. - { - Tessellator t; - auto path = PathBuilder{}.AddLine({0, 0}, {0, 1}).TakePath(FillType::kOdd); - 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); - } -} - TEST(TessellatorTest, TessellateConvex) { { Tessellator t; From 1b05f6461713572dc4d135e0d2f04f8650e04152 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 16 Apr 2024 08:34:04 -0700 Subject: [PATCH 07/18] remove bad include. --- impeller/renderer/renderer_unittests.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/impeller/renderer/renderer_unittests.cc b/impeller/renderer/renderer_unittests.cc index 591de6c2262d9..5ff7bdd781b05 100644 --- a/impeller/renderer/renderer_unittests.cc +++ b/impeller/renderer/renderer_unittests.cc @@ -7,7 +7,6 @@ #include "impeller/core/formats.h" #include "impeller/core/host_buffer.h" #include "impeller/core/sampler_descriptor.h" -#include "impeller/entity/geometry/geometry.h" #include "impeller/fixtures/array.frag.h" #include "impeller/fixtures/array.vert.h" #include "impeller/fixtures/box_fade.frag.h" From e9f8b93cc76d55da260886ddeee4074c34a67404 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 16 Apr 2024 16:38:59 -0700 Subject: [PATCH 08/18] explicitly insert polyline begin. --- impeller/geometry/path.cc | 9 +++++++++ impeller/geometry/path_builder.cc | 2 +- impeller/geometry/path_component.cc | 6 ------ 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/impeller/geometry/path.cc b/impeller/geometry/path.cc index 73daedaab3b97..ab5ed9be7f65a 100644 --- a/impeller/geometry/path.cc +++ b/impeller/geometry/path.cc @@ -385,6 +385,15 @@ void Path::WritePolyline(Scalar scale, VertexWriter& writer) const { continue; } writer.EndContour(); + + // Insert contour start. + const auto& next_component = path_components[component_i + 1]; + // It doesn't matter what type this is as all structs are laid out + // with p1 as the first member. + const LinearPathComponent* linear = + reinterpret_cast( + &path_points[next_component.index]); + writer.Write(linear->p1); break; } } diff --git a/impeller/geometry/path_builder.cc b/impeller/geometry/path_builder.cc index 5cc9e28127cb3..b0b9e77d10881 100644 --- a/impeller/geometry/path_builder.cc +++ b/impeller/geometry/path_builder.cc @@ -38,7 +38,7 @@ PathBuilder& PathBuilder::MoveTo(Point point, bool relative) { } PathBuilder& PathBuilder::Close() { - LineTo(subpath_start_); + // LineTo(subpath_start_); SetContourClosed(true); AddContourComponent(current_); return *this; diff --git a/impeller/geometry/path_component.cc b/impeller/geometry/path_component.cc index 7e1b558a2e49f..dbbaac3dc783a 100644 --- a/impeller/geometry/path_component.cc +++ b/impeller/geometry/path_component.cc @@ -23,12 +23,6 @@ void VertexWriter::EndContour() { auto start = contour_start_; auto end = points_.size() - 1; - // 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 (points_[end] == points_[start]) { - end--; - } if (contour_start_ > 0) { // Triangle strip break. From 732edd5e6183364da3a4304e313fb4f3a542f0af Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 16 Apr 2024 16:45:45 -0700 Subject: [PATCH 09/18] add unit test for path lacking explicit close. --- impeller/tessellator/tessellator_unittests.cc | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index b967ee6b20467..2fdfa217ed4bc 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -47,6 +47,26 @@ TEST(TessellatorTest, TessellateConvex) { } } +// Filled Paths without an explicit close should still be closed +TEST(TessellatorTest, TessellateConvexUnclosedPath) { + Tessellator t; + std::vector points; + std::vector indices; + + // Create a rectangle that lacks an explicit close. + Path path = PathBuilder{} + .LineTo({100, 0}) + .LineTo({100, 100}) + .LineTo({0, 100}) + .TakePath(); + t.TessellateConvexInternal(path, points, indices, 1.0); + + std::vector expected = {{0, 0}, {10, 0}, {10, 10}, {0, 10}}; + std::vector expected_indices = {0, 1, 3, 2}; + EXPECT_EQ(points, expected); + EXPECT_EQ(indices, expected_indices); +} + TEST(TessellatorTest, CircleVertexCounts) { auto tessellator = std::make_shared(); From 3d0100a483cb637fd345f769a3b72818bf79ed08 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 16 Apr 2024 16:49:09 -0700 Subject: [PATCH 10/18] update tests. --- impeller/tessellator/tessellator_unittests.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index 2fdfa217ed4bc..a72553b9fe002 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -23,7 +23,7 @@ TEST(TessellatorTest, TessellateConvex) { PathBuilder{}.AddRect(Rect::MakeLTRB(0, 0, 10, 10)).TakePath(), points, indices, 1.0); - std::vector expected = {{10, 0}, {10, 10}, {0, 10}, {0, 0}}; + std::vector expected = {{0, 0}, {10, 0}, {10, 10}, {0, 10}}; std::vector expected_indices = {0, 1, 3, 2}; EXPECT_EQ(points, expected); EXPECT_EQ(indices, expected_indices); @@ -39,8 +39,8 @@ TEST(TessellatorTest, TessellateConvex) { .TakePath(), points, indices, 1.0); - std::vector expected = {{10, 0}, {10, 10}, {0, 10}, {0, 0}, - {30, 20}, {30, 30}, {20, 30}, {20, 20}}; + 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, 3, 2, 2, 4, 4, 5, 7, 6}; EXPECT_EQ(points, expected); EXPECT_EQ(indices, expected_indices); @@ -61,7 +61,7 @@ TEST(TessellatorTest, TessellateConvexUnclosedPath) { .TakePath(); t.TessellateConvexInternal(path, points, indices, 1.0); - std::vector expected = {{0, 0}, {10, 0}, {10, 10}, {0, 10}}; + std::vector expected = {{0, 0}, {100, 0}, {100, 100}, {0, 100}}; std::vector expected_indices = {0, 1, 3, 2}; EXPECT_EQ(points, expected); EXPECT_EQ(indices, expected_indices); From f3ad07ab44c736bf7471b7de71b6a4555c3b562d Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 16 Apr 2024 17:06:27 -0700 Subject: [PATCH 11/18] addon. --- impeller/aiks/aiks_unittests.cc | 19 +++++++++++++++++++ impeller/geometry/path_builder.cc | 1 - testing/impeller_golden_tests_output.txt | 3 +++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 6e4cde04a925e..147e24776fdc9 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -3135,6 +3135,25 @@ TEST_P(AiksTest, DrawAtlasPlusWideGamut) { ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); } +// Ensure that a stroked path that calls close does not actually close. +TEST_P(AiksTest, StrokedClosedPathDrawnCorrectly) { + PathBuilder builder; + Path path = builder.MoveTo({0, 400}) + .LineTo({0, 0}) + .LineTo({400, 0}) + .Close() + .TakePath(); + + Canvas canvas; + canvas.DrawPath(path, { + .color = Color::Red(), + .stroke_width = 10, + .stroke_cap = Cap::kRound, + .style = Paint::Style::kStroke, + }); + ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); +} + } // namespace testing } // namespace impeller diff --git a/impeller/geometry/path_builder.cc b/impeller/geometry/path_builder.cc index b0b9e77d10881..33b19fb717591 100644 --- a/impeller/geometry/path_builder.cc +++ b/impeller/geometry/path_builder.cc @@ -38,7 +38,6 @@ PathBuilder& PathBuilder::MoveTo(Point point, bool relative) { } PathBuilder& PathBuilder::Close() { - // LineTo(subpath_start_); SetContourClosed(true); AddContourComponent(current_); return *this; diff --git a/testing/impeller_golden_tests_output.txt b/testing/impeller_golden_tests_output.txt index 688d0d6b9cc09..4f631de253ba5 100644 --- a/testing/impeller_golden_tests_output.txt +++ b/testing/impeller_golden_tests_output.txt @@ -726,6 +726,9 @@ impeller_Play_AiksTest_SrgbToLinearFilterSubpassCollapseOptimization_Vulkan.png impeller_Play_AiksTest_StrokedCirclesRenderCorrectly_Metal.png impeller_Play_AiksTest_StrokedCirclesRenderCorrectly_OpenGLES.png impeller_Play_AiksTest_StrokedCirclesRenderCorrectly_Vulkan.png +impeller_Play_AiksTest_StrokedClosedPathDrawnCorrectly_Metal.png +impeller_Play_AiksTest_StrokedClosedPathDrawnCorrectly_OpenGLES.png +impeller_Play_AiksTest_StrokedClosedPathDrawnCorrectly_Vulkan.png impeller_Play_AiksTest_SubpassWithClearColorOptimization_Metal.png impeller_Play_AiksTest_SubpassWithClearColorOptimization_OpenGLES.png impeller_Play_AiksTest_SubpassWithClearColorOptimization_Vulkan.png From 235d02f353cbc982ffe9f08660d292add5a936a8 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 16 Apr 2024 17:54:57 -0700 Subject: [PATCH 12/18] fixes to path unit tests and current position. --- impeller/geometry/path_builder.cc | 1 + impeller/geometry/path_unittests.cc | 38 ++++++++++++++--------------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/impeller/geometry/path_builder.cc b/impeller/geometry/path_builder.cc index 33b19fb717591..69043c2dff9d6 100644 --- a/impeller/geometry/path_builder.cc +++ b/impeller/geometry/path_builder.cc @@ -38,6 +38,7 @@ PathBuilder& PathBuilder::MoveTo(Point point, bool relative) { } PathBuilder& PathBuilder::Close() { + current_ = subpath_start_; SetContourClosed(true); AddContourComponent(current_); return *this; diff --git a/impeller/geometry/path_unittests.cc b/impeller/geometry/path_unittests.cc index 6ffd69a1de56a..012a1a8c0f349 100644 --- a/impeller/geometry/path_unittests.cc +++ b/impeller/geometry/path_unittests.cc @@ -131,12 +131,12 @@ TEST(PathTest, PathCreatePolylineGeneratesCorrectContourData) { .Close() .TakePath() .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); - ASSERT_EQ(polyline.contours[1].is_closed, true); - ASSERT_EQ(polyline.contours[1].start_index, 2u); + EXPECT_EQ(polyline.points->size(), 5u); + EXPECT_EQ(polyline.contours.size(), 2u); + EXPECT_EQ(polyline.contours[0].is_closed, false); + EXPECT_EQ(polyline.contours[0].start_index, 0u); + EXPECT_EQ(polyline.contours[1].is_closed, true); + EXPECT_EQ(polyline.contours[1].start_index, 2u); } TEST(PathTest, PolylineGetContourPointBoundsReturnsCorrectRanges) { @@ -154,7 +154,7 @@ TEST(PathTest, PolylineGetContourPointBoundsReturnsCorrectRanges) { ASSERT_EQ(a1, 0u); ASSERT_EQ(a2, 2u); ASSERT_EQ(b1, 2u); - ASSERT_EQ(b2, 6u); + ASSERT_EQ(b2, 5u); } TEST(PathTest, PathAddRectPolylineHasCorrectContourData) { @@ -165,12 +165,11 @@ TEST(PathTest, PathAddRectPolylineHasCorrectContourData) { 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->size(), 4u); 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(PathTest, PathPolylineDuplicatesAreRemovedForSameContour) { @@ -187,19 +186,18 @@ TEST(PathTest, PathPolylineDuplicatesAreRemovedForSameContour) { .LineTo({0, 100}) // Insert duplicate at end of contour. .TakePath() .CreatePolyline(1.0f); - ASSERT_EQ(polyline.contours.size(), 2u); - ASSERT_EQ(polyline.contours[0].start_index, 0u); + EXPECT_EQ(polyline.contours.size(), 2u); + EXPECT_EQ(polyline.contours[0].start_index, 0u); ASSERT_TRUE(polyline.contours[0].is_closed); - ASSERT_EQ(polyline.contours[1].start_index, 4u); + ASSERT_EQ(polyline.contours[1].start_index, 3u); ASSERT_FALSE(polyline.contours[1].is_closed); - ASSERT_EQ(polyline.points->size(), 7u); - 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)); + ASSERT_EQ(polyline.points->size(), 6u); + EXPECT_EQ(polyline.GetPoint(0), Point(50, 50)); + EXPECT_EQ(polyline.GetPoint(1), Point(100, 50)); + EXPECT_EQ(polyline.GetPoint(2), Point(100, 100)); + EXPECT_EQ(polyline.GetPoint(3), Point(50, 50)); + EXPECT_EQ(polyline.GetPoint(4), Point(0, 50)); + EXPECT_EQ(polyline.GetPoint(5), Point(0, 100)); } TEST(PathTest, PolylineBufferReuse) { From 9c8eb61de52e47e5d186a8b1450b74ebf65cf7f7 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Tue, 16 Apr 2024 19:40:01 -0700 Subject: [PATCH 13/18] Update path_builder.cc --- impeller/geometry/path_builder.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/impeller/geometry/path_builder.cc b/impeller/geometry/path_builder.cc index 69043c2dff9d6..33b19fb717591 100644 --- a/impeller/geometry/path_builder.cc +++ b/impeller/geometry/path_builder.cc @@ -38,7 +38,6 @@ PathBuilder& PathBuilder::MoveTo(Point point, bool relative) { } PathBuilder& PathBuilder::Close() { - current_ = subpath_start_; SetContourClosed(true); AddContourComponent(current_); return *this; From 0bb3ca3611dd15b3f25dd8e56d64b4b6f455dd68 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 16 Apr 2024 20:01:06 -0700 Subject: [PATCH 14/18] add this back. --- impeller/geometry/path_builder.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/impeller/geometry/path_builder.cc b/impeller/geometry/path_builder.cc index 33b19fb717591..69043c2dff9d6 100644 --- a/impeller/geometry/path_builder.cc +++ b/impeller/geometry/path_builder.cc @@ -38,6 +38,7 @@ PathBuilder& PathBuilder::MoveTo(Point point, bool relative) { } PathBuilder& PathBuilder::Close() { + current_ = subpath_start_; SetContourClosed(true); AddContourComponent(current_); return *this; From e5606eb3f7204c6d50da4bafd0d72c1cc5291fbc Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 17 Apr 2024 09:43:45 -0700 Subject: [PATCH 15/18] ++ --- impeller/aiks/aiks_unittests.cc | 29 +++++++++++++++++++++++++++++ impeller/geometry/path_builder.cc | 2 +- impeller/geometry/path_component.cc | 7 +++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 147e24776fdc9..41d6581beca5a 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -3145,6 +3145,7 @@ TEST_P(AiksTest, StrokedClosedPathDrawnCorrectly) { .TakePath(); Canvas canvas; + canvas.Translate({50, 50, 0}); canvas.DrawPath(path, { .color = Color::Red(), .stroke_width = 10, @@ -3154,6 +3155,34 @@ TEST_P(AiksTest, StrokedClosedPathDrawnCorrectly) { ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); } +// All shapes should be closed. +TEST_P(AiksTest, StrokedGeometry) { + PathBuilder builder; + + Paint paint = { + .color = Color::Red(), + .stroke_width = 10, + .stroke_cap = Cap::kRound, + .style = Paint::Style::kStroke, + }; + + Canvas canvas; + canvas.Translate({50, 50, 0}); + canvas.DrawPath( + PathBuilder{}.AddRect(Rect::MakeLTRB(50, 50, 250, 250)).TakePath(), + paint); + canvas.DrawPath(PathBuilder{} + .AddRoundedRect(Rect::MakeLTRB(200, 200, 400, 400), + Size::MakeWH(8, 8)) + .TakePath(), + paint); + canvas.DrawPath( + PathBuilder{}.AddOval(Rect::MakeLTRB(150, 250, 550, 450)).TakePath(), + paint); + + ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); +} + } // namespace testing } // namespace impeller diff --git a/impeller/geometry/path_builder.cc b/impeller/geometry/path_builder.cc index 69043c2dff9d6..5cc9e28127cb3 100644 --- a/impeller/geometry/path_builder.cc +++ b/impeller/geometry/path_builder.cc @@ -38,7 +38,7 @@ PathBuilder& PathBuilder::MoveTo(Point point, bool relative) { } PathBuilder& PathBuilder::Close() { - current_ = subpath_start_; + LineTo(subpath_start_); SetContourClosed(true); AddContourComponent(current_); return *this; diff --git a/impeller/geometry/path_component.cc b/impeller/geometry/path_component.cc index dbbaac3dc783a..3751ba1726a79 100644 --- a/impeller/geometry/path_component.cc +++ b/impeller/geometry/path_component.cc @@ -23,6 +23,13 @@ void VertexWriter::EndContour() { auto start = contour_start_; auto end = points_.size() - 1; + // All filled paths are drawn as if they are closed, but if + // there is an explicit close then a lineTo to the origin + // is inserted. This point isn't strictly necesary to + // correctly render the shape and can be dropped. + if (points_[end] == points_[start]) { + end--; + } if (contour_start_ > 0) { // Triangle strip break. From 85d729b7524c24d25b8881f99cb6ba94cb497964 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 17 Apr 2024 09:53:16 -0700 Subject: [PATCH 16/18] ++ --- impeller/aiks/aiks_unittests.cc | 57 +++--------------------- testing/impeller_golden_tests_output.txt | 9 ---- 2 files changed, 5 insertions(+), 61 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 41d6581beca5a..3613191280824 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -941,13 +941,14 @@ TEST_P(AiksTest, CanDrawPaintMultipleTimes) { TEST_P(AiksTest, FormatWideGamut) { EXPECT_EQ(GetContext()->GetCapabilities()->GetDefaultColorFormat(), PixelFormat::kB10G10R10A10XR); - EXPECT_TRUE(IsAlphaClampedToOne( - GetContext()->GetCapabilities()->GetDefaultColorFormat())); } TEST_P(AiksTest, FormatSRGB) { - EXPECT_TRUE(IsAlphaClampedToOne( - GetContext()->GetCapabilities()->GetDefaultColorFormat())); + PixelFormat pixel_format = + GetContext()->GetCapabilities()->GetDefaultColorFormat(); + EXPECT_TRUE(pixel_format == PixelFormat::kR8G8B8A8UNormInt || + pixel_format == PixelFormat::kB8G8R8A8UNormInt) + << "pixel format: " << PixelFormatToString(pixel_format); } TEST_P(AiksTest, TransformMultipliesCorrectly) { @@ -3135,54 +3136,6 @@ TEST_P(AiksTest, DrawAtlasPlusWideGamut) { ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); } -// Ensure that a stroked path that calls close does not actually close. -TEST_P(AiksTest, StrokedClosedPathDrawnCorrectly) { - PathBuilder builder; - Path path = builder.MoveTo({0, 400}) - .LineTo({0, 0}) - .LineTo({400, 0}) - .Close() - .TakePath(); - - Canvas canvas; - canvas.Translate({50, 50, 0}); - canvas.DrawPath(path, { - .color = Color::Red(), - .stroke_width = 10, - .stroke_cap = Cap::kRound, - .style = Paint::Style::kStroke, - }); - ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); -} - -// All shapes should be closed. -TEST_P(AiksTest, StrokedGeometry) { - PathBuilder builder; - - Paint paint = { - .color = Color::Red(), - .stroke_width = 10, - .stroke_cap = Cap::kRound, - .style = Paint::Style::kStroke, - }; - - Canvas canvas; - canvas.Translate({50, 50, 0}); - canvas.DrawPath( - PathBuilder{}.AddRect(Rect::MakeLTRB(50, 50, 250, 250)).TakePath(), - paint); - canvas.DrawPath(PathBuilder{} - .AddRoundedRect(Rect::MakeLTRB(200, 200, 400, 400), - Size::MakeWH(8, 8)) - .TakePath(), - paint); - canvas.DrawPath( - PathBuilder{}.AddOval(Rect::MakeLTRB(150, 250, 550, 450)).TakePath(), - paint); - - ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); -} - } // namespace testing } // namespace impeller diff --git a/testing/impeller_golden_tests_output.txt b/testing/impeller_golden_tests_output.txt index 4f631de253ba5..d0796e370311f 100644 --- a/testing/impeller_golden_tests_output.txt +++ b/testing/impeller_golden_tests_output.txt @@ -57,9 +57,6 @@ impeller_Play_AiksTest_BlendModeMultiply_Vulkan.png impeller_Play_AiksTest_BlendModeOverlay_Metal.png impeller_Play_AiksTest_BlendModeOverlay_OpenGLES.png impeller_Play_AiksTest_BlendModeOverlay_Vulkan.png -impeller_Play_AiksTest_BlendModePlusAdvanced_Metal.png -impeller_Play_AiksTest_BlendModePlusAdvanced_OpenGLES.png -impeller_Play_AiksTest_BlendModePlusAdvanced_Vulkan.png impeller_Play_AiksTest_BlendModePlusAlphaColorFilterWideGamut_Metal.png impeller_Play_AiksTest_BlendModePlusAlphaWideGamut_Metal.png impeller_Play_AiksTest_BlendModePlus_Metal.png @@ -149,9 +146,6 @@ impeller_Play_AiksTest_BlendModeSrcAlphaMultiply_Vulkan.png impeller_Play_AiksTest_BlendModeSrcAlphaOverlay_Metal.png impeller_Play_AiksTest_BlendModeSrcAlphaOverlay_OpenGLES.png impeller_Play_AiksTest_BlendModeSrcAlphaOverlay_Vulkan.png -impeller_Play_AiksTest_BlendModeSrcAlphaPlusAdvanced_Metal.png -impeller_Play_AiksTest_BlendModeSrcAlphaPlusAdvanced_OpenGLES.png -impeller_Play_AiksTest_BlendModeSrcAlphaPlusAdvanced_Vulkan.png impeller_Play_AiksTest_BlendModeSrcAlphaPlus_Metal.png impeller_Play_AiksTest_BlendModeSrcAlphaPlus_OpenGLES.png impeller_Play_AiksTest_BlendModeSrcAlphaPlus_Vulkan.png @@ -726,9 +720,6 @@ impeller_Play_AiksTest_SrgbToLinearFilterSubpassCollapseOptimization_Vulkan.png impeller_Play_AiksTest_StrokedCirclesRenderCorrectly_Metal.png impeller_Play_AiksTest_StrokedCirclesRenderCorrectly_OpenGLES.png impeller_Play_AiksTest_StrokedCirclesRenderCorrectly_Vulkan.png -impeller_Play_AiksTest_StrokedClosedPathDrawnCorrectly_Metal.png -impeller_Play_AiksTest_StrokedClosedPathDrawnCorrectly_OpenGLES.png -impeller_Play_AiksTest_StrokedClosedPathDrawnCorrectly_Vulkan.png impeller_Play_AiksTest_SubpassWithClearColorOptimization_Metal.png impeller_Play_AiksTest_SubpassWithClearColorOptimization_OpenGLES.png impeller_Play_AiksTest_SubpassWithClearColorOptimization_Vulkan.png From 3fff1cbe5a068e89ea286445a66628ffe9f3d9ec Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 17 Apr 2024 11:08:32 -0700 Subject: [PATCH 17/18] ++ --- impeller/geometry/path_unittests.cc | 38 +++++++++++++++-------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/impeller/geometry/path_unittests.cc b/impeller/geometry/path_unittests.cc index 012a1a8c0f349..6ffd69a1de56a 100644 --- a/impeller/geometry/path_unittests.cc +++ b/impeller/geometry/path_unittests.cc @@ -131,12 +131,12 @@ TEST(PathTest, PathCreatePolylineGeneratesCorrectContourData) { .Close() .TakePath() .CreatePolyline(1.0f); - EXPECT_EQ(polyline.points->size(), 5u); - EXPECT_EQ(polyline.contours.size(), 2u); - EXPECT_EQ(polyline.contours[0].is_closed, false); - EXPECT_EQ(polyline.contours[0].start_index, 0u); - EXPECT_EQ(polyline.contours[1].is_closed, true); - EXPECT_EQ(polyline.contours[1].start_index, 2u); + 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); + ASSERT_EQ(polyline.contours[1].is_closed, true); + ASSERT_EQ(polyline.contours[1].start_index, 2u); } TEST(PathTest, PolylineGetContourPointBoundsReturnsCorrectRanges) { @@ -154,7 +154,7 @@ TEST(PathTest, PolylineGetContourPointBoundsReturnsCorrectRanges) { ASSERT_EQ(a1, 0u); ASSERT_EQ(a2, 2u); ASSERT_EQ(b1, 2u); - ASSERT_EQ(b2, 5u); + ASSERT_EQ(b2, 6u); } TEST(PathTest, PathAddRectPolylineHasCorrectContourData) { @@ -165,11 +165,12 @@ TEST(PathTest, PathAddRectPolylineHasCorrectContourData) { 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(), 4u); + ASSERT_EQ(polyline.points->size(), 5u); 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(PathTest, PathPolylineDuplicatesAreRemovedForSameContour) { @@ -186,18 +187,19 @@ TEST(PathTest, PathPolylineDuplicatesAreRemovedForSameContour) { .LineTo({0, 100}) // Insert duplicate at end of contour. .TakePath() .CreatePolyline(1.0f); - EXPECT_EQ(polyline.contours.size(), 2u); - EXPECT_EQ(polyline.contours[0].start_index, 0u); + 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, 3u); + ASSERT_EQ(polyline.contours[1].start_index, 4u); ASSERT_FALSE(polyline.contours[1].is_closed); - ASSERT_EQ(polyline.points->size(), 6u); - EXPECT_EQ(polyline.GetPoint(0), Point(50, 50)); - EXPECT_EQ(polyline.GetPoint(1), Point(100, 50)); - EXPECT_EQ(polyline.GetPoint(2), Point(100, 100)); - EXPECT_EQ(polyline.GetPoint(3), Point(50, 50)); - EXPECT_EQ(polyline.GetPoint(4), Point(0, 50)); - EXPECT_EQ(polyline.GetPoint(5), Point(0, 100)); + ASSERT_EQ(polyline.points->size(), 7u); + 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(PathTest, PolylineBufferReuse) { From cdbb63e7712e61377622cf0c82adb4f850119400 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 17 Apr 2024 11:24:54 -0700 Subject: [PATCH 18/18] update unittests. --- impeller/tessellator/tessellator_unittests.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index a72553b9fe002..6a80b756bd8da 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -23,7 +23,9 @@ TEST(TessellatorTest, TessellateConvex) { PathBuilder{}.AddRect(Rect::MakeLTRB(0, 0, 10, 10)).TakePath(), points, indices, 1.0); - std::vector expected = {{0, 0}, {10, 0}, {10, 10}, {0, 10}}; + // Note: the origin point is repeated but not referenced in the indices + // below + std::vector expected = {{0, 0}, {10, 0}, {10, 10}, {0, 10}, {0, 0}}; std::vector expected_indices = {0, 1, 3, 2}; EXPECT_EQ(points, expected); EXPECT_EQ(indices, expected_indices); @@ -40,8 +42,9 @@ TEST(TessellatorTest, TessellateConvex) { points, indices, 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, 3, 2, 2, 4, 4, 5, 7, 6}; + {0, 0}, {20, 20}, {30, 20}, {30, 30}, + {20, 30}, {20, 20}}; + std::vector expected_indices = {0, 1, 3, 2, 2, 5, 5, 6, 8, 7}; EXPECT_EQ(points, expected); EXPECT_EQ(indices, expected_indices); }