Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 27 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
174 changes: 174 additions & 0 deletions impeller/display_list/aiks_dl_basic_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1529,6 +1529,180 @@ TEST_P(AiksTest, PipelineBlendSingleParameter) {
ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
}

TEST_P(AiksTest, HorizontalHairlinesPixelRegistration) {
const DlStrokeCap caps[] = {
DlStrokeCap::kButt,
DlStrokeCap::kSquare,
DlStrokeCap::kRound,
};

DlPaint stroke_paint;
stroke_paint.setDrawStyle(DlDrawStyle::kStroke);
stroke_paint.setStrokeWidth(0.0f);
DlPaint fill_paint;
fill_paint.setDrawStyle(DlDrawStyle::kFill);

const int pad = 5;
const int line_length = 20;
const int x_pad = line_length + pad;
const int y_pad = pad;
const int x_test_offset = x_pad * 2 + pad;
const int y_test_offset = y_pad + pad * 2;

auto draw_one = [&stroke_paint, &fill_paint](DlCanvas& canvas,
DlScalar x_base, DlScalar y_base,
DlScalar cap_pad) {
DlScalar x0 = x_base + cap_pad;
DlScalar x1 = x0 + line_length;
DlScalar y0 = y_base + 0.5f;
DlScalar y1 = y0;

SkRect expected_rect =
SkRect::MakeLTRB(x0 - cap_pad, y0 - 0.5f, x1 + cap_pad, y1 + 0.5f);
SkPath expected_path = SkPath::Line({x0, y0}, {x1, y1});

canvas.DrawLine(DlPoint{x0, y0}, DlPoint{x1, y1}, stroke_paint);
if (stroke_paint.getStrokeCap() == DlStrokeCap::kRound) {
SkRRect expected_rrect =
SkRRect::MakeRectXY(expected_rect, cap_pad, cap_pad);
canvas.DrawRRect(expected_rrect.makeOffset(x_pad, 0), fill_paint);
canvas.DrawRRect(expected_rrect.makeOffset(0, y_pad), fill_paint);
} else {
canvas.DrawRect(expected_rect.makeOffset(x_pad, 0), fill_paint);
canvas.DrawRect(expected_rect.makeOffset(0, y_pad), fill_paint);
}
canvas.DrawPath(expected_path.offset(x_pad, y_pad), stroke_paint);
};

DisplayListBuilder builder;
builder.DrawColor(DlColor::kWhite(), DlBlendMode::kSrc);

DlScalar x_base = pad;
for (auto cap : caps) {
DlScalar cap_pad;
DlColor color;
switch (cap) {
case flutter::DlStrokeCap::kButt:
color = DlColor::kBlack();
cap_pad = 0.0f;
break;
case flutter::DlStrokeCap::kSquare:
color = DlColor::kBlue();
cap_pad = 0.5f;
break;
case flutter::DlStrokeCap::kRound:
color = DlColor::kGreen();
cap_pad = 0.5f;
break;
}
fill_paint.setColor(color);
stroke_paint.setStrokeCap(cap);
stroke_paint.setColor(color);
DlScalar y_base = pad;
for (int i = 0; i <= 10; i++) {
DlScalar subpixel_offset = (i / 10.0f);

DlScalar x = x_base;
draw_one(builder, x + subpixel_offset, y_base, cap_pad);
x += x_test_offset;
draw_one(builder, x, y_base + subpixel_offset, cap_pad);

y_base += y_test_offset;
}
x_base += x_test_offset * 2 + pad * 2;
}

auto dl = builder.Build();
ASSERT_TRUE(OpenPlaygroundHere(dl));
}

TEST_P(AiksTest, VerticalHairlinesPixelRegistration) {
const DlStrokeCap caps[] = {
DlStrokeCap::kButt,
DlStrokeCap::kSquare,
DlStrokeCap::kRound,
};

DlPaint stroke_paint;
stroke_paint.setDrawStyle(DlDrawStyle::kStroke);
stroke_paint.setStrokeWidth(0.0f);
DlPaint fill_paint;
fill_paint.setDrawStyle(DlDrawStyle::kFill);

const int pad = 5;
const int line_length = 20;
const int x_pad = pad;
const int y_pad = line_length + pad;
const int x_test_offset = x_pad + pad * 2;
const int y_test_offset = y_pad * 2 + pad;

auto draw_one = [&stroke_paint, &fill_paint](DlCanvas& canvas,
DlScalar x_base, DlScalar y_base,
DlScalar cap_pad) {
DlScalar x0 = x_base + 0.5f;
DlScalar x1 = x0;
DlScalar y0 = y_base + cap_pad;
DlScalar y1 = y0 + line_length;

SkRect expected_rect =
SkRect::MakeLTRB(x0 - 0.5f, y0 - cap_pad, x1 + 0.5f, y1 + cap_pad);
SkPath expected_path = SkPath::Line({x0, y0}, {x1, y1});

canvas.DrawLine(DlPoint{x0, y0}, DlPoint{x1, y1}, stroke_paint);
if (stroke_paint.getStrokeCap() == DlStrokeCap::kRound) {
SkRRect expected_rrect =
SkRRect::MakeRectXY(expected_rect, cap_pad, cap_pad);
canvas.DrawRRect(expected_rrect.makeOffset(x_pad, 0), fill_paint);
canvas.DrawRRect(expected_rrect.makeOffset(0, y_pad), fill_paint);
} else {
canvas.DrawRect(expected_rect.makeOffset(x_pad, 0), fill_paint);
canvas.DrawRect(expected_rect.makeOffset(0, y_pad), fill_paint);
}
canvas.DrawPath(expected_path.offset(x_pad, y_pad), stroke_paint);
};

DisplayListBuilder builder;
builder.DrawColor(DlColor::kWhite(), DlBlendMode::kSrc);

DlScalar y_base = pad;
for (auto cap : caps) {
DlScalar cap_pad;
DlColor color;
switch (cap) {
case flutter::DlStrokeCap::kButt:
color = DlColor::kBlack();
cap_pad = 0.0f;
break;
case flutter::DlStrokeCap::kSquare:
color = DlColor::kBlue();
cap_pad = 0.5f;
break;
case flutter::DlStrokeCap::kRound:
color = DlColor::kGreen();
cap_pad = 0.5f;
break;
}
fill_paint.setColor(color);
stroke_paint.setStrokeCap(cap);
stroke_paint.setColor(color);
DlScalar x_base = pad;
for (int i = 0; i <= 10; i++) {
DlScalar subpixel_offset = (i / 10.0f);

DlScalar y = y_base;
draw_one(builder, x_base, y + subpixel_offset, cap_pad);
y += y_test_offset;
draw_one(builder, x_base + subpixel_offset, y, cap_pad);

x_base += x_test_offset;
}
y_base += y_test_offset * 2 + pad * 2;
}

auto dl = builder.Build();
ASSERT_TRUE(OpenPlaygroundHere(dl));
}

// Creates an image matrix filter that scales large content such that it would
// exceed the max texture size. See
// https://github.com/flutter/flutter/issues/128912
Expand Down
1 change: 1 addition & 0 deletions impeller/entity/contents/filters/blend_filter_contents.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,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
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
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 @@ -42,11 +41,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

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

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

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

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

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

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

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
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 Vertex generator for either filled or stroked circles.
class CircleGeometry final : public Geometry {
public:
explicit CircleGeometry(const Point& center, Scalar radius);
Expand Down
6 changes: 2 additions & 4 deletions impeller/entity/geometry/geometry.cc
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

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

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
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
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
Loading