Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Maya: fix image prefix warning in validator #3128

Merged

Conversation

antirotor
Copy link
Member

Fix

This is fixing issue when Merge AOV option forces render settings validator to complain about superfluous <RenderPass> token. But if removed, it issues warning about image prefix not being the one recommended.

Test

Create scene with render instance, in Render Settings check Merge AOVs in Arnold settings. Leave prefix as it is. Hit Publish -> Validate. Render Settings Validator should fail on unnecessary <RenderPass> token. Remove it from the prefix, validate again and it should pass.

Note

This is quick fix but I think Render Settings validator still needs big overhaul to deal with different renderer settings more gracefully. Ideally providing validator for every renderer.

Close #3127

@antirotor antirotor added type: bug Something isn't working host: Maya labels May 2, 2022
@antirotor antirotor requested a review from m-u-r-p-h-y May 2, 2022 16:17
@antirotor antirotor self-assigned this May 2, 2022
@ynbot
Copy link
Contributor

ynbot commented May 2, 2022

@@ -92,6 +92,7 @@ def process(self, instance):
def get_invalid(cls, instance):

invalid = False
multipart = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at all the changes this addition seems redundant.

Suggested change
multipart = False

Comment on lines 218 to 223
# remove aov token from prefix to pass validation
# first resolve it and then remove if dangling
default_prefix = default_prefix.replace(
"{aov_separator}", instance.data.get("aovSeparator", "_"))
default_prefix = default_prefix.rstrip(
instance.data.get("aovSeparator", "_"))
Copy link
Collaborator

@BigRoy BigRoy May 2, 2022

Choose a reason for hiding this comment

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

Why first resolve it and then strip the right hand side aov separator tokens, that seems redundant and maybe even not as explicit as one might want. If it's stripping it anyway shouldn't it just replace the explicit {aov_separator} placeholder with an empty string?

Suggested change
# remove aov token from prefix to pass validation
# first resolve it and then remove if dangling
default_prefix = default_prefix.replace(
"{aov_separator}", instance.data.get("aovSeparator", "_"))
default_prefix = default_prefix.rstrip(
instance.data.get("aovSeparator", "_"))
# remove aov token separator
default_prefix = default_prefix.replace("{aov_separator}", "")

Additionally, since the image prefix is hardcoded in the validator anyway (doesn't seem to allow to be overridden by settings) this "fix" could even be made more explicit by explicitly replacing the end of the default image prefix alltogether?

We could even get by with just a single re.sub(re.escape("{aov_separator}<RenderPass>") + "$", "", default_prefix)? Or even as simple as prefix = prefix[:-len("{aov_separator}<RenderPass>")] to just strip off the hardcoded end or prefix = prefix.split("{aov_separator}")[0]?

Copy link
Member

Choose a reason for hiding this comment

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

@antirotor just pinging you here in case you didn't notice

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.

Validators are passing correctly with Merge AOV option turned on

image

@antirotor antirotor merged commit 71334fa into develop Jun 21, 2022
@antirotor antirotor deleted the bugfix/OP-3171_render-settings-validator-prefix-warning branch June 21, 2022 13:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
host: Maya type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maya Render Settings validator prefix confusion
5 participants