Skip to content

Conversation

Splines
Copy link
Member

@Splines Splines commented Nov 10, 2024

Closes #38.

Feature

Here we add the functionality to export a scene as described in my comment here. Or rather to be guided through some configuration to then obtain an executable command. This command will be pasted into a new terminal such that the user can review it and then execute it by pressing enter. This also allows to add any further flags before executing the command.

image

Known limitations

The action is triggered by a new codelens Export Scene at any line of Python code that defines a class. To limit this to classes that define Manim scenes, we can tackle #12 with the heuristic described there in a future PR.

@Splines Splines self-assigned this Nov 10, 2024
@Splines Splines marked this pull request as ready for review November 11, 2024 18:54
@bhoov
Copy link
Collaborator

bhoov commented Nov 17, 2024

It works! 🎉🎉🎉

My initial impressions after playing around with it:

  1. As a keyboard-oriented user, I kept looking for a command in the palette called "Manim Notebook: Export current scene" before realizing this functionality was only a button in the UI. I like the design philosophy that searching the command palette for "Manim Notebook" returns all the primary interactions with this tool. Does it make sense to include this as a cmd?

  2. The workflow would benefit from some indication to the user that we are just "generating the command for export" instead of doing the actual export -- you and I have the advantage of knowing what the expected workflow is. Few new users will. Some suggestions:

    • Rename the "Export Scene" button (and palette command) to "Generate cmd to export scene"
    • Show some alert to user indicating "Export command pasted in terminal"

Additionally, we should copy the export command to the clipboard so users are able to play around with additional settings without losing the generated command. Also useful if a user does not want to use the built in terminal (e.g., a very long scene will take a while to generate and they want it to run in a dedicated terminal in a background)

@Splines
Copy link
Member Author

Splines commented Nov 17, 2024

Thanks for your review 🙏, yes both points absolutely make sense. I've addressed them in the newest commits. Please also review my "Note this comment" thread regarding the multi-step quick pick util file and resolve if d'accord.

@Splines
Copy link
Member Author

Splines commented Nov 17, 2024

By the way, you can also post such comments as a review with the option Request changes in the Files changed tab. This way, I can re-request a review from you once I've addressed all the points (which is now the case).

Another advantage is that I can directly reply to your comment and resolve it later without cluttering the view.

@bhoov
Copy link
Collaborator

bhoov commented Dec 11, 2024

you can also post such comments as a review with the option Request changes in the Files changed tab

Sounds good @Splines! I have not yet figured the optimal way to exchange feedback on Github, thanks for your tip! :)

I'm playing around with the changes you kindly implemented per my feedback, and I'm starting to prefer your original, simpler "Export" wording that you had before... Especially with the new alerts that are added, I think the process is more self-explanatory. Please see my small updates and see if you like the phrasing.

  1. Include "Manim scene" to parallel "Manim cell" elsewhere
  2. Specify "current scene" to palette command

@Splines Splines merged commit 48f6919 into main Dec 11, 2024
@Splines Splines deleted the feature/export branch December 11, 2024 23:34
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.

Export current scene as mp4

2 participants