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

ggsave should use plot theme for default background #4057

Closed
hadley opened this issue Jun 10, 2020 · 12 comments · Fixed by #4164
Closed

ggsave should use plot theme for default background #4057

hadley opened this issue Jun 10, 2020 · 12 comments · Fixed by #4164

Comments

@hadley
Copy link
Member

hadley commented Jun 10, 2020

I think @djnavarro tweeted about this recently, and I hit it too. It seems like in a situation like this:

plot %>%
  ...
  theme(
   plot.background = element_rect(fill = NA, colour = NA)
  )

ggsave("ratios.png", width = 8, height = 4.5, bg = NA)

ggsave() could automatically figure out what bg should be and pass it along to the graphics device.

@thomasp85

This comment has been minimized.

@hadley

This comment has been minimized.

@mcanouil
Copy link

mcanouil commented Jun 22, 2020

Hi,

i have been using a customised ggsave for that purpose, although it might not be the best approach to handle that (edit: not exactly for NA though, but for consistency with the plot background at least which is related).
Here it is (initially from https://github.com/mcanouil/mctemplates/blob/8a42d07300841ab2e3936ab63fe92a25a0781bff/R/theme_black.R#L277)

ggsave <- function(
  filename,
  plot = ggplot2::last_plot(),
  device = NULL,
  path = NULL,
  scale = 1,
  width = NA,
  height = NA,
  units = c("in", "cm", "mm"),
  dpi = 300,
  limitsize = TRUE,
  ...
) {
  if (is.null(plot$theme$plot.background$colour)) {
    bc <- ggplot2::theme_get()$plot.background$colour
  } else {
    bc <- plot$theme$plot.background$colour
  }

  ggplot2::ggsave(
    filename = filename,
    plot = plot,
    device = device,
    path = path,
    scale = scale,
    width = width,
    height = height,
    units = units,
    dpi = dpi,
    limitsize = limitsize,
    bg = bc,
    ...
  )
}

I did the same tweak for the print method https://github.com/mcanouil/mctemplates/blob/8a42d07300841ab2e3936ab63fe92a25a0781bff/R/theme_black.R#L231

I'll probably make my first PR to ggplot2 in the next few days.

@clauswilke
Copy link
Member

@mcanouil I'm not sure this is the right approach, as it short-circuits the regular ggplot theme calculations.

In general, this is a tricky problem that I expect will require us to separately call ggplot_build() and ggplot_gtable() to be able to extract the theme without generating any grobs before the graphics device is set up.

@thomasp85
Copy link
Member

@clauswilke which other places could the plot background be calculated? I think it is fair to expect it to either be set on the plot theme or get it from the default theme in lieu of having to implement yet another rendering routine

@clauswilke
Copy link
Member

Most importantly, I think it's a matter of keeping the code clean. We just spent a lot of effort removing one-off interactions with the theme system and trying to boil all interactions down to a small set of shared functions that are reliably called. This proposed patch would go in the opposite direction.

To your specific question, this is the code the calculates the final theme:

ggplot2/R/theme.r

Lines 420 to 447 in 112f960

plot_theme <- function(x, default = theme_get()) {
theme <- x$theme
# apply theme defaults appropriately if needed
if (is_theme_complete(theme)) {
# for complete themes, we fill in missing elements but don't do any element merging
# can't use `defaults()` because it strips attributes
missing <- setdiff(names(default), names(theme))
theme[missing] <- default[missing]
} else {
# otherwise, we can just add the theme to the default theme
theme <- default + theme
}
# if we're still missing elements relative to fallback default, fill in those
missing <- setdiff(names(ggplot_global$theme_default), names(theme))
theme[missing] <- ggplot_global$theme_default[missing]
# Check that all elements have the correct class (element_text, unit, etc)
if (is_theme_validate(theme)) {
mapply(
validate_element, theme, names(theme),
MoreArgs = list(element_tree = get_element_tree())
)
}
theme
}

One issue that the current proposed patch doesn't handle is if the global theme does not have a properly defined background element.

@clauswilke
Copy link
Member

The other issue is that theme elements really should be calculated with calc_element() is we want to keep the code maintainable and predictable going forward.

calc_element <- function(element, theme, verbose = FALSE, skip_blank = FALSE) {

@thomasp85
Copy link
Member

Those are fair points. I think the path forward should be to provide a utility function that can properly calculate a given theme element and then use that. Splitting up the build process for this seems like the wrong direction to take as well

@clauswilke
Copy link
Member

If you don't mind that ggsave() will duplicate the theme calculation, you could simply do:

bg <- calc_element("plot.background", plot_theme(plot))
bg_colour <- bg$colour

@thomasp85
Copy link
Member

If that is all it takes then I think we should do that (if background is set to NULL)

@clauswilke
Copy link
Member

Yeah, I think this would work. Note we need fill, not colour.

library(ggplot2)

theme_set(theme_gray() + theme(plot.background = element_rect(fill = "cornsilk")))

p1 <- ggplot(mtcars, aes(disp, mpg)) + geom_point()
p2 <- p1 + theme(plot.background = element_rect(fill = "skyblue"))
p3 <- p1 + theme_gray()

calc_element("plot.background", ggplot2:::plot_theme(p1))$fill
#> [1] "cornsilk"
calc_element("plot.background", ggplot2:::plot_theme(p2))$fill
#> [1] "skyblue"
calc_element("plot.background", ggplot2:::plot_theme(p3))$fill
#> [1] "white"

Created on 2020-06-22 by the reprex package (v0.3.0)

@mcanouil
Copy link

mcanouil commented Jun 23, 2020

The other issue is that theme elements really should be calculated with calc_element() is we want to keep the code maintainable and predictable going forward.

calc_element <- function(element, theme, verbose = FALSE, skip_blank = FALSE) {

At least, now I am aware of those internals (thanks!).
I wrote the tweak/patch with v2.2.0 at that time and did not update that part at all since then.

I wonder, if there is a computational "burden" in ggsave because of the double calculation with the proposed path using calc_element().
Is it worst or the same within the print/plot method, if we want the "interactive" device (x11/cairo/...) to get the background information as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants