-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Fallback to no index buffer when tesselation count is large, split up nonZero contours. #46282
[Impeller] Fallback to no index buffer when tesselation count is large, split up nonZero contours. #46282
Changes from 5 commits
2666377
42d5fef
30682b5
da27136
8f60191
d5bc00a
2ea9c22
2158807
364acc4
8804000
f65c055
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -58,6 +58,10 @@ static int ToTessWindingRule(FillType fill_type) { | |||||
| return TESS_WINDING_ODD; | ||||||
| } | ||||||
|
|
||||||
| /// @brief An arbitrary value to determine when a multi-contour non-zero fill | ||||||
| /// path should be split into multiple tessellations. | ||||||
| static constexpr size_t kMultiContourThreshold = 30u; | ||||||
|
|
||||||
| Tessellator::Result Tessellator::Tessellate( | ||||||
| FillType fill_type, | ||||||
| const Path::Polyline& polyline, | ||||||
|
|
@@ -78,51 +82,141 @@ Tessellator::Result Tessellator::Tessellate( | |||||
| 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 // | ||||||
| // If we have a larger polyline and the fill type is non-zero, we can split | ||||||
| // the tessellation up per contour. Since in general the complexity is at | ||||||
| // least nlog(n), this speeds up the processes substantially. | ||||||
| if (polyline.contours.size() > kMultiContourThreshold && | ||||||
| fill_type == FillType::kNonZero) { | ||||||
| std::vector<Point> points; | ||||||
| std::vector<float> data; | ||||||
|
|
||||||
| //---------------------------------------------------------------------------- | ||||||
| /// Feed contour information to the tessellator. | ||||||
| /// | ||||||
| size_t total = 0u; | ||||||
| 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 vertexItemCount = tessGetVertexCount(tessellator) * kVertexSize; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
These variable names don't conform to the c++ style guide.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||
| auto vertices = tessGetVertices(tessellator); | ||||||
| for (int i = 0; i < vertexItemCount; i += 2) { | ||||||
| points.emplace_back(vertices[i], vertices[i + 1]); | ||||||
| } | ||||||
|
|
||||||
| int elementItemCount = tessGetElementCount(tessellator) * kPolygonSize; | ||||||
| auto elements = tessGetElements(tessellator); | ||||||
| total += elementItemCount; | ||||||
| for (int i = 0; i < elementItemCount; i++) { | ||||||
| data.emplace_back(points[elements[i]].x); | ||||||
| data.emplace_back(points[elements[i]].y); | ||||||
| } | ||||||
| points.clear(); | ||||||
| } | ||||||
| if (!callback(data.data(), data.size(), nullptr, total)) { | ||||||
| return Result::kInputError; | ||||||
| } | ||||||
| } else { | ||||||
| //---------------------------------------------------------------------------- | ||||||
| /// 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) | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| //---------------------------------------------------------------------------- | ||||||
| /// 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 vertexItemCount = tessGetVertexCount(tessellator) * kVertexSize; | ||||||
| auto vertices = tessGetVertices(tessellator); | ||||||
| int elementItemCount = tessGetElementCount(tessellator) * kPolygonSize; | ||||||
| auto elements = tessGetElements(tessellator); | ||||||
| // libtess uses an int index internally due to usage of -1 as a sentinel | ||||||
| // value. | ||||||
| std::vector<uint16_t> indices(elementItemCount); | ||||||
| for (int i = 0; i < elementItemCount; i++) { | ||||||
| indices[i] = static_cast<uint16_t>(elements[i]); | ||||||
| } | ||||||
| if (!callback(vertices, vertexItemCount, indices.data(), elementItemCount)) { | ||||||
| return Result::kInputError; | ||||||
| if (result != 1) { | ||||||
| return Result::kTessellationError; | ||||||
| } | ||||||
|
|
||||||
| int elementItemCount = tessGetElementCount(tessellator) * kPolygonSize; | ||||||
|
jonahwilliams marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| // 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 (elementItemCount < 65535) { | ||||||
|
jonahwilliams marked this conversation as resolved.
Outdated
|
||||||
| int vertexItemCount = tessGetVertexCount(tessellator) * kVertexSize; | ||||||
| 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<uint16_t> indices(elementItemCount); | ||||||
| for (int i = 0; i < elementItemCount; i++) { | ||||||
| indices[i] = static_cast<uint16_t>(elements[i]); | ||||||
| } | ||||||
| if (!callback(vertices, vertexItemCount, indices.data(), | ||||||
| elementItemCount)) { | ||||||
| return Result::kInputError; | ||||||
| } | ||||||
| } else { | ||||||
| std::vector<Point> points; | ||||||
| std::vector<float> data; | ||||||
|
|
||||||
| int vertexItemCount = tessGetVertexCount(tessellator) * kVertexSize; | ||||||
| auto vertices = tessGetVertices(tessellator); | ||||||
| for (int i = 0; i < vertexItemCount; i += 2) { | ||||||
| points.emplace_back(vertices[i], vertices[i + 1]); | ||||||
| } | ||||||
|
|
||||||
| int elementItemCount = tessGetElementCount(tessellator) * kPolygonSize; | ||||||
| auto elements = tessGetElements(tessellator); | ||||||
| for (int i = 0; i < elementItemCount; i++) { | ||||||
| data.emplace_back(points[elements[i]].x); | ||||||
| data.emplace_back(points[elements[i]].y); | ||||||
| } | ||||||
| if (!callback(data.data(), data.size(), nullptr, elementItemCount)) { | ||||||
| return Result::kInputError; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return Result::kSuccess; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,10 +44,17 @@ 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. The [vertices_size] is the size of the vertices array | ||
| /// and not the number of vertices, which is usually vertices_size / 2. | ||
| /// In cases where the indices is nullptr, vertex_or_index_count will | ||
| /// contain the count of indices. | ||
| using BuilderCallback = std::function<bool(const float* vertices, | ||
| size_t vertices_size, | ||
| const uint16_t* indices, | ||
| size_t indices_size)>; | ||
| size_t vertex_or_index_count)>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overloading this variable doesn't seem prudent. Can't it just be indices_size and be zero if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason this is overloaded is a bit of an issue yes, I should just fix it. Really the problem is that these variables aren't counts, they are the value to multiply with sizeof(float)/sizeof(uint16_t) to compute offsets. But I just make them counts, then this problem goes away.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
|
||
| //---------------------------------------------------------------------------- | ||
| /// @brief Generates filled triangles from the polyline. A callback is | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| #include "flutter/testing/testing.h" | ||
| #include "gtest/gtest.h" | ||
| #include "impeller/geometry/path.h" | ||
| #include "impeller/geometry/path_builder.h" | ||
| #include "impeller/tessellator/tessellator.h" | ||
|
|
||
|
|
@@ -82,6 +83,52 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { | |
| ASSERT_EQ(polyline.points.size(), 2u); | ||
| ASSERT_EQ(result, Tessellator::Result::kInputError); | ||
| } | ||
|
|
||
| // More than 30 contours, non-zero fill mode. | ||
| { | ||
| Tessellator t; | ||
| PathBuilder builder = {}; | ||
| for (auto i = 0; i < 60; i++) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using a hardcoded 60, you can use the variable you created now to make this test a bit more robust to changes in that number.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| builder.AddCircle(Point(i, i), 4); | ||
| } | ||
| auto polyline = builder.TakePath().CreatePolyline(1.0f); | ||
| bool no_indices = false; | ||
| Tessellator::Result result = t.Tessellate( | ||
| FillType::kNonZero, polyline, | ||
| [&no_indices](const float* vertices, size_t vertices_size, | ||
| const uint16_t* indices, size_t indices_size) { | ||
| no_indices = indices == nullptr; | ||
| return true; | ||
| }); | ||
|
|
||
| ASSERT_TRUE(no_indices); | ||
| ASSERT_EQ(result, Tessellator::Result::kSuccess); | ||
| } | ||
|
|
||
| // More than uint16 points, odd fill mode. | ||
| { | ||
| Tessellator t; | ||
| PathBuilder builder = {}; | ||
| for (auto i = 0; i < 1000; i++) { | ||
| builder.AddCircle(Point(i, i), 4); | ||
| } | ||
| auto polyline = builder.TakePath(FillType::kOdd).CreatePolyline(1.0f); | ||
| bool no_indices = false; | ||
| size_t indices_count = 0u; | ||
| Tessellator::Result result = | ||
| t.Tessellate(FillType::kOdd, polyline, | ||
| [&no_indices, &indices_count]( | ||
| const float* vertices, size_t vertices_size, | ||
| const uint16_t* indices, size_t indices_size) { | ||
| no_indices = indices == nullptr; | ||
| indices_count = indices_size; | ||
| return true; | ||
| }); | ||
|
|
||
| ASSERT_TRUE(no_indices); | ||
| ASSERT_TRUE(indices_count >= 65535); | ||
| ASSERT_EQ(result, Tessellator::Result::kSuccess); | ||
| } | ||
| } | ||
|
|
||
| } // namespace testing | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name here belies the fact that it actually can represent 2 different things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed