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

Incorrect lineheight on high-resolution non-quartz devices #33

Closed
cpsievert opened this issue Apr 28, 2020 · 10 comments
Closed

Incorrect lineheight on high-resolution non-quartz devices #33

cpsievert opened this issue Apr 28, 2020 · 10 comments

Comments

@cpsievert
Copy link
Contributor

In the example below, note that without showtext enabled, all three plots below produce plots with consistent lineheight, but with showtext, the last two produce a much smaller lineheight

library(showtext)
library(ggplot2)

showtext::showtext_auto()

# lineheight appears ok
tmp <- tempfile(fileext = ".png")
png(tmp, type = "quartz", res = 220, width = 600, height = 400, units = "px")
print(qplot(1:10))
dev.off()
file.show(tmp)

# lineheight is much smaller (with showtext enabled)
tmp <- tempfile(fileext = ".png")
png(tmp, type = "cairo", res = 220, width = 600, height = 400, units = "px")
print(qplot(1:10))
dev.off()
file.show(tmp)

# lineheight is much smaller (with showtext enabled)
tmp <- tempfile(fileext = ".png")
Cairo::CairoPNG(tmp, res = 220, width = 600, height = 400, units = "px")
print(qplot(1:10))
dev.off()
file.show(tmp)
@yixuan
Copy link
Owner

yixuan commented Apr 28, 2020

The second is easy to fix. You just need to call showtext::showtext_opts(dpi = 220) before plotting.

The CairoPNG device is a bit complicated, and I'm thinking of ways to fix it.

@cpsievert
Copy link
Contributor Author

Great, thank you. Should knitr be setting showtext_opts(dpi = opts_current$get("dpi")) (when fig.showtext is TRUE)?

(cc @yihui)

@cpsievert
Copy link
Contributor Author

cpsievert commented May 1, 2020

Thanks again for your response @yixuan! Just curious, what are the prospects of a fix for CairoPNG (i.e., do you think it'll ever happen)? Totally ok to be honest if it's unlikely, just want to know what to expect.

@yixuan
Copy link
Owner

yixuan commented May 1, 2020

@cpsievert This is a bit more complicated than I thought. What showtext needs to know is whether a specific graphics device is bitmap-based (e.g. PNG) or vector-based (PDF), and the current implementation of showtext uses a guess based on the device name. But for Cairo devices, they share the same device name "Cairo", so showtext does not know whether it is a PNG or PDF or something else.

One possible solution is to ask the user to provide such information. This makes dealing with fonts easier, but is just "less automatic". Or I need to read the C code of the Cairo package to see if there is anything hidden in the internals that can help me distinguish different types.

@cpsievert
Copy link
Contributor Author

cpsievert commented May 1, 2020

Oh, good to know. And yea, maybe it's worth contributing to Cairo itself to add a filepath attribute to .Device (then you could try to infer based on the file extension), which is what grDevices does, for example:

> png("foo.png")
> .Device
[1] "quartz_off_screen"
attr(,"filepath")
[1] "foo.png"

I think we could just add that information here (I'm not very familiar with C, so I don't immediately see how that'd be done, but perhaps you'd know how to?)

@yixuan
Copy link
Owner

yixuan commented May 2, 2020

I think I have fixed this issue in the master branch. Can you give it a try?

@cpsievert
Copy link
Contributor Author

Yes, CairoPNG looks much better now with showtext_opts(dip = 220), thank you!

I also noticed in 39bcd28 that you've added ragg awareness, but I'm noticing some degradation in the font rendering. It seems like it'd be better to do nothing for ragg as it has its own API for rendering custom fonts (via systemfonts::register_font()).

library(showtext)
library(ggplot2)
showtext::showtext_auto()
showtext::showtext_opts(dpi = 220)
tmp <- tempfile(fileext = ".png")
ragg::agg_png(tmp, res = 220, width = 600, height = 400, units = "px")
print(qplot(1:10))
dev.off()
file.show(tmp)

Screen Shot 2020-05-02 at 5 20 12 PM

@yixuan
Copy link
Owner

yixuan commented May 3, 2020

I think this is a bug of ragg. You can find that if you change the bit size to be 16 in agg_png, the output image is correct.

library(showtext)
library(ggplot2)
showtext::showtext_auto()
showtext::showtext_opts(dpi = 220)
tmp <- tempfile(fileext = ".png")
ragg::agg_png(tmp, res = 220, width = 600, height = 400, units = "px", bitsize = 16)
print(qplot(1:10))
dev.off()
file.show(tmp)

a

I'll create an example and report there.

@cpsievert
Copy link
Contributor Author

Thanks again for the CairoPNG fix (and for submitting that fix to ragg)! 🍻

Looks as though this is working in all the cases I care about now, so I'm happy to close.

Just curious, do you have a timeline for a new showtext CRAN release?

@yihui
Copy link

yihui commented May 5, 2020

@cpsievert I just made the change in knitr to pass the dpi option to showtext_opts(). Thanks!

clrpackages pushed a commit to clearlinux-pkgs/R-knitr that referenced this issue Jun 25, 2020
Aaron A King (1):
      add engine for c++ to be compiled by 'R CMD SHLIB' (#1832)

Carson Sievert (2):
      Support for ragg::agg_png device (#1834)
      Pass options to showtext() (#1847)

Christophe Dervieux (1):
      fix #1845: correctly pass the dev.args option to the ragg_png device (#1846)

Ellis Hughes (1):
      fix #1838: format() columns individually for non-matrix objects in kable() (#1827)

Randy Lai (1):
      sql engine: check if there is more than one element in the first column (#1837)

Salim B (1):
      improve documentation (#1835)

Yihui Xie (44):
      start the next version
      forgot to update news in 4a50ee68
      the argument `error` of `include_graphics()` takes value from the global R option `knitr.graphics.error` by default
      fix the issue reported by @XiangyunHuang at https://d.cosx.org/d/421249
      allow users to customize the `sep` argument of the cat engine
      only use the echo option to control whether a chunk is included in the output via the `asis` engine
      for the dot and asy engines, first create the output file under the current working directory, then move it to the actual figure path
      roxygenize and bump version
      don't update texlive
      when there are multiple plots in a chunk, create labels by separateing labels and numbers with dashes
      roxygenize (after #1835) and bump version
      suggest ragg (amend #1834)
      support the chunk option dev.close = FALSE, and plan to drop the package option global.device = TRUE in future
      always restore par() if the package option global.par = TRUE
      indicate if a device is open by the package option dev.open
      set the package option dev.open outside chunk_device(), and after chunk_device() has been called successfully
      use a single argument `options` in chunk_device() since all other arguments are already in it
      don't deprecate the package option global.device, because it is actually quite complicated to implement a global device with a chunk option... the dev.close option won't work for child documents (e.g. this example failed: https://travis-ci.org/github/yihui/knitr/jobs/677962281)
      put the on.exit() call after when a new device has been opened (no need to store the device number in opts_knit)
      on exit, store par() in global.pars before the device is closed in a later on.exit() call
      pass the dpi option to showtext::showtext_opts(): yixuan/showtext#33 (comment)
      not sure if it matters, but set the dpi before showtext_begin()
      fix rstudio/bookdown#883: for LaTeX output in bookdown, do not write <div class="environment"> for Pandoc >= 2.7.3, because such a div would be converted to a LaTeX environment
      travis-ci.org -> .com
      add a markdown_mode argument to parse_block(), so parse_block() does not have to rely on opts_knit$get('out.format'), and users can specify if the document is in the markdown mode
      do not cache texlive
      not sure why LaTeX fails; try my own version of TeX Live instead
      include the content of the code chunk in the error message, to make it easier to find the chunk label that was duplicated in the document
      add a new function kables() to create a table containing multiple tables that are individually created from kable()
      c() all ... arguments in create_label(), instead of assuming that each of ... is of length 1 or 0
      amend ea7f3070ef42989d0ccc945eadfdd3c2454d0740: use dashes before fig numbers in the labels consistently in other output formats besides HTML
      move the R implementation of base64 encoding to the xfun package, and use xfun::base64_uri() instead of the unexported markdown:::.b64EncodeFile()
      a news item for 76fef5b9c535afa244989732b53cc8dfb7ffb471
      the first step for #1864: warn if markdown is not a dependency of the package containing vignettes based on the knitr::knitr engine
      amend 6907b428572c130982f6b3d6c91a164a15b94a30 to fix rstudio/bookdown#883 (comment): regardless of the Pandoc version, just don't write the <div> when the output format is LaTeX, because rmarkdown will apply its Pandoc filter to convert any div to a LaTeX environment
      close #1862: add an empty line after each bib entry in write_bib()
      factor out the code to get the env var _R_CHECK_PACKAGE_NAME_, and add a function to check if the package being checked is lower or equal to a version
      make sure the current knitr submission is safe on CRAN; will remove the hacks once arsenal and skimr are updated on CRAN
      check against oldrel, release, and devel versions of R on Travis
      do not use matrix and jobs together, but only use jobs
      change \link[pkg]{function} to pkg::\link{function} because R-devel now warns against the former usage:
      rename the job
      for help links to packages that are not hard dependencies, both the package name and the topic name have to be specified in \link[pkg:topic]{function}
      CRAN release v1.29

atusy (1):
      fix #1788 and also fix #1817: change the default table format from Pandoc's simple tables to pipe tables for R Markdown documents (#1830)
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

No branches or pull requests

3 participants