-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Refactored the Command Line Interface to use Click instead of Argparse #1013
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
Conversation
@kolibril13 This is intended as this PR is a breaking change -- there's not much we can do about this other than give a deprecation warning since click standardizes our CLI to be POSIX compliant. It's not in the realm of possibility to add reverse compatibility for the the old Argparse way. The deprecation warning is already given in the CLI: Note: I still think this PR can be merged for v0.5.0, just waiting on @naveen521kk's review. I believe it's stable enough. I'm just wondering whether it'd be better to introduce it at the beginning of the next release cycle for v0.6.0, or towards the end for v0.5.0 |
No problem we can fix it later. Please open an issue.
Looks like changelog is missing it. Can you update it? |
Updated and opened #1207 |
Alright, I am definitely fine with merging it then also before 0.5.0. |
@kolibril13 manim/manim/utils/ipython_magic.py Line 68 in bfe3f1e
I discussed this very briefly with @behackl, but it didn't fly because there can be code blocks with more than one Scene? Or something of that nature. But this is using the code block directly as stdin and passing it to be rendered. I only use the second instance of running the command to get the output file's directory so that it can be displayed. IMO, this is a cleaner way to do this, but it'd be good if the config object could be returned so it didn't have to be run a second time just for the output file path. |
Looks like everything from #1013 (comment) is fixed. Also, I couldn't find the changelog of I will do a review soon. |
Renderer is introduced in ManimConfig and is also a new available option. I'll write something for it right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Good point. However, I think having more than one Scene in one cell in jupyter is very unlikely because, in my opinion, the whole point of using jupyter is the ability to use multiple cells for multiple scenes. |
That's what I was thinking when I went ahead and implemented it -- every example jupyter notebook I've seen only puts one scene in the cell. I haven't actually seen multiple Scenes in a cell, or tried it myself tbh. I'm genuinely unsure if manim currently even supports multiple Scenes in jupyter-lab. Also, I think it would be possible to use stdin ending with |
It seems like we could still include this in I'm slightly leaning towards including it in |
The thought of doing a patch release hadn't crossed my mind -- especially since most of our releases have had some fairly big PRs (webgl, svg, opengl, etc.) -- but this sounds good to me. I've merged upstream master, feel free to merge when ready -- or after the release if you change your mind. I do have some stuff I'd still like to integrate but these can (and should) be done in a future PR. |
1) There's a new file for each option group 2) render is now a cloup.Command, not a Group Fixed issue when an animation is cached, manim can't merge the partial movie files. (ManimCommunity#1192) * fixed issue * fixed tests * Update manim/renderer/cairo_renderer.py Co-authored-by: Darylgolden <darylgolden@gmail.com> * added tests * imrpoved test * fixed logic * added new test * check if the file has been outputed * added test when caching is enabled * fixed tests on windows * black * Update manim/renderer/cairo_renderer.py Co-authored-by: Naveen M K <naveen@syrusdark.website> * Update tests/assert_utils.py Co-authored-by: Naveen M K <naveen@syrusdark.website> Co-authored-by: KingWampy <9156604+WampyCakes@users.noreply.github.com> Co-authored-by: Darylgolden <darylgolden@gmail.com> Co-authored-by: Naveen M K <naveen@syrusdark.website> Added :ref_methods: to the manim directive (ManimCommunity#1209) * fix manim_directive for methods * added ref_methods to Angle example * black * added new ref_methods references * sort out ref_functions vs ref_methods in examples.rst Co-authored-by: Jason Villanueva <a@jsonvillanueva.com> Co-authored-by: Benjamin Hackl <devel@benjamin-hackl.at> Fixed issue when an animation is cached, manim can't merge the partial movie files. (ManimCommunity#1192) * fixed issue * fixed tests * Update manim/renderer/cairo_renderer.py Co-authored-by: Darylgolden <darylgolden@gmail.com> * added tests * imrpoved test * fixed logic * added new test * check if the file has been outputed * added test when caching is enabled * fixed tests on windows * black * Update manim/renderer/cairo_renderer.py Co-authored-by: Naveen M K <naveen@syrusdark.website> * Update tests/assert_utils.py Co-authored-by: Naveen M K <naveen@syrusdark.website> Co-authored-by: KingWampy <9156604+WampyCakes@users.noreply.github.com> Co-authored-by: Darylgolden <darylgolden@gmail.com> Co-authored-by: Naveen M K <naveen@syrusdark.website> Added :ref_methods: to the manim directive (ManimCommunity#1209) * fix manim_directive for methods * added ref_methods to Angle example * black * added new ref_methods references * sort out ref_functions vs ref_methods in examples.rst Co-authored-by: Jason Villanueva <a@jsonvillanueva.com> Co-authored-by: Benjamin Hackl <devel@benjamin-hackl.at> Fixed unnecessary args dict
|
Maybe we can add an extra sentence here, e.g. |
Motivation
Closes #452
This is a refactor which moves Manim away from argparse and towards click in order to allow future developers to easily add future subcommands, options, and arguments.
Changelog / Overview
This change breaks the CLI API to organize the structure of Manim Community's commands, options, and arguments.
To be more in line with POSIX compliant CLI conventions, options for commands are given BEFORE their arguments.
In Argparse:
manim basic.py -p -ql
With Click:
manim -p -ql basic.py
Although this is primarily a refactor and most of the common options are still there, some options have been added/removed. Use the
manim
command's-h
,--help
option, or simply run the command without providing options/arguments to view the help page with the full list of subcommands/options/arguments.--fps
/--frame_rate
option which allows for custom fps that don't have to be integer (i.e. 29.97, 23.98, etc.). Users no longer have to specify the FPS from within a config file. Additionally, the--webgl_renderer_fps
option has been removed. Use--fps
or--frame_rate
instead.--renderer
option which you can use to select your choice of renderer (e.g.--renderer=opengl
). There are currently THREE renderers to choose from!--background_color
option. Reassigned the--background_color
option's shorthand-c
to--config_file
.--leave_progress_bars
option. Use--progress_bars=leave
instead.-l
,-m
,-h
,-k
.--sound
option. It lost support long ago with the removal of SoX.By default, the
manim
group calls themanim render
subcommand through some decorator magic (i.e.manim -pql basic.py
is a shorthand formanim render -pql basic.py
). All of the subcommands and their options are contained within themanim/cli
directory which is where the focus of the code should go. Most testcases that involve using the CLI had to be refactored as well to accomodate the new method of providing options before arguments ( arguments being the file/scene(s) ).Testing Status
All test cases are currently pass on all OSs in our CI!
Further Comments
There will be some refactoring in future commits to this PR, particularly documentation of the functions. This is a successful first draft though.
The intent is to eventually redo the ManimConfig object as well using a similar decorator style approach to easily add config options with customized validation logic.
The new CLI structure is as follows:
If you're curious to see the new help messages without adding my remote:
manim -h
manim -h
manim render -h
manim render -h
manim cfg -h
manim cfg -h
manim cfg export -h
manim cfg show -h
manim cfg write -h
manim plugins -h
manim plugins -h
Acknowledgements
Reviewer Checklist