ropensci / magick

Magic, madness, heaven, sin
https://docs.ropensci.org/magick
Other
462 stars 65 forks source link

Image sizing inconsistency with knitr `github_document`'s with inline code chunks #310

Open rundel opened 3 years ago

rundel commented 3 years ago

I'm seeing some fairly bizarre behavior when trying to output magick images in a github_document. Below is a reprex to demonstrate the issue.

---
output: github_document
---

```{r setup, include = FALSE}
library(magick)
image_read_svg('http://jeroen.github.io/images/tiger.svg', width = 128)

Markdown text r image_read_svg('http://jeroen.github.io/images/tiger.svg', width = 32) more text.



When knit the Rmd produces a document where the first tiger image is blurred as the base64enc image is only 32x32 and being scaled back up to 128x128.

If the inline code chunk is removed the first image displays properly at 128x128 - more bizarrely if  `image_read_svg('http://jeroen.github.io/images/tiger.svg', width = 32)` is included in the chunk before the 128x128 image then all three images will be correctly sized but this is not the case if this line is included in a different chunk.

Varying the size of this proceeding image also has some strange behavior, making it smaller than 32x32 everything appears to work but the resulting image is actually intrinsically 32x32 but being shrunk to the smaller size. If this image is bigger, then the original issue occurs for this image but now the 2nd 128x128 image is fine.

I believe similar behavior is also happening with `html_document`s but I haven't explored it as much and it is a bit confounded by the `fig.retina` option. If the underlying issue actually is with knitr, I'm happy to move this over there.
rundel commented 3 years ago

After digging a bit deeper this seems like a knitr bug - inline code chunks have the same label as the proceeding chunk so the behavior I am seeing is because inline chunk is overwritting unnamed-chunk-1-1.png.

Maybe this is intended but it seems like plot_counter() is being reset between the two chunk types which is causing this issue.

cderv commented 3 years ago

I have commented in the other issue but writing here the main why I think this happens. In the knit_print method in this package internal knitr function are used to create the filename using smae knitr's logic https://github.com/ropensci/magick/blob/56f746be5edb3a9e7c40b2c91ef3bd0b51bbb0f1/R/base.R#L117-L131

If this method is used in inline chunk, the current process will be the same (it could be different by catching inline = TRUE) and internal plot_counter() will be used to create figure name with knitr::fig_path but these are currently supposed to be used with block chunk and not inline one, which have no label - internal knitr::opts_current$get("label") is only for block chunk.

We would need to think about supporting creation of fig.path in inline chunk, or the knit_print method here needs to create the filename another way (possibly only when use in inline chunk).

Nice investigation @rundel ! That helped a lot ! thanks.

rundel commented 3 years ago

@jeroen Any thoughts?

jeroen commented 3 years ago

What could I do about this on my end? It sounds like an issue in knitr?

rundel commented 3 years ago

See the discussion in https://github.com/yihui/knitr/issues/1988#issuecomment-825080732 - the current assumption (which I had also made) was that inline chunks behave the same way as regular chunks which is not the case.

Longer term I hope this will be changed in knitr but in the shorter term it seems like magick may need to check for the chunk type and use an alternative naming approach for images coming from inline chunks.

jeroen commented 3 years ago

OK. Can you suggest a PR for that? I also wonder if it makes much sense to show images in inline chunks in the frist place? Do people really use that?

rundel commented 3 years ago

I ran across this because I was trying to do exactly this with a package rundel/rsicons which Mine and I were planning to use to embed things like the Git button icons in Rmds for teaching purposes. The original plan was to use \<img> tags but that doesn't generalize as well (e.g. to pdf). I think there are similar use cases around things like font awesome.

I'm happy to take a stab at a PR and let things progress from there.

cderv commented 3 years ago

I think it is not as usual to show image in inline code and that is why knitr mechanism for auto naming file from figures created by R does not support it currently. This is not a small change in knitr and we'll need to think about it.

Also, for now packages that needs to this (mainly icon packages it seems) does not rely on internal knitr naming mechanism like magick does. fontawesome package is an example and for HTML it uses inline svg, for other format it will save an image but name the file itself. So the knit_print method there take that into account https://github.com/rstudio/fontawesome/blob/b6a916199ffe34aac2507d770327467d525dc6cf/R/knit_print.R#L44

rsicons package could also do like fontawesome and have an internal knit_print methods for its object to handle inline case correctly.

On magick side, knit_print.magick-image could make sure to work only for block chunk with the current mechanism and works or not for inline chunk. knit_print generic will pass options and inline to methods - inline will be TRUE when this is used inline. See knitr vignette for example: https://cran.r-project.org/web/packages/knitr/vignettes/knit_print.html#customize-printing

Hope the above can help to solve this with the current state of knitr