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

Maya: Refactor moved usage of CreateRender settings #3643

Merged
merged 6 commits into from
Aug 15, 2022

Conversation

BigRoy
Copy link
Collaborator

@BigRoy BigRoy commented Aug 10, 2022

Brief description

This is a draft untested PR to fix #3642

Description

Please test publishing from Maya with up-to-date saved settings.

Testing

  1. Ensure render images file rule works as expected.
  2. Ensure aov separator works as expected

@BigRoy BigRoy changed the title Refactor moved usage of CreateRender settings Maya: Refactor moved usage of CreateRender settings Aug 10, 2022
Comment on lines 61 to 65
aov_separator = self._aov_chars[(
self._project_settings["maya"]
["create"]
["CreateRender"]
["RenderSettings"]
["aov_separator"]
)]
Copy link
Collaborator Author

@BigRoy BigRoy Aug 10, 2022

Choose a reason for hiding this comment

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

Both before and after the code change I feel the except KeyError code isn't the most "explicit" we have now. We are currently unable to identify whether the key error is due to the self._aov_chars dictionary not having the right key or whether the Project Settings didn't have the stored setting key.

I feel like both are different errors and potentially should be handled differently. No?

@BigRoy BigRoy marked this pull request as ready for review August 10, 2022 15:07
@BigRoy
Copy link
Collaborator Author

BigRoy commented Aug 10, 2022

Works for us now.

Only thing still missing to be able to submit renders from Maya again is the actual populating of full_exp_files and thus expectedFiles remaining empty currently. It's missing this line that was there originally.

This commit would fix it.

Wasn't sure whether it made sense to include it in this PR.

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.

shouldn't we put some default value here?

image

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.

still not passing through

// Error: pyblish.plugin : Traceback (most recent call last):
  File "D:\REPO\OpenPype\.venv\lib\site-packages\pyblish\plugin.py", line 522, in __explicit_process
    runner(*args)
  File "D:\REPO\OpenPype\openpype\hosts\maya\plugins\publish\validate_render_image_rule.py", line 30, in process
AssertionError: Workspace's `images` file rule must be set to: renders/
Traceback (most recent call last):
  File "D:\REPO\OpenPype\.venv\lib\site-packages\pyblish\plugin.py", line 522, in __explicit_process
    runner(*args)
  File "<string>", line 30, in process
AssertionError: Workspace's `images` file rule must be set to: renders/ // 

image

@BigRoy
Copy link
Collaborator Author

BigRoy commented Aug 11, 2022

still not passing through

I believe you should set it to just renders without the slash? (We could strip the slash in the validator if you'd want to support ending with a slash in settings as well.)

shouldn't we put some default value here?

I'd just put whatever the default workspace.mel sets that comes with OP, which is indeed renders

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

Yes, it is the slash so we should change the wording in the validator message

@BigRoy
Copy link
Collaborator Author

BigRoy commented Aug 11, 2022

Yes, it is the slash so we should change the wording in the validator message

Technically this would also work renders/lighting or whatever subfolders you'd like.
What wording would describe it better for you?

Might just be best to strip any starting/trailing slashes when checking against it to avoid user error anyway?

Also I'm not sure whether the workspace.mel actually allows storing the value with a slash at the end - if it does then technically it could also explicitly check the value directly with the trailing slash.

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

as a "user" I was strictly following the validators messages to use the trailing slash:

Validates "images" file rule is set to "renders/"
and
Workspace's images file rule must be set to: renders/

image

@BigRoy
Copy link
Collaborator Author

BigRoy commented Aug 11, 2022

@m-u-r-p-h-y How about like this? (Just configured it to foobar to test)

afbeelding

@BigRoy
Copy link
Collaborator Author

BigRoy commented Aug 11, 2022

Changed default value and changed the validator's wording.

Might just be best to strip any starting/trailing slashes when checking against it to avoid user error anyway?

Would you still like me to update the validator for this?

@BigRoy
Copy link
Collaborator Author

BigRoy commented Aug 11, 2022

Another question. Is there any reason why this:

def get_file_rule(rule):
    """Workaround for a bug in python with cmds.workspace"""
    return mel.eval('workspace -query -fileRuleEntry "{}"'.format(rule))

value = get_file_rule("images")

Isn't just this:

value = cmds.workspace(fileRuleEntry="images")

And is there a reason why we're setting the workspace rule with Pymel using:

        pm.workspace.fileRules["images"] = default
        pm.system.Workspace.save()

Instead of doing this:

cmds.workspace(fileRule=("images", default))
cmds.workspace(saveWorkspace=True)

Especially since we might want to use as little Pymel as possible also with things like #2353 and #3233


Implemented these changes with 8fe2048 - please test.

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

Might just be best to strip any starting/trailing slashes when checking against it to avoid user error anyway?

Would you still like me to update the validator for this?

I'd just treat it the standard way we do it in other places where we deal with paths. No special treatment.

This avoids saving it many times on repair in scenes with many renderlayers and thus many renderlayer instances since repair runs per instance.
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.

render images file rule works as expected now

I have some troubles with aov separator:
(trying to break it on purpose)

set the aov separator to - (dash)
created couple of aovs

this is the default Redshift aov settings:
<BeautyPath>/<BeautyFile>.<RenderPass>

but the error I got from Render Settings validator

// Error: pyblish.ValidateRenderSettings : AOV (BeautyAux) image prefix is not set correctly <BeautyPath>/<BeautyFile>.<RenderPass> != <BeautyPath>/<BeautyFile>_<RenderPass> // 
// Error: pyblish.ValidateRenderSettings : AOV (Cryptomatte) image prefix is not set correctly <BeautyPath>/<BeautyFile>.<RenderPass> != <BeautyPath>/<BeautyFile>_<RenderPass> // 
// Error: pyblish.ValidateRenderSettings : AOV file format is not the same as the one set globally 1 != 6 // 
// Error: pyblish.ValidateRenderSettings : AOV (Z) image prefix is not set correctly <BeautyPath>/<BeautyFile>.<RenderPass> != <BeautyPath>/<BeautyFile>_<RenderPass> // 
// Error: pyblish.plugin : Traceback (most recent call last):
  File "D:\REPO\OpenPype\.venv\lib\site-packages\pyblish\plugin.py", line 522, in __explicit_process
    runner(*args)
  File "D:\REPO\OpenPype\openpype\hosts\maya\plugins\publish\validate_rendersettings.py", line 91, in process
AssertionError: Invalid render settings found for 'Main'!
Traceback (most recent call last):
  File "D:\REPO\OpenPype\.venv\lib\site-packages\pyblish\plugin.py", line 522, in __explicit_process
    runner(*args)
  File "<string>", line 91, in process
AssertionError: Invalid render settings found for 'Main'! // 

is using underscore

also, the file format error is not very clear, as the user is not aware of 1 and 6 and what format it actually represents.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Aug 11, 2022

Nice! :)


// Error: pyblish.ValidateRenderSettings : AOV file format is not the same as the one set globally 1 != 6 // 

This is a known bug for Redshift Cryptomattes: #3410


// Error: pyblish.ValidateRenderSettings : AOV (Z) image prefix is not set correctly <BeautyPath>/<BeautyFile>.<RenderPass> != <BeautyPath>/<BeautyFile>_<RenderPass> // 

The others show that it's not being validated against the correct AOV separator.

That bug comes from the fact that it validates against instance.data["aovSeparator"] which is the aov separator actually being collected by the layer/AOV as collected in CollectRender and not the aov separator that the settings define.

As such, instead the validator should just be doing instance.data["aovSeparator"] != settings_aov_separator however that would technically be incorrect since Redshift allows a separator per AOV to be set instead of per layer and thus we should be validating that PER aov and not per layer. Thus even worse it shows that the collected aovSeparator in instance.data is wrong to begin with too. The separator isn't defined per layer/instance but per AOV and thus should be collected as such too.

As such this identifies an issue that might not be a simple fix - should we separate into an issue of its own? Or if not, if someone could take over this PR from here that'd be great :)

@antirotor
Copy link
Member

should we separate into an issue of its own?

Yeah, it is not only redshift that allows separator to be defined per AOV (or rather the whole prefix, but that is question if we even want to support). So I vote for another issue as that one will be harder to solve. I thought we have one already for this but I cannot find it.

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

should this be merged than to get images file rule fixed?

@BigRoy
Copy link
Collaborator Author

BigRoy commented Aug 11, 2022

I'd say so. This is a decent hotfix now for the issue at hand except for maybe this missing commit:

This commit would fix it.

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.

approved for images file rule

@antirotor antirotor merged commit cafd9a1 into ynput:develop Aug 15, 2022
@BigRoy BigRoy deleted the fix_3642 branch March 20, 2024 15:21
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: Validate Images File Rule (Workspace) always validates "None"
3 participants