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

Changes to allow rgl (and other?) graphics systems to work like base graphics #1892

Merged
merged 12 commits into from
Apr 21, 2021
Merged

Changes to allow rgl (and other?) graphics systems to work like base graphics #1892

merged 12 commits into from
Apr 21, 2021

Conversation

dmurdoch
Copy link
Contributor

@dmurdoch dmurdoch commented Sep 1, 2020

This patch is related to my comments on #1881 and my issue #1853, to allow rgl to work more seamlessly with knitr. The idea is that, like base graphics, rgl has "high level" and "low level" commands to change the current display, and users typically only want to see output when a new high level command is run.

To do this, I needed to export two internal functions from knitr: wrap() and is_low_change(). I also needed to change is_low_change() to a generic function, so the test for low versus high could take place in rgl, and I introduced a new class name, "knit_other_plot" so that knitr could tell that my rgl graphics objects are actually plots that need handling. The access to wrap() allows rgl graphics to efficiently share data: if part of a plot has previously appeared, it won't be written to the HTML output.

This PR shouldn't have bad effects on anyone as far as I know. The current development version of rgl on R-forge doesn't take advantage of the changes; it uses a complicated (and fragile, I worry) set of hooks to replace a bunch of code in knitr. You can see the code that does work with this PR in https://github.com/dmurdoch/rgl branch rglpatch.

@dmurdoch
Copy link
Contributor Author

dmurdoch commented Sep 3, 2020

Travis seems confused: some checks have been sitting there for a few days, stuck at "Queued -- Build Created".

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2020

CLA assistant check
All committers have signed the CLA.

@dmurdoch
Copy link
Contributor Author

The main branch of rgl on github.com/rforge/rgl now supports this PR, as well as the version of knitr on CRAN. I'd like to hold off a CRAN release of rgl until this PR is accepted (or not), in case details change.

hadley pushed a commit to r-lib/downlit that referenced this pull request Mar 19, 2021
* Allow custom output handler in evaluate_and_highlight.
* Allow packages like rgl to have low level and high level objects (see yihui/knitr#1892)
Merge branch 'master' into rglpatch

# Conflicts:
#	R/block.R
@dmurdoch
Copy link
Contributor Author

dmurdoch commented Apr 7, 2021

Similar changes to these were merged into pkgdown in r-lib/pkgdown#1548 and r-lib/downlit#78 . Some differences:

  • In this PR, I exported wrap and is_low_change, but in the pkgdown changes the corresponding functions (downlit:::replay_html and downlit:::is_low_change) weren't exported.

  • The new class name "knit_other_plot" was called "otherRecordedplot".

If you'd like one or both of those changes in this PR, I'd be happy to make them.

Copy link
Owner

@yihui yihui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall. I'll make some minor and doc changes before merging. Thanks!

@dmurdoch
Copy link
Contributor Author

Great!

@yihui yihui linked an issue Apr 21, 2021 that may be closed by this pull request
@yihui yihui merged commit affa754 into yihui:master Apr 21, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: export knitr:::wrap
3 participants