Skip to content

Conversation

@Sudarshanhavale
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@garvitverma garvitverma left a comment

Choose a reason for hiding this comment

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

Please keep the collector comments in mind for the publish plugin as well.

Comment on lines +636 to +638
project_asset_version_name_xgen_file: '@project_asset_publish_name.v{version}__[{palette_name}]'
sequence_asset_version_name_xgen_file: '@sequence_asset_publish_name.v{version}__[{palette_name}]'
shot_asset_version_name_xgen_file: '@shot_asset_publish_name.v{version}__[{palette_name}]'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is palette_name optional? Why are we using this as an optional field?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the earlier approach of collecting xgen palettes as a separate item, The collector was collecting disconnected palettes as well because of the same path patterns. To resolve this issue I referred the collect (layer) render images example in which we are providing node names while collecting item. So followed the same thing here and made palette names optional and passing valid values during collection.

Apparently we moved this collection logics to the as session items properties but I didn't modify those already defined templates. But yeah, since we are passing entire dependency paths from the session, maybe no need of making those as optional fields. 

I will check that once and make the required updates. Thanks for pointing this out.

Copy link

@aparna-d2 aparna-d2 Feb 7, 2020

Choose a reason for hiding this comment

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

Whatever the method of collection, this template looks wrong. making only palette_name optional as in this template means you are looking for a file called name.v001__. Only make something optional in a template if it is truly optional in real life i.e. if a path should be allowed without that part of the template.
Same goes for xgen_collection_subdirs as well - do we expect only xgen/collections to exist without a palette_name subfolder? If not, make it not optional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, palette_name not an optional field in the updated templates and also confirmed that its working fine with items that are added as session item properties.

Comment on lines +89 to +180
schema["file.xgen"] = {
"type": "list",
"description": "List of xgen file publish settings.",
"skip_validation": "True",
"values": {
"type": "dict",
"description": "",
"items": {
"extensions": {
"type": "str"
},
"publish_type": {
"type": "str"
},
"publish_path_template": {
"type": "template",
"default": {},
"description": "Xgen file publish path template"
}
}
},
}

# Schema for Xgen patch cache
schema["file.xgen.patch"] = {
"type": "list",
"description": "List of xgen file patch publish settings.",
"skip_validation": "True",
"values": {
"type": "dict",
"description": "",
"items": {
"extensions": {
"type": "str"
},
"publish_type": {
"type": "str"
},
"publish_path_template": {
"type": "template",
"default": {},
"description": "Xgen file patch publish path template"
}
}
},
}
# Schema for Xgen collection folder
schema["file.xgen.collection"] = {
"type": "list",
"description": "List of xgen collection publish settings.",
"skip_validation": "True",
"values": {
"type": "dict",
"description": "",
"items": {
"extensions": {
"type": "str"
},
"publish_type": {
"type": "str"
},
"publish_path_template": {
"type": "template",
"default": {},
"description": "Xgen collection publish path template"
}
}
},
}
# Schema for 3dPaintTextures
schema["file.3dPaintTextures"] = {
"type": "list",
"description": "List of 3dPaintTextures publish settings.",
"skip_validation": "True",
"values": {
"type": "dict",
"description": "",
"items": {
"extensions": {
"type": "str"
},
"publish_type": {
"type": "str"
},
"publish_path_template": {
"type": "template",
"default": {},
"description": "3dPaintTextures publish path template"
}
}
},
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks messy... we should be able to create one single setting that can collect all the xgen specific work_path_templates for this session and assign some sort of identifier to each of them which can then be picked up in the publish plugin, define the publish_type and publish_path_template in publish settings not in collector settings.

import sgtk
from sgtk import TankError
from sgtk.templatekey import SequenceKey
from maya_xgen_handle import MayaXgenHandle
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this maya_xgen_handle module coming from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maya_xgen_handle is coming from the supermodule. (sgtk_apps/src/maya/maya_xgen_handle.py)

Choose a reason for hiding this comment

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

Comment on lines +304 to +347
def resolve_custom_path_template(self, settings, parent_item, item_key, work_path):
"""
Resolve custom work and publish path_template from the given work_path for the
specified item.
:param settings: collector settings
:param parent_item: parent item
:param item_key: valid settings item key
:param work_path: source work path to resolve publish type and publish path
:return: Resolved values in dict format: {publish_type:{work_path:, publish_path:}}
"""

resolved_values = dict()

work_tmpl = self.sgtk.template_from_path(work_path)
fields = work_tmpl.get_fields(work_path)

if item_key == 'file.xgen.collection':
"""
By default xgen collection doesn't maintain versions
and link with source maya file. So to resolve this issue,
We are collecting session item fields and passing the
missing values to xgen collections publish template
fields which creates the folder with session file name
and publishes collections inside.
"""
# Resolve session work path template
session_work_path = parent_item.properties.path
session_work_template = self.sgtk.template_from_path(session_work_path)
session_item_fields = session_work_template.get_fields(session_work_path)

# Adding required session name fields
fields['Step'] = session_item_fields['Step']
fields['name'] = session_item_fields['name']
fields['version'] = session_item_fields['version']

publish_tmp_name = settings[item_key]['publish_path_template'].value
publish_tmp_obj = self.sgtk.templates[publish_tmp_name]
publish_path = publish_tmp_obj.apply_fields(fields)
publish_type = settings[item_key]['publish_type'].value

resolved_values[publish_type] = {'work_path': str(work_path), 'publish_path': str(publish_path)}

return resolved_values
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's restrict figuring out publish_type and publish_path to the publish plugin instead of resolving those values in the collector. The above properties should be figured out in validate, please look at color_process_files hook as an example.
Collector should ideally care more about whether a work_path_template matches the paths that you have collected or not.

Comment on lines +30 to +37
description_name:
type: str
filter_by: '^[a-zA-Z](\w+)?[a-zA-Z0-9]$'

patch_name:
type: str
filter_by: '^[a-zA-Z](\w+)?[a-zA-Z0-9]$'

Choose a reason for hiding this comment

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

Are these keys in use? If not, please remove them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. This was added considering the Loader requirements but yeah, removing for now as we can add it later whenever required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants