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

Only show ManimCells in actual Manim files #12

Closed
Splines opened this issue Oct 21, 2024 · 11 comments · Fixed by #112
Closed

Only show ManimCells in actual Manim files #12

Splines opened this issue Oct 21, 2024 · 11 comments · Fixed by #112
Labels
enhancement-archive Don't use this label anymore.

Comments

@Splines
Copy link
Member

Splines commented Oct 21, 2024

See my comment here:

Question: "Can we implement logic to only show this [Manim Cells with fold ranging] when we are in a manim file?"

Sure I could give this a try, maybe this is a better fit for a subsequent PR [...]. To detect if we're in a manim file, I'd try to find the class we're in right now and see if that one inherits from either Scene or InteractiveScene. Searching for a manim-related import string would be a poor choice in my opinion as even in 3b1b's video repo, he's using something like from manim_imports_ext import * where `manim_imports_ext_ is a custom file at the root of the repo. So imports don't even have to include "manim" as string and yet it could be a Manim-related file.

@Splines
Copy link
Member Author

Splines commented Oct 21, 2024

One easy solution (for us) would be to force users to name their files properly e.g. myScene.manim.py. But this might be too restrictive.

@bhoov
Copy link
Collaborator

bhoov commented Oct 22, 2024

Agree, a naming convention for manim scene files would be too restrictive, and indeed we should not do string matching in case users put their manim imports in a separate file.

Here's a proposed workflow, ignoring developer effort:

  1. For any given .py file, check if classes inherit from Scene or InteractiveScene. Next to each non-empty line in the .construct submethod of these classes, add a "Start Manim Notebook at this line" (making an assumption about the official name for this extension) button that will run the manimgl <fname.py> <SceneName> -se <currLineNum> command.
    • (Is only putting initialization buttons in the construct method too restricting? I can imagine some users will want to abstract functionality of the construct method to external functions)
  2. Before an interactive manim session is running, the "Run Active Cell button" starts the manimgl interactive session up to the lineNumber at the end of that cell
  3. If a file contains no Scene or InteractiveScene classes, manim code folding, active cell running, and buttons to initialize the interactive manim session are disabled

@VladimirFokow
Copy link
Collaborator

VladimirFokow commented Oct 22, 2024

Another easy option:

require that each "manim file" contains a special line, e.g.:

# Manimfile

kind of like xml version="1.0" in xml
(but in xml it's optional – here it would be mandatory)

(not necessarily the first line - to allow to have e.g. license headers)




About your proposed option: .manim.py -
I personally don't think it's restrictive at all - because you can have any file name; you're just adding .manim before the .py
However I slightly prefer my first option (above).

@Splines
Copy link
Member Author

Splines commented Oct 22, 2024

Next to each non-empty line in the .construct submethod of these classes, add a "Start Manim Notebook at this line"

If there was a button next to every single line of my Python method, I would definitely panic. This should rather be a command IMHO.

Before an interactive manim session is running, the "Run Active Cell button" starts the manimgl interactive session up to the lineNumber at the end of that cell

Love that behavior, which goes along with #2. But I'd rather want that manimgl gets passed the start of the cell, since I want to preview from the start point, right? I.e. the line where Preview Manim Cell is shown.

For any given .py file, check if classes inherit from Scene or InteractiveScene

This sounds reasonable. Maybe then just add appropriate Manim behavior to every function of such classes? I mean, if users don't want to use a Manim Cell in a custom function living inside a Scene class, they just don't use ## in their comments and that's it. I wouldn't restrict it to the construct() method since users can call any other method from within construct() or setup().

We should make sure that there's no other way than creating a class inheriting from Scene or Inheriting Scene to define Manim code.

Another easy option: require that each "manim file" contains a special line [...]

While a # Manimfile preamble or file name like .manim.py would work, even something simple like this can create friction for new users. It'd be neat to aim for a "works out of the box" solution. We already have to inform users that ## starts a Manim Cell, but maybe that's all they need to know to preview cells and interact with the extension?

@bhoov
Copy link
Collaborator

bhoov commented Oct 22, 2024

If there was a button next to every single line of my Python method, I would definitely panic. This should rather be a command IMHO.

😂😂 Fair. I was envisioning hijacking the "debug" button to launch manimgl interactive. the debug button only shows if it is cursed over, and maybe we can even hijack the tooltip and icon in manim files (aspirational), but this would be one way of doing this without introducing any visual chaos.

Export-1729612019907

I'd rather want that manimgl gets passed the start of the cell

Yes fair. I agree this is the better behavior

While a # Manimfile preamble or file name like .manim.py would work, even something simple like this can create friction for new users.

I agree with @Splines here. We should use these tricks only as a fallback if we can't come up with anything else. In general, I feel that the rule should be:

"Other people have already built awesome manim videos using their own naming conventions and scripts. How can we activate this manim extension seamlessly and automatically for those scripts, while minimizing the number of manim-unrelated scripts we activate on?"

I will continue to think on this. Maybe even having a vscode popup that says "Possible Manim file detected, vscode-manim automatically activated. Deactivate? [y/N]"

We should make sure that there's no other way than creating a class inheriting from Scene or Inheriting Scene to define Manim code.

I'm not familiar enough with how people use Manim to answer this question... Clearly it is easiest to make a Manim video from these classes 🤷

@bhoov
Copy link
Collaborator

bhoov commented Oct 25, 2024

This is also a nice feature, but not necessary for v0.1.0

@bhoov bhoov added the enhancement-archive Don't use this label anymore. label Oct 25, 2024
@Splines
Copy link
Member Author

Splines commented Oct 27, 2024

Fair. I was envisioning hijacking the "debug" button to launch manimgl interactive

If you search for that feature, it's called a gutter icon, see also my comment here.

@Splines
Copy link
Member Author

Splines commented Oct 27, 2024

Note that just checking if a class inherits from Scene or InteractiveScene is not sufficient as seen with some examples in 3b1b's videos repo. E.g. this scene:

class LightExposingFilm(DiffractionGratingScene):

@Splines
Copy link
Member Author

Splines commented Oct 28, 2024

Ok, I think a good heuristic would be to find the comments with ##, then:

  • For the code that defines the ## line, check if it is living anywhere in a class that defines a construct method as direct child (+1 indentation level away from the class line). If yes, allow the construction of Manim Cells anywhere in this class (+x indentation levels where x starts with 1 and can be arbitrarily large)
  • We can use the same mechanism (class defines a construct method +1 indentation level away) to detect on which classes to show Export scene codelenses (see Export current scene as mp4  #38).

With this heuristic, false positives would be:

  • When I write a Python file and declare a class which has a construct method, I would see an Export scene codelens. Whenever I use ## inside this class I would get a Manim cell. The latter is IMHO not a big issue (just don't use ## for comments then 😅). The first could be a bit annoying.
  • We could improve by even checking that the class inherits from anything (e.g. just check that the class is defined with parentheses (...). This would further reduce the false positives. As described above we shouldn't check for inheritance from Scene or InteractiveScene as that does not always have to be the case and we definitely want to avoid false negatives (we don't populate Manim Cells even though we deal with Manim code).

Do you think that'd be a good approach?

@VladimirFokow
Copy link
Collaborator

VladimirFokow commented Oct 29, 2024

  • I would just disable this extension to not see the ## cells
    (it's fast and clear)

Adding some additional convoluted mental overhead to what constitutes a "manim file" doesn't look attractive to me,
I personally would vote to close this issue
(at least for now)
(maybe also add a tag "wontfix" to easily find it in the future, in case someone would need to find it)

  • or VSCode profiles

@Splines
Copy link
Member Author

Splines commented Oct 29, 2024

Adding some additional convoluted mental overhead to what constitutes a "manim file" doesn't look attractive to me,

I mean it's mental overhead that can be packed and contained inside a function isManimFile(code: string) (or isManimClass(code: string) or similar). Yes, it's some more checking and parsing and certainly some developer effort, but in the end the user has a more appealing extension. In my opinion, as an end-user, I just want to install an extension and then it should work out of the box (like for example the amazing GitHub Copilot extension).

For what concerns the Manim ## cells: that's really not a big issue as I guess this is rarely used to introduce comments. But having an Export scene codelense above my normal Python classes would definitely annoy me (I don't want to think about Manim when I work on a completely unrelated Python project). Having to disable the extension just to work in regular Python files really doesn't sound too sexy to me ;)

I agree that we don't need this for now, but I wouldn't close this issue as it's not resolved (and I'd like to fix it later).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement-archive Don't use this label anymore.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants