-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Renderer refactor #532
Conversation
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.
Some comments :D
@huguesdevimeux Why did you mark this as a draft? |
Because you said it is WIP, isn't it ? |
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. |
I'm sorry then. I unmarked it |
I thought I left comments and not approved, it appears that I missclicked. Sorry for that
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.
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.
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.
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.
This PR addresses some of #524.
CairoRenderer
class containing all of the frame handling / caching / skipping logic previously done inScene
test_camera.py
and addedtest_renderer.py
(which does the same thing)Scene.camera
toScene.renderer.camera
, likewise forScene.file_writer
There is still more work to be done here.
CairoRenderer
can't be instantiated without passing it aScene
first, i.e. the following is impossible:Scene
in theSceneFileWriter
Camera
class still need to removed in favor of a more general oneCamera
class should either be renamed into something likeFrameBuilder
or absorbed into theCairoRenderer
that holds it