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
Holed polygons #3128
Changes from 3 commits
60e4f48
858ca8f
2d8b1ff
1901c51
99f0166
5f6925f
5cdc09a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think computing this value at build time is correct. Instead, I think you should just inline into |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. In any case, it's your call. I won't insist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.