Manim-Notebook / manim-notebook

Simple commands to replicate the manim dev workflow in VSCode
MIT License
5 stars 0 forks source link

Add commands to start/stop/clear the scene #17

Closed VladimirFokow closed 1 week ago

VladimirFokow commented 1 week ago

This PR adds the following commands

To start the scene, the manimgl command is run in the terminal with its current line number

manimgl <file_name> <ClassName> [-se <line_number>]

Users can use it to start their manim session: They place the cursor -> run this command -> manim starts in this scene, animation state is already at the point where the cursor is.


Note that 3b1b's name for the runScene command is manim_run_scene as found here

bhoov commented 1 week ago

Functionality works! A few requests before this is merged:

  1. It is my preference that all commands in this extension default to the same "prefix" (currently, we are using cmd+' cmd+...). Could we update this command to default to cmd+' cmd+shift+r? Or, to avoid the shift modifier, perhaps cmd+' cmd+s which is a good mnemonic for "start" or "scene"?
  2. It seems like it would be good to accompany this PR with an equivalently easy "stop scene" (perhaps bound by default to cmd+' cmd+w because I use cmd+w to kill tabs all the time)

Re: the command name runScene, I am happy to keep it, but my understanding is that this functionality is more like "start interactive session". Once an interactive session is running, this command no longer works.

Maybe we can choose something like initInteractiveSession?

VladimirFokow commented 1 week ago

Could we also indicate this to users in this case? It could be frustrating to click on a command and realize that nothing at all happens.

Note that if the users open another VSCode terminal, they can start another scene in parallel. They can open many terminals -> and have many running scenes at the same time. Which scene is influenced -> depends on which terminal they have opened currently

VladimirFokow commented 1 week ago

Re: the command name runScene, I am happy to keep it, but my understanding is that this functionality is more like "start interactive session". Once an interactive session is running, this command no longer works.

Maybe we can choose something like initInteractiveSession?

Thanks for the feedback!

how about startScene?

bhoov commented 1 week ago

This PR is shaping up! I love the new keybindings and naming convention, very intuitive.

In playing around with the functionality, i detected a subtle but annoying bug. If you run the startScene command in the middle of a continuing python line, (e.g., typing cmd+' cmd+s on line 190), manim tries to insert self.embed() in the following line which is the middle of a python expression.

image

This causes the following error, which "silently fails" for any user who does not have the terminal visible

 File ".../example_scenes_insert_embed.py", line 191
    self.embed()
    ^^^^^^^^^^^^
SyntaxError: invalid syntax. Perhaps you forgot a comma?

I see two options:

  1. Propagate this error to the user in a vscode notification
  2. Implement logic to detect the end of the python expression on the selected line

1 is easier, and sufficient for the scope of this PR, but 2 certainly feels more elegant. What do you think?

VladimirFokow commented 1 week ago

interesting! Yes, inside of a multi-line python expression..

I don't know how to do option (1). For option (2) - we'd need to detect the boundaries of a python expression..


Exploring for option (2):


Currently, I don't know how to do it, or whether it's possible at all. The links above have a lot of info.. I haven't read all of it.

bhoov commented 1 week ago

I'm fine with introducing the python extension dependency. If 2 is easier, let's do that!

VladimirFokow commented 1 week ago

I would merge the current state of this PR because I don't know how to do either (1) or (2).. That can become an issue for future development.

Splines commented 1 week ago

I will review this soon (next few hours).

Splines commented 1 week ago

Update: well... my Internet didn't work at all yesterday and still isn't :( Will try to review this evening (CEST time). Hope my access works by then, I will see...

Splines commented 1 week ago

Don't forget to Squash and merge this PR instead of Merge commiting it.

VladimirFokow commented 1 week ago

I've made some purely stylistic code changes

Great! Thanks!



you try to comment on every line

yes, ts isn't my primary language, & I like reading only comments in the future instead of the code)

yes, I know the philosophy that a line reads like a sentence - the reason why I had the row name, and that comment - was to have a direct correspondance with 3b1b's code. But the code works - so we don't need this correspondance now, so thanks for renaming it.



I'm writing in this repo for fun, rather for perfectionism, that's why for me simplicity with many comments is better than having hundreds of small functions (speed of iteration is faster) (Of course, if some code is repeated in several places - it should be a function)



Thanks for the review!