Skip to content
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

Maya: Set default value for RenderSetupIncludeLights option #3944

Merged
merged 5 commits into from Oct 12, 2022

Conversation

antirotor
Copy link
Member

Fix

RenderSetupIncludeLights must be either set to 1 or 0 or not at all. This PR is adding default value for edge cases where this is not handled. Without it, DL is crashing.

RenderSetupIncludeLights must be either set to 1 or 0 or not set at all
@antirotor antirotor added type: bug Something isn't working host: Maya module: Deadline AWS Deadline related features labels Oct 5, 2022
@antirotor antirotor self-assigned this Oct 5, 2022
Copy link
Member

@m-u-r-p-h-y m-u-r-p-h-y left a comment

Choose a reason for hiding this comment

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

submitted Maya render with old render instance with missing Render Setup Include Lights attribute.

Deadline job has the attribute in job properties with value 1 so Deadline wont crash anymore.

The question is, if we are really assigning default value from project settings
image

image

@m-u-r-p-h-y
Copy link
Member

another question is, for cases with the missing attribute on the instance and in settings, if the "safer" default value should be 0 instead of 1

@antirotor
Copy link
Member Author

another question is, for cases with the missing attribute on the instance and in settings, if the "safer" default value should be 0 instead of 1

I think it should follow Maya default and that is 1. Added code to pull default value from the Settings.

Comment on lines 204 to 208
default_rs_include_lights = instance.context.data\
.get('project_settings')\
.get('maya')\
.get('RenderSettings')\
.get('enable_all_lights')
Copy link
Collaborator

Choose a reason for hiding this comment

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

This .get().get().get() feels wrong. As soon as one of the keys doesn't exist it'd be None and doing .get() would fail. So it's just as prone to error as just doing ["project_settings"]["maya"]["RenderSettings"]["enable_all_lights"]

Also the fallback down below to the default value could thus still fall back to "None" otherwise if enable_all_lights wouldn't exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, I've just copy&pasted it from somewhere. I guess it was like that because of readability but that is questionable anyway.

@m-u-r-p-h-y
Copy link
Member

I think it should follow Maya default and that is 1. Added code to pull default value from the Settings.

that's exactly what I meant. I thought our default 0 is Maya's default. You are correct it is 1, so we should change our defaults in openpype\settings\defaults\project_settings\maya.json to 1 to follow Maya's default value.

Copy link
Member

@m-u-r-p-h-y m-u-r-p-h-y left a comment

Choose a reason for hiding this comment

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

It is correctly assigning the value from project settings, in case the value is missing on render instance,

image

@antirotor antirotor merged commit 10cd563 into develop Oct 12, 2022
@antirotor antirotor deleted the bugfix/maya_include-all-lights-default branch October 12, 2022 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host: Maya module: Deadline AWS Deadline related features type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants