-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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 reload()
command for interactive scene reloading
#2240
Open
Splines
wants to merge
29
commits into
3b1b:master
Choose a base branch
from
Splines:feature/reload
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The motivation for this change is described in this discussion. In summary: the
checkpoint_paste()
mechanism is great but doesn't handle all situations. A simple example is when you use some values that are defined outside of the class that inherits fromScene
(see test code down below).In the current workflow, users have to exit the IPython terminal, i.e. also close the Manim window. Then execute the
manimgl
command manually again to reload the scene. This is cumbersome to do during iterative scene development where such a workflow might be frequently used.Tip
Therefore, we introduce a new
reload()
command to the IPython shell which will quit the active IPython embedded shell and restart the scenes with the same arguments that were passed tomanimgl
initially. During all of this, the ManimGL window stays open and is reused. As optional argument,reload()
takes a line number where we should start at. See the video down below for an example.Proposed changes
I did my best to add docstrings to the respective classes and methods so the code should already give away the main changes. But for a summary:
reload()
method to thescene.py
. It raises aKillEmbedded
exception by means of theexit_raise
IPython line magic.ReloadManager
that will run awhile True
loop and catchesKillEmbedded
exceptions to restart the scenes.manimlib.extract_scene.main(config)
will return a list of scenes. I haven't quite understood how the user can invoke running multiple scenes. Therefore this scenario of multiple scenes is not tested by me. As heuristic for the window (see theretrieve_scenes_and_run
method): I will reuse the window of the first scene (when iterating the array) that has a window other thanNone
, which should be just the window of the first scene in the list.Known limitations
reload()
can be marked asexperimental
. If users find strange behavior, they should test if it works when quitting, then runningmanimgl
again. Is there some kind of test suite available to avoid manual testing all the time?reload()
can be called with any line number. If this line number doesn't correspond to a comment,checkpoint_paste()
will be screwed up. I think this can be accepted as known limitation. A future PR could tackle this case and e.g. decrease the line number until a comment inside theconstruct()
method is found.Proposal for developer well-being
Expand
While this PR was a lot of fun to work with, a major pain point was missing linting in this repo. Whenever I save a file in VSCode, my Python code will be linted automatically, which is a good thing as I think it's quite tedious to discuss stylistic changes in PRs (like: "don't use this ternary operator here", "use snake case here", "use double quotes, not single quotes" etc.). With linting we just accept the style that is prescribed by the repo and can focus on what is more important: the semantics of the code.
I didn't want to turn off the automatic linting in VSCode via Pylint because I still wanted that my new code changes were formatted nicely. Therefore, what I had to do in the end is write a bit of code, then use the
Stage selected ranges
features to only stage the ranges in the Git diff that I modified (and leave those out that were automatically modified by the linting). As you can imagine this was quite a tedious process to do.For some inspiration: in this repo, I've set up pylint for the repository here and here specifically for VSCode as well as here, then also added a linter workflow that runs in GitHub Actions to see if every PR conforms 100%. Ideally, one would set up the linter, possibly define some exceptions to the rule (e.g. having to add a docstring to every method might be overkill in this repo), then run the linter for every Python file in the codebase and commit this.
Test
Call via:
manim-reload-sample.mp4
In the video, just ignore the other things that are going on in our
manim-notebook
extension for VSCode that is currently under development. Clicking onPreview Manim Cell
will just copy the content to the clipboard and then execute thecheckpoint_paste()
command as usual.