Add justification and width arguments to silhouette plotting functions

[willgearty]

This adds justification arguments/aesthetics to geom_phylopic, add_phylopic, and add_phylopic_base. They generally seem to work well, but there are some issues where the silhouette will over-justify due to interactions between the aspect ratio of the plot and the aspect ratio of the silhouette. I'll look into it a little more, but it might not be possible to address that 100%. It'd be great if you could test and review this @LewisAJones.

Fixes #101.

[willgearty]

@pmur002, I'm running into some issues/gripes with grImport2::grid.picture as part of this PR.

First, it appears that both the hjust and vjust arguments are completely ignored. I've been able to get around this by just combining them into a vector and supplying that to the just argument, but it seems confusing that the arguments exist but are never passed anywhere.

Second, would it be possible to make the function accept a NULL value for the width/height arguments just as grid::grid.raster does (https://github.com/thomasp85/grid/blob/1cf78ea669c93e8b870c2df3156e8785547ebbc1/R/primitives.R#L1219) and handle that during render time? I ask because I'm currently trying to hackily set the height and width before render time to ensure the pictures are justified properly; however, when you then adjust the plot window size after rendering, the set width is then wrong and the justification is then off. grid::grid.raster appears to get around this by recalculating the width (if not specified) to maintain both the aspect ratio and the specified justification of the raster object?

I know you don't do much development for grImport2 nowadays, but I'd appreciate any help you can provide. I'm happy to help however I can.

[pmur002]

Sounds like reasonable things to add/fix. I will take a look.

[willgearty]

I've added width/height arguments (and deprecated size/ysize arguments), which fixes #103. For many cases, this actually seems to help the justification. For example, specifying width instead of height while changing the hjust appears to work well (and likewise with height while changing the vjust). However, there are still some issues where silhouettes will plot smaller than they should because of the interaction between the silhouette aspect ratio and the size and aspect ratio of the plot area. I believe that this should all be resolved if/when grImport2::grid.picture allows for the width/height to be NULL.

[willgearty]

@pmur002 any chance you've been able to look into this? Let me know if we can help in any way.

[pmur002]

Sorry, intense onset of teaching at the moment. Feel free to keep prompting so that I do eventually get back to this.

[willgearty]

@pmur002 just sending another prompting your way to remind you about this.

[pmur002]

Thanks for the reminder! Still snowed under with teaching, but it would help to clarify a couple of things:
are you still interested in hjust/vjust changes as well as width/height changes (in grImport2)? can you please provide some as-simple-as-possible test cases that demonstrate the current problem?

[willgearty]

@pmur002 Ideally we'd like grid.picture/pictureGrob to function just like grid.raster/rasterGrob, including:

• Being able to have width or height be set to NULL and then autocalculated at render time (this functionality just appears to be missing from grImport2)
• Being able to specify hjust/vjust justification separately (currently just works, so this just seems like a minor bug where hjust/vjust aren't used if supplied)

So, to answer your question, yes, we are still interested in being able to have width/height and justification changes in grImport2. I think implementing the first point above should fix all of the problems in rphylopic (including the interaction between justification and size), but we'd appreciate the second point being fixed as well, if possible.

Here are some minimal test cases that demonstrate the current differences between the two sets of functions:

library(grid)
library(grImport2)
library(rphylopic)
uuid <- "16cfde1b-d577-4de8-82b9-62b760aacba5"
img_svg <- recolor_phylopic(get_phylopic(uuid, format = "vector"), fill = "blue")
img_png <- recolor_phylopic(get_phylopic(uuid, format = "raster"), fill = "red")

plot(1:10, type = "n")
# default width is NULL, which is autocalculated at render time
# horizontal justification works as expected
grid.raster(img_png, height = .25, x = .5, y = .5, just = c(0, 1))
# default width is unit(1, "npc")
# so horizontal justification doesn't work like in grid.raster
grid.picture(img_svg, height = .25, x = .5, y = .5, just = c(0, 1))

plot(1:10, type = "n")
# same problem with height and vertical justification
grid.raster(img_png, width = .25, x = .5, y = .5, just = c(0.5, 0))
grid.picture(img_svg, width = .25, x = .5, y = .5, just = c(0.5, 0))

# so, I guess we need to specify both the height and the width...
svg_ar <- rphylopic:::aspect_ratio(img_svg)

plot(1:10, type = "n")
grid.raster(img_png, width = .5, x = .5, y = .5, just = c(0.5, 0))
# except the plot window aspect ratio can change...
# doesn't match in size, but maybe matches in justification?:
grid.picture(img_svg, height = .5, width = .5 * svg_ar, x = .5, y = .5, just = c(0.5, 0))

# matches in size but not justification when plot window is taller:

^{Created on 2024-03-11 with reprex v2.1.0}

[pmur002]

Awesome. Thanks for the clarification and the examples. It's on my list!

[willgearty]

@pmur002 any progress on this?

[pmur002]

Sorry, still behind on teaching. Please keep reminding.

[willgearty]

@pmur002 any chance you have time to look into this now?

[pmur002]

The bad news: no. The good news: getting closer. A reminder in July or August might actually produce some action. Hopefully.

[willgearty]

Hi @pmur002, I'm travelling for the next three weeks, so I wanted to give you a poke beforehand to see if you'll have time to work on this soon?

[willgearty]

@pmur002 ?

[pmur002]

It's a funny time of year for a christmas miracle, but I have (finally) committed an attempt at this request on r-forge.

Needs regression testing before it can go anywhere near CRAN, but you might like to take it for a spin to see if it works.

A couple of points about the examples:

1. To make a fair comparison with grid.raster() you need grid.picture(..., expansion=0). I think your third example actually works just with this.
2. To make a fair comparison, both width and height need to be the same in both grid.raster() and grid.picture() calls. I think your third example (with taller device) has this complication.

Thanks heaps for your patience! I hope this fix is along the right lines.

[pmur002]

{grImport2} version 0.3-3 is now on its way to CRAN

[willgearty]

Thank you so much for your changes to grImport2 @pmur002, everything looks good to me (and it's great to see it on CRAN already!). @LewisAJones I just need to add a few more tests then it should be ready for review.

[willgearty]

Phew, OK, I think that does it! @LewisAJones, it should be all set for you to take a look.

[pmur002]

Thank you so much for your changes to grImport2 @pmur002, everything looks good to me (and it's great to see it on CRAN already!).

No worries. I think a turn around of 18 months qualifies as "the least I could do" :)

[LewisAJones]

Hi @willgearty, I've been through this PR now and it is looking really good to me! I noticed a few things we might want to address/discuss but other than those very minor comments, the code and docs looks good to me and it seems to be working really well.

[willgearty]

Whoops, totally forgot to poke you @LewisAJones once I addressed this review. Let me know what you think! (we chatted about the justification stuff offline)

[LewisAJones]

Thanks for addressing those changes @willgearty, I've had a quick look through again and I think this is good to go! We should have some happy peeps 😄