Possible fix for polyline layer

[ibrierley]

#1510 has an issue with borderColor being missing. I think there are 2 issues.

The main one, I think is that the main path is drawn before the border path which is thicker, so we don't actually get to see it (hence the color isn't seen).
So firstly the ordering of the paths displayed has been swapped, so the main path is drawn after the borders.

There is also another subtle bug, where because paths are potentially batched, the saveLayer doesn't actually do anything (it saves a layer, adds a path to a batch and the restores, but the paths are drawn later which is too late).

So I have moved the saveLayer code to around the actual painting, and also added an "if saveLayers draw immediately".

2 extra things to note. Performance may be regressed back to pre v4 (am unclear if a saveLayer/restore without anything happening inbetween has a performance penalty the same or not, I suspect it may do)

I think we also draw the polygon layer in the wrong order like this (there aren't saveLayers in that code), so polygon layer will need a sanity check as well.

[ignatz]

Quick question: It was never quite clear to me when saveLayers is actually recommended and/or needed. The docstring is relativley vague:

  /// By default, this value is set to `false` to improve performance on
  /// layers containing a lot of polylines.
  ///
  /// You might want to set this to `true` if you get unwanted darker lines
  /// where they overlap but, keep in mind that this might reduce the
  /// performance of the layer.

... the tests don't use saveLayers, the examples do not, and I've never either. Is there any chance that the easiest fix could be to remove this untested feature?

[ibrierley]

saveLayers is needed as an option, or odd things can happen :D. Mainly around overlapping lines, opacity, transparency and things like that...
Example...with saveLayers on...this is a polyline with a transparent color, and border color...

Same one with saveLayers off...

[JaffaKetchup]

Ah ha, that second image looks suspiciously similar to #1420!

[ibrierley]

Yeah I was thinking about that earlier, just wasn't sure why it would be black and not white (but that may be misunderstanding on my part, maybe it depends on background color).

[ibrierley]

we could probably have that example as a test on the main polyline page I guess, I just edited the existing to...

PolylineLayer(
                            saveLayers: true,
                            polylines: snapshot.data!,
                            polylineCulling: true,
                          ),

final polyLines = [
      Polyline(
        points: [
          LatLng(50.5, -0.09),
          LatLng(51.3498, -6.2603),
          LatLng(53.8566, 2.3522),
        ],
        strokeWidth: 20,
        color: Colors.transparent,
        borderStrokeWidth: 5,
        borderColor: Colors.red
      ),
    ];

[JaffaKetchup]

It was black, but maybe it just depends on some other factor? Does your second screenshot have the same thing of occasionally having correctly, especially after the map is just loaded?

[ibrierley]

Maybe it was Colors.transparent.black ;)

[ignatz]

Thanks for providing the example. Having an example we can all gather around would be very helpful. Do I understand correctly that your example above is already "fixed" and/or "pre-regression"?

I'd still like to take a closer look . I'm drawing many semi-transparent polygons in my app and I've never seen that issue. More generally, it's not clear to me what canvas behavior should be responsible for it. My understanding of https://api.flutter.dev/flutter/dart-ui/Canvas/saveLayer.html is that it would make a difference if you're overlaying multiple transparent paths. In other words, w/o saveLayers i'd expect that semi-transparent paths stack up but the transparent color itself should still be drawn accurately. Am I missing something?

[ibrierley]

Yes, in the example screenshots, thats with the code from this PR (fixing mainly the borders, I was also testing to match behaviour to pre-v4 code, whether it be correct or not, just to look for potential issues).

I haven't taken a look at polygons at all, but I don't think that has a saveLayer attribute, but we do need to check if borders work there in v4, as the code doesn't look quite right, but I haven't had chance to test.

I think we should have a couple more examples, as mentioned the transparent and overlapping opacity lines in polylines, and something similar in polygons (maybe with borders).

(Also possibly antialiased on/off adjoining polys and things like that)

[ignatz]

I had a bit more time to look at this. Your change makes a lot of sense. The only thing that I'm not clear of is:

if (lastHash != null && (saveLayers || (lastHash != hash))) {

What happens if you drop the saveLayers condition? With the save/restore code within the drawPath, does the batching not work? Otherwise....

Performance may be regressed back to pre v4 (am unclear if a saveLayer/restore without anything happening inbetween has a performance penalty the same or not, I suspect it may do)

Your suspicion is likely correct. Specifically in the saveLayer case (if saveLayers is off, there's no regression) you'll en up with a lot more draw calls, which is what's ultimately expensive.

May I suggest a larger change? I would entirely remove "saveLayers" from the PolygonLayers api. Whether saveLayer is needed or not is an implementation detail of how outlined polylines are drawn and whether it's needed or not can be determined for each polyline individually. This way, you're only paying the cost for polylines that need it. Also imagine the case where we wanted to change the implementation of how outlines are draw, e.g. specify the vertices of the outline rather than blending to thick lines, it really shouldn't be in the public API.

[ignatz]

I gave it a shot. Something like: https://github.com/ignatz/flutter_map-clone/blob/save_layers_when_needed/lib/src/layer/polyline_layer.dart#L122-L156

[ibrierley]

What happens if you drop the saveLayers condition?

Very good question on the saveLayers check, and I'm unsure myself now :D, so lets prove it.

My thinking was this...(and maybe it should at least be commented, if the code stays in, as we'll prob hit this in future)

Lets suppose we have 2 polylines (with the same style) that need a separate layer. If we don't have a check to draw the paths immediately, they will be batched up, so saveLayer will only have been implemented once for all the paths. So let's create a test...I'll use grey color that has some opacity in here.

So with the PR and the saveLayer check left in, we get

If we remove that, they get batched, and we end up with...

Weirdly, the 2nd one "looks" kinda better, but it's merged the lines which is incorrect (erm unless that's the effect one is after! But not normally what I would expect from layered opacities)

Regarding the saveLayer complete removal, I don't particularly like that idea currently (within this PR and v4 context). I think it's in there for a reason that potentially resolves a number of edge cases, but I may be wrong. I think firstly it would be good to get back to the expected behaviour that isn't breaking.

Example code

final polyLines = [
      Polyline(
        points: [
          LatLng(50.5, -0.09),
          LatLng(51.3498, -6.2603),
          LatLng(53.8566, 2.3522),
        ],
        strokeWidth: 20,
        color: Colors.black38,
        borderStrokeWidth: 20,
        borderColor: Colors.white30
      ),

      Polyline(
          points: [
            LatLng(50.2, -0.08),
            LatLng(51.2498, -7.2603),
            LatLng(54.8566, 1.3522),
          ],
          strokeWidth: 20,
          color: Colors.black38,
          borderStrokeWidth: 20,
          borderColor: Colors.white30
      ),
    ];

[ibrierley]

Btw, apologies on the last comment, I was thinking you were meaning remove the concept of saving a layer, rather than just the parameter, but I think probably the same applies just for the terms of this fix and maybe better as part of a separate ponder...

[JaffaKetchup]

Those two images represent what I meant by this:

/// By default, this value is set to false to improve performance on
/// layers containing a lot of polylines.
///
/// You might want to set this to true if you get unwanted darker lines
/// where they overlap but, keep in mind that this might reduce the
/// performance of the layer.

Btw, @ibrierley do you remember there was a person recently drawing a route, and he wanted overlaps to be darker? Cant remember the issue number/link now, but I'm assuming that was what he meant.

[ignatz]

I agree that the former image is probably more expected. Then again, I never felt the need to have outlined lines :) (unlike outlined polygons).

Btw, apologies on the last comment, I was thinking you were meaning remove the concept of saving a layer, rather than just the parameter, but I think probably the same applies just for the terms of this fix and maybe better as part of a separate ponder...

We can always pull it into a separate pull request as long as we get rid of it. The more I think of it the stronger I feel about it. It's such a leaky abstraction. Understanding the option requires you to understand canvas drawing and how someone chose to implement outlined lines....

[ignatz]

FWIW, I updated my take to yield the same drawing result (i.e. overlapping) but with implicit layer saving...

[ignatz]

FYI, I send out a PR to deprecate/remove the saveLayers parameter from the PolylineLayer (polygons don't have one since they're not stenciling): #1519. No strong feelings, very happy if you wanted to merge this fix first. Thanks Ian

[JaffaKetchup]

Hey, how does this need to be moved forward?

[JaffaKetchup]

Closed in favour of #1519.