-
Notifications
You must be signed in to change notification settings - Fork 23
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
fix(submitter): read includeAllLights setting correctly #207
fix(submitter): read includeAllLights setting correctly #207
Conversation
Signed-off-by: Jericho Tolentino <[email protected]>
31a6818
to
6583acd
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.
thank you.
Because this is a per-machine setting, we should be updating our job bundle output test infrastructure to ensure we can have a test so we don't regress on this. Will require us to change how we set that value on the test machines as we basically want to control this at the test level. |
Sounds good, I'll do this in a follow-up PR! #208 |
Fixes: #206
What was the problem/requirement? (What/Why)
The IncludeAllLights by default setting for render layers is not being captured correctly by the submitter. The current implementation queries if the corresponding optionVar is set, however if it is not set, then it treats the value as false, even if the actual check box is checked. This value is fed into the
RenderSetupIncludeLights
job parameter which is used by the adaptor to enable/disable the feature for all render layers. This causes confusion when renders submitted to Deadline Cloud return images with incorrect lighting that cannot be reproduced locally since the option is checked locally but not in Deadline Cloud.What was the solution? (How)
Revert the logic for populating the
RenderSetupIncludeLights
job parameter to the original implementationWhat is the impact of this change?
The
RenderSetupIncludeLights
job parameter reflects the current state of the corresponding optionHow was this change tested?
Did you run the "Job Bundle Output Tests"? If not, why not? If so, paste the test results here.
Was this change documented?
N/A
Is this a breaking change?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.