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

Adding path validator for non-maya nodes #4271

Merged
merged 12 commits into from
Jan 25, 2023

Conversation

moonyuet
Copy link
Member

Brief description

Adding a path validator for filepaths from non-maya nodes, which are created by plugins such as Renderman, Yeti and abcImport.

Description

As File Path Editor cannot catch the wrong filenpaths from non-maya nodes such as AlembicNodes, It is neccessary to have a new validator to ensure the existence of the filepaths from the nodes.

Additional info

There are some possibilities that some non-maya nodes with filepath attributes missing in the validator.
Please suggest if you catch one or more missing.

Testing notes:

  1. Launch Maya from Openpype
  2. Create non-maya nodes from the plugins(like importing abc)
  3. Run the publisher
  4. It will error out and show that the nodes with wrong filepath if it does not exist.

@moonyuet moonyuet added type: enhancement Enhancements to existing functionality host: Maya labels Dec 30, 2022
@moonyuet moonyuet self-assigned this Dec 30, 2022
@ynbot
Copy link
Contributor

ynbot commented Dec 30, 2022

Task linked: OP-4651 paths validator

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

we should create some easy way to add new nodes with path attributes to the validation as we will miss some or there will be new nodes coming with every renderer in maya.

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

Redshift nodes with file path attribute

RedshiftSprite.tex0
RedshiftBokeh.dofBokehImage
RedshiftLensDistortion.LDimage
RedshiftCameraMap.tex0
RedshiftEnvironment.tex0
RedshiftEnvironment.tex1
RedshiftEnvironment.tex2
RedshiftEnvironment.tex3
RedshiftEnvironment.tex4
RedshiftDomeLight.tex0
RedshiftDomeLight.tex1
RedshiftIESLight.profile
RedshiftLightGobo.tex0
RedshiftNormalMap.tex0
RedshiftProxyMesh.fileName
RedshiftVolumeShape.fileName

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

Vray nodes with file path attribute

VRayTexGLSL.fileName
VRayMtlGLSL.fileName
VRayVRmatMtl.fileName
VRayPtex.ptexFile
VRayLightIESShape.iesFile
VRayMesh.fileName2
VRayMesh.overrideFileName
VRayMesh.alembicLayersFileName
VRayMesh.materialAssignmentsFile
VRayMtlOSL.fileName
VRayTexOSL.fileName
VRayTexOCIO.ocioConfigFile
VRaySettingsNode.causticsFile2
VRaySettingsNode.causticsAutoSaveFile2
VRaySettingsNode.lc_fileName
VRaySettingsNode.lc_autoSaveFile
VRaySettingsNode.imap_fileName2
VRaySettingsNode.imap_autoSaveFile2
VRaySettingsNode.pmap_file2
VRaySettingsNode.pmap_autoSaveFile2
VRayScannedMtl.file
VRayScene.FilePath
VRayScene.parameterOverrideFilePath
VRayMtlMDL.filename
VRayProxy.fileName
VRaySimbiont.file

@moonyuet
Copy link
Member Author

moonyuet commented Jan 3, 2023

we should create some easy way to add new nodes with path attributes to the validation as we will miss some or there will be new nodes coming with every renderer in maya.

Yes I agree I think it would be great to let the user just add nodes and the attributes in the openpype settings. I will add some default examples for references in the next update PR

@moonyuet
Copy link
Member Author

moonyuet commented Jan 4, 2023

I have updated the options to allow users adding filename attributes and the related nodes in the setting. Like what you said, it is easier option to deal with the issue as the list of nodes with filename attributes are way too long.
image

I will set the attributes below as default settings for the user references, it wont affect the validator works if the nodes do not exist.
image

@antirotor
Copy link
Member

I agree with the solution, but lets add the nodes we know (and @m-u-r-p-h-y provided) as default settings so they are already present... No harm in doing that and it can save a lot of time configuring.

@moonyuet
Copy link
Member Author

moonyuet commented Jan 9, 2023

VRaySimbiont

I have added all the file attributes from the list @m-u-r-p-h-y provided into the settings in the updated PR.

Copy link
Collaborator

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

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

Nice stuff - I have added some comments.

Another thing that stood out to me is that it doesn't explicitly tell the user which attributes failed. I think we might want to self.log.error the node attribute with the invalid value:

File does not exist: node.attr = "/path/to/file.png"

Should we log the specific attributes involved?

targets = cmds.ls(type=node)
path_attr = ".{0}".format(attr)

if targets:
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to check if targets. If it's an empty targets list the for loop already wouldn't do anything.

instance.context.data["project_settings"]["maya"]["publish"]
)
file_attr = validate_path["ValidatePathForPlugin"]
if file_attr:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd try to push for less indentation and change this to:

if not file_attr:
    return []

for node, attr in file_attr.items():
# check the related nodes
targets = cmds.ls(type=node)
path_attr = ".{0}".format(attr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're formatting the string in the for loop anyway I see no reason why we need to prefix attr here already.

if targets:
for target in targets:
# get the filepath
filepath = cmds.getAttr(target + path_attr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
filepath = cmds.getAttr(target + path_attr)
filepath = cmds.getAttr("{}.{}".format(target, attr))

Comment on lines 16 to 18
"windows": [
"C:\\Users\\Kayla\\Downloads\\OpenColorIO-Configs-master\\OpenColorIO-Configs-master\\aces_1.2"
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change shouldn't be here.

Comment on lines 316 to 325
{
"type": "dict-modifiable",
"collapsible": true,
"key": "ValidatePathForPlugin",
"label": "Non-existent Paths in Non-Maya Nodes",
"use_label_wrap": true,
"object_type": {
"type": "text"
}
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this is the wrong settings schema for this plugin. Instead it should be like the other plugins where it can also have optional, enabled , etc. for the ValidatePathForPlugin key.

As such, the attributes it should validate should be a subkey, like project_settings/maya/publish/ValidatePathForPlugin/attributes

@antirotor - thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

sure, something like:

{
            "type": "dict",
            "collapsible": true,
            "key": "ValidatePathForPlugin",
            "label": "Non-existent Paths in Non-Maya Nodes",
            "children": [
            {
                "type": "boolean",
                "key": "enabled",
                "label": "Enabled"
            },
            {
                "type": "boolean",
                "key": "optional",
                "label": "Optional"
            },
            {
                "type": "boolean",
                "key": "active",
                "label": "Active"
            },
            {
                "type": "dict-modifiable",
                "store_as_list": true,
                "key": "attributes",
                "label": "Attributes",
                "use_label_wrap": true,
                "object_type": {
                    "type": "text"
                }
         }
}

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

OpenVDB

dlOpenVDBShape.filename

GPU Cache

<shape>.cacheFileName

Yeti

pgYetiMayaShape.cfn
pgYetiMayaShape.ofn
pgYetiMayaShape.labf
pgYetiMayaShape.trf
pgYetiMayaShape.liveABCFilename

@BigRoy
Copy link
Collaborator

BigRoy commented Jan 9, 2023

Note

Be aware that the logic involved is relatively rudimentary but the filepaths involved for some of these attributes can be quite different. E.g. some might support <UDIM>, #### (frame padding?), %04d (frame padding, yeti?).

Then additionally if these would involved checking sequences - should it invalidate if ANY frame is missing inside the frame range? Or should we just check to see whether there's any image on disk in that range at all? If we were to check the full range of the sequence against e.g. instance.data["frameStartHandle" and instance.data["frameEndHandle"] then note that some of these attributes might also be influenced by additional attributes that could offset a sequence, or in some cases even have a custom inputTime attribute driving the node to what frames it would sample.

Currently if the logic here would check an attribute that has a dynamic file pattern like a sequence or a UDIM texture it would invalidate the publishing.

@moonyuet
Copy link
Member Author

moonyuet commented Jan 9, 2023

Note

Be aware that the logic involved is relatively rudimentary but the filepaths involved for some of these attributes can be quite different. E.g. some might support <UDIM>, #### (frame padding?), %04d (frame padding, yeti?).

Then additionally if these would involved checking sequences - should it invalidate if ANY frame is missing inside the frame range? Or should we just check to see whether there's any image on disk in that range at all? If we were to check the full range of the sequence against e.g. instance.data["frameStartHandle" and instance.data["frameEndHandle"] then note that some of these attributes might also be influenced by additional attributes that could offset a sequence, or in some cases even have a custom inputTime attribute driving the node to what frames it would sample.

Currently if the logic here would check an attribute that has a dynamic file pattern like a sequence or a UDIM texture it would invalidate the publishing.

I think we can try to use clique library to query those files to see if possible to validate the files, but definitely needs time to write and test something out.

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

Again we should first provide the feature and improve it over time (not waiting to be perfect from the first go)
🙏
adding support to all tokens <####> <%04d> <F> <f> <UDIM> <udim> <frame> can be another PR

@moonyuet
Copy link
Member Author

Again we should first provide the feature and improve it over time (not waiting to be perfect from the first go) 🙏 adding support to all tokens <####> <%04d> <F> <f> <UDIM> <udim> <frame> can be another PR

I agree we can provide the feature first, while we can work on validating the tokens in the next PR, as it needs some time to test and see if it works as expected.

Comment on lines 346 to 395
"AlembicNode": "abc_File",
"VRayProxy": "fileName",
"RenderManArchive": "filename",
"pgYetiMaya": "cacheFileName",
"aiStandIn": "dso",
"RedshiftSprite": "tex0",
"RedshiftBokeh": "dofBokehImage",
"RedshiftCameraMap": "tex0",
"RedshiftEnvironment": "tex0",
"RedshiftEnvironment": "tex1",
"RedshiftEnvironment": "tex2",
"RedshiftDomeLight": "tex0",
"RedshiftDomeLight": "tex1",
"RedshiftIESLight": "profile",
"RedshiftLightGobo": "tex0",
"RedshiftNormalMap": "tex0",
"RedshiftProxyMesh": "fileName",
"RedshiftVolumeShape": "fileName",
"VRayTexGLSL": "fileName",
"VRayMtlGLSL": "fileName",
"VRayVRmatMtl": "fileName",
"VRayPtex": "ptexFile",
"VRayLightIESShape": "iesFile",
"VRayMesh": "fileName2",
"VRayMesh": "overrideFileName",
"VRayMesh": "alembicLayersFileName",
"VRayMesh": "materialAssignmentsFile",
"VRayMtlOSL": "fileName",
"VRayTexOSL": "fileName",
"VRayTexOCIO": "ocioConfigFile",
"VRaySettingsNode": "causticsFile2",
"VRaySettingsNode": "causticsAutoSaveFile2",
"VRaySettingsNode": "lc_fileName",
"VRaySettingsNode": "lc_autoSaveFile",
"VRaySettingsNode": "imap_fileName2",
"VRaySettingsNode": "imap_autoSaveFile2",
"VRaySettingsNode": "pmap_file2",
"VRaySettingsNode": "pmap_autoSaveFile2",
"VRayScannedMtl": "file",
"VRayScene": "FilePath",
"VRayScene": "parameterOverrideFilePath",
"VRayMtlMDL": "filename",
"VRaySimbiont": "file",
"dlOpenVDBShape": "filename",
"pgYetiMayaShape": "cfn",
"pgYetiMayaShape": "ofn",
"pgYetiMayaShape": "labf",
"pgYetiMayaShape": "trf",
"pgYetiMayaShape": "liveABCFilename",
"gpuCache": "cacheFileName"
Copy link
Member

Choose a reason for hiding this comment

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

Missing indentation for attributes entries.

openpype/settings/defaults/project_settings/maya.json Outdated Show resolved Hide resolved
Comment on lines 346 to 377
"AlembicNode": "abc_File",
"VRayProxy": "fileName",
"RenderManArchive": "filename",
"pgYetiMaya": "cacheFileName",
"aiStandIn": "dso",
"RedshiftSprite": "tex0",
"RedshiftBokeh": "dofBokehImage",
"RedshiftCameraMap": "tex0",
"RedshiftEnvironment": "tex2",
"RedshiftDomeLight": "tex1",
"RedshiftIESLight": "profile",
"RedshiftLightGobo": "tex0",
"RedshiftNormalMap": "tex0",
"RedshiftProxyMesh": "fileName",
"RedshiftVolumeShape": "fileName",
"VRayTexGLSL": "fileName",
"VRayMtlGLSL": "fileName",
"VRayVRmatMtl": "fileName",
"VRayPtex": "ptexFile",
"VRayLightIESShape": "iesFile",
"VRayMesh": "materialAssignmentsFile",
"VRayMtlOSL": "fileName",
"VRayTexOSL": "fileName",
"VRayTexOCIO": "ocioConfigFile",
"VRaySettingsNode": "pmap_autoSaveFile2",
"VRayScannedMtl": "file",
"VRayScene": "parameterOverrideFilePath",
"VRayMtlMDL": "filename",
"VRaySimbiont": "file",
"dlOpenVDBShape": "filename",
"pgYetiMayaShape": "liveABCFilename",
"gpuCache": "cacheFileName"
Copy link
Member

Choose a reason for hiding this comment

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

Less indentation than normal?

@antirotor antirotor marked this pull request as ready for review January 23, 2023 13:34
Copy link
Member

@antirotor antirotor left a comment

Choose a reason for hiding this comment

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

Apart of the guard statement that is in this case more of cosmetic issue, it looks good.

Comment on lines 27 to 41
file_attr = validate_path["ValidatePathForPlugin"]["attribute"]
if file_attr:
# get the nodes and file attributes
for node, attr in file_attr.items():
# check the related nodes
targets = cmds.ls(type=node)

for target in targets:
# get the filepath
file_attr = "{}.{}".format(target, attr)
filepath = cmds.getAttr(file_attr)

if filepath and not os.path.exists(filepath):
self.log.error("File {0} not exists".format(filepath)) # noqa
invalid.append(target)
Copy link
Member

Choose a reason for hiding this comment

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

A would use here the guard statement to ease up on nesting and making the code more readable:

Suggested change
file_attr = validate_path["ValidatePathForPlugin"]["attribute"]
if file_attr:
# get the nodes and file attributes
for node, attr in file_attr.items():
# check the related nodes
targets = cmds.ls(type=node)
for target in targets:
# get the filepath
file_attr = "{}.{}".format(target, attr)
filepath = cmds.getAttr(file_attr)
if filepath and not os.path.exists(filepath):
self.log.error("File {0} not exists".format(filepath)) # noqa
invalid.append(target)
file_attr = validate_path["ValidatePathForPlugin"]["attribute"]
if not file_attr:
return invalid
# get the nodes and file attributes
for node, attr in file_attr.items():
# check the related nodes
targets = cmds.ls(type=node)
for target in targets:
# get the filepath
file_attr = "{}.{}".format(target, attr)
filepath = cmds.getAttr(file_attr)
if filepath and not os.path.exists(filepath):
self.log.error("File {0} not exists".format(filepath)) # noqa
invalid.append(target)

@moonyuet
Copy link
Member Author

moonyuet commented Jan 24, 2023

Apart of the guard statement that is in this case more of cosmetic issue, it looks good.

I have updated the PR with the fix on the cosmetic issue. Thx!

@antirotor antirotor merged commit ab9f8b2 into develop Jan 25, 2023
@antirotor antirotor deleted the enhancement/OP-4651_path_validator branch January 25, 2023 15:25
@github-actions github-actions bot added this to the next-patch milestone Jan 25, 2023
validate_path = (
instance.context.data["project_settings"]["maya"]["publish"]
)
file_attr = validate_path["ValidatePathForPlugin"]["attribute"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This PR has broken maya publishing because THIS ValidatePathForPlugin attribute does not exist in settings. It is ValidatePluginPathAttributes instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host: Maya type: enhancement Enhancements to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants