Remove saveLayers property from PolylineLayer & save layers automatically when required

[ignatz]

The saveLayers argument is a leaky abstraction. It entails a performance penalty and reasoning about when it's actually needed requires an understanding of the canvas API and how the PolylineLayer implementation draws outlines.

With this change the PolylineLayer implementation will keep layer saving as an implementation detail and call it automatically on a per-polyline basis and only when it's needed (spoilers: outlined lines with transparent fill).

This change results in

• Better encapsulation
• Better performance when only some polylines require layer saving.
• Simpler

(Personally, I find "outlined lines" are such a niche feature. I'm almost a bit surprised the core PolylineLayer supports it.... but that's besides the point)

[ibrierley]

I'm not sure this would work for some examples like Colors.black38 or Colors.transparent (which I think was the original reasoning for a canvas.save), but I may be wrong, and there may be others I can't think of off the top of my head (actually also borderColor would need to be checked). I think my main issue, is that saveLayers was introduced because originally it was on for every polyline and there are often edge cases that break without it, and I'm not a fan of having no method at all to resolve those cases (just yet...).

I'm kinda swinging both ways on this one, we can keep adding in those cases to check to see if a canvas save is needed, but that also feels a bit clunky.

[ignatz]

I'm not sure this would work for some examples like Colors.black38 or Colors.transparent (which I think was the original reasoning for a canvas.save), but I may be wrong,

Could you expand on why you think this is? As far as I understand, the layer saving is only needed when you have to stencil out the inside, which is only when the fill is transparent. I just changed it to not even draw the the "filter" path to make this even clearer and save more cycles.

See expanded example:

I'm kinda swinging both ways on this one, we can keep adding in those cases to check to see if a canvas save is needed, but that also feels a bit clunky.

Maybe you have more context and/or I'm not seeing why the layer saving would be needed beyond transparent fill colors. I can't think of a case where a user would have to manually toggle saveLayers. If you had a pathological example, this would help me greatly.

[ibrierley]

Thanks @ignatz that works better than when I tested it for some reason, not sure what I was doing wrong (certain shaded colors didn't work). So I'm a lot more relaxed about it now.

Context, was just experiencing bugs with canvas and have had to use canvas save a bit, but ultimately at that level, people will probably just roll their own plugin anyway.

[ignatz]

Glad to hear it works for you 👍

Context, was just experiencing bugs with canvas and have had to use canvas save a bit, but ultimately at that level, people will probably just roll their own plugin anyway.

Double checking, is this anecdotal or do you foresee issues with this approach? Are you worried that this approach may be overly dependent on the respective canvas implementation, i.e. platform?

I still can't think of any cases that should have worked pre-regression that shouldn't work with this approach... but then I'm also not very creative :)

[ibrierley]

No I don't foresee any (but I would say specifically if I did), I'm just overly cautious about the canvas stuff I'm not aware of or have forgotten :D. I've hit bugs like this https://www.youtube.com/shorts/3Gqm6H4rbeM where I align polys with the same colour, but I don't think that can happen with the current poly layers (and is fixed in impeller anyway, so I feel good things about that).

Basically It's just me wanting to be lazy and not wanting to think about stuff too much :D. Thanks for your work on this!

[JaffaKetchup]

Hey, how does this need to be moved forward?

[ignatz]

I'd love to hear Ian's thoughts as well. From my side, I'm not aware of any issues. AFAICT it's strictly preferable option to address the same issues as: #1513 , i.e. outlined lines with a transparent fill, Just:

1. Better encapsulation, i..e not leaking implementation details. "saveLayers" is an artifact of how the implementation chose to draw the outline.
2. Simpler for users to use: one less parameter to worry about. I don't think it's clear today when saveLayers is needed and when not, i.e. outlined lines with transparent fill.
3. Faster: pay the cost cost saveLayers only for polylines that are actually outlined with transparent fill as opposed to always all polylines.

[ibrierley]

Apologies for the delay, have been focused on other bits. I'm fine with this last I tested, thanks for all of your work!