-
Notifications
You must be signed in to change notification settings - Fork 3
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
Rework ManimGL version detection #121
Conversation
@mitkonikov So with this PR, I've made startup a lot quicker, see the description. Hopefully this is what you had in mind with #119. Feel free to give a review. Tests are now also starting up quicker in VSCode locally, yeah ;) |
OMG! @Splines Now we are talking! This is amazing! Everything is super fast and super slick! Also, all tests pass locally on Windows now! Great job! Let me just check the code, if there are some details left to be improved, but either way, it's awesome! |
Will come back to this later today or tomorrow with fresh eyes to look over the code changes again, before I merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! You can simplify the python.exe
to the python
alias in Windows. Other than that merge it.
Closes #119.
Manim Version detection
The previous approach of how to detect the user's Manim version was brittle and very slow since we directly called
manimgl --version
and also spawned a separate VSCode Terminal. Instead, we now use thechild_process.exec()
function from Node.js directly and execute the following command in the background:From some manual timing-tests, we see that this only takes less than 80ms compared to 1.8s of
manimgl --version
(of course on my machine and only a few runs).Why do we need the ManimGL version?
We need it to decide on several things, e.g.
manimgl
with the new--autoreload
CLI flag? If the user has an older version of ManimGL installed, we don't want to include--autoreload
, since then ManimGL would not even start correctly and complain about the unknown flag.reload()
command available inside the IPython shell? Admittedly, for this, we could also send this Python command instead:print("For this functionality, ManimGL vX.X.X is required.") if 'reload' not in globals() else reload()
. However, with our detection logic we have the possibility to better integrate into the VSCode framework, i.e. show an information message with a button to let the user try determining the version again or even opening our walkthrough containing an installation guide.More benefits:
Known limitations
Reviewers
There are many different code paths, please try to test them all, by manually inserting statements. E.g. one scenario is: Manim version could not be determined at the beginning, but after "Try again" it can be determined. Or another: Manim version could not be determined, user clicks on Preview & Reload, warning message shows up, user clicks on "Try to determine again", then version is determined and command should work now. Etc.