It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

Also, does this implementation work if the scale is 0.000001? The work done elsewhere will work regardless of scale.

Neither the padding nor the minimum stroke should be computed globally for the operation. What if there is a 10x scale in one direction and a 1x scale in the other? You can't pick a single width or aa pixel size that won't be stretched too much in one direction or squeezed too much in the other. This must be computed in the SDF calculations perpendicular to the outline.

This is a bad implementation. We don't special case 0.0.

The stroke should always be at least a pixel wide everywhere. The default value of 0.0 relies on that rule to produce hairlines, but the same issue would come into play if the developer specified a width of 10 and a scale of 0.0001...

There should be no "test" on the width, there should be a clamp on the smallest stroke value to be a pixel, and that clamp would be computing the width perpendicular to the shape at its location.

Look at the original Circle SDF at the work done with dFdxy and dot products to make sure we did not have a width value that went less than 1 pixel.

I actually did check the behavior of 0.0 vs small values (less than a device pixel). From my tests, it did seem like 0.0 was special cased to render at 1 device pixel, and would render thicker than non-zero stroke width values that are less than 1 device pixel.

Flutter web example showing this: https://dartpad.dev/?id=c4c1fe8149859f9c743317820a25a3e6

I now realize why my tests showed this. I was testing with a Skia backend, not Impeller (I was testing with Mac app, not Flutter web like the link above. But both of those use Skia). I do see that Impeller does force a minimum stroke width of a device pixel, and running the linked app above with impeller enabled does show this minimum stroke width. This is different from Skia's behavior, but I think it makes sense. I'll update my PR to enforce a minimum value.

Also, does this implementation work if the scale is 0.000001? The work done elsewhere will work regardless of scale.

Neither the padding nor the minimum stroke should be computed globally for the operation. What if there is a 10x scale in one direction and a 1x scale in the other? You can't pick a single width or aa pixel size that won't be stretched too much in one direction or squeezed too much in the other. This must be computed in the SDF calculations perpendicular to the outline.

I think it properly supports all of this. This PR modifies the padding to treat the X and Y components separately. So if there's a 10x scale in one direction and a 1x scale in the other, the padding should be properly handled in both axes.

And the SDF calculation uses fwidth(base_sdf) to size the hairline width to be 1 device pixel perpendicular to the outline.

Ugh, I missed the X/Y scale adjustments. But I'm not sure they are the right equation. We need "width across the outline" and so I'd need to think more about the math to consider if those are the right values.

WRT fwidth in the shader code - that equation is not recommended for our purposes because it computes a "taxicab geometry" dimension by adding the absolute values of the dimensions rather than rotating them to the correct orientation and taking the length of that vector. The older Circle SDF shader used fwidth and we got a bug report on it because the 45 degree parts of circles were too thick (see #182708 and the fix https://github.com/flutter/flutter/pull/183184/changes#diff-3a0c3ce6c5c0303a024bbef598aab0863c70a1072c7ae6243e6be63f77d9d4d3).

I like that Skia does try to represent "less than a pixel width" using ?? alpha ?? reducing coverage ?? I'm not sure. I think we also have parts of Impeller that adjust the alpha of the color when objects get small, so I'm not sure we are 100% consistent.

If apps have depended on Skia for a long time then they may be used to its behavior, so is the Impeller enforcement of "always at least a pixel" really correct? We also have a year or two of app developers getting used to the Impeller rules, so perhaps we should keep going with this philosophy. Also, we've been dealing with MSAA in the past so enforcing a minimum width is almost needed to keep the width from disappearing entirely if we miss all of the sub-samples in a pixel - it's the only tool we have to bring to bear. But with SDF we have a choice to compute smaller coverages instead.

I need to review more of the math here. I think we are better computing this stuff in the shader. If the code here is already doing fwidth then it is already paying the cost of dFdx/y and those directional functions plus a dot product can compute the values anyway, so the CPU adjustments are sort of unnecessary.

The SDF shaders have 2 issues where we need to adjust for (relative) pixel dimensions.

• We don't want strokes to disappear so we need to make sure we aren't computing the outer-inner difference on pixels that are too close together. What coordinate space is best for making sure this doesn't happen? Doing an adjustment on the device results or the source geometry?
• We want the AA pixel gradient to go from full to zero coverage over the approximate distance of aa_pixels. That computation definitely needs to be based on the size of a pixel, but since it is applied to the results of the sdf distance which are computed in local space - dFdx/dFdy can help us in that transformation.

One approach is that we could just have the SDF computations compute the distance to the outline. The differential of that value vec2(dFdx(sdf_value), dFdy(sdf_value)) will point in the direction of maximum change in the gradient (is that true?) and so gives us the direction across (perpendicular to?) the outline. Normalizing that vector helps us in 2 ways, it helps us figure out the pixel size, and provides a minimum stroke width.

We can fold those 2 cases together by:

• for filled primitives, just use the sdf_result.
• for stroke primitives, use the computed pixel size as a minum and compute abs(sdf_result) - half-stroke-width and then we can just proceed as if we are filled.
• for both primitives we then do a smoothstep on the resulting value from -/+half_aa_pixel_size

That technique doesn't work for primitive that have a join consideration, currently only rects, but it should work for other primitives. For round joins, the above results might work, but for miter and bevel we need different geometries and the regular outer - inner calculation and making sure it honors "minimum stroke width".

Those are good suggestions. I implemented them and I think the results are improved.

I think the suggestion to change our pixel size measurement to use euclidean distance (length(vec2(dFdx(sdf_value), dFdy(sdf_value)))) rather than taxicab distance(fwidth(sdf_value)) is correct. However, it produces a ton of golden changes that are not directly related to this hairline stroke PR.

I want to decouple the pixel size change from this hairline stroke change, so I'm going to make a separate PR for the pixel size change. I'm going to try to get that PR submitted first, then I'll update this PR once the other one is merged. This PR on hold for now.

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

autosubmit label was removed for flutter/flutter/184895, because - The status or check suite Linux linux_fuchsia has failed. Please fix the issues identified (or deflake) before re-applying this label.

An existing Git SHA, 5b964e092b6bc76b9595e8c17b353a21182d75f3, was detected, and no actions were taken.

To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with --force) that already was pushed before, push a blank commit (git commit --allow-empty -m "Trigger Build") or rebase to continue.