feat: add inverted fill option to PolygonLayer

[monsieurtanuki]

What

• Introduced a bool justHoles field for polygon:
  • the "full" points won't be considered and will be replaced by the full map
  • the "holes" points will be considered as the only "not colored" parts of the polygon
• New example named "Advanced polygons".
• The typical use-case would be using a single polygon with several holes.
• In that case only the holes are clickable.
• Could be that eventually we may consider that feature to be applied to the map instead, and have all polygons considered as holes. The thing is that currently we cannot say: "just hit hole number 2", because the label is attached to the "polygon", and all holes share the same polygon.
• cc @enricostrijks and [💰] Add option for PolygonLayer to have fill color around Polygons #2034

Fixes #2034. Potentially eligible for bounty.

Screenshots

new menu item	example

multi-world example	hit example

Files

New file:

• advanced_polygons.dart: Example dedicated to polygons with advanced features.

Impacted files:

• main.dart: added the new AdvancedPolygonsPage page
• menu_drawer.dart: added the new AdvancedPolygonsPage page
• painter.dart: managed the justHoles case; refactored a bit borderPaint
• polygon.dart: added the bool justHoles field
• polygon_layer.dart: special case for justHoles
• projected_polygon.dart: special case for `justHoles

[enricostrijks]

LGTM

[JaffaKetchup]

Thanks :). I've made a couple changes, namely creating a new constructor for the polygons instead of a property, which allows us to enforce some usage patterns, and renamed it to "inverted".

Two questions, do we always want the hitting to be inverted? Surely sometimes they want to detect not hitting a hole still? Also, it's now impossible to have multiple holes with different hitValues - is there anything we can do about this (I'm aware that's quite a tricky problem in itself, and that you've now done quite a lot of work extra for all of this!)

[monsieurtanuki]

Thanks :). I've made a couple changes, namely creating a new constructor for the polygons instead of a property, which allows us to enforce some usage patterns, and renamed it to "inverted".

I'm OK with that.

Two questions, do we always want the hitting to be inverted? Surely sometimes they want to detect not hitting a hole still? Also, it's now impossible to have multiple holes with different hitValues - is there anything we can do about this (I'm aware that's quite a tricky problem in itself, and that you've now done quite a lot of work extra for all of this!)

As already mentioned, it's a bit strange to set this property to a polygon, as it impacts the whole map (e.g. the alpha background).
Perhaps a map property called bool invertedPolygons instead? In that case we would have more control over hitValues, as we would be displaying several distinct polygons, but as holes from the map.
About the hitting being inverted or not, it could indeed be deactivated, so that the hit ignores the polygons altogether.

All of that depends on typical use-cases:

• would it be expected to display inverted polygons and "normal" polygons on the same map?
• would it be expected to display inverted polygons with holes?

[JaffaKetchup]

I would say we should support both those use-cases - as we did before.

That becomes a really difficult solution I guess. A solution is to allow an entire PolygonLayer to be inverted instead (PolygonLayer.inverted)? At that point, I'm not sure how much that would complicate the painter. And we'd somehow have to factor in a color to the layer. Maybe Color? invertedFill?

I'm also not 100% sure still why we couldn't make the current usage pattern work, also for the PolylineLayer. Would it be possible to have a property that stops the element from passing over the anti-meridian? I haven't tested it, but surely this current solution would still have the exact same issue as before. As unlikely the real world use case, what if someone wanted something like below (ofc they could just use multiple polygons, but as far as I understand, they can at the moment as well):

[monsieurtanuki]

I would say we should support both those use-cases - as we did before.

Ok.

That becomes a really difficult solution I guess [...]

PolygonLayer.inverted with Color invertedFill sounds reasonable.
Or even just Color? invertedFill without any new constructor.

I'm also not 100% sure still why we couldn't make the current usage pattern work, also for the PolylineLayer. Would it be possible to have a property that stops the element from passing over the anti-meridian?

Just adding intermediary longitude points would do the trick, as the next point of a poly is the closest (the problem is when you try to jump directly from -180 to 180).

I haven't tested it, but surely this current solution would still have the exact same issue as before. As unlikely the real world use case, what if someone wanted something like below (ofc they could just use multiple polygons, but as far as I understand, they can at the moment as well):

No problem with intermediary longitude points.

That wouldn't support the "hit an inverted polygon/hole" feature, though.

[monsieurtanuki]

Fun fact: with my 1980's background the "inverted" polygon feature looks a bit like what was then called "video reverse". But it wasn't easy to find that term on the web now because I rather found tutorials on how to play a video backwards - which wasn't a topic back then.

https://en.m.wikipedia.org/wiki/Reverse_video

[JaffaKetchup]

Yeah, so I guess we introduce a new property Color? invertedFill, and we can also introduce bool invertHitting. We don't particularly need a new constructor for it, and both of those properties can be used seperately. Polygon.fillColor then applies to holes in each polygon. How much do you think that would complicate the painter?

(Yep, I can imagine "reverse video" being difficult to search for :D)

[JaffaKetchup]

I have realised that bool invertHitting doesn't really make sense. Instead, we could introduce a R? hitValue on the layer level, returning that as hit if no elements are hit. Again, this could be used independently.

[monsieurtanuki]

@JaffaKetchup The more I think about it, the more I do believe we should put the "inverted" info at the PolygonLayer level, with a new Color? invertedFill field.
We could even have a fully alpha map (except for the polygons inside this layer) PLUS alpha polygons with a different color.
Basically, that would say that we would somehow keep this PR's display of polygons as holes, and then display normally each polygon.

[JaffaKetchup]

The more I think about it, the more I do believe we should put the "inverted" info at the PolygonLayer level, with a new Color? invertedFill field.

Agreed.

We could even have a fully alpha map (except for the polygons inside this layer) PLUS alpha polygons with a different color.
Basically, that would say that we would somehow keep this PR's display of polygons as holes, and then display normally each polygon.

I'm not quite following what you mean here? If you mean moving the inverted option all the way to MapOptions, then that might not give enough flexibility.

In regards to allowing the hit testing to be inverted, I've opened #2050. It also gives some other options, and applies to all the feature layers.

[monsieurtanuki]

I'm not quite following what you mean here?

I'll show you a screenshot when I'm ready.

In regards to allowing the hit testing to be inverted, I've opened #2050.

I'm not sure we need the hit testing to be inverted, or at least I cannot imagine a typical use-case.

[monsieurtanuki]

@JaffaKetchup This is what I'm coding right now:

• Color? invertedFill at the layer level
• "normal" polygons, with points
• clickable polygons
• polygons with different border colors, potentially with no fill color

Something like that:

[monsieurtanuki]

@JaffaKetchup I think I've nailed it (latest push).

[JaffaKetchup]

This has really complicated things huh :D. Thanks for spending even more time on this again.

Unfortunately I think this is the sort of thing really difficult to test and iron out all of the edge cases.

Here's one (inverting the existing example page) - the intersection of two overlapping polygons should be transparent (as both the polygons are), but the inverted color is shown instead:

When hovering, another transparent polygon is added. Again, it should be transparent through to the map. Instead, the aforementioned intersection becomes transparent, and the other area becomes the inverted fill. I guess the blending or winding is wrong again - there's no issue with transparent over filled, just transparent over transparent or intersecting.

[monsieurtanuki]

@JaffaKetchup I guess the code needs some fixing.
I wouldn't say that things are "more complicated" now, though. I believe the "hole polygons" idea was a dead-end (e.g. for border styles and hit values).
The code is easier to read now, especially if you read the diffs from before this PR - and not from before the latest push.

We're now dealing with features not simple to apprehend: intersecting polygons on an inverse layer. Not an "of course we should display the map that way" situation.

I'm not sure I understand correctly all the issues, but I think what you'd want would be:

• painting all the map in black
• painting all the polygons (points and holes) as white
• use this "map" as a mask on top of the real map
• and then, display "normally" the polygons

Or something like that.

Another solution would be to say that the inverted polygon layer isn't compatible (temporarily?) with all features, e.g. intersecting polygons or holes.

[JaffaKetchup]

Yeah, I think what you've said there is what I would be aiming for, but we want holes in inverted polygons to appear black (as they currently are in this PR). It should be the same behaviour as could be created before (by creating the big polygon with several holes).

One thing to consider is overlaps and outlines, and overlaps and holes. Take the following picture:

What do we do with the hole area here, which overlaps the polygon underneath? What do we do with the outlines?

[monsieurtanuki]

@JaffaKetchup Again, that's going too far, I cannot picture what we want in the end.

I suggest we switch from theoretical mode ("what alpha polygons (with partially outside holes) intersecting each other are supposed to look like on an inverted alpha polygon layer?") (no idea) to engineer mode ("alpha inverted polygon layers would be a nice feature - displaying full polygons makes sense, displaying polygons with realistic holes makes sense too, but beyond that there are probably no use-cases and anyway the output wouldn't be implicitly understandable) (which is probably in contradiction with the use of a map)

[JaffaKetchup]

Ok, sorry, not 100% sure what you mean! I fully agree that the new approach is much better than doing it per-Polygon.

displaying full polygons makes sense, displaying polygons with realistic holes makes sense too, but beyond that there are probably no use-cases and anyway the output wouldn't be implicitly understandable) (which is probably in contradiction with the use of a map)

I'm not suggesting necessarily doing anything beyond. In my mind, inverted means everything outside of any polygon should be the specified color. Alternatively, it could mean literally painting the opposite of what we're currently doing.
In that case, an intersection of two polygons in the normal view, if they were both transparent, left a transparency. Therefore, this should not leave a transparency - it should fill with the specified color. That is what this PR is currently doing. Maybe that's a good approach, but I feel as though it could be unexpected? Wdyt?
However, in the first case (everything outside of any polygon should be the specified color), the overlapping transparencies should remain transparent, since that area is still within a polygon. It also leads that holes should appear in the specified color since they are out of a polygon. But it is not clear to me what would be expected if the hole overlapped another polygon. This isn't true-inverse though, so I can see why maybe that's not clear.

[JaffaKetchup]

Just given it a test, and made useEvenOdd dependent on kIsWeb. I think it's all good, but I'll give it a more thorough test tomorrow!

[JaffaKetchup]

I've completed the testing, it seems to work fine. There does unfortunately appear to be a performance penalty with Path.combine, I guess due to the repetitive variable re-assignment. Is there any way around this?

And before merging, I'll also add a bounty.

[monsieurtanuki]

I've completed the testing, it seems to work fine. There does unfortunately appear to be a performance penalty with Path.combine, I guess due to the repetitive variable re-assignment. Is there any way around this?

@JaffaKetchup I'm not sure what you mean with "repetitive variable re-assignment"

      holePaths = Path.combine(
        PathOperation.union,
        holePaths,
        Path()..addPolygon(offsets, true),
      );

1. You mean holePaths being used as a return value AND as a parameter? Don't know how much it costs. If needed we can use a dedicated buffer variable.
2. As a former Java boy, I would say repeatedly using a new Path() isn't such a good idea. In that case, the optim would be to reset the same Path tmpPath initialized as Path().
3. Perhaps, for the PathOperation.union we would be better off just adding the polygon, like holePaths.addPolygon(offsets, true), but TBH I've lost the track of when to use what.
4. Another solution would be to use Path.combine only in the inverted fill mode, assuming that in the "standard" mode the evenOdd mode was working with the same results and faster.

[josxha]

I'm currently not able to test the behavior but the code changes look alright to me. @monsieurtanuki thanks for your pull request!

[monsieurtanuki]

Thank you @JaffaKetchup @josxha for your reviews and comments!

[JaffaKetchup]

You mean holePaths being used as a return value AND as a parameter? Don't know how much it costs. If needed we can use a dedicated buffer variable.

Yeah, this is kinda what I meant. It creates a new _NativePath behind the scenes everytime.

As a former Java boy, I would say repeatedly using a new Path() isn't such a good idea. In that case, the optim would be to reset the same Path tmpPath initialized as Path().

Just experimented with this, it makes no significant difference whatsoever - maybe 1ms in the stress test on the raster thread, so almost nothing in the real world.

Also tried minimizing the number of combine operations by adding multiple polys to one path first, but of course, that gives the wrong result (since each poly cuts into each other).

Perhaps, for the PathOperation.union we would be better off just adding the polygon, like holePaths.addPolygon(offsets, true), but TBH I've lost the track of when to use what.

This is what we do for evenOdd, and it doesn't seem to work properly.

Another solution would be to use Path.combine only in the inverted fill mode, assuming that in the "standard" mode the evenOdd mode was working with the same results and faster.

I've done this. evenOdd does work on native for non-inverted, so I've enabled that.

[JaffaKetchup]

LGTM at last! Before we merge, I'll write a bounty spec and assign this.
I won't include the feature for PolylineLayer and CircleLayer - I'll leave this open for the future, since it wasn't possible before. If I have time, I'll try to implement it based on this.

[monsieurtanuki]

@JaffaKetchup We may not need the "Inverted Polygons" / "Advanced polygons" example page anymore, now that you've updated the standard "Polygon Layer" page.
Anyway "Inverted Polygons" is a deprecated term now ;)

[JaffaKetchup]

Ah, good point, forgot to remove it. I'll do it at some point.

I've setup the bounty on Polar, but it turns out they're sunsetting bounties soon (which is a bit annoying), so they need to mark this one as complete manually due to a bug or something. Hopefully it won't take too long.

[JaffaKetchup]

We've paid the bounty through GitHub Sponsors instead :). You should get the entire $170 USD (we paid $10.20 in fees).

[monsieurtanuki]

We've paid the bounty through GitHub Sponsors instead :). You should get the entire $170 USD (we paid $10.20 in fees).

Thank you so much!

[JaffaKetchup]

Just a note:

We might need to change the default of _useEvenOdd again. I think the performance impacts (~3ms on the stress test) of combine are not bad enough to justify preferring even/odd. And the artifacts from even/odd on intersecting holes are annoying, especially as they are not consistent with the behaviour in inverted mode (which is correct, since that always uses combine) which is confusing.

[monsieurtanuki]

We might need to change the default of _useEvenOdd again.

Whatever works. As long as it works, the performances are good and it's easy to maintain ;)