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

[Impeller] Change Path::CreatePolyline from tolerance to scale, and make it required #39917

Merged
merged 2 commits into from
Feb 27, 2023
Merged

[Impeller] Change Path::CreatePolyline from tolerance to scale, and make it required #39917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8cbf8e0
Change Path::CreatePolyline from tolerance to scale, and make it non-…
dnfield Feb 27, 2023
16e07ae
doc nit
dnfield Feb 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion impeller/entity/contents/texture_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ bool TextureContents::Render(const ContentContext& renderer,
VertexBufferBuilder<VS::PerVertexData> vertex_builder;
{
const auto tess_result = renderer.GetTessellator()->Tessellate(
path_.GetFillType(), path_.CreatePolyline(),
path_.GetFillType(), path_.CreatePolyline(1.0f),
[this, &vertex_builder, &coverage_rect, &texture_size](
const float* vertices, size_t vertices_size,
const uint16_t* indices, size_t indices_size) {
Expand Down
47 changes: 19 additions & 28 deletions impeller/entity/geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,11 @@ GeometryResult FillPathGeometry::GetPositionBuffer(
const ContentContext& renderer,
const Entity& entity,
RenderPass& pass) {
auto tolerance =
kDefaultCurveTolerance / entity.GetTransformation().GetMaxBasisLength();

VertexBuffer vertex_buffer;
auto& host_buffer = pass.GetTransientsBuffer();
auto tesselation_result = renderer.GetTessellator()->Tessellate(
path_.GetFillType(), path_.CreatePolyline(tolerance),
path_.GetFillType(),
path_.CreatePolyline(entity.GetTransformation().GetMaxBasisLength()),
[&vertex_buffer, &host_buffer](
const float* vertices, size_t vertices_count, const uint16_t* indices,
size_t indices_count) {
Expand Down Expand Up @@ -151,7 +149,7 @@ StrokePathGeometry::JoinProc StrokePathGeometry::GetJoinProc(Join stroke_join) {
join_proc = [](VertexBufferBuilder<VS::PerVertexData>& vtx_builder,
const Point& position, const Point& start_offset,
const Point& end_offset, Scalar miter_limit,
Scalar tolerance) {
Scalar scale) {
CreateBevelAndGetDirection(vtx_builder, position, start_offset,
end_offset);
};
Expand All @@ -160,7 +158,7 @@ StrokePathGeometry::JoinProc StrokePathGeometry::GetJoinProc(Join stroke_join) {
join_proc = [](VertexBufferBuilder<VS::PerVertexData>& vtx_builder,
const Point& position, const Point& start_offset,
const Point& end_offset, Scalar miter_limit,
Scalar tolerance) {
Scalar scale) {
Point start_normal = start_offset.Normalize();
Point end_normal = end_offset.Normalize();

Expand Down Expand Up @@ -189,7 +187,7 @@ StrokePathGeometry::JoinProc StrokePathGeometry::GetJoinProc(Join stroke_join) {
join_proc = [](VertexBufferBuilder<VS::PerVertexData>& vtx_builder,
const Point& position, const Point& start_offset,
const Point& end_offset, Scalar miter_limit,
Scalar tolerance) {
Scalar scale) {
Point start_normal = start_offset.Normalize();
Point end_normal = end_offset.Normalize();

Expand All @@ -216,7 +214,7 @@ StrokePathGeometry::JoinProc StrokePathGeometry::GetJoinProc(Join stroke_join) {

auto arc_points = CubicPathComponent(start_offset, start_handle,
middle_handle, middle)
.CreatePolyline(tolerance);
.CreatePolyline(scale);

VS::PerVertexData vtx;
for (const auto& point : arc_points) {
Expand All @@ -238,8 +236,7 @@ StrokePathGeometry::CapProc StrokePathGeometry::GetCapProc(Cap stroke_cap) {
switch (stroke_cap) {
case Cap::kButt:
cap_proc = [](VertexBufferBuilder<VS::PerVertexData>& vtx_builder,
const Point& position, const Point& offset,
Scalar tolerance) {
const Point& position, const Point& offset, Scalar scale) {
VS::PerVertexData vtx;
vtx.position = position + offset;
vtx_builder.AppendVertex(vtx);
Expand All @@ -249,8 +246,7 @@ StrokePathGeometry::CapProc StrokePathGeometry::GetCapProc(Cap stroke_cap) {
break;
case Cap::kRound:
cap_proc = [](VertexBufferBuilder<VS::PerVertexData>& vtx_builder,
const Point& position, const Point& offset,
Scalar tolerance) {
const Point& position, const Point& offset, Scalar scale) {
VS::PerVertexData vtx;

Point forward(offset.y, -offset.x);
Expand All @@ -260,7 +256,7 @@ StrokePathGeometry::CapProc StrokePathGeometry::GetCapProc(Cap stroke_cap) {
CubicPathComponent(
offset, offset + forward * PathBuilder::kArcApproximationMagic,
forward + offset * PathBuilder::kArcApproximationMagic, forward)
.CreatePolyline(tolerance);
.CreatePolyline(scale);

vtx.position = position + offset;
vtx_builder.AppendVertex(vtx);
Expand All @@ -276,8 +272,7 @@ StrokePathGeometry::CapProc StrokePathGeometry::GetCapProc(Cap stroke_cap) {
break;
case Cap::kSquare:
cap_proc = [](VertexBufferBuilder<VS::PerVertexData>& vtx_builder,
const Point& position, const Point& offset,
Scalar tolerance) {
const Point& position, const Point& offset, Scalar scale) {
VS::PerVertexData vtx;

Point forward(offset.y, -offset.x);
Expand Down Expand Up @@ -305,9 +300,9 @@ VertexBuffer StrokePathGeometry::CreateSolidStrokeVertices(
Cap cap,
const StrokePathGeometry::JoinProc& join_proc,
const StrokePathGeometry::CapProc& cap_proc,
Scalar tolerance) {
Scalar scale) {
VertexBufferBuilder<VS::PerVertexData> vtx_builder;
auto polyline = path.CreatePolyline();
auto polyline = path.CreatePolyline(scale);

VS::PerVertexData vtx;

Expand All @@ -333,8 +328,8 @@ VertexBuffer StrokePathGeometry::CreateSolidStrokeVertices(
switch (contour_end_point_i - contour_start_point_i) {
case 1: {
Point p = polyline.points[contour_start_point_i];
cap_proc(vtx_builder, p, {-stroke_width * 0.5f, 0}, tolerance);
cap_proc(vtx_builder, p, {stroke_width * 0.5f, 0}, tolerance);
cap_proc(vtx_builder, p, {-stroke_width * 0.5f, 0}, scale);
cap_proc(vtx_builder, p, {stroke_width * 0.5f, 0}, scale);
continue;
}
case 0:
Expand Down Expand Up @@ -381,7 +376,7 @@ VertexBuffer StrokePathGeometry::CreateSolidStrokeVertices(
}
auto cap_offset = direction * stroke_width * 0.5;
cap_proc(vtx_builder, polyline.points[contour_start_point_i], cap_offset,
tolerance);
scale);
}

// Generate contour geometry.
Expand All @@ -402,7 +397,7 @@ VertexBuffer StrokePathGeometry::CreateSolidStrokeVertices(

// Generate join from the current line to the next line.
join_proc(vtx_builder, polyline.points[point_i], previous_offset,
offset, scaled_miter_limit, tolerance);
offset, scaled_miter_limit, scale);
}
}

Expand All @@ -412,10 +407,10 @@ VertexBuffer StrokePathGeometry::CreateSolidStrokeVertices(
Vector2(-contour.end_direction.y, contour.end_direction.x) *
stroke_width * 0.5;
cap_proc(vtx_builder, polyline.points[contour_end_point_i - 1],
cap_offset, tolerance);
cap_offset, scale);
} else {
join_proc(vtx_builder, polyline.points[contour_start_point_i], offset,
contour_first_offset, scaled_miter_limit, tolerance);
contour_first_offset, scaled_miter_limit, scale);
}
}

Expand All @@ -437,15 +432,11 @@ GeometryResult StrokePathGeometry::GetPositionBuffer(
Scalar min_size = 1.0f / sqrt(std::abs(determinant));
Scalar stroke_width = std::max(stroke_width_, min_size);

auto tolerance =
kDefaultCurveTolerance /
(stroke_width_ * entity.GetTransformation().GetMaxBasisLength());

auto& host_buffer = pass.GetTransientsBuffer();
auto vertex_buffer = CreateSolidStrokeVertices(
path_, host_buffer, stroke_width, miter_limit_ * stroke_width_ * 0.5,
stroke_cap_, GetJoinProc(stroke_join_), GetCapProc(stroke_cap_),
tolerance);
entity.GetTransformation().GetMaxBasisLength());

return GeometryResult{
.type = PrimitiveType::kTriangleStrip,
Expand Down
6 changes: 3 additions & 3 deletions impeller/entity/geometry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,14 @@ class StrokePathGeometry : public Geometry {
std::function<void(VertexBufferBuilder<VS::PerVertexData>& vtx_builder,
const Point& position,
const Point& offset,
Scalar tolerance)>;
Scalar scale)>;
using JoinProc =
std::function<void(VertexBufferBuilder<VS::PerVertexData>& vtx_builder,
const Point& position,
const Point& start_offset,
const Point& end_offset,
Scalar miter_limit,
Scalar tolerance)>;
Scalar scale)>;

// |Geometry|
GeometryResult GetPositionBuffer(const ContentContext& renderer,
Expand All @@ -167,7 +167,7 @@ class StrokePathGeometry : public Geometry {
Cap cap,
const JoinProc& join_proc,
const CapProc& cap_proc,
Scalar tolerance);
Scalar scale);

static StrokePathGeometry::JoinProc GetJoinProc(Join stroke_join);

Expand Down
2 changes: 1 addition & 1 deletion impeller/geometry/geometry_benchmarks.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ static void BM_Polyline(benchmark::State& state, Args&&... args) {
size_t point_count = 0u;
size_t single_point_count = 0u;
while (state.KeepRunning()) {
auto polyline = path.CreatePolyline();
auto polyline = path.CreatePolyline(1.0f);
single_point_count = polyline.points.size();
point_count += single_point_count;
if (tessellate) {
Expand Down
13 changes: 6 additions & 7 deletions impeller/geometry/geometry_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include "impeller/geometry/rect.h"
#include "impeller/geometry/scalar.h"
#include "impeller/geometry/size.h"
#include "path_component.h"

namespace impeller {
namespace testing {
Expand Down Expand Up @@ -577,7 +576,7 @@ TEST(GeometryTest, EmptyPath) {
path.GetContourComponentAtIndex(0, c);
ASSERT_POINT_NEAR(c.destination, Point());

Path::Polyline polyline = path.CreatePolyline();
Path::Polyline polyline = path.CreatePolyline(1.0f);
ASSERT_TRUE(polyline.points.empty());
ASSERT_TRUE(polyline.contours.empty());
}
Expand Down Expand Up @@ -1596,7 +1595,7 @@ TEST(GeometryTest, PathCreatePolyLineDoesNotDuplicatePoints) {
path.AddContourComponent({40, 40});
path.AddLinearComponent({40, 40}, {50, 50});

auto polyline = path.CreatePolyline();
auto polyline = path.CreatePolyline(1.0f);

ASSERT_EQ(polyline.contours.size(), 2u);
ASSERT_EQ(polyline.points.size(), 5u);
Expand Down Expand Up @@ -1682,7 +1681,7 @@ TEST(GeometryTest, PathCreatePolylineGeneratesCorrectContourData) {
.LineTo({200, 200})
.Close()
.TakePath()
.CreatePolyline();
.CreatePolyline(1.0f);
ASSERT_EQ(polyline.points.size(), 6u);
ASSERT_EQ(polyline.contours.size(), 2u);
ASSERT_EQ(polyline.contours[0].is_closed, false);
Expand All @@ -1699,7 +1698,7 @@ TEST(GeometryTest, PolylineGetContourPointBoundsReturnsCorrectRanges) {
.LineTo({200, 200})
.Close()
.TakePath()
.CreatePolyline();
.CreatePolyline(1.0f);
size_t a1, a2, b1, b2;
std::tie(a1, a2) = polyline.GetContourPointBounds(0);
std::tie(b1, b2) = polyline.GetContourPointBounds(1);
Expand All @@ -1713,7 +1712,7 @@ TEST(GeometryTest, PathAddRectPolylineHasCorrectContourData) {
Path::Polyline polyline = PathBuilder{}
.AddRect(Rect::MakeLTRB(50, 60, 70, 80))
.TakePath()
.CreatePolyline();
.CreatePolyline(1.0f);
ASSERT_EQ(polyline.contours.size(), 1u);
ASSERT_TRUE(polyline.contours[0].is_closed);
ASSERT_EQ(polyline.contours[0].start_index, 0u);
Expand All @@ -1738,7 +1737,7 @@ TEST(GeometryTest, PathPolylineDuplicatesAreRemovedForSameContour) {
.LineTo({0, 100})
.LineTo({0, 100}) // Insert duplicate at end of contour.
.TakePath()
.CreatePolyline();
.CreatePolyline(1.0f);
ASSERT_EQ(polyline.contours.size(), 2u);
ASSERT_EQ(polyline.contours[0].start_index, 0u);
ASSERT_TRUE(polyline.contours[0].is_closed);
Expand Down
4 changes: 3 additions & 1 deletion impeller/geometry/path.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <optional>

#include "impeller/geometry/path_component.h"
#include "path_component.h"

namespace impeller {

Expand Down Expand Up @@ -221,8 +222,9 @@ bool Path::UpdateContourComponentAtIndex(size_t index,
return true;
}

Path::Polyline Path::CreatePolyline(Scalar tolerance) const {
Path::Polyline Path::CreatePolyline(Scalar scale) const {
Polyline polyline;
auto tolerance = kDefaultCurveTolerance / scale;
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, it's acceptable for tolerance to end up being NaN (when the max basis length is 0). Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, curves ending reverting to lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#38497 (comment) is where you discussed this before


std::optional<Point> previous_contour_point;
auto collect_points = [&polyline, &previous_contour_point](
Expand Down
8 changes: 7 additions & 1 deletion impeller/geometry/path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <vector>

#include "impeller/geometry/path_component.h"
#include "path_component.h"

namespace impeller {

Expand Down Expand Up @@ -122,7 +123,12 @@ class Path {
bool UpdateContourComponentAtIndex(size_t index,
const ContourComponent& contour);

Polyline CreatePolyline(Scalar tolerance = kDefaultCurveTolerance) const;
/// Callers must provide the scale factor for how this path will be
/// 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) const;

std::optional<Rect> GetBoundingBox() const;

Expand Down
2 changes: 1 addition & 1 deletion impeller/renderer/renderer_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ TEST_P(RendererTest, CanRenderInstanced) {
PathBuilder{}
.AddRect(Rect::MakeXYWH(10, 10, 100, 100))
.TakePath()
.CreatePolyline(),
.CreatePolyline(1.0f),
[&builder](const float* vertices, size_t vertices_size,
const uint16_t* indices, size_t indices_size) {
for (auto i = 0u; i < vertices_size; i += 2) {
Expand Down
11 changes: 6 additions & 5 deletions impeller/tessellator/tessellator_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) {
// Zero points.
{
Tessellator t;
auto polyline = PathBuilder{}.TakePath().CreatePolyline();
auto polyline = PathBuilder{}.TakePath().CreatePolyline(1.0f);
Tessellator::Result result = t.Tessellate(
FillType::kPositive, polyline,
[](const float* vertices, size_t vertices_size, const uint16_t* indices,
Expand All @@ -27,7 +27,8 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) {
// One point.
{
Tessellator t;
auto polyline = PathBuilder{}.LineTo({0, 0}).TakePath().CreatePolyline();
auto polyline =
PathBuilder{}.LineTo({0, 0}).TakePath().CreatePolyline(1.0f);
Tessellator::Result result = t.Tessellate(
FillType::kPositive, polyline,
[](const float* vertices, size_t vertices_size, const uint16_t* indices,
Expand All @@ -40,7 +41,7 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) {
{
Tessellator t;
auto polyline =
PathBuilder{}.AddLine({0, 0}, {0, 1}).TakePath().CreatePolyline();
PathBuilder{}.AddLine({0, 0}, {0, 1}).TakePath().CreatePolyline(1.0f);
Tessellator::Result result = t.Tessellate(
FillType::kPositive, polyline,
[](const float* vertices, size_t vertices_size, const uint16_t* indices,
Expand All @@ -58,7 +59,7 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) {
auto coord = i * 1.0f;
builder.AddLine({coord, coord}, {coord + 1, coord + 1});
}
auto polyline = builder.TakePath().CreatePolyline();
auto polyline = builder.TakePath().CreatePolyline(1.0f);
Tessellator::Result result = t.Tessellate(
FillType::kPositive, polyline,
[](const float* vertices, size_t vertices_size, const uint16_t* indices,
Expand All @@ -72,7 +73,7 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) {
{
Tessellator t;
auto polyline =
PathBuilder{}.AddLine({0, 0}, {0, 1}).TakePath().CreatePolyline();
PathBuilder{}.AddLine({0, 0}, {0, 1}).TakePath().CreatePolyline(1.0f);
Tessellator::Result result = t.Tessellate(
FillType::kPositive, polyline,
[](const float* vertices, size_t vertices_size, const uint16_t* indices,
Expand Down