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

Init Reload & Preview Command/CodeLens #95

Merged
merged 15 commits into from
Dec 25, 2024
Merged

Conversation

Splines
Copy link
Member

@Splines Splines commented Dec 5, 2024

Closes #77.

Tip

In the manim repo, I've added a reload() command via 3b1b/manim#2240. This one is already merged and available on the master branch. The follow-up PR 3b1b/manim#2257 improves that command by reloading all user-defined modules. As of 2024-12-05, it is not merged yet. It is also available on the master branch of manim.

This PR makes use of this new reload() command by providing a new code lens that will reload the scene at that point before previewing it. If you want to test that the reloading actually takes place, checkout my branch from 3b1b/manim#2257 and see the test files over there in the PR comment (a.py, b.py and c.py).

image

Additionall small refactoring: We also move part of the preview mechanism from the extension.ts to previewCode.ts to have it in one place.


Linked issues

@Splines Splines self-assigned this Dec 5, 2024
@Splines Splines changed the title Init Reload & Preview Command Init Reload & Preview Command/CodeLens Dec 5, 2024
@bhoov
Copy link
Collaborator

bhoov commented Dec 11, 2024

Wow @Splines, you've been busy! After updating my version of manim, I'm still getting a weird bug on this behavior:

  • If I run Reload & Preview, my terminal says Reloading... and I cannot run any further Preview commands. "Simultaneous Manim commands are not currently supported on MacOS...". See my screenshot below where the terminal resets its numbering and the alert is in the bottom corner. Nothing is running, but nothing more can be run. I do NOT have this problem if the "reload&preview" command is used to start the interactive session
20241211T125734--screenshot

Also, to make this feature more accessible to users, we can consider the following:

  1. Only display Reload & preview if manimgl is of an appropriate version. I'm sure many early adopters to this extension will already have some older version of manimgl already installed (and in general, manimgl is not good at being backwards compatible 🙈).
  2. When I tested with the older version, I got the cryptic error NameError: name 'reload' is not defined. We could display an alert to pip install manimgl>=xx.x to allow for interactive session reload

@Splines
Copy link
Member Author

Splines commented Dec 12, 2024

I'm still getting a weird bug on this behavior:

Could you send me a log file, please?

Only display Reload & preview if manimgl is of an appropriate version.

Yes, good idea! I opened #97 for that and will tackle this in a future PR.

When I tested with the older version, I got the cryptic error NameError: name 'reload' is not defined. We could display an alert to pip install manimgl>=xx.x to allow for interactive session reload

If we go for option 1, this would not be necessary as it cannot happen, right? Or do you speak of Manim itself should display such an error?

@Splines Splines marked this pull request as draft December 12, 2024 11:49
@Splines
Copy link
Member Author

Splines commented Dec 12, 2024

I identified the problem and converted this PR to a draft as it needs some more time to be finished.

@bhoov
Copy link
Collaborator

bhoov commented Dec 12, 2024

Could you send me a log file, please?

Not sure if this is the problem you identified, but here is the log

2024-12-12 10:09:38.775 [info] [logger.ts:72:16] [unknown] 📜 Logfile found and cleared at 2024-12-12T18:09:38.775Z
2024-12-12 10:09:39.824 [info] [logger.ts:236:16] [unknown] 📜 Logfile recording started
2024-12-12 10:09:39.824 [info] [logger.ts:76:16] [unknown] Operating system: Darwin 23.4.0 arm64
2024-12-12 10:09:39.824 [info] [logger.ts:77:16] [unknown] Process versions: {"node":"20.16.0","acorn":"8.11.3","ada":"2.8.0","ares":"1.31.0","base64":"0.5.2","brotli":"1.0.9","cjs_module_lexer":"1.2.2","cldr":"43.0","icu":"73.1","llhttp":"8.1.2","modules":"123","napi":"9","nghttp2":"1.61.0","openssl":"0.0.0","simdutf":"5.2.8","tz":"2024a","undici":"6.19.2","unicode":"15.0","uv":"1.46.0","uvwasi":"0.0.21","v8":"12.4.254.20-electron.0","zlib":"1.3.0.1-motley","electron":"30.5.1","chrome":"124.0.6367.243"}
2024-12-12 10:09:39.824 [info] [logger.ts:82:20] [unknown] Manim notebook version: 0.0.4
2024-12-12 10:09:39.824 [info] [logger.ts:93:16] [unknown] --------------------------
2024-12-12 10:09:40.362 [info] [extension.ts:25:11] [unknown] 💠 Command requested: Reload & Preview Manim Cell, startLine=221
2024-12-12 10:09:40.362 [warning] [logger.ts:170:16] [unknown] 💡 Simultaneous Manim commands are not currently supported on MacOS. Please wait for the current operations to finish before initiating a new command.
2024-12-12 10:09:40.365 [warning] [logger.ts:170:16] [unknown] 💡 Simultaneous Manim commands are not currently supported on MacOS. Please wait for the current operations to finish before initiating a new command.
2024-12-12 10:09:43.632 [info] [extension.ts:60:11] [unknown] 💠 Command requested: Record Log File
2024-12-12 10:09:46.855 [info] [extension.ts:25:11] [unknown] 💠 Command requested: Reload & Preview Manim Cell, startLine=221
2024-12-12 10:09:46.855 [warning] [logger.ts:170:16] [unknown] 💡 Simultaneous Manim commands are not currently supported on MacOS. Please wait for the current operations to finish before initiating a new command.
2024-12-12 10:09:46.859 [warning] [logger.ts:170:16] [unknown] 💡 Simultaneous Manim commands are not currently supported on MacOS. Please wait for the current operations to finish before initiating a new command.
2024-12-12 10:09:47.381 [info] [extension.ts:25:11] [unknown] 💠 Command requested: Reload & Preview Manim Cell, startLine=221
2024-12-12 10:09:47.381 [warning] [logger.ts:170:16] [unknown] 💡 Simultaneous Manim commands are not currently supported on MacOS. Please wait for the current operations to finish before initiating a new command.
2024-12-12 10:09:47.388 [warning] [logger.ts:170:16] [unknown] 💡 Simultaneous Manim commands are not currently supported on MacOS. Please wait for the current operations to finish before initiating a new command.
2024-12-12 10:09:51.202 [info] [extension.ts:25:11] [unknown] 💠 Command requested: Reload & Preview Manim Cell, startLine=221
2024-12-12 10:09:51.202 [warning] [logger.ts:170:16] [unknown] 💡 Simultaneous Manim commands are not currently supported on MacOS. Please wait for the current operations to finish before initiating a new command.
2024-12-12 10:09:51.206 [warning] [logger.ts:170:16] [unknown] 💡 Simultaneous Manim commands are not currently supported on MacOS. Please wait for the current operations to finish before initiating a new command.
2024-12-12 10:09:53.056 [info] [extension.ts:77:11] [unknown] 💠 Command requested: Finish Recording Log File

If we go for option 1, this would not be necessary as it cannot happen, right? Or do you speak of Manim itself should display such an error?

Correct, we do not need both 1 and 2, but either can do the trick

@Splines Splines marked this pull request as ready for review December 12, 2024 22:13
@Splines
Copy link
Member Author

Splines commented Dec 12, 2024

So the problem you were encountering should be fixed by now such that this PR is ready for review 🙌

  • There might be still a caching issue when you use reload(), but that is out of Manim Notebook control. I've created an issue for this at Manim, see my PR comment above. -> Checkout commit 33dbf04985 of Manim or newer to avoid this problem.
  • Another problem is that the window position offsets for me every time I restart, but that is an pyglet issue (I've opened an issue for this as well). -> Partially mitigated by commit 3d9a0cd25e

@Splines
Copy link
Member Author

Splines commented Dec 12, 2024

In a follow-up PR, we could add this reload() behavior to the Start scene at cursor command, see #101.

@Splines Splines mentioned this pull request Dec 19, 2024
@Splines
Copy link
Member Author

Splines commented Dec 19, 2024

The next PR #102 is already building upon this PR. To move on, I will merge this one in the next couple of days. For me, the reload() feature works fine now.

@bhoov
Copy link
Collaborator

bhoov commented Dec 22, 2024

I haven't had time to thoroughly test this feature yet, but go ahead and merge. I'll check the full functionality in #102

@Splines Splines merged commit e197d00 into main Dec 25, 2024
@Splines Splines deleted the feature/reload-preview branch December 25, 2024 22:56
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.

Reinitialize scene without having to quit and then restart (reload() command)
2 participants