Add possibility to plot holes in plolygons (support for `grid.path()`)

[courtiol]

As discussed elsewhere (oscarperpinan/rastervis#101) it would be great to add native support in lattice for drawing polygons with holes.

FYI, here is how they did it in sp:
https://github.com/edzer/sp/blob/fe2993112e1d7d6f0c56b8595041f0a5ee5ef895/R/spplot.R#L1-L47

[courtiol]

@deepayan shall I attempt a PR, or is it something you could do soonish?
(I am not sure I can manage -- a little out of my depth here--, but I badly need this to be solve since I moved away from sp to terra in a package -- IsoriX -- and I oversaw this critical point for my usage, so now a key feature of IsoriX is broken...)

[deepayan]

Sure, let me try...

[deepayan]

I have added a rule argument to lpolygon.default(); check if this works for you. You will need to specify rule="evenodd" or rule="winding". For the example in oscarperpinan/rastervis#101, the following works:

rasterVis::levelplot(worldelev_terra, margin = FALSE) +
  latticeExtra::layer(lpolygon.SpatVector(oceans_terra, border = "yellow", 
                                          col = "blue", rule = "winding"))

This is slow for the on-screen quartz device, but fast for pdf.

[courtiol]

Wonderful, it seems to work fine on my side.

One open question for me is what the default value for rule should be?
I understand that from a theoretical backward compatibility perspective, "none" is the right choice.
Yet, I don't know if in practice anyone would combine polygons within the same shape if not to draw holes (in such a case "evenodd" would be more convenient)...
I guess we will never be sure and should perhaps stick to "none".

In any case, perhaps, to better illustrate the rule argument, it would be great to add the following examples adapted from Murrell's paper (https://journal.r-project.org/archive/2012-2/RJournal_2012-2_Murrell2.pdf):

x <- c(.1, .5, .9, NA,
       .4, .5, .6, NA,
       .4, .6, .5)

y <- c(.1, .8, .1, NA,
       .5, .4, .5, NA,
       .3, .3, .2)

d <- data.frame(x = x, y = y)

lattice::xyplot(y ~ x, data = d, panel = lattice::panel.polygon, rule = "none", col = "grey")

lattice::xyplot(y ~ x, data = d, panel = lattice::panel.polygon, rule = "evenodd", col = "grey")

lattice::xyplot(y ~ x, data = d, panel = lattice::panel.polygon, rule = "winding", col = "grey")

^{Created on 2023-10-11 with reprex v2.0.2}

Thanks again!

[deepayan]

Yes, rule="none" would be useful very rarely, but I would like to keep that the default for the sake of backwards compatibility (and perhaps efficiency for some devices). It may make sense to change the default for lpolygon.SpatVector().

It may also make sense to add lpolypath() etc. similar to graphics::polypath() in lattice.

[courtiol]

Shall we define lpolypath() as a wrapper around lpolygon.default() with rule = "evenodd" as default?
I am happy to PR that while adding the example above if that helps.

[deepayan]

No need for a PR, it's going to be trivial... The default of rule = "evenodd" would then be different from the default in polypath(), but I guess that's OK.

[courtiol]

My 2 cts...

I have little experience about the effect of the rules for practical applications.

From the examples I found, "evenodd" seems to reveal more holes, while "winding" (called "nonzero" below) seems to keep polygons pilling on top of each others (see e.g. section More background below).

In "sp", they used "winding" as default:
https://github.com/edzer/sp/blob/fe2993112e1d7d6f0c56b8595041f0a5ee5ef895/R/AAA.R#L9

And as you said, in polypath() they did the same.
So I think it would be best for you to stick to "winding" as default for lpolypath().

If that is not best for "rasterVis" applications (but I think it makes no difference in most cases), we can always change the default there, but "lattice" being a package for widespread use, I would be consistent with base R choices.

More background

From https://www.w3.org/TR/SVG/painting.html#FillRuleProperty:

[deepayan]

Yes, I think it's best to be consistent. I have now added lpolypath() etc.

[courtiol]

Ok, I think you can now close this issue.
Do you already know your time line before the next CRAN release of lattice?
Many thanks again!

[deepayan]

I'm hoping to do a release 'soon'. Let's keep this open till then.