Implement GeomSf$handle_na()

[yutannihilation]

Fix #3536

[yutannihilation]

No idea why builds on 3.3~3.5 fails... https://travis-ci.org/tidyverse/ggplot2/jobs/588242890#L4107-L4120

[clauswilke]

It seems strange to me to only handle points and to only handle them when all rows contain exclusively points. I suspect there needs to be a separate treatment for sf types point, line, and other, and it'll probably require a reimplementation of remove_missing() for this specific case.

[thomasp85]

I think the pragmatic approach would be to handle this in sf_grob() as we are already inspecting each rows geometry type there. @yutannihilation does that make sense to you?

[yutannihilation]

Yes, it makes sense because it's only sf that can access to the individual geometry including the ones in GEOMETRYCOLLECTION. I was thinking how I can implement in the way Claus suggested above, but I couldn't see how to do make ggplot2 generate the same plot in the following two ggplot() codes...:

library(sf)
library(ggplot2)

pt <- st_point(1:2)

outer <- matrix(c(0,0,10,0,10,10,0,10,0,0),ncol=2, byrow=TRUE)
pl <- st_polygon(list(outer))

sf1 <- st_sf(geometry = st_sfc(pt, pl))
ggplot(sf1) + geom_sf(fill = "red", colour = NA)

sf2 <- st_sf(geometry = st_sfc(st_geometrycollection(list(pt, pl))))
ggplot(sf2) + geom_sf(fill = "red", colour = NA)

In my understanding, in both case, the point should be removed, but the polygon should not. However, it seems difficult to handle it on ggplot2's side.

[thomasp85]

sf_grob() is a ggplot2 function, so you can do it there... before it passes the data on to st_as_grob()

[yutannihilation]

Hmm, sorry, I confused it with st_as_grob()...

[clauswilke]

I think that what all of this shows is that our current approach to generating sf grobs is broken. We are not handling nested geometries correctly. See the following reprex.

library(sf)
#> Linking to GEOS 3.7.2, GDAL 2.4.2, PROJ 5.2.0
library(ggplot2)

pt <- st_point(1:2)

outer <- matrix(c(0,0,10,0,10,10,0,10,0,0),ncol=2, byrow=TRUE)
pl <- st_polygon(list(outer))

# point is shown
sf1 <- st_sf(geometry = st_sfc(pl, pt))
ggplot(sf1) + geom_sf()

# point is not shown, because GeomPolygon$default_aes doesn't contain a shape value
sf2 <- st_sf(geometry = st_sfc(st_geometrycollection(list(pl, pt))))
ggplot(sf2) + geom_sf()

# now it's shown (but notice size difference from first plot)
GeomPolygon$default_aes$shape <- 19
ggplot(sf2) + geom_sf()

^{Created on 2019-09-27 by the reprex package (v0.3.0)}

I think the options are to either implement our own version of sf::st_as_grob() or work with the sf package authors to change how graphical parameters are handed to that function and how missing values are handled. It may be easier to implement our own function.

[thomasp85]

Well, I'm the author of the implementation in sf, but better support for GeometryCollection is orthogonal to this issue I think.

I don't see why we do not filter in sf_grobs() it is a natural place to do it as all necessary information is available there

[clauswilke]

I don't see why we do not filter in sf_grobs() it is a natural place to do it as all necessary information is available there

I think the point is this: If we want to remove, say, all points that have a color of NA, and to do it right, we'd have to recursively descend into GeometryCollections to make sure we catch all cases. At that point, we might as well reimplement all of sf::st_as_grob() because it'll be a comparable amount of code anyways.

[thomasp85]

I disagree. GeometryCollections is really a niche case due to bad performance and poor support for styling so reimplementing the whole grob conversation for that is overkill. I’ll throw together a poc for what I have in mind tomorrow so I can show what I’m getting at

[yutannihilation]

Sorry, I didn't follow the discussion. Thank you Thomas for #3546, I'm closing this PR in favor of yours.

[lock]

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/