-
Notifications
You must be signed in to change notification settings - Fork 77
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
Refactor RenderConfig interface #1561
base: master
Are you sure you want to change the base?
Refactor RenderConfig interface #1561
Conversation
…g term goal is to reduce the amount of context which is pushed around for rendering with the RenderManager/RenderContext) TODO later: split SceneIOProvider from RenderContext
TODO: add PluginAPI
abd7168
to
471416f
Compare
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 like the idea in general, just a few comments: (github removed my comments for some reason)
protected void startRender(Renderer renderer) throws InterruptedException { | ||
renderer.render(this); | ||
} |
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.
Is this for plugins?
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 believe I added this to be overridable by custom RenderManagers to add pre-start logic.
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.
e.g. setting data in the new worker state / ray factories
@@ -0,0 +1,40 @@ | |||
package se.llbit.chunky.renderer; | |||
|
|||
public class ModifiableRenderOptions implements RenderOptions { |
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.
See above. Can we improve this so that modifying the render options is safe and gets applied after the user confirms (or reset otherwise)?
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.
Pretty sure that this would be the job of the UI logic or something in the render code which copies its state from this object. At the moment, the user can only set the tile size
and spp per pass
via CMD argument, right? And render thread count had some special handling somewhere which already required restarting the rendering or Chunky (?). Definitively a useful suggestion for a follow-up issue + PR :)
(long term goal is to reduce the amount of context which is pushed around for rendering with the RenderManager/RenderContext)