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

jonahwilliams · 2024-09-16T16:43:39Z

For hariline (<= 1.0 physial pixel) strokes and points, we can use the line and point geometry primitives instead of tessellation. This gives substantially better performance.

flutter/flutter#152702

Still need to make sure we only handle single contour lines this way, since we can't insert degenerate triangles to break a line strip. We may be able to do this via primitive restart, but we don't yet use that.

flutter-dashboard · 2024-09-26T20:07:14Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #55230 at sha 2d8f1eb

jonahwilliams · 2024-09-26T21:26:15Z

I also adjusted some of the alpha scaling params since this doesn't do MSAA specific scaling anymore.

jonahwilliams · 2024-09-26T21:26:34Z

impeller/entity/geometry/stroke_path_geometry.cc

@@ -17,43 +17,36 @@ using VS = SolidFillVertexShader;

 namespace {

-template <typename VertexWriter>
-using CapProc = std::function<void(VertexWriter& vtx_builder,
+class PositionWriter {


I removed the template code here because we never specialize.

jonahwilliams · 2024-09-27T19:34:16Z

I don't know how many times I've mashed rerun but the bot failure seems unrelated.

flar · 2024-09-30T17:46:44Z

impeller/entity/geometry/circle_geometry.h

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


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

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"?

That's better, I'll use that

impeller/entity/geometry/geometry.cc

flar · 2024-09-30T17:55:32Z

impeller/entity/geometry/line_geometry.cc

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


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

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

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

PointFieldGeometry also bails on basis == 0.

flar · 2024-09-30T18:40:41Z

impeller/entity/geometry/stroke_path_geometry.cc

+
+  // 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_hairline && path_.IsSingleContour()) {


Does IsSingleContour guarantee no overdraw?

Why would it need to?

To prevent overdraw of alpha lines. If you stroke it as a path then the overdraw of a contour self-intersection is prevented, but breaking it into multiple line primitives would bring back that overdraw.

You'd need this test to be IsSingleNonSelfIntersectingContour()

Right, I still need to use the prevent overdraw setting

flar · 2024-09-30T18:42:34Z

impeller/entity/geometry/stroke_path_geometry.cc

+        .transform = entity.GetShaderTransform(pass),
+    };
+  }
+  // For harline strokes that must be tessellated, switch to the cheaper


flar · 2024-09-30T18:45:08Z

impeller/entity/geometry/stroke_path_geometry.cc

+  Cap cap = stroke_cap_;
+  if (is_hairline) {
+    join = Join::kBevel;
+    cap = Cap::kButt;


I fear we might miss pixels/MSAA-samples with these. Round is really the one you want to avoid as that will attempt to generate a tesselated circle. Square caps shouldn't be a problem as it is just one additional vector add. Miter is more than bevel, but by how much?

The caps/joins are made of triangles, but this change makes hairline strokes visible via line strip primitives. We can't draw the cap/join with connected points, and its not clear that we need to as they'd certainly be subpixel already.

butt cap are 1 line width shorter than square caps. Bevel might miss any samples on the vertex that the true outline of a miter or round join would capture. These could produce squares that are missing corners depending on the sub-pixel registration of the line.

hmm, I could bump the ends out by about that much then? So that would be add 1px to each end for square/round and nothing for butt?

I'm just going to remove this bit, its not really that important.

flar · 2024-09-30T18:50:29Z

impeller/entity/shaders/gradients/fast_gradient.vert

@@ -18,6 +18,7 @@ in mediump vec4 color;
 out mediump vec4 v_color;

 void main() {
+  gl_PointSize = 1.0;


Future opt - what does gl_PointSize render if the size > 1.0? Does it mimic any of the cap-styled points we need? Could we use this for non-hairline points of particular cap-styles?

There is also apparently a glLineWidth setting.

What gl_Point does at larger sizes it somewhat backend dependent. on Vulkan for example, we can draw larger points but they are drawn as squares. GL has an optional extension to anti alias the point, but we can't guarantee its supported.

practically, we can only use this for hairline drawPoints

gl_LineWidth is similar in that we can't guarantee anti aliased lines, and unfortunately its a pipeline setting so we'd need a new PSO per width which isn't practical

… primitive_geom

flutter-dashboard · 2024-10-04T03:54:05Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #55230 at sha eb7f841

flar · 2024-10-04T11:41:06Z

Some of the golden failures don't make sense as they look like something that shouldn't have been affected.

I think some of the other failures could be due to pixel registration. What exactly is the equation for a line primitive? Does it extend half a pixel on both sides of the line?

jonahwilliams · 2024-10-04T22:33:37Z

I suspect some of the goldens are just from switching to MaxBasisXY in the fill code. I'm removing that for now.

… primitive_geom

flutter-dashboard · 2024-10-04T23:16:10Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #55230 at sha 6ff4f7f

jonahwilliams · 2024-10-05T00:12:03Z

I pulled the max basis change into #55670 (thanks for the review), so whatever is left should just be from this change

flar · 2024-10-05T00:25:32Z

I pulled the max basis change into #55670 (thanks for the review), so whatever is left should just be from this change

I don't see how this change could cause the diffs in ImageFilteredSaveLayerWithUnboundedContents

jonahwilliams · 2024-10-25T18:04:58Z

Okay, this is up to date and I've disabled the optimization for round/square caps. Filled flutter/flutter#157618 . I need to do some refactoring of the start/end vector to handle the arbtirary path case.

flar

Most of these are just questions and comments, but at least one looks like it might result in a bug.

flar · 2024-10-28T19:07:46Z

impeller/entity/geometry/line_geometry.cc

+std::pair<Scalar, bool> LineGeometry::ComputePixelHalfWidth(Scalar max_basis,
+                                                            Scalar width) {
+  Scalar min_size = kMinStrokeSize / max_basis;
+  return std::make_pair(std::max(width, min_size) * 0.5f, width <= min_size);


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

flar · 2024-10-28T19:08:11Z

impeller/entity/geometry/circle_geometry.h

@@ -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 for or either filled or stroked circles


"for or" typo

flar · 2024-10-28T20:52:01Z

impeller/entity/geometry/circle_geometry.cc

+    auto [width, _] = LineGeometry::ComputePixelHalfWidth(
+        transform.GetMaxBasisLengthXY(), stroke_width_);
+    half_width = width;
+  }


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.

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

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

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.

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"...

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

I re-arranged this code.

flar · 2024-10-28T20:54:30Z

impeller/entity/geometry/geometry.cc

    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);


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?

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.

impeller/entity/geometry/point_field_geometry.cc

impeller/tessellator/tessellator.cc

Based on flutter#55230 Fixes flutter/flutter#156438

…56223) Based on #55230 Fixes flutter/flutter#156438

… primitive_geom

jonahwilliams · 2024-10-31T23:38:15Z

I'm having a hard time tracking down the failures in this PR. Instead, I'm going to split this up into smaller changes so i can hopefully find where the error was introduced.

Split off from #55230 . Adds getter that tracks if a Path is a single contour. ################################ flutter/flutter#152702

…lutter/engine#56223) Based on flutter/engine#55230 Fixes flutter#156438

…ine#56340) Split off from flutter/engine#55230 . Adds getter that tracks if a Path is a single contour. ################################ flutter#152702

[Impeller] use point/line primitve for hairlines.

813d4bd

github-actions bot added the e: impeller label Sep 16, 2024

jonahwilliams added 3 commits September 25, 2024 15:37

Merge branch 'main' of github.com:flutter/engine into primitive_geom

1c362df

single contour.

beee6ae

add chck

e3cc898

jonahwilliams changed the title ~~[WIP][Impeller] use point/line primitve for hairlines.~~ [Impeller] use point/line primitve for hairlines. Sep 25, 2024

jonahwilliams marked this pull request as ready for review September 25, 2024 23:29

++

2d8f1eb

flutter-dashboard bot added the will affect goldens label Sep 26, 2024

Merge branch 'main' into primitive_geom

032cb2f

jonahwilliams requested a review from flar September 26, 2024 21:25

jonahwilliams commented Sep 26, 2024

View reviewed changes

jonahwilliams requested a review from chinmaygarde September 26, 2024 21:26

Merge branch 'main' into primitive_geom

a968e8a

flar suggested changes Sep 30, 2024

View reviewed changes

jonahwilliams added 4 commits October 2, 2024 11:52

Merge branch 'main' of github.com:flutter/engine into primitive_geom

ce71dab

add back prevent overdraw and fix typos.

544cb56

Merge branch 'primitive_geom' of github.com:jonahwilliams/engine into…

2f3c2a6

… primitive_geom

Merge branch 'main' into primitive_geom

eb7f841

++

07017d1

Merge branch 'primitive_geom' of github.com:jonahwilliams/engine into…

6ff4f7f

… primitive_geom

fix rrect descriptors.

185291f

jonahwilliams requested review from flar and removed request for chinmaygarde October 25, 2024 18:05

jonahwilliams marked this pull request as ready for review October 25, 2024 18:08

Merge branch 'main' of github.com:flutter/engine into primitive_geom

4385efc

jonahwilliams marked this pull request as draft October 28, 2024 17:08

jonahwilliams added 2 commits October 28, 2024 10:20

Merge branch 'main' of github.com:flutter/engine into primitive_geom

2eeee37

++

897ff2f

flar suggested changes Oct 28, 2024

View reviewed changes

jason-simmons added a commit to jason-simmons/flutter_engine that referenced this pull request Oct 29, 2024

[Impeller] Remove the use of a subpixel minimum stroke size for MSAA

f805231

Based on flutter#55230 Fixes flutter/flutter#156438

jason-simmons mentioned this pull request Oct 29, 2024

[Impeller] Remove the use of a subpixel minimum stroke size for MSAA #56223

Merged

jonahwilliams added 4 commits October 30, 2024 09:21

use radius.

a91974b

typo

126f665

++

b7152ee

Merge branch 'main' into primitive_geom

1fd08d6

auto-submit bot pushed a commit that referenced this pull request Oct 30, 2024

[Impeller] Remove the use of a subpixel minimum stroke size for MSAA (#…

c337a59

…56223) Based on #55230 Fixes flutter/flutter#156438

jonahwilliams added 3 commits October 30, 2024 20:13

++

9975825

refactor circle position logic.

d76a6cc

Merge branch 'primitive_geom' of github.com:jonahwilliams/engine into…

322ede8

… primitive_geom

jonahwilliams marked this pull request as ready for review October 31, 2024 04:19

jonahwilliams added 3 commits October 30, 2024 21:46

++

44f5082

testing.

011e724

++

1b4da9f

jonahwilliams mentioned this pull request Nov 2, 2024

[Impeller] geometry changes to support line/point style. #56340

Merged

chinmaygarde closed this Nov 11, 2024

auto-submit bot pushed a commit that referenced this pull request Nov 11, 2024

[Impeller] geometry changes to support line/point style. (#56340)

980e4d2

Split off from #55230 . Adds getter that tracks if a Path is a single contour. ################################ flutter/flutter#152702

nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024

[Impeller] Remove the use of a subpixel minimum stroke size for MSAA (f…

d24eb30

…lutter/engine#56223) Based on flutter/engine#55230 Fixes flutter#156438

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

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

Uh oh!

Conversation

jonahwilliams commented Sep 16, 2024

Uh oh!

flutter-dashboard bot commented Sep 26, 2024

Uh oh!

jonahwilliams commented Sep 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Sep 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Oct 4, 2024

Uh oh!

flar commented Oct 4, 2024

Uh oh!

jonahwilliams commented Oct 4, 2024

Uh oh!

flutter-dashboard bot commented Oct 4, 2024

Uh oh!

jonahwilliams commented Oct 5, 2024

Uh oh!

flar commented Oct 5, 2024

Uh oh!

jonahwilliams commented Oct 25, 2024

Uh oh!

flar left a comment

Choose a reason for hiding this comment

flar Oct 30, 2024 •

edited

Loading