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

[Impeller] use point/line primitve for hairlines. #55230

Closed
jonahwilliams wants to merge 33 commits into from
Closed

[Impeller] use point/line primitve for hairlines. #55230

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
813d4bd
[Impeller] use point/line primitve for hairlines.
Sep 8, 2024
1c362df
Merge branch 'main' of github.com:flutter/engine into primitive_geom
Sep 25, 2024
beee6ae
single contour.
Sep 25, 2024
e3cc898
add chck
Sep 25, 2024
2d8f1eb
++
Sep 26, 2024
032cb2f
Merge branch 'main' into primitive_geom
Sep 26, 2024
a968e8a
Merge branch 'main' into primitive_geom
Sep 27, 2024
ce71dab
Merge branch 'main' of github.com:flutter/engine into primitive_geom
Oct 2, 2024
544cb56
add back prevent overdraw and fix typos.
Oct 2, 2024
2f3c2a6
Merge branch 'primitive_geom' of github.com:jonahwilliams/engine into…
Oct 2, 2024
eb7f841
Merge branch 'main' into primitive_geom
Oct 4, 2024
07017d1
++
Oct 4, 2024
6ff4f7f
Merge branch 'primitive_geom' of github.com:jonahwilliams/engine into…
Oct 4, 2024
e4340b5
add jim test.
Oct 8, 2024
32779af
skip round and square caps.
Oct 10, 2024
4c75e18
Merge branch 'main' into primitive_geom
Oct 10, 2024
201b54b
++
Oct 10, 2024
0c6c969
++
Oct 11, 2024
7f98866
Merge branch 'main' of github.com:flutter/engine into primitive_geom
Oct 25, 2024
185291f
fix rrect descriptors.
Oct 25, 2024
4385efc
Merge branch 'main' of github.com:flutter/engine into primitive_geom
Oct 26, 2024
2eeee37
Merge branch 'main' of github.com:flutter/engine into primitive_geom
Oct 28, 2024
897ff2f
++
Oct 28, 2024
a91974b
use radius.
Oct 30, 2024
126f665
typo
Oct 30, 2024
b7152ee
++
Oct 30, 2024
1fd08d6
Merge branch 'main' into primitive_geom
Oct 30, 2024
9975825
++
Oct 31, 2024
d76a6cc
refactor circle position logic.
Oct 31, 2024
322ede8
Merge branch 'primitive_geom' of github.com:jonahwilliams/engine into…
Oct 31, 2024
44f5082
++
Oct 31, 2024
011e724
testing.
Oct 31, 2024
1b4da9f
++
Oct 31, 2024
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
1 change: 1 addition & 0 deletions impeller/entity/contents/filters/blend_filter_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "impeller/geometry/color.h"
#include "impeller/renderer/render_pass.h"
#include "impeller/renderer/snapshot.h"
#include "impeller/renderer/vertex_buffer_builder.h"

namespace impeller {

Expand Down
1 change: 1 addition & 0 deletions impeller/entity/contents/framebuffer_blend_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "impeller/entity/contents/content_context.h"
#include "impeller/renderer/render_pass.h"
#include "impeller/renderer/vertex_buffer_builder.h"

namespace impeller {

Expand Down
12 changes: 6 additions & 6 deletions impeller/entity/geometry/circle_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#include "flutter/impeller/entity/geometry/circle_geometry.h"

#include "flutter/impeller/entity/geometry/line_geometry.h"
#include "impeller/core/formats.h"
#include "impeller/entity/geometry/geometry.h"

namespace impeller {
Expand Down Expand Up @@ -40,11 +39,12 @@ GeometryResult CircleGeometry::GetPositionBuffer(const ContentContext& renderer,
RenderPass& pass) const {
auto& transform = entity.GetTransform();

Scalar half_width = stroke_width_ < 0
? 0.0
: LineGeometry::ComputePixelHalfWidth(
transform, stroke_width_,
pass.GetSampleCount() == SampleCount::kCount4);
Scalar half_width = 0;
if (stroke_width_ > 0) {
auto [width, _] = LineGeometry::ComputePixelHalfWidth(
transform.GetMaxBasisLengthXY(), stroke_width_);
half_width = width;
}
Copy link
Contributor

@flar flar Oct 28, 2024

Choose a reason for hiding this comment

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

I was wondering why we pass along the 0.0 here and just noticed a bug.

The StrokedCircle generator below says it will convert to a filled circle if the inner radius is <= 0, but if you look at the implementation it converts if the half_width is < 0... which I don't think happens ever? So, there's a bug there.

But, fixing that bug or not, it calls into question why we pass along a 0 in the first place. Why isn't <=0 clamped at the minimum pixel width?

We can file a bug against the StrokedCircle generator for fixing separately, but I think the passing along of a 0 here results in a thin circle turning into a filled circle (whereas passing in the min_width would not), so this calculation is suspect.

Copy link
Contributor

@flar flar Oct 28, 2024

Choose a reason for hiding this comment

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

And if we don't pass it along to the stroke generator, does it generate an empty shape when tessellating?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 30, 2024

Choose a reason for hiding this comment

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

Is the comment just wrong? a quick read through and I think this code is doing reasonable things? half_width is always 0 for fills though its confusing that we go through the stroke code to get there...

Copy link
Contributor

@flar flar Oct 30, 2024

Choose a reason for hiding this comment

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

I think I see now, this is totally confusing. So the local field stroke_width_ is set to -1 to indicate a filled circle. The ?: code used to convert this to 0 and the half_width would always be >0 for the other case (stroked). So, yes, for the truly filled case we are calling a method called StrokedCircle which is odd and confusing, but the comment indicates that the called method will also do a filled circle for the case where the inner radius disappears - and it does no such thing.

Copy link
Contributor

@flar flar Oct 30, 2024

Choose a reason for hiding this comment

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

This code would make a lot more sense if there was a more explicit indicator of filled and it simply said:

if (filled) {
  generator = FilledCircle(...)
} else {
  half_width = ...;
  generator = StrokedCircle(..., half_width);
}

Fixing StrokedCircle to detect the degenerate case would be a separate fix, but until then, the comment below is just a red herring, maybe if it said "should simplify" instead of "will simplify"...

Copy link
Contributor

@flar flar Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Or, it would be OK if that pseudo-code used "stroke_width_ < 0" as the test, as long as it doesn't try to fold the cases together via strange values in half_width...

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 31, 2024

Choose a reason for hiding this comment

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

I re-arranged this code.


const std::shared_ptr<Tessellator>& tessellator = renderer.GetTessellator();

Expand Down
3 changes: 1 addition & 2 deletions impeller/entity/geometry/circle_geometry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@

namespace impeller {

// Geometry class that can generate vertices (with or without texture
// coordinates) for either filled or stroked circles
/// @brief Generator for vertices or either filled or stroked circles
Copy link
Contributor

@flar flar Sep 30, 2024

Choose a reason for hiding this comment

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

"for vertices or"? "for vertices for"?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 2, 2024

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@flar flar Oct 30, 2024

Choose a reason for hiding this comment

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

It still looks odd. "vertices or either"? Should this be

"Generator for vertices for either filled or stroked circles"?

Or, more simply

"Vertex generator for either filled or stroked circles"?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 30, 2024

Choose a reason for hiding this comment

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

That's better, I'll use that

class CircleGeometry final : public Geometry {
public:
explicit CircleGeometry(const Point& center, Scalar radius);
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/geometry/fill_path_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ GeometryResult FillPathGeometry::GetPositionBuffer(
}

VertexBuffer vertex_buffer = renderer.GetTessellator()->TessellateConvex(
path_, host_buffer, entity.GetTransform().GetMaxBasisLength());
path_, host_buffer, entity.GetTransform().GetMaxBasisLengthXY());

return GeometryResult{
.type = PrimitiveType::kTriangleStrip,
Expand Down
6 changes: 2 additions & 4 deletions impeller/entity/geometry/geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,11 @@ bool Geometry::CanApplyMaskFilter() const {
Scalar Geometry::ComputeStrokeAlphaCoverage(const Matrix& transform,
Scalar stroke_width) {
Scalar scaled_stroke_width = transform.GetMaxBasisLengthXY() * stroke_width;
// If the stroke width is 0 or greater than kMinStrokeSizeMSAA, don't apply
// any additional alpha. This is intended to match Skia behavior.
if (scaled_stroke_width == 0.0 || scaled_stroke_width >= kMinStrokeSizeMSAA) {
if (scaled_stroke_width == 0.0 || scaled_stroke_width >= kMinStrokeSize) {
return 1.0;
}
// This scalling is eyeballed from Skia.
return std::clamp(scaled_stroke_width * 2.0f, 0.f, 1.f);
return std::clamp(scaled_stroke_width, 0.1f, 1.f);
Copy link
Contributor

@flar flar Oct 28, 2024

Choose a reason for hiding this comment

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

I'm still confused on this code.

So, 0.0 means 1.0.
And >= min stroke size means 1.0
But there is a range between 0 and min_stroke where the alpha is <1?

That would create a visual break when reducing the width down to 0, would it not?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Oct 30, 2024

Choose a reason for hiding this comment

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

Yes there will be a visual break, but this is the behavior of skia. 0 means explicitly 1 pixel, whereas smaller widths are simulated by modulating alpha.

}

} // namespace impeller
6 changes: 0 additions & 6 deletions impeller/entity/geometry/geometry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,11 @@
#include "impeller/entity/contents/content_context.h"
#include "impeller/entity/entity.h"
#include "impeller/renderer/render_pass.h"
#include "impeller/renderer/vertex_buffer_builder.h"

namespace impeller {

class Tessellator;

/// @brief The minimum stroke size can be less than one physical pixel because
/// of MSAA, but no less that half a physical pixel otherwise we might
/// not hit one of the sample positions.
static constexpr Scalar kMinStrokeSizeMSAA = 0.5f;

static constexpr Scalar kMinStrokeSize = 1.0f;

struct GeometryResult {
Expand Down
20 changes: 10 additions & 10 deletions impeller/entity/geometry/geometry_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,13 @@ namespace impeller {

class ImpellerEntityUnitTestAccessor {
public:
static std::vector<SolidFillVertexShader::PerVertexData>
GenerateSolidStrokeVertices(const Path::Polyline& polyline,
Scalar stroke_width,
Scalar miter_limit,
Join stroke_join,
Cap stroke_cap,
Scalar scale) {
static std::vector<Point> GenerateSolidStrokeVertices(
const Path::Polyline& polyline,
Scalar stroke_width,
Scalar miter_limit,
Join stroke_join,
Cap stroke_cap,
Scalar scale) {
return StrokePathGeometry::GenerateSolidStrokeVertices(
polyline, stroke_width, miter_limit, stroke_join, stroke_cap, scale);
}
Expand Down Expand Up @@ -140,14 +140,14 @@ TEST(EntityGeometryTest, AlphaCoverageStrokePaths) {
auto matrix = Matrix::MakeScale(Vector2{3.0, 3.0});
EXPECT_EQ(Geometry::MakeStrokePath({}, 0.5)->ComputeAlphaCoverage(matrix), 1);
EXPECT_NEAR(Geometry::MakeStrokePath({}, 0.1)->ComputeAlphaCoverage(matrix),
0.6, 0.05);
EXPECT_NEAR(Geometry::MakeStrokePath({}, 0.05)->ComputeAlphaCoverage(matrix),
0.3, 0.05);
EXPECT_NEAR(Geometry::MakeStrokePath({}, 0.05)->ComputeAlphaCoverage(matrix),
0.15, 0.05);
EXPECT_NEAR(Geometry::MakeStrokePath({}, 0.01)->ComputeAlphaCoverage(matrix),
0.1, 0.1);
EXPECT_NEAR(
Geometry::MakeStrokePath({}, 0.0000005)->ComputeAlphaCoverage(matrix),
1e-05, 0.001);
0.10, 0.001);
EXPECT_EQ(Geometry::MakeStrokePath({}, 0)->ComputeAlphaCoverage(matrix), 1);
EXPECT_EQ(Geometry::MakeStrokePath({}, 40)->ComputeAlphaCoverage(matrix), 1);
}
Expand Down
64 changes: 37 additions & 27 deletions impeller/entity/geometry/line_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.

#include "impeller/entity/geometry/line_geometry.h"
#include "impeller/core/formats.h"
#include "impeller/entity/geometry/geometry.h"

namespace impeller {
Expand All @@ -12,22 +13,15 @@ LineGeometry::LineGeometry(Point p0, Point p1, Scalar width, Cap cap)
FML_DCHECK(width >= 0);
}

Scalar LineGeometry::ComputePixelHalfWidth(const Matrix& transform,
Scalar width,
bool msaa) {
Scalar max_basis = transform.GetMaxBasisLengthXY();
if (max_basis == 0) {
return {};
}

Scalar min_size = (msaa ? kMinStrokeSize : kMinStrokeSizeMSAA) / max_basis;
return std::max(width, min_size) * 0.5f;
std::pair<Scalar, bool> LineGeometry::ComputePixelHalfWidth(Scalar max_basis,
Scalar width) {
Scalar min_size = kMinStrokeSize / max_basis;
Copy link
Contributor

@flar flar Sep 30, 2024

Choose a reason for hiding this comment

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

What if max_basis is 0? There used to be protection for that...

Copy link
Contributor

@flar flar Sep 30, 2024

Choose a reason for hiding this comment

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

Maybe returning 0 wasn't the answer, but will returning inf cause issues?

Copy link
Contributor

@flar flar Sep 30, 2024

Choose a reason for hiding this comment

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

Hmmm, if the basis is 0, we might not get here if we notice the degenerate transform elsewhere, but are we sure that happens?

Copy link
Contributor

@flar flar Sep 30, 2024

Choose a reason for hiding this comment

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

PointFieldGeometry also bails on basis == 0.

return std::make_pair(std::max(width, min_size) * 0.5f, width <= min_size);
Copy link
Contributor

@flar flar Oct 28, 2024

Choose a reason for hiding this comment

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

This method is called "compute pixel ...", but it is returning a local space width (same as before). A confusing name, but one we've been living with already...

}

Vector2 LineGeometry::ComputeAlongVector(const Matrix& transform,
bool allow_zero_length,
bool msaa) const {
Scalar stroke_half_width = ComputePixelHalfWidth(transform, width_, msaa);
Vector2 LineGeometry::ComputeAlongVector(Scalar max_basis,
bool allow_zero_length) const {
auto [stroke_half_width, _] = ComputePixelHalfWidth(max_basis, width_);
if (stroke_half_width < kEhCloseEnough) {
return {};
}
Expand All @@ -46,10 +40,9 @@ Vector2 LineGeometry::ComputeAlongVector(const Matrix& transform,
}

bool LineGeometry::ComputeCorners(Point corners[4],
const Matrix& transform,
bool extend_endpoints,
bool msaa) const {
auto along = ComputeAlongVector(transform, extend_endpoints, msaa);
Scalar max_basis,
bool extend_endpoints) const {
auto along = ComputeAlongVector(max_basis, extend_endpoints);
if (along.IsZero()) {
return false;
}
Expand Down Expand Up @@ -77,24 +70,41 @@ GeometryResult LineGeometry::GetPositionBuffer(const ContentContext& renderer,
RenderPass& pass) const {
using VT = SolidFillVertexShader::PerVertexData;

auto& transform = entity.GetTransform();
auto radius = ComputePixelHalfWidth(
transform, width_, pass.GetSampleCount() == SampleCount::kCount4);
Scalar max_basis = entity.GetTransform().GetMaxBasisLengthXY();
auto& host_buffer = renderer.GetTransientsBuffer();

if (max_basis == 0) {
return {};
}

auto [radius, is_harline] = ComputePixelHalfWidth(max_basis, width_);

// This is a harline stroke and can be drawn directly with line primitives,
// which avoids extra tessellation work, cap/joins, and overdraw prevention.
if (is_harline) {
Point points[2] = {p0_, p1_};
return GeometryResult{
.type = PrimitiveType::kLineStrip,
.vertex_buffer = {.vertex_buffer = host_buffer.Emplace(
points, sizeof(points), alignof(Point)),
.vertex_count = 2,
.index_type = IndexType::kNone},
.transform = entity.GetShaderTransform(pass),
};
}

if (cap_ == Cap::kRound) {
auto& transform = entity.GetTransform();
std::shared_ptr<Tessellator> tessellator = renderer.GetTessellator();
auto generator = tessellator->RoundCapLine(transform, p0_, p1_, radius);
return ComputePositionGeometry(renderer, generator, entity, pass);
}

Point corners[4];
if (!ComputeCorners(corners, transform, cap_ == Cap::kSquare,
pass.GetSampleCount() == SampleCount::kCount4)) {
if (!ComputeCorners(corners, max_basis, cap_ == Cap::kSquare)) {
return kEmptyResult;
}

auto& host_buffer = renderer.GetTransientsBuffer();

size_t count = 4;
BufferView vertex_buffer = host_buffer.Emplace(
count * sizeof(VT), alignof(VT), [&corners](uint8_t* buffer) {
Expand All @@ -120,8 +130,8 @@ GeometryResult LineGeometry::GetPositionBuffer(const ContentContext& renderer,

std::optional<Rect> LineGeometry::GetCoverage(const Matrix& transform) const {
Point corners[4];
// Note: MSAA boolean doesn't matter for coverage computation.
if (!ComputeCorners(corners, transform, cap_ != Cap::kButt, /*msaa=*/false)) {
if (!ComputeCorners(corners, transform.GetMaxBasisLengthXY(),
cap_ != Cap::kButt)) {
return {};
}

Expand Down
14 changes: 5 additions & 9 deletions impeller/entity/geometry/line_geometry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,8 @@ class LineGeometry final : public Geometry {

~LineGeometry() = default;

static Scalar ComputePixelHalfWidth(const Matrix& transform,
Scalar width,
bool msaa);
static std::pair<Scalar, bool> ComputePixelHalfWidth(Scalar max_basis,
Scalar width);

// |Geometry|
bool CoversArea(const Matrix& transform, const Rect& rect) const override;
Expand All @@ -44,13 +43,10 @@ class LineGeometry final : public Geometry {
//
// @return true if the transform and width were not degenerate
bool ComputeCorners(Point corners[4],
const Matrix& transform,
bool extend_endpoints,
bool msaa) const;
Scalar max_basis,
bool extend_endpoints) const;

Vector2 ComputeAlongVector(const Matrix& transform,
bool allow_zero_length,
bool msaa) const;
Vector2 ComputeAlongVector(Scalar max_basis, bool allow_zero_length) const;

// |Geometry|
GeometryResult GetPositionBuffer(const ContentContext& renderer,
Expand Down
16 changes: 15 additions & 1 deletion impeller/entity/geometry/point_field_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "impeller/core/formats.h"
#include "impeller/entity/geometry/geometry.h"
#include "impeller/renderer/command_buffer.h"
#include "impeller/renderer/vertex_buffer_builder.h"

namespace impeller {

Expand All @@ -23,15 +24,28 @@ GeometryResult PointFieldGeometry::GetPositionBuffer(
return {};
}
const Matrix& transform = entity.GetTransform();
HostBuffer& host_buffer = renderer.GetTransientsBuffer();

Scalar max_basis = transform.GetMaxBasisLengthXY();
if (max_basis == 0) {
return {};
}

Scalar min_size = 0.5f / max_basis;
Scalar radius = std::max(radius_, min_size);
if (radius_ <= min_size) {
// Hairline points can be drawn with the point primitive.
return GeometryResult{
.type = PrimitiveType::kPoint,
.vertex_buffer = {.vertex_buffer = host_buffer.Emplace(
points_.data(), sizeof(Point) * points_.size(),
alignof(Point)),
.vertex_count = points_.size(),
.index_type = IndexType::kNone},
.transform = entity.GetShaderTransform(pass),
};
}

HostBuffer& host_buffer = renderer.GetTransientsBuffer();
VertexBufferBuilder<SolidFillVertexShader::PerVertexData> vtx_builder;
if (round_) {
// Get triangulation relative to {0, 0} so we can translate it to each
Expand Down
Loading