We talked about this offline - should we have some specialized codepath for this since we know we're trying to approximate a circle? If not for this patch, for a follow up bug?

Filled flutter/flutter#143012 .

Per thread? Per process?

Per process, tessellator.h is not thread safe.

Document this - what's getting copied, is it copy on write, etc.

Everything - Done

Look at the line_geometry.cc which does round caps using direct tessellation.

It looks like the EllipticalVertexGenerator would do what I want. Does that generate two full circles for the each center? I think to incorporate it with the existing triangle strip I'd need to do one at a time.

That class is a very thin veil over the generator proc. The generator procs get a list of trig values (sin/cos) spaced reasonably for a circle of a given radius. The trig arrays are basically a single quarter circle quadrant of a full circle and they are iterated in various ways 4 times over to produce zig-zag triangle strips. They are the raw trig values so you still need to multiply by the distance (and potentially the direction) - the thing that varies based on the pixel size is how many of them to produce.

The GenerateRoundCapLine proc is close to what we need since it generates the quadrants at an angle based on a direction vector and breaks it down into 2 vectors - one along the line and one across the line both sized by the stroke width. But it generates a triangle strip instead of a poly-line so it's mostly a matter of the order in which the trigs are iterated and multiplied by the along and across vectors. The (triangle) vertices for a single line segment start at the very outward end of the round cap (the top of the semicircle) and zig zag down it until they meet the base of the cap (which is where a butt cap would just cut across) - it then does the reverse on the far round cap since it is responsible for both ends of a drawLine and the jump from one cap to the other naturally covers the body of the line segment itself.

For path tracing you only deal with one round cap at a time and since you are generating polylines instead of triangle strips you would need to iterate in a different order - basically start at the base of the semi-circle and run up one side and then down the other - iterating the trigs twice and moving along the semi-circle instead of zig-zagging down it from the top.

A couple of tricks - the symmetry of the sin/cos values means that whether you are going from 0 to 90 degrees or 90 to 0 degrees you can iterator forward over the array and just swap the sin/cos values. So, flipping from going up one side of the semicircle and then down the other side would just involve iterating forward over the trigs twice, once using cos/sin and the second time swapping to use sin/cos and changing the sign of whether the across vector is added or subtracted from the segment endpoint.

Since this will be the first (potentially only) generator proc that produces a polyline instead of a triangle strip we should probably make that clear in the name(s).

While we do generate a polyline for the stroke, that doesn't include the caps/joins, those are appended as triangle strips as we process the polyline.

Ah, then it gets even easier. It's more a matter of only generating half of the circle than switching the vertex ordering - for the caps. The joins would still require computing the correct subset of the trigs to use which is something none of the current generators try to do.

I'm a bit confused - these don't seem to be used in this PR, is there a reason they were added to the headers?

(these = all the static void CreateXCap methods)

They are used, see https://github.com/flutter/engine/pull/50379/files#diff-c75818e02ee25c31e8f8a6d5cac9a4f80b33edf3066d39104cd805268310625dR267

Previous this used to pass around closures, but I updated these to be static functions, since that is what they are anyway.

Here you are storing a raw pointer to vtx_builder, can you instead pass this in as an argument to Generate?

Since this is an private class I think we can maintain this invaraint?

It's kind of leaving a footgun in the code, seems like an easy change to remove it unless I'm missing something.

How is it a footgun?

Here's the official stance of the c++ style guide: https://google.github.io/styleguide/cppguide.html#Ownership_and_Smart_Pointers

This particular case is a grey area. You have a non-owning raw pointer which is at risk of becoming a dangling pointer.

Here's one example where it can go wrong:

class Foo {
public:
  void good();
  Bar bad();
};

class Bar {
public:
  Bar(Foo& foo) : foo_(foo) {}
  Foo& foo_;
};

void Foo::good() {
  Bar bar(*this);
}

Bar Foo::bad() {
  return Bar(*this);
}

You can imagine a few other ways this can go wrong. We can try to avoid them, but that's why it's a foot gun and not a error. There is no compile-time guarantee that you won't introduce a dangling pointer and that's of the intent of ownership section.

If you remove the field that is storing a raw pointer you remove the risk that it can become a dangling pointer.

At a bare minimum, any C++ class that has a raw pointer should define copy constructors which would remove many of the cases where you'd get dangling pointers. I think just removing the field is easier option though.

My read of both of those is to be careful with functions that return references, if the lifetime is potentially too short. but in this case I'm not returning a reference, I'm using the class as a storage for a reference.

Happy to make it non-moveable and copyable though.

My read of both of those is to be careful with functions that return references,

The key text is "Avoid defining functions that require a const reference parameter to outlive the call". The internal version is "Avoid defining functions that require a reference parameter to outlive the call." That's what we have here with the constructor. I'm happy to deviate from the style guide if we have a valid reason.

The reference already outlives the call, and is used in a class as is. Its equivalent to closing over a non-const reference, which we do all the time. I'd argue that is actually less safe, since I could more easily call the closure in a scope where the reference had been destructed.

At any rate, it turns out its trivial to just pass the reference in so I've made this change.

C++ style guide has guidance for lambda too: "Use default capture by reference ([&]) only when the lifetime of the lambda is obviously shorter than any potential captures."

I don't know if it is ever truly obvious in c++ haha.

Yeah I feel like this is a case where the class holding the reference has a more obvious lifetime than the closures, but perhaps you could say that the closures we had didn't follow the style guide anyway?