-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
huguesdevimeux
left a comment
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
leotrs
left a comment
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.
behackl
left a comment
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.
CairoRendererclass containing all of the frame handling / caching / skipping logic previously done inScenetest_camera.pyand addedtest_renderer.py(which does the same thing)Scene.cameratoScene.renderer.camera, likewise forScene.file_writerThere is still more work to be done here.
CairoRenderercan't be instantiated without passing it aScenefirst, i.e. the following is impossible:Scenein theSceneFileWriterCameraclass still need to removed in favor of a more general oneCameraclass should either be renamed into something likeFrameBuilderor absorbed into theCairoRendererthat holds it