Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Holed polygons #3128

Merged
merged 7 commits into from Feb 12, 2019
Merged

Holed polygons #3128

merged 7 commits into from Feb 12, 2019

Conversation

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Feb 11, 2019

This PR adds the ability to draw polygons with holes to geom_polygon. It does so by adding a new, optional, aesthetic called subgroup. If provided, and R >= 3.6 the geom will use the new improved pathGrob to draw the polygons.

Questions to mull over:

  • Do you agree on the subgroup name. I've elected for a generic name so it can be reused elsewhere if secondary grouping is needed, rather than using something that signifies the "hole"-ness of it
  • Currently the geom will draw the path using the "evenodd" rule as this is how most people will probably think it works (winding order is quite specialised knowledge). It could be made a parameter of the geom, but isn't currently as I think it would just muddy the waters

Example:

ids <- factor(c("1.1", "2.1", "1.2", "2.2", "1.3", "2.3"))

values <- data.frame(
  id = ids,
  value = c(3, 3.1, 3.1, 3.2, 3.15, 3.5)
)

positions <- data.frame(
  id = rep(ids, each = 4),
  x = c(2, 1, 1.1, 2.2, 1, 0, 0.3, 1.1, 2.2, 1.1, 1.2, 2.5, 1.1, 0.3,
        0.5, 1.2, 2.5, 1.2, 1.3, 2.7, 1.2, 0.5, 0.6, 1.3),
  y = c(-0.5, 0, 1, 0.5, 0, 0.5, 1.5, 1, 0.5, 1, 2.1, 1.7, 1, 1.5,
        2.2, 2.1, 1.7, 2.1, 3.2, 2.8, 2.1, 2.2, 3.3, 3.2)
)
datapoly <- merge(values, positions, by = c("id"))

holes <- do.call(rbind, lapply(split(datapoly, datapoly$id), function(df) {
  df$x <- df$x + 0.5 * (mean(df$x) - df$x)
  df$y <- df$y + 0.5 * (mean(df$y) - df$y)
  df
}))
datapoly$subid <- 1L
holes$subid <- 2L
datapoly <- rbind(datapoly, holes)

p <- ggplot(datapoly, aes(x = x, y = y)) +
  geom_polygon(aes(fill = value, group = id, subgroup = subid))
p

image

@thomasp85 thomasp85 requested review from hadley and clauswilke Feb 11, 2019
@clauswilke
Copy link
Member

@clauswilke clauswilke commented Feb 11, 2019

Thanks for making this happen! I can’t really test this right now, because I don’t want to touch my R install during the final stages of book production, in case I have to re-render any figures.

I like the name subgroup, since as you say it could be used in other contexts as well.

I think winding order should be made available as an option. I originally thought I’d need it for isobands, but thinking some more about it it may not matter. Am I correct that for perfectly nested polygon rings running in alternating directions evenodd and winding order give the same result?

It would be good to test this with the isoband output on some non-trivial datasets. Would you mind trying this out? The isobands() function returns a list of polygons specified just like grid.path() expects them, with x, y, and id, so should be clear how to convert this into four columns x, y, group, subgroup.

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Feb 11, 2019

One question: Do the polygon paths need to be closed (last point = first point) or not? grid.polygon() and grid.path() behave differently in this regard, I believe.

R/geom-polygon.r Outdated Show resolved Hide resolved
R/geom-polygon.r Outdated Show resolved Hide resolved
munched <- munched[order(munched$group), ]

# For gpar(), there is one entry per polygon (not one entry per point).
# We'll pull the first value from each group, and assume all these values
Copy link
Member

@clauswilke clauswilke Feb 11, 2019

Other geoms check this assumption and issue a warning if an aesthetic changes where it’s not supposed to. See e.g. geom_area(). Needed here?

Copy link
Member Author

@thomasp85 thomasp85 Feb 11, 2019

I think that may become prohibitly expensive to check for the kind of data that people will throw at this... Maybe make it more clear in the docs?

Copy link
Member

@clauswilke clauswilke Feb 11, 2019

Do you think a single call to unique() is that much more expensive than, say, the ordering of the data that is also happening?

ggplot2/R/geom-ribbon.r

Lines 80 to 83 in 18be30e

aes <- unique(data[c("colour", "fill", "size", "linetype", "alpha")])
if (nrow(aes) > 1) {
stop("Aesthetics can not vary with a ribbon")
}

At a minimum, this might be worth a careful benchmark on a very large dataset, such as the output from isobands() on a large raster image.

Copy link
Member Author

@thomasp85 thomasp85 Feb 11, 2019

I’ll happily do the benchmark, but the big difference is that geom_ribbon uses a draw_group method whereas geom_polygon uses draw_panel. This means that geom_polygon would have to split up the data and call unique on each sub-data.frame all for the sake of a possible warning. For geom ribbon the split has already been done so it is rather cheap

Copy link
Member

@clauswilke clauswilke Feb 11, 2019

I see. In any case, it's your call. I won't insist.

Copy link
Member

@hadley hadley Feb 12, 2019

We may wanted to eventually tackle this problem by providing a new geom where there's one row per polygon and the vertices are stored in a nested data frame.

R/geom-polygon.r Outdated Show resolved Hide resolved
R/geom-polygon.r Show resolved Hide resolved
@thomasp85
Copy link
Member Author

@thomasp85 thomasp85 commented Feb 11, 2019

For the example you describe "evenodd" and "winding" should give the same result. I don't feel strongly about omitting it so I'll add a rule argument to control it (which will default to "evenodd")

pathGrob uses the same open polygon spec as polygonGrob so no need to repeat the last point

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Feb 12, 2019

I just wanted to confirm that I tried "evenodd" with the current geom.path() and it renders my polygons as I'd expect. So I'm good with "evenodd" as the default.

@@ -44,3 +44,5 @@ ggplot_global$all_aesthetics <- .all_aesthetics
)

ggplot_global$base_to_ggplot <- .base_to_ggplot

grid_has_multipath <- packageVersion('grid') >= "3.6"
Copy link
Member

@hadley hadley Feb 12, 2019

I don't think computing this value at build time is correct. Instead, I think you should just inline into geom_polygon().

hadley
hadley approved these changes Feb 12, 2019
Copy link
Member

@hadley hadley left a comment

I'm fine with subgroup, and don't have particularly strong feelings otherwise (except that computing grid_has_multipath when the package is built is not correct).

@thomasp85 thomasp85 merged commit 9a87145 into tidyverse:master Feb 12, 2019
0 of 2 checks passed
@lock
Copy link

@lock lock bot commented Aug 11, 2019

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/

@lock lock bot locked and limited conversation to collaborators Aug 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants