Support rgl output in snippets.

[dmurdoch]

Closes #404

This gets knitr to include snapshots of rgl output if rgl is loaded within the snippet.

It should have almost no side effects if rgl is not used.

To disable snapshot inclusion in a code snippet, you can run

rgl::setupKnitr(autoprint = FALSE)

or, before running any other code that would load rgl,

setHook(packageEvent("rgl", "onLoad"), NULL, "replace")

[dmurdoch]

I didn't add a test of the new feature. It should be something like

test_that("rgl output is included", {
  skip_on_cran()
  reprex(input = "library(rgl); plot3d(1:10, 1:10, 1:10)\n")
  # Something here to check for the snapshot
})

but I don't know how to test for the correct output.

[jennybc]

Thanks, yes it works for me locally!

@cderv Would you take a quick look at this? I'd appreciate your confirmation that this is the most logical way to insert the rgl feature. I suppose I expected to see it in the render function, more than in the output format. But I haven't thought about reprex and output formats, specifically, in a long time.

[jennybc]

As for a test, there are no existing tests for even more plain vanilla plot handling. So I think it is OK / self-consistent to leave this untested. This package is only meant to be used interactively. So my basic posture for tricky-to-test features is if something is truly useful and well-used and it stops working? Someone will tell me.

[cderv]

If really added in reprex, this*makes sense to me to add rgl::setupKnitr() in the output format pre_knit part, as the rgl function sets some hooks and methods for knitr. This is the closer to knitr from reprex point of view.

We could also wonder why not add this directly in rmarkdown or knitr if this is needed anyway when rgl is used in a Rmd document. I am not quite sure from https://dmurdoch.github.io/rgl/dev/reference/setupKnitr.html if this is safe (and recommended) to run in every document that uses rgl. What is the recommendation @dmurdoch ?

And if this is something that is required no matter the output format, I also wonder if this kind of setup should be done instead in the used package itself. 🤔
On this, I know the example of glue which sets some knitr hooks itself when loaded.
https://github.com/tidyverse/glue/blob/d47d6c7701f738f8647b951df418cfd5e6a7bf76/R/zzz.R#L14-L21
I remember this had some subtle side effects (now fixed) on package installation and could become quite complex when setting more than engines. However, it seems to me a good way that a package, that require special knitr configs, sets it itself.
So maybe we need a (new) good mechanism that allows any package to set some hooks and other configuration for knitr when used in knitr. If knitr would have a way to run this configuration if found in a loaded package, maybe that would be easier. I'll think more about that.

Anyway, in the current state of things, if using rgl object in a reprex requires to run rlg::setupKnitr(), doing that in the output format function seems logical to me.

[dmurdoch]

The setupKnitr() function was an attempt to rationalize the different ad hoc ways that rgl output had been included in knitr and rmarkdown documents. With the recommended setupKnitr(autoprint = TRUE), the document mimics the behaviour of interactive rgl, but there were lots of documents needing other options. My PR forces the recommended choice here since reprex doesn't have the legacy use to support.

This could probably be done in rgl instead of in reprex if there's a function available to test whether code is running in reprex::reprex() at package load time.

[jennybc]

This could probably be done in rgl instead of in reprex if there's a function available to test whether code is running in reprex::reprex() at package load time.

Yes, there is an option, that is set inside reprex_render() that, if defined, should indicate we're reprex-ing and for what venue (reprex.current_venue):

a4e3efe

This was added so that the renv package could tailor its inclusion of the lockfile and seems related to this rgl-specific request.

More re: the context can be read here:

#313 (comment)

I'll wait on this PR a bit, until it's clear if it makes more sense to do something inside rgl, instead of reprex.

[dmurdoch]

I think it would make more sense to have this in rgl then, in case the initialization code needs to change. So I'm going to close the PR. If something comes up when I try to add this, I'll let you know! Thanks for taking a look at it.