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

Move guide building to ggplot_build() #5483

Merged
merged 10 commits into from Oct 30, 2023

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented Oct 13, 2023

This PR aims to take a step towards #3648.

Briefly, we might want to expose the plot data to the guides, so that the guides can observe the data they're supposed to represent. For this to happen, we cannot use the data available in ggplot_gtable() because the after_scale() transformations have been applied already and the original data is no longer suitable. Hence, we would need to move guide building to a point before after_scale() transformations. That last part is what this PR does and it is important to note that it does not do the data exposure (yet).

In order to achieve this, the following has to be taken into account:

  • The Guides$draw() step needs to remain in ggplot_gtable() because it needs access to the theme.
  • All earlier Guides methods need to be independent of the theme.

This means that guides don't know about the position or direction of the guide until the Guide$draw() step. That has the following implications:

  • For the most part, this simply required validating directions/positions at the draw method instead of the train method.
  • The hash used for checking uniqueness of guides can't incorporate the direction or position anymore, so some random order of guides have changed. I've fixed the order in some unit tests to prevent similar issues in the future.

More granular details in the comments below:

R/plot-build.R Outdated Show resolved Hide resolved
@teunbrand
Copy link
Collaborator Author

teunbrand commented Oct 24, 2023

I've now added the plumbing to make sure that the plot's data reaches the guide building stage where guides interact with layers (at the Guide$get_layer_key method).

For this to make slightly more sense, I've added a process_layers method to guides, which only job currently is to see if a layer should be included and pass on the correct layers to get_layer_key().

As a small demonstration that the plumbing works, here is a quick and dirty guide that reports which keys are actually represented in layers.

devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2

orig <- guide_legend()
new <- ggproto(NULL, orig, get_layer_key = function(params, layers, data) {
  x <- params$key$.value %in% data[[1]]$colour
  print(setNames(x, params$key$.value))
  GuideLegend$get_layer_key(params, layers, data = data)
})

p <- ggplot(mtcars, aes(disp, mpg)) +
  geom_point(aes(colour = as.factor(cyl))) +
  scale_colour_discrete(limits = factor(4:8)) +
  guides(colour = new)

b <- ggplot_build(p)
#>     4     5     6     7     8 
#>  TRUE FALSE  TRUE FALSE  TRUE

Created on 2023-10-24 with reprex v2.0.2

@teunbrand teunbrand marked this pull request as ready for review October 24, 2023 13:44
@teunbrand teunbrand changed the title WIP: Move guide building to ggplot_build() Move guide building to ggplot_build() Oct 24, 2023
Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Heads up @yutannihilation, @clauswilke, @hadley, @karawoo, @paleolimbot - this alters the build order which is generally sacred but we do this for good reasons.

It opens the door for much richer guides together with the guide extension possibilities that @teunbrand has developed

@teunbrand teunbrand merged commit 42a764d into tidyverse:main Oct 30, 2023
12 checks passed
@teunbrand teunbrand deleted the separate_guide_building_drawing branch October 30, 2023 09:36
@teunbrand teunbrand mentioned this pull request Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants