[Impeller] Directly tessellate filled ellipses

[flar]

Add support for filling ellipses/ovals and greatly simplify the primitive tessellation mechanisms.

[flutter-dashboard]

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 #48770 at sha 03597d6

[jonahwilliams]

I like the new abstraction for vertex generation, but I don't love that it requires another heap allocation. If the calling code doesn't need to store VertexGenerator, I think you could return a stack allocated VertexGenerator.

[flar]

I couldn't return the sub-classes until I did something like this. Maybe there is magic to do that, but I couldn't find it and then found an answer on the web saying that you can't return a sub-class for a super-class and need to use a managed pointer to do it.

[jonahwilliams]

This would have to return FilledCircleGenerator instead of VertexGenerator

[flar]

Which would involve exposing the utility classes. Also, some of them might want to return one of a set of various sub-classes depending on the data.

[jonahwilliams]

But now we're doing a new heap allocation per piece of data we're processing, which mid-long term we'll need to unwind to deal with scudo (android malloc implementation) being a slow allocator in general.

[flar]

We could have it contain a union of all possible data that any of them might need and then just sub in a generator proc. That could be limiting in the long run, but for now we only have half a dozen primitives and they all have several pieces of data in common. Then the various methods just have to figure out which generator proc to supply, and that proc would only use some of the data. (Or an actual union{}, but that might leave us open to splicing the data rather than just filling it all out even for the simple operations that only need a couple of the fields?).

For example, everything is a round rect, it has 4 center points and 2 radii, a stroke width and possibly an along and an across vector . But, if all four center points are the same (because it was instantiated from Tessellator::FilledCircle) then the proc installed would just grab one of them and ignore the other 3. The along and across vectors might be {1,0} and {0,1} for most use cases except for a RoundCapLine so they are ignored by all procs except for the one specifically designed to produce round caps (and even then it could compute those from the first 2 points). Even arc tessellation only adds 2 angle values. So, we'd have:

  class VertexGenerator {
   public:
    const PrimitiveType triangle_type;
    const size_t vertex_count;

    void GenerateVertices(const PointProc& proc) { impl_(*this, proc); }
    // or a switch statement and a shape_type enum?

   private:
    const function<void(const VertexGenerator& generator, const PointProc& proc)>& impl_;

    const Point corners_[4];
    const Size radii_;
    const Scalar stroke_width_;
    // const Radians start_angle_;
    // const Radians end_angle_;  (or extent)
  }

Not every use case needs all of those values, but the stack allocated object will have default values for all of them and assigned values for the ones needed for a given use case. That's 4 Points a Size and a (or 1-3) Scalar(s), but it saves heap allocation. And I don't think it needs a destructor.

Then:

• FilledCircle sets all corners to the center, and the radii to {radius, radius}
• StrokedCircle sets all of those and the stroke_width;
• RoundCapLine sets the corners to p0, p1, + dups and the radii to {half_line_width, half_line_width}
  • Potentially it could also precompute the along and across vectors, but these aren't used more than once in a typical use case so that isn't really necessary.
• FilledEllipse sets all corners to the center of the oval bounds and the radii_ to half the size of the bounds.
• FilledRoundRect sets the corners to the interior corners of the round rect and the radii_ to the corner radii (only works for same radii at all 4 corners)
• FilledArc would only need the start and end angles over FilledCircle.

[jonahwilliams]

I don't think you necessarily need the VertexGenerator to have the function pointer, it could have an enum or state bit and a shared_ptr to the original tessellator instance. Then when you call generate vertices, you jump back into the shared_ptr tessellator with the state you need.

[flar]

I basically have it working by having Tessellator have static functions that implement the signature and a reference to one of them is stashed in the VG object. Basically all of the subclasses of VG turn into static methods. I could also do it with an enum, but this seems more direct.

Using the std::function to declare the signature seemed to have problems with the initialization so I switched to a function typedef and that worked fine.

I have a strange intermittent bug that is biting me that should be unrelated, but I need to squash that before I commit the new version.

[flar]

I'm using:

    typedef void GeneratorProc(const Trigs& trigs,
                               const Data& data,
                               const TessellatedVertexProc& proc);

  // ...

  static void GenerateFilledCircle(const Trigs& trigs,
                                   const VertexGenerator::Data& data,
                                   const TessellatedVertexProc& proc);

  static void GenerateStrokedCircle(const Trigs& trigs,
                                    const VertexGenerator::Data& data,
                                    const TessellatedVertexProc& proc);

  static void GenerateRoundCapLine(const Trigs& trigs,
                                   const VertexGenerator::Data& data,
                                   const TessellatedVertexProc& proc);

  static void GenerateFilledEllipse(const Trigs& trigs,
                                    const VertexGenerator::Data& data,
                                    const TessellatedVertexProc& proc);

Those static Generator* methods do the actual tessellation work and get set up using something like:

    return VertexGenerator(Tessellator::GenerateRoundCapLine,
                           GetTrigsForDivisions(divisions),
                           PrimitiveType::kTriangleStrip, 4,
                           VertexGenerator::Data{
                               .reference_centers = {p0, p1},
                               .radii = {radius, radius},
                               .half_width = -1.0f,
                           });

[flar]

The new version of this interface is now pushed.

[bdero]

Would it be possible to compute this table with a constexpr at compile time?

[flar]

Hmm, I looked at macros, but didn't think to use constexpr. Let me try that. (Note that this is already in there, it is copied from the deleted CircleTessellator...)

[flar]

I thought of just using a static initializer but we've been bitten by "order of class initializers" in the past and I wanted to avoid that.

[flar]

I'm going to skip this for now as it just moved from one file to another and isn't "changed" here. Feel free to file an Issue or take it on if you want to see this happen...

[bdero]

That's cool with me. :) This is just nice-to-have if we're at a point with clang + our STL where it's possible (I'm not sure myself).

[flar]

Some performance results using my "basic primitive basher" Flutter app (measuring end-to-end for a Flutter app using ui.Canvas in a custom paint of 5k data points):

[flutter-dashboard]

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

Changes reported for pull request #48770 at sha 942b622

[flar]

I'll leave the goldens for reviews. It's just the new images for the new playground test on filled ellipses...

[jonahwilliams]

Thanks for removing the heap allocations, I think this approach looks reasonable. Just looking at it closer now.

[jonahwilliams]

LGTM!