Skip to content

Renderer refactor #532

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

Merged
merged 27 commits into from
Oct 15, 2020
Merged

Conversation

eulertour
Copy link
Member

This PR addresses some of #524.

  • Creates a CairoRenderer class containing all of the frame handling / caching / skipping logic previously done in Scene
  • Removed test_camera.py and added test_renderer.py (which does the same thing)
  • Updated references to Scene.camera to Scene.renderer.camera, likewise for Scene.file_writer

There is still more work to be done here.

  • The CairoRenderer can't be instantiated without passing it a Scene first, i.e. the following is impossible:
renderer = CairoRenderer() if args.use_cairo else JsRenderer()
scene = SceneClass(renderer)
  • There are still references to the Scene in the SceneFileWriter
  • The variants of the Camera class still need to removed in favor of a more general one
  • The current Camera class should either be renamed into something like FrameBuilder or absorbed into the CairoRenderer that holds it

@eulertour eulertour requested a review from leotrs October 10, 2020 20:29
huguesdevimeux
huguesdevimeux previously approved these changes Oct 11, 2020
Copy link
Member

@huguesdevimeux huguesdevimeux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments :D

@huguesdevimeux huguesdevimeux marked this pull request as draft October 11, 2020 18:25
@eulertour
Copy link
Member Author

eulertour commented Oct 11, 2020

@huguesdevimeux Why did you mark this as a draft?

@huguesdevimeux
Copy link
Member

Because you said it is WIP, isn't it ?

@eulertour
Copy link
Member Author

It addresses part of the issue from #524 but I'd rather fix that issue gradually to keep the PRs small and avoid having to continually rebase.

@huguesdevimeux
Copy link
Member

I'm sorry then.

I unmarked it

@huguesdevimeux huguesdevimeux marked this pull request as ready for review October 11, 2020 20:39
@eulertour eulertour requested a review from behackl October 12, 2020 16:18
@huguesdevimeux huguesdevimeux dismissed their stale review October 13, 2020 11:22

I thought I left comments and not approved, it appears that I missclicked. Sorry for that

Copy link
Contributor

@leotrs leotrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM in general. I haven't reviewed every single line because most of it looks like refactoring and moving stuff around and simplifying logic. That and the tests pass, so it's all good.

Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like how this cleans up the mess in scene/scene.py by really a lot. Thank you very much for your efforts!

I just added a few suggestions here and there (most of them concerning unused imports), but none of them are actually blocking. Feel free to handle them in any way you like.

@eulertour eulertour merged commit 4693af3 into ManimCommunity:master Oct 15, 2020
@eulertour eulertour deleted the renderer-refactor branch March 4, 2021 04:39
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.

4 participants