I don't usually say this but there is a lot of dense math happening here that takes a while to parse. Some comments outlining the procedure would be helpful so people don't have to parse everything in order to orient themselves.

This, in general, in a lot of our code...

I think I've got the right understanding here, I took Flar's math directly and looked at it for a while until it made sense. @flar, does the comments sound about right to you?

Perhaps for a general comment at the top of this function we could say something like:

// We need to make sure that for stroked circles we have a cross section
// that is at least as wide as a pixel. The cross section is measured towards
// the center of the circle which is the same direction in both local and screen
// coordinates for uniformly scale transforms and only slightly off for skewed
// transforms.

This method is used for both AA and non-AA circles, only the caller knows which. This call should be moved up to the canvas.cc methods that create the geometry and know which type they want.

Are you talking about CircleContents::Make? It's only used for AA circles. The contents is the thing that controls which shader to use, and the circle_contents uses the SDF shader which applies antialiasing.

The name doesn't suggest that.
It has the same arguments as a constructor that is used for non-AA circles.
How is the caller to know this?

If, in the future, the non-AA case decides it wants to move from a stack allocated contents to a shared_ptr contents, it will suddenly generate the wrong geometry. And vide-versa.

I'm facing a similar situation with the UberSDFContents. I like Jim's point on surfacing the idea of antialiasing somehow into the canvas level. An easy solution is to make the canvas set the antialias amount on the geometry. We're going to have to foist that value up to the canvas level at some point.

On thing that is weird here too is that by adding the antialias amount to the geometry we create data on it that is ignored in other instances. Don't you find that odd? I guess the alternative is to have an AntialiasedCircleGeometry or storing the antialias amount on the contents which might be awkward?

If, in the future, the non-AA case decides it wants to move from a stack allocated contents to a shared_ptr contents, it will suddenly generate the wrong geometry. And vide-versa.

If someone were to somehow refactor the non-AA case to use a CircleContents (and created it via CircleContents::Make()), they may be surprised to end up with both the extra geometry and an SDF-antialiasing shader being applied (the latter being applied with no regard for the geometry).

I guess the alternative is to have an AntialiasedCircleGeometry or storing the antialias amount on the contents which might be awkward?

The contents (in this PR) stores and controls the antialias amount. It needs to at least store it in order to pass it along to the shader.

Maybe we need an AntialiasedCircleGeometry and an AntialiasedCircleContents to disambiguate the API? I also think that this will all change very soon with https://github.com/orgs/flutter/projects/240. My main goal in this PR is to address the immediate visual issues users are experiencing.

On thing that is weird here too is that by adding the antialias amount to the geometry we create data on it that is ignored in other instances. Don't you find that odd? I guess the alternative is to have an AntialiasedCircleGeometry or storing the antialias amount on the contents which might be awkward?

It's not "ignored" in other instances. The other instances are simply asking for no padding for antialiasing. It's default is (should be?) 0. But, it is applied in all uses, it just has no effect when it is left at 0.

I suspect that the way we reuse the CircleGeometry for both AA and non-AA cases will be reflected in other geometries as well. Their basic role is the same regardless of whether they've been padded for AA.

One thing that we will need to decide about. There is a difference between "aa pixels" and "aa padding". For a rectangle, they are the same, but for a circle, the padding is theoretically sqrt2*aa_pixels. Rather than have a configurable "padding amount", the geometries should have a configurable "aa pixels" and then they can decide what their padding is based on how their geometry works.

Of course, in practice, when I brought up the issue of the sqrt2 before, I eventually convinced myself that the slightly larger radius didn't matter. But, it points out that the caller is not necessarily in the best context to know how much padding will be needed.

The padding vs pixels may matter more for AA lines which really care more about the angular fidelity compared to a circle where that fidelity is constantly changing through its circumference. Also, we may find that developers will care about the tiny amount of fidelity that padding by sqrt2 matters for circles. It's best not to assume that geometry constructor/callers understand the issues we see today and that we discover tomorrow wrt the actual best number of pixels to pad any given geometry by.

@flar we are going to revisit this for the UberSDF. I think this is an acceptable approach for now. How about we accept this and wrap the cleanup into the ubersdf work? We don't want to mix this up with the ubersdf since that will make it hard to cherry-pick. I share your concerns and we'll get it hashed out.

I'm good with punting these issues to the Uber shader. We currently have a misspelling, but we can fix that either here or there as well...

This configuration should be similar across all AA rendering so perhaps this should be moved to a more central location. Canvas?

Also, it's misspelled. Antilias -> Antialias

We can leave this for the Uber work to sort.

Fixed the typo.

Are these deleted lines intentional?

Yeah, that's what caused the background diff. I found it easier to look at the result with a white background. Both you and Willie commented on it though, so I'm probably the weird one - changing back.

I just wanted to make sure it was intentional...