Refactor facet panel drawing code #5917
Conversation
|
TODO: try unifying |
|
The |
| if (!is.null(aspect_ratio) && (space$x || space$y)) { | ||
| cli::cli_abort("Free scales cannot be mixed with a fixed aspect ratio.") | ||
| } |
There was a problem hiding this comment.
This won't be triggered for FacetWrap as it has no params$space_free, which defaults to list(x = FALSE, y = FALSE).
| placement <- if (inside_x) -1 else -2 | ||
| strip_pad <- axis_size$top | ||
| draw_panels = function(self, panels, layout, x_scales, y_scales, ranges, coord, data, theme, params) { | ||
| if (inherits(coord, "CoordFlip")) { |
There was a problem hiding this comment.
If failed to find a smarter solution to working out this interaction with CoordFlip, so FacetWrap does this first, then calls the parent method.
| @@ -33,7 +33,7 @@ | |||
|
|
|||
| # facet_grid throws errors at bad layout specs | |||
|
|
|||
| `coord_fixed()` doesn't support free scales. | |||
| `facet_grid()` can't use free scales with `coord_fixed()`. | |||
There was a problem hiding this comment.
This wording changed slightly because FacetGrid and FacetWrap now share the method that emits this warning
|
CI is a bit out of whack because non-MacOS builds use most recent {cli} 3.6.3 whil I presume MacOS still uses an older version. |
This PR aims to fix #5482.
Briefly, the panel drawing code is now divided over 3 Facet ggproto methods:
init_gtable(),attach_axes()andattach_strips().My main gripe with the previous code was that it was hard to follow along with what it was doing. I think reasons why it was hard for me to follow are the following:
facet_grid()was first setting up axes, then setting up strips, then initialising the table, then adding the axes and finally adding the strips.ifblocks deep which is hard to keep track of.The primary benefit is that it should now be easier to 'just' tweak the axes, or 'just' tweak the strips in extensions.
Quick benchmark showed no speed difference between current main and this PR.