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

Add framework for animation options (fixed) #19

Merged
merged 14 commits into from
Sep 10, 2018

Conversation

gadenbuie
Copy link
Owner

Fixed version of #18

This is an extension of #16 and that will help us move towards resolving #17.

It sets up a framework for creating, setting and transporting the animation options that may need to be passed through several helper functions to get to the final animate_plot() or static_plot() call.

It also lets

  1. The user set animation options for the current session, much like theme_set() in ggplot2
  2. Us create "default" animation options by verb family
  3. Us define or change the global defaults easily from one place.

I've created a more detailed explanation in this gist that also demonstrates some of the work from #16.

I'm bundling this as a PR to a PR #16 in hopes that it's a little easier to review 😄

And demonstrates usage with the tidyr gather/spread animators. In essence, user-facing functions take an argument `anim_opts = anim_options()` as a last argument (after ... if applicable). These options should be passed through to lower level functions and should be given to `animate_plot()` and `static_plot()`.

This implementation actually doesn't break previous functions, but the join and set families will need to be updated to match.
And update documentation for both options
Set option "tidyexplain.strict_dots" to TRUE for checking for issues with extra arguments being passed in ... to the _plot() functions.
ggproto objects like those created by enter_fade() or exit_appear() evaluate to enviroments making it impossible to compare two ggproto objects to determine if they are the same (both will be different environments). To get around this, I store the expression used to create the enter/exit arguments so that we can later compare two objects created by anim_options().
- Collected in animate_options.R
- Sets global defaults in plot_settings environment
- Adds ability to set animation options, similar to theme_set() from ggplot2
- Adds getters for current (if set) or default animation options

Merge branch 'pkg-add-anim-options-merge' into pkg-add-anim-options

# Conflicts:
#	NAMESPACE
#	R/animate_tidyr.R
#	R/plot_helpers.R
#	man/anim_options.Rd
#	man/animate_gather.Rd
#	man/animate_spread.Rd
#	man/static_plot.Rd
Leads to better documentation where it is easier to understand how to use the anim_opts parameter
@gadenbuie gadenbuie requested a review from DavZim September 4, 2018 12:38
@gadenbuie gadenbuie changed the title Pkg add anim opts fixed Add framework for animation options (fixed) Sep 4, 2018
@gadenbuie gadenbuie added this to the Package v0.1 milestone Sep 4, 2018
@DavZim
Copy link
Collaborator

DavZim commented Sep 10, 2018

Loving the idea with the animation options and all the other work you have put into this.

Some thoughts that I had while reading through the code:

  • currently we use our default values as defined in zzz-package.R only if ggplot has not set another value (getFromNamespace("theme_env", "ggplot2")$current$text$size) see https://github.com/gadenbuie/tidy-animated-verbs/blob/bbb8410c787fdbdf1231c12aafde295665072d71/R/animate_options.R#L203-L205 etc. Should we maybe revers the order, that the ggplot theme is only used if somehow our default value is not set? Currently on my (high-DPI) display, the font-text is way too large at its default (when using ggplots-values). For me around 5 works for the text-size.
  • I don't quite understand the advantages of get_input_text(), what is the advantage of creating a list first and then pasting its x and y argument again? 5dcdf5f and following. Or is it just necessary for the quosures?
  • Currently we pass the dots to the animation-function in the join and set functions, but not to the tidyr animation functions. Are they still in use? If so, should we maybe standardise its usage?

Otherwise I didn't find anything that needed changing.

@gadenbuie
Copy link
Owner Author

getFromNamespace("theme_env", "ggplot2")$current$text$size: Should we maybe reverse the order, that the ggplot theme is only used if somehow our default value is not set? Currently on my (high-DPI) display, the font-text is way too large at its default (when using ggplots-values). For me around 5 works for the text-size.

I think I'd rather just take this out. Initially, I was thinking we could use ggplot2::theme_set() for any plot settings that overlap between ggplot2 and our settings needs. But we call theme_void() in static_plot() anyway and now that we have the rest of the options setting code I'll take out that part.

For now, you can run set_font_size() at the start of your session to set the sizes globally to something more reasonable.

I don't quite understand the advantages of get_input_text(), what is the advantage of creating a list first and then pasting its x and y argument again?

get_input_text() is a simple helper to get the text name of the x and y inputs, which may or may not be quoted (they are quoted if passed from a wrapper function like animate_left_join()). Because they may be quoted, the make_named_data() is needed to make sure that they are unquoted for the remainder of the function. I included adding the variable names to this list so that in the future I can write something like this

data <- make_named_data(x, y, c(get_input_text(x), get_input_text(y)))

and then data can carry around the data and the names. But I didn't want to break the readability of your code, so the default is to call those elements .$x and .$y.

Currently we pass the dots to the animation-function in the join and set functions, but not to the tidyr animation functions. Are they still in use? If so, should we maybe standardise its usage?

Yup, the join/set functions haven't been updated yet. I was using the tidy family as the example of how the anim_opts argument and anim_options() functions can be used... If all looks good I'll move on to updating those function families as well.

@DavZim DavZim merged commit dde12e6 into pkg-updates Sep 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants