-
Notifications
You must be signed in to change notification settings - Fork 11
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
130 ay 6965 default output paths and support custom staging directories. #200
base: develop
Are you sure you want to change the base?
Conversation
I tested with publishing the model product. the staging directory set correctly Please fix in the separated PR as they are not related to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment above
Nice work!
Also wanted to mention that there is quite some overlap between this feature and the Custom Staging Dir feature which would define where published work renders should go to. As such, even though I like this implementation it may very well happen down the line that this should get superseded by using the custom staging dir setings (which I think are in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main concerns are mentioned here - other than that I think the implementation is very clean and nice.
However, I do think the:
`chs('AYON_productName')`
part should be part of the setting, not hardcoded in the setting of the path.
As such, the default would be this instead:
staging_dir = "$HIP/ayon/`chs('AYON_productName')`"
I also wonder whether making the new default render/
instead of ayon/
would make way more sense. It'd at least align more with other host integrations like Maya.
@antirotor thoughts?
…t-path-update-pyblish-to-something-less-legacy
This won't work with HDAs, as its file name is not saved in a string parameter.
I wonder if Houdini artists might find it confusing because it doesn't align with Houdini defaults where geo files will be saved in |
Regarding point 1 and 3 about being more granular about file names and not using hardcoded subdirectories. |
I see, thanks for clarifying that. Still - it feels odd to still have that part of the path be hardcoded for all the others. Not sure what the best middle ground is - but in a way, it's still not really customizable. Also, similar to "custom staging dir" - a studio may want their renders in a different folder than their geo caches. Which also hints at needing settings PER creator instead of a single global one. Yet I do agree - it'd be a lot of settings to maintain. It could be profile based - where the HDA creator just happens to have a different default profile. Tagging @dee-ynput @antirotor for their thoughts.
Valid point - potentially even more reason to 'remain at the defaults' as per my point 2) above. Side note: I'm personally also NOT a fan of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the meeting we had with @antirotor and @MustafaJafar this became the changes:
- Add settings toggle to enable/disable path control completely (to allow a studio to completely manage it themselves)
- If enabled, we use the default path as you have it set up ✅
- However, if matching custom staging dir profile is specified in
ayon-core
settings then we use that custom staging dir directory instead. (This also only applies if "directory management" is enabled" as per my first point).
…pyblish-to-something-less-legacy' of https://github.com/ynput/ayon-houdini into enhancement/130-ay-6965_rop-default-output-path-update-pyblish-to-something-less-legacy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some note.
@@ -36,6 +61,10 @@ class GeneralSettingsModel(BaseSettingsModel): | |||
|
|||
DEFAULT_GENERAL_SETTINGS = { | |||
"add_self_publish_button": False, | |||
"rop_output": { | |||
"enabled": True, | |||
"default_output_dir": "$HIP/ayon/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(cosmetics)
we don't need the trailing /
although keeping it give the feeling it's a directory.
so, I think we should remove it when reading/getting the staging directory.
"default_output_dir": "$HIP/ayon/" | |
"default_output_dir": "$HIP/ayon" |
|
||
if self.enable_staging_dir: | ||
# keep dynamic link to product name in file path. | ||
self.staging_dir = get_custom_staging_dir(self.product_type, product_name) or self.staging_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When calling this - it'd be good to pass the project settings from the create context to the get_custom_staging_dir
which can then be passed to get_staging_info
I'm also not sure about using the get_current_context
in that function. We should be passing the context of the create_context
to be explicit, and preferably even that of the instance (if targeting another folder path and task than current context, which of the ones should it be using? I assume the target one?)
I'd move that to the HoudiniCreator
base class and just create a get_staging_dir(self, instance: CreatedInstance)
from which I assume we should be able to get all data to compute the resulting staging dir - moving all the heavy lifting of that method to the base class instead of having to duplicate it to each inherited class.
Also, I'd avoid overwriting self.staging_dir
with this result - AYON plugin systems tend to re-use base classes and instances in different areas (not sure if creators do that too) but it's prone to somehow 'spilling' into the next call of a creator if it does. In this case there's also no need to store it on self
since you're just using the result anyway. ;) So just make it the result of that self.get_staging_dir()
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @BigRoy. We can instead create a function for the default/custom staging_dir or just do self.get_staging_dir
call in the HoudiniCreator
(Guess most of the creators inheriting from that, right?), and We dont need to assign or set condition on every script lasted in create
folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing that out, I didn't know that Creator.get_staging_dir exists.
I only knew about get_staging_dir_info, Extractor.staging_dir and get_instance_staging_dir
(if targeting another folder path and task than current context, which of the ones should it be using?
The creator tab (at least what I'm seeing in the UI) doesn't allow creating an instance in a different context.
and users can change that in the publish tab later.
Maybe for that case, we may just create a validator that checks the staging dir path🤔?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any ideas about how to use the get_staging_dir_info with HDA creator which uses create_instance_node
method that doesn't have access to the instance
object.
I can only think of the current function get_custom_staging_dir
that I made in the lib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried using get_staging_dir_info
By overriding it in HoudiniCreator
, I wanted to allow overriding the instance type so that I can use the real product type for render product creators e.g. "mantra_rop"
creator should actually get the staging directory of "render"
product type.
The following snippet doesn't work because of TypeError: cannot pickle 'WeakMethod' object
error.
def get_staging_dir(self, instance, product_type_override=None):
if product_type_override:
instance = copy.deepcopy(instance)
instance["productType"] = product_type_override
return super().get_staging_dir(instance) or self.staging_dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes nice. Now should we move the expandString
call into the method too? So that it's just one place where we're expanding it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The creator tab (at least what I'm seeing in the UI) doesn't allow creating an instance in a different context.
and users can change that in the publish tab later.
@MustafaJafar it's the left section:
A different folder path and task is a different context. Be aware even that technically you can even deselect task there. 🤯 To publish outside of a task context similar to tray publisher could do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes nice. Now should we move the
expandString
call into the method too? So that it's just one place where we're expanding it?
I think this should be done in a separate PR because as far as I know, it'll affect other places. e.g. ValidateWorkfilePaths
and publishing workfiles.. it should also be checked with deadline.
A different folder path and task is a different context. Be aware even that technically you can even deselect task there. 🤯 To publish outside of a task context similar to tray publisher could do.
oh I forgot about that, I was focusing on different contexts within other projects.
Thank you! let me check it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current status of this PR doesn't respect the changes in the context (folder_path
and task
) which leads to exporting/saving the files into the same context.. which is mostly the same directory of my workfile.
Very important, the creator tab in the publisher UI doesn't allow changing the project, so, we are always using the current project which means we won't need to get and pass any project settings.
we should also take into considerations that when respecting the changes in the context, our files will be exported in another location other that the current working directory.
anyways, I tried finding ways to consider the changes in the context.
I thought about using self.create_context
. but I found out it is just a ayon_core.pipeline.create.CreateContext
object which doesn't include info about the selected folder path and task in the publisher UI.
Also, found out that the selection is saved in the instance
object, so we would need to pass the instance
like the get_staging_dir
in super class.
The get_staging_dir
in super class covers more use cases but sometimes, we don't want to use it in Houdini as
- it doesn't work with HDA creator as
create_instance_node
is called before theinstance
object creation. - it doesn't allow overriding the product type (which is needed with render rops). so admins will need to use the product type for render rops like
mantra_rop
,karma_rop
and etc.
I'm not sure what would be the ideal solution.. I can think of these ideas:
- use
get_staging_dir
from super class and I've no idea what to do with HDAs and render creators. - copy the implementation of
get_staging_dir
from super class and tweak it to be Houdini friendly where I can implement some overrides. I can passproduct_name
,product_type
andfolder_path
but no idea about getting the version asget_staging_dir
includes this callself.get_next_versions_for_instances([instance])
.. the version is used as template data. - continue with the current implementation and add a note/todo that mentions the limitations about respecting changes in the context selection in the publisher UI leading to always following the current context.
Regarding the second idea, Maybe something like this will work:
def get_staging_dir(self, product_type, product_name, folder_path, version):
fake_instance = {
"productName": product_name,
"productType": product_type,
"folderPath": folder_path,
"version": version
}
return super().get_staging_dir(fake_instance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current status of this PR doesn't respect the changes in the context (
folder_path
andtask
) which leads to exporting/saving the files into the same context.. which is mostly the same directory of my workfile.Very important, the creator tab in the publisher UI doesn't allow changing the project, so, we are always using the current project which means we won't need to get and pass any project settings.
we should also take into considerations that when respecting the changes in the context, our files will be exported in another location other that the current working directory.
anyways, I tried finding ways to consider the changes in the context.
I thought about using
self.create_context
. but I found out it is just aayon_core.pipeline.create.CreateContext
object which doesn't include info about the selected folder path and task in the publisher UI.Also, found out that the selection is saved in the
instance
object, so we would need to pass theinstance
like theget_staging_dir
in super class.The
get_staging_dir
in super class covers more use cases but sometimes, we don't want to use it in Houdini as
- it doesn't work with HDA creator as
create_instance_node
is called before theinstance
object creation.- it doesn't allow overriding the product type (which is needed with render rops). so admins will need to use the product type for render rops like
mantra_rop
,karma_rop
and etc.I'm not sure what would be the ideal solution.. I can think of these ideas:
- use
get_staging_dir
from super class and I've no idea what to do with HDAs and render creators.- copy the implementation of
get_staging_dir
from super class and tweak it to be Houdini friendly where I can implement some overrides. I can passproduct_name
,product_type
andfolder_path
but no idea about getting the version asget_staging_dir
includes this callself.get_next_versions_for_instances([instance])
.. the version is used as template data.- continue with the current implementation and add a note/todo that mentions the limitations about respecting changes in the context selection in the publisher UI leading to always following the current context.
Regarding the second idea, Maybe something like this will work:
def get_staging_dir(self, product_type, product_name, folder_path, version): fake_instance = { "productName": product_name, "productType": product_type, "folderPath": folder_path, "version": version } return super().get_staging_dir(fake_instance)
Have you tried to get staging directory without super class?(I know it's dumb to ask but it should be possible to do so?)
Speaking of this, you may consider taking reference from what Robin and Jeza did for the custom staging directory in Nuke. ynput/ayon-nuke#19.
Changelog Description
This PR adds a main setting for configuring output directory for created ROP nodes via AYON creator plugins.
{default_path}/`chs('AYON_productName')`/hardcoded_filename
{default_path}/`chs('AYON_productName')`/hardcoded_foldername/hardcoded_filename
ayon-core
settings then we use that custom staging dir directory instead. (This also only applies if "directory management" is enabled" as per the first point).resolve #130 and resolve #21
Demo
houdini_cache
templatehoudini_Render
templateTesting notes: