r-lib / svglite

A lightweight svg graphics device for R
https://svglite.r-lib.org
180 stars 39 forks source link

adjustcolor alpha.f has no effect on svglite output #60

Closed nathanwebb closed 8 years ago

nathanwebb commented 8 years ago

Hi,

Using base svg, I had been adding text using textGrob and adjusted some colours using adjustcolor and it's alpha.f option. This was working as expected. However, in svglite, it seems that the transparency option doesn't seem to get set when using this. Interestingly, the example code below doesn't use textGrob, and it doesn't work either. When run in the console, the output is fine (i.e. the text is greyish), but the saved file shows black text.

In the SVG file, I would expect to see "fill: rgba(0,0,0,0.60);" in the text tag's style attribute, but it's not there.

BTW, I've just tested using "alpha" in the gpar() function and it doesn't seem to work either,

library(ggplot2)
library(svglite)

q <- ggplot(iris, aes(Sepal.Length, Petal.Length, col = Species)) +
  geom_point() +
  theme(text=element_text(size=12, colour = adjustcolor("black", alpha.f = 0.6)))
###
# BTW, this line works, so adjustcolor is working normally, just no alpha channel
#  theme(text=element_text(size=12, colour = adjustcolor("black", offset = c(0.1, 0.4, 0.4, 0))))
###
q

svglite(file="test_svglite.svg")
  q
dev.off()
nathanwebb commented 8 years ago

EDIT: Just noticed that it works unless text.color == 'black' or #000000! So I'm now setting the colour to #010101 if it was originally black.

nathanwebb commented 8 years ago

Found it - the is_black() function in src/devSVG.cpp doesn't include the alpha channel. I'll submit a PR now.

jtoloe commented 8 years ago

I seem to have found a related issue. If alpha is exactly 0.5 and colour is black, the col integer seem to equal NA_INTEGER. The checks in place then prevent any attributes from being added and nothing is visible in the plot.

The relevant functions seem to be is_filled() and write_style_col() but I'm unsure which one is the culprit, if not both.

nathanwebb commented 8 years ago

Slightly related, but more problematic, from what I can see! This looks like an issue upstream, possibly in the Rcpp package.

I think the issue is that black with alpha 0.5 being returned is -2147483648, which in C is a valid integer (smallest possible integer), but in R is below the range of integers, and so equals NA_INTEGER

> is.integer(-2147483648L)
[1] FALSE
Warning message:
non-integer value 2147483648L qualified with L; using numeric value 
> is.integer(-2147483647L)
[1] TRUE
jtoloe commented 8 years ago

Seems to be a rather significant issue then? How is this dealt with in other places? If R stores colour values as integers, how does it deal with the case when colour=NA, or is that not really allowed?

jtoloe commented 8 years ago

From R_ext/GraphicsDevice.h:

 * Changes as from 2.0.0:  use top 8 bits as full alpha channel
 *      255 = opaque, 0 = transparent
 *      [to conform with SVG, PDF and others]
 *      and everything in between is used
 *      [which means that NA is not stored as an internal colour;
 *       it is converted to R_RGBA(255, 255, 255, 0)]
 */

So if colour is specified as NA it should be converted to completely transparent white. But it does not say that we can assume that a colour with value NA_INTEGER can be assumed to be NA. So to me it seems that the correct way to handle it in svglite would be to not have checks for NA_INTEGER and instead check the colour and if it is completely transparent white not add any attribute?

jtoloe commented 8 years ago

Also, it seems as if R base graphics drops anything containing an alpha value of 0 before it even reaches the graphics device and grid graphics properly converts NA values to completely transparent white. This would mean that a check for NA_INTEGER within the graphics device is unnecessary.

nathanwebb commented 8 years ago

@jtoloe yes, that makes sense. I had a think about this over the weekend as well, and there's nothing wrong with NA_INTEGER from Rcpp, as that check is to make sure that a value is an Integer according to R. It's just, as you've said, the check to NA_INTEGER shouldn't apply to this code. You could fork this and submit a PR. (EDIT: just saw your PR ;)

Personally I don't know enough about C to know if this defensive code can be safely dropped without potentially causing exceptions. The other option is to add a specific exception for -2147483648, which seems like a crude hack to me! @hadley can you offer any assistance with this?

jtoloe commented 8 years ago

R_ext/GraphicsDevice.h actually specifies:

     * If "col" is NA_INTEGER then no border should be drawn
     * If "fill" is NA_INTEGER then the circle should not
     * be filled.

But from looking at the source code, none of the standard graphics devices in R seem to be doing that. I might have missed something though. It could be historical since in earlier versions of R the alpha channel was not used and so only 24 bit of the full 32 bit integer was used to specify a colour and so -2147483648 was actually not a valid color and a check for NA_INTEGER would have been valid.