Add interface to JS renderer

[eulertour]

Adds the ability to communicate with the electron frontend at https://github.com/ManimCommunity/manim-renderer, doing some refactoring in scene.py and __main__.py in the process.

This PR adds the JsScene and JsCamera classes, which are used in place of Scene and Camera respectively when using javascript for rendering. It also adds a FrameServer class which communicates with the renderer over gRPC. The FrameServer runs in python on port 50051 and the RenderServer runs in javascript on port 50052.

A few things to note:

• The files in manim/grpc/gen/ were generated by grpc, so there's no need to review them.
• The UpdateFrontendRenderer class in manim/grpc/impl/frame_server_impl.py will eventually allow for automatic updates upon code changes but currently doesn't do anything, so there's no need to review it.
• At the bottom of manim/scene/js_scene.py theres a line which overwrites Scene with JsScene when javascript rendering is requested. This allows for both javascript and ffmpeg rendering without the user changing their code, but if some code were to use Scene prior to this line when javascript rendering is requested, this could potentially cause problems.
• get_module(), get_scene_classes_from_module(), and get_scenes_to_render() were moved from __main__.py to utils/module_ops.py, in order to share them with the FrameServer.
• handle_play_like_call() was moved out of Scene in order to share it with JsScene.
• Scene.progress_through_animations() was refactored to be a little simpler. It's still not simple enough, as play() should ideally look something like

def play(self, animations):
    for t in self.get_animation_time_progression(animations):
        self.update_animations_to_time(t)
        self.camera.take_snapshot()
        self.renderer.render()

• self.display_funcs in Camera was moved into the function where it's used. The reason is that initializing it early causes problems when the Camera is copied (FrameServer shares the same Camera instance between many Scene instances, but the initialization forms a closure around self (a single Camera instance)).

It'd also be helpful if you try to build and run the renderer at https://github.com/ManimCommunity/manim-renderer. I've only used it on Linux and one person has run it on Windows a while ago, but I don't have a good idea of how hard it is to set up from scratch.

[naveen521kk]

one person has run it on Windows a while ago

It's me and it works. Pretty much I will write some docs on how-to for other users. Pretty much I will suggest we can use the-action to build executables. I will make a PR on that in that repo.

[eulertour]

I haven't looked at the "Run module doctests" step in the CI before, but it's more likely a problem with some sort of configuration than the actual implementation.

[behackl]

I haven't looked at the "Run module doctests" step in the CI before, but it's more likely a problem with some sort of configuration than the actual implementation.

The relative import warning can be fixed by adding empyt __init__.py files to these directories.

[eulertour]

It seems like CodeFactor doesn't know how if statements work in python? It considers it to be a problem if an if-block containing a return is followed by an elif or else block.

[kilacoda-old]

It's probably considering it to be unreachable code or something.

[eulertour]

CodeFactor doesn't like exec, TODOs, if statements, and an admittedly weird import that happens to be necessary here. Not only do I not think it's worth it to satisfy it, but in the case of exec I don't even think it's possible.

[naveen521kk]

Codefactor is just running PyLint. You can run one in you computer also and fix it. Usually default in many ide's.

[eulertour]

Running all PRs through pylint is a pretty big change to make with no notice. We should get a .pylintrc, add the dependency to poetry, and update the docs with this.

[leotrs]

@eulertour this is my fault - like I said on discord, I meant to try out CodeFactor on my own but didn't realize it was installed on the whole repo. I left it on because I didn't think it was too obtrusive, as we have been merging PRs with the CodeFactor check still in red in some cases.

I agree that enforcing it with no notice is a big change, though we haven't really been enforcing it.

So the paths forward are to ignore it and leave it here (because I do think it's somewhat useful) but ignore it, or just uninstall it.

[eulertour]

I'd prefer to ignore it for until we make the changes from my previous comment and start using it then.

[naveen521kk]

I suggest we can merge this only after first release.

[eulertour]

We can tag a current or recent commit for the release. We can still do the review now.

[eulertour]

Full disclosure, if nobody is willing/able to review this I'll probably merge it soon. Not that that's necessarily a bad thing, but it'll cost us a lot of progress in the long run to leave complex developments for >1 week.

[leotrs]

I'll review it. In fact, I have already reviewed half the files. It's just that my time is not my own this week. Sorry for the wait!

…

On Thu, Oct 1, 2020 at 11:59 AM Devin Neal ***@***.***> wrote: Full disclosure, if nobody is willing/able to review this I'll probably merge it soon. Not that that's necessarily a bad thing, but it'll cost us a lot of progress in the long run to leave complex developments for >1 week. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#465 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAILYAHTRXLVXMKVYEJXOILSISRNZANCNFSM4RUGKKHA> .

-- www.leotrs.com | [email protected] PhD candidate at the Network Science Institute, Northeastern University

[leotrs]

I've reviewed everything except for the frame server class. A couple quick things:

1. please don't forget to edit the changelog too
2. there's a major discussion to be had about how to test this
3. what's your opinion on merging this before or after the v0.1 release?

[behackl]

There is one thing I am curious about: you say that

• The files in manim/grpc/gen/ were generated by grpc, so there's no need to review them.

If they are automatically generated, do we really need to keep them under version control? If possible, I'd like to have them generated during the installation of manim, for example. (But I don't know anything about grpc, sorry if the question does not make sense. :-))

[eulertour]

@behackl the question makes sense but it should happen separately from this, as poetry.lock should also be handled this way.

[leotrs]

Yes we should totally have a (separate) PR removing certain files from version control

(though it would be great to not add them in the first place 👀 )

[behackl]

Sure, I am fine with delegating this to a later PR.

However, the situation for the lock file is different from these autogenerated code files: they ensure a more deterministic setup (especially when it comes to the pipelines / testing) and should be kept (see https://stackoverflow.com/questions/39990017/should-i-commit-the-yarn-lock-file-and-what-is-it-for or similar questions -- the answer there is for yarn, but the arguments still apply).

We can come back to this question whenever we deal with other autogenerated stuff. :-)

[eulertour]

It'd certainly be better to generate the files at install time, but learning the proper way to do it with poetry and finding a way to test it would add a lot of unnecessary overhead to this PR, especially since it's rather large already. I'd be happy to open an issue when this is merged.

To answer you questions @leotrs

1. please don't forget to edit the changelog too

Done

2. there's a major discussion to be had about how to test this

Not only this, but all of the renderers. IIUC our current tests only validate the pixel data before it's sent to be rendered, so even the de facto cairo renderer isn't being tested right now.

3. what's your opinion on merging this before or after the v0.1 release?

I'd prefer just to do it as soon as possible, not so that it's included in the release but so I can continue to simplify scene.py and factor out the rendering logic. For future releases we should decide on using stable/dev branches or tagging a commit early on so releases don't hinder development progress.

[behackl]

but learning the proper way to do it with poetry

I did a little bit of searching: poetry does not support post install scripts at all, which is very frustrating. (https://stackoverflow.com/questions/59802468/post-install-script-with-python-poetry -- grpc is also mentioned in the issue linked in the answer.)

Anyhow, let us revisit this at a later time, as you suggest. I agree with including the files for the time being.

[leotrs]

The tests pass, and this does't seem to change the current workflow of users in any way.

Is this at a stage where it is ready to be use-tested by end users? If so, I'd suggest adding documentation. If not, then we can just merge this silently and not ask anyone to use it yet.

[eulertour]

It's not quite there yet, so I'd be fine merging it silently.

[leotrs]

SGTM.

[behackl]

I have primarily reviewed your changes to existing files and rather superficially read the new additions; everything looks good to me (and the option of having an alternative render backend is really exciting; pretty cool stuff!).

I only have a minor change request before I also approve: the fact that rendering simply by creating a Scene object has changed to rendering on calling Scene.render() should be documented explicitly in the changelog, I feel. Could you add another explicit entry for that?

[behackl]

Thanks! Feel free to merge after resolving the merge conflict.

[kolibril13]

This commit breaks the set_test_scene function to produce control data.