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

Introduce Manim Shell abstraction to preview terminal-free #30

Merged
merged 32 commits into from
Oct 27, 2024

Conversation

Splines
Copy link
Member

@Splines Splines commented Oct 25, 2024

Closes #24.

In this PR we introduce an additional abstraction layer for the so-called ManimShell. This word means a VSCode Terminal that has a running Manim sessinon (IPython session). The notion of "just a terminal", without a running Manim session, is not needed, as we always ensure that commands are run inside an active Manim session.

With this new abstraction layer, that provides methods like executeCommand(), we fully capture the output of Manim inside the IPython session and react accordingly. This allows us to implement many niceties in this PR:

  • If a Manim session is not running yet, spawn a new one in a new VSCode terminal, wait for Manim to have fully initialized, then execute the actual command like checkpoint_paste() etc.
  • If a Manim session is already running, reuse that one to execute new commands like checkpoint_paste().
  • Whenever an error is thrown by Manim, the terminal will automatically focus such that user can read the error message. When testing this feature: please make sure to test different kinds of errors (like SyntaxError or ClassNotFound error).
  • If the start scene command is invoked while a preview is already running, close the current session and invoke a new one.
  • Trying to quit or clear will result in an error if there is not active Manim session running.

The best thing: new users just have to have manimgl installed, and that's it. Then they click on Preview Manim Cell and it should automatically work. Look at the video in #33 to see how everything comes together.

Known limitations

  • For every new "session spawn", we use a new VSCode terminal. This might be improved such that existing terminals can be reused if we think this behavior is annoying. But this should probably be done in a future PR.
  • With the new features here, we are technically in the position to tackle [bug] Quits if "Preview Manim Cell" before it finishes #16 and to introduce platform-dependent behavior, i.e. on MacOS don't allow to execute another preview command while one is actually running. Since now we can actually capture what it means for a Manim command to have finished running inside the IPython session. However, this PR is already huge, so this can be done in a future PR.
    Note that lockDuringStartup is NOT doing what is described in [bug] Quits if "Preview Manim Cell" before it finishes #16, instead it only locks the startup of a new scene, not the complete command execution.
  • The spawning of an initial session might take some time (and even longer on slow machines). During this time, the user is not at all notified about what is going on if they don't have the terminal open. This can be tackled together with Show progress in VSCode #14 in the future based on the changes here.
  • Reloading the VSCode window while having a Manim session open, then interacting with the extension is not a supported use case. Here, the current implementation will spawn a new terminal with a completely new session instead of reusing the existing one. In this case, users can just close the old session and that's it. This shouldn't really be a limitation.

Some Implementation details

So this one PR took me quite a while, it was very hard to figure out how the VSCode extension APIs worked and how to actually read the data stream (and then those anoying ANSI escape chars 🙄). In ManimShell, there's kind of a lot going on. When reviewing it's probably a good idea to start at the initiateTerminalDataReading() method.

  • We strip ANSI escape chars from the console
  • See the beginning of manimShell.ts for regex that are used to match specific events like the end of a cell execution.
  • Watch out for async. Just because the method is marked async does not mean we actually wait until the end of the command execution. Sometimes it suffices to wait for other events inside the method. This should be noted accordingly in docstrings.
  • The chain command executes -> scene not spawned yet -> new scene spawn is triggered -> command is executed in newly spawned Manim session might be a bit hard to follow through in the code due to how startScene() is used when invoked via the command palette as well as when requested when we actually want to execute another command (see retrieveOrInitActiveShell()). The debugger might greatly help here.

@Splines Splines self-assigned this Oct 25, 2024
@Splines Splines changed the title Introduce Manim Shell abstraction layer Introduce Manim Shell abstraction layer to preview terminal-free Oct 25, 2024
@Splines Splines changed the title Introduce Manim Shell abstraction layer to preview terminal-free Introduce Manim Shell abstraction to preview terminal-free Oct 25, 2024
@Splines Splines marked this pull request as ready for review October 25, 2024 17:35
@VladimirFokow
Copy link
Collaborator

VladimirFokow commented Oct 25, 2024

wow!😮
I'll try to find time this weekend to check this out!


on MacOS don't allow to execute another preview command while one is actually running

a great possibility that this PR opens!

@bhoov
Copy link
Collaborator

bhoov commented Oct 27, 2024

I am going through this PR -- it's a doozy. @Splines you continue to impress 💪

I will edit this comment as I go through your code and think of small improvements. All features mentioned in this comment are not critical to me approving this PR.

  1. When starting an interactive session, automatically focus on the iPython terminal that is running Manim (like what happens when an error message occurs)
  2. Re: If the start scene command is invoked while a preview is already running, close the current session and invoke a new one -- I find this a bit jarring that we automatically kill, even if the hotkey is accidentally pressed. From a user-experience perspective, I would prefer a pop-up question "Would you like to kill the existing manim shell to start a new shell at the cursor? [y/N]"

Overall, I find this PR a HUGE usability improvement over the previous version. And, because several other huge QoL PRs depend on this one, I vote to merge this in to get the pipeline moving forward. Let me quickly go through the code to make sure I understand everything. @Splines your coding standards are far higher than mine, so expect few if any stylistic comments

Copy link
Collaborator

@bhoov bhoov left a comment

Choose a reason for hiding this comment

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

I strongly approve. Let's merge.

@bhoov bhoov merged commit 311b461 into main Oct 27, 2024
@bhoov bhoov deleted the feature/manim-shell branch October 27, 2024 00:41
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.

Use already open Manim interactive shell
3 participants