Memoise descent

[thomasp85]

The current caching of descentDetails calls is based on the device in use, along with all relevant font specifications.

I believe @clauswilke has looked into something similar but unsure what he ended up with suggesting - this works well, though...

[hadley]

If we do more memoisation, I think adding a dependency on memoise would be fine.

[thomasp85]

Agree - this started off by using memoise, but the digest call was a measurable overhead when the arguments to memoise on were simple strings and integers that could be pasted together to a key

[clauswilke]

Yes, this is basically what I did here: https://github.com/clauswilke/gridtext/blob/f46c0caa018ed32fefc2ba13240e272b37ba7eed/R/grob-descent.R#L25-L55

Two comments:

1. This does not yet fix Descent heights still not right #2687, correct?

2. Should the descent cache be added to ggplot2_global? I had added that global environment a while back to collect all these global variables in one place.
   

   ggplot2/R/ggplot-global.R

   Lines 1 to 4 in f5a88a7

   	# Environment that holds various global variables and settings for ggplot,
   	# such as the current theme. It is not exported and should not be directly
   	# manipulated by other packages.
   	ggplot_global <- new.env(parent = emptyenv())

[thomasp85]

1. It does not - I had forgot about that issue, but will incorporate it

2. I'm a bit unsure about the purpose of ggplot_global so I can't really say if it should be added

[hadley]

I think it's probably better to have multiple independent environments that are declared close to the code that uses them.

[thomasp85]

1 is now fixed in #2996

[clauswilke]

The history of ggplot_global is that I started working on user-customizable theme elements and for that needed to put the element tree into an environment. While doing that, I found a few other global variables that were strewn about in the code and thought it might be good to put them all into one place. This is the PR: #2741, though it doesn't seem to have the associated discussion on this topic.

One difference between those global variables and the font cache here is that the font cache should never be accessed by anything other than the font_descent() function. By contrast, the element tree is accessed by numerous functions all over the codebase.

Of course, then @hadley suggested to make the element tree part of the theme itself, and if we went that route then we won't ever need to interactively change the default element tree that is stored in the global element tree variable, and thus could conceivably get rid of ggplot_global after all. There's a stalled PR on that topic, I'll have to revive it at some point: #2784

[lock]

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/