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

Macro fps support #4594

Merged
merged 6 commits into from
Feb 2, 2023
Merged

Macro fps support #4594

merged 6 commits into from
Feb 2, 2023

Conversation

arikheinss
Copy link
Contributor

Hello.

I rewrote the _animate function, so that the animation macros @gif, @animate and @apng support an extra fps-parameter.
This includes some restructuring which could make possible future parameter additions somewhat simpler.

I also added Documentation for these macros to properly communicate this functionality.

added support for fps specification for animation macros. Also changed the way parameters are parsed to simplify possible future additions of extra parameters.
Extended documentation for animation macros, to include the new fps parameter as well as filter mechanics
Comment on lines +292 to 298
This macro supports additional parameters, that may be added after the main loop body.
- Add `fps=n` with positive Integer n, to specify the desired frames per second.
- Add `every n` with positive Integer n, to take only one frame every nth iteration.
- Add `when <cond>` where `<cond>` is an Expression resulting in a Boolean, to take a
frame only when `<cond>` returns `true`. Is incompatible with `every`.
"""
macro apng(forloop::Expr, args...)
Copy link
Member

@t-bltg t-bltg Dec 9, 2022

Choose a reason for hiding this comment

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

I think you can use @doc (@doc gif) macro apng to repeat a documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this, with

let moreDocumentation = """This macro supports ... """
    @doc string(@doc @gif)*moreDocumentation :(@gif)
    @doc string(@doc @apng)*moreDocumentation :(@apng)
    @doc string(@doc @animate)*moreDocumentation :(@animate)
end

but instead of extending the documentation it added a whole new one. It was not very pretty.

@t-bltg
Copy link
Member

t-bltg commented Dec 9, 2022

Thanks for this PR. You might want to read https://docs.juliaplots.org/stable/contributing/#Write-code,-and-format.

It would be great to add some tests in https://github.com/JuliaPlots/Plots.jl/blob/master/test/test_animations.jl.

@arikheinss
Copy link
Contributor Author

Short FYI, this is my first real Pull request ever, so I don't quite know what I am doing, but I just got an automated email pointing me to this page, which should help.
I will see whether I can improve the request over the weekend.

@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Base: 90.76% // Head: 90.77% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (58d9c0a) compared to base (d1a8506).
Patch coverage: 94.44% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4594      +/-   ##
==========================================
+ Coverage   90.76%   90.77%   +0.01%     
==========================================
  Files          41       41              
  Lines        8767     8800      +33     
==========================================
+ Hits         7957     7988      +31     
- Misses        810      812       +2     
Impacted Files Coverage Δ
src/animation.jl 97.27% <94.44%> (-1.78%) ⬇️
src/args.jl 84.98% <0.00%> (-0.23%) ⬇️
src/backends/gr.jl 91.50% <0.00%> (-0.16%) ⬇️
src/axes.jl 89.87% <0.00%> (-0.09%) ⬇️
src/shorthands.jl 96.15% <0.00%> (ø)
RecipesPipeline/src/RecipesPipeline.jl 100.00% <0.00%> (ø)
src/components.jl 90.62% <0.00%> (+0.02%) ⬆️
src/utils.jl 92.65% <0.00%> (+0.15%) ⬆️
src/backends/gaston.jl 95.77% <0.00%> (+0.24%) ⬆️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

added Tests for testing extra macro parameters for that juicy code coverage
@arikheinss
Copy link
Contributor Author

Greetings

Over the weekend I cleaned up and formatted the code. I also added some tests.

BTW, since the macro just passes some data to the animate function, and my commit enables it to read in kwargs, this might be an opportunity to think about which parameters @gif should be allowed to pass down (maybe loop or variable_palette?).
If any new kwargs are to be allowed they just have to be appended in the tuple at if lhs in (:fps,). If any kwarg should be allowed the check should just be removed.

@arikheinss arikheinss requested a review from t-bltg December 22, 2022 20:20
src/animation.jl Outdated Show resolved Hide resolved
@BeastyBlacksmith BeastyBlacksmith merged commit 985808d into JuliaPlots:master Feb 2, 2023
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.

3 participants