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

130 ay 6965 default output paths and support custom staging directories. #200

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

MustafaJafar
Copy link
Contributor

@MustafaJafar MustafaJafar commented Dec 18, 2024

Changelog Description

This PR adds a main setting for configuring output directory for created ROP nodes via AYON creator plugins.

  • 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 follows {default_path}/`chs('AYON_productName')`/hardcoded_filename
    • intermediate render directories get a hardcoded folder name as follows{default_path}/`chs('AYON_productName')`/hardcoded_foldername/hardcoded_filename
  • 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 the first point).

image

resolve #130 and resolve #21

Demo

staging templates custom staging directories
image image

image

  1. follows houdini_cache template
  2. follows houdini_Render template
  3. follows the default value in Houdini settings as it doesn't match any profile.

Testing notes:

  1. Create new instances in Houdini, you should find the new output paths.
  2. Publishing new and old instances should work without issues.

@MustafaJafar MustafaJafar added type: enhancement Improvement of existing functionality or minor addition sponsored This is directly sponsored by a client or community member labels Dec 18, 2024
@MustafaJafar MustafaJafar self-assigned this Dec 18, 2024
@moonyuet
Copy link
Member

moonyuet commented Dec 23, 2024

I tested with publishing the model product. the staging directory set correctly
image
But later I find out that two of the validators are broken, guess we need to implement the fixes on these two.
They are Validate Primitive to Detail(abc) and Validate Single Frame
The attachments are the json report to show the error of the validator(s).
publish-report-241223-16-42.json
This error of Validate Single Frame showed after the fix in Validate Primitive to Detail(abc)
publish-report-241223-16-44.json

Please fix in the separated PR as they are not related to this PR.

Copy link
Member

@moonyuet moonyuet left a 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

@BigRoy
Copy link
Contributor

BigRoy commented Jan 7, 2025

Nice work!

  1. I'm not entirely convinced that ONLY allowing to customize the output directory is sufficient - but maybe it is! ❤️ Would need input from the community to fully grasp whether this is the best approach. Like it may make sense to be more concrete about the filename as well?
  2. Whenever we discuss this 'feature' I always keep thinking whether we are doing it the right Houdini-way. I'm wondering if we should have a checkbox in settings that instead disables setting the path completely so that it's possible for a studio to actually define the output path completely by setting their own node attribute defaults directly in Houdini. Again, possibly something we should check with the Houdini-community using AYON. Does it actually make sense that we set it through code instead of relying on the node's defaults (which can be set by a studio, or even an artist specifically) to be adopted instead.
  3. Having a single configurable entry does keep it VERY simple to manage - I like that. However, it does disallow completely to go more granular (which wasn't possible anyway, so could be something for later instead).

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 core as a feature?)

Copy link
Contributor

@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.

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
@MustafaJafar
Copy link
Contributor Author

MustafaJafar commented Jan 7, 2025

However, I do think the:

`chs('AYON_productName')`

part should be part of the setting, not hardcoded in the setting of the path.

This won't work with HDAs, as its file name is not saved in a string parameter.
Also for reference, path in settings is not actually a staging directory it's more of a root output directory and every product can have multiple files like render has render and intermediate render files.
Here's a list of the current paths in this PR (the root is the path in settings: $HIP/ayon)

- Camera, Model, abc pointcache: "{root}/`chs('AYON_productName')`/$OS.abc"
- Arnold 🍑: "{root}/`chs('AYON_productName')`/$OS.$F4{ext}"
- Arnold: 
  - render: "{root}/`chs('AYON_productName')`/$OS.$F4.{ext}"
  - 🍑: "{root}/`chs('AYON_productName')`/ass/$OS.$F4.ass"
- Bgeo pointcache: "{root}/`chs('AYON_productName')`/$OS.$F4.{ext}"
- Cop: "{root}/`chs('AYON_productName')`/$OS.$F4{ext}"
- HDAs: "{root}/HDAs/{node_name}.hda"
- Karma: 
  - render: "{root}/`chs('AYON_productName')`/$OS.$F4.{ext}"
  - checkpoint: "{root}/`chs('AYON_productName')`/checkpoint/$OS.$F4.checkpoint"
  - USD output: "{root}/`chs('AYON_productName')`/usd/$OS_$RENDERID"
- Mantra: 
  - render: "{root}/`chs('AYON_productName')`/$OS.$F4.{ext}"
  - ifd: "{root}/`chs('AYON_productName')`/ifd/$OS.$F4.ifd"
- RS proxy: "{root}/`chs('AYON_productName')`/$OS.$F4.rs"
- Redshift:
  - render: "{root}/`chs('AYON_productName')`/$OS.$AOV.$F4.{ext}"
  - proxy: "{root}/`chs('AYON_productName')`/rs/$OS.$F4.rs
- review: "{root}/`chs('AYON_productName')`/$OS.$F4.{ext}"
- staticmesh: "{root}/`chs('AYON_productName')`/$OS.fbx"
- USD: "{root}/`chs('AYON_productName')`/$OS.usd"
- USD render: "{root}/`chs('AYON_productName')`/usd/$HIPNAME/$OS
- VDB: "{root}/`chs('AYON_productName')`/$OS.$F4.vdb"
- VRay:
  - with AOV: "{root}/`chs('AYON_productName')`/$OS.$AOV.$F4.{ext}"
  - without AOV: "{root}/`chs('AYON_productName')`/$OS.$F4.{ext}"

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.

I wonder if Houdini artists might find it confusing because it doesn't align with Houdini defaults where geo files will be saved in render/ instead of /geo.

@MustafaJafar
Copy link
Contributor Author

Regarding point 1 and 3 about being more granular about file names and not using hardcoded subdirectories.
This was implemented in the far first few commits in the PR before reverting these changes.
IMO, the settings were too many making them hard to use.
ynput/ayon-core#9

@BigRoy
Copy link
Contributor

BigRoy commented Jan 7, 2025

This won't work with HDAs, as its file name is not saved in a string parameter. Also for reference, path in settings is not actually a staging directory it's more of a root output directory and every product can have multiple files like render has render and intermediate render files. Here's a list of the current paths in this PR (the root is the path in settings: $HIP/ayon)

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.


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.

I wonder if Houdini artists might find it confusing because it doesn't align with Houdini defaults where geo files will be saved in render/ instead of /geo.

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 $HIP getting expanded on create either - but I guess that's also how it was already.

Copy link
Contributor

@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.

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).

@MustafaJafar
Copy link
Contributor Author

As such, the default would be this instead:

staging_dir = "$HIP/ayon/`chs('AYON_productName')`"

For reference, I gave this a test and it won't work because of we are expanding string hou.text.expandString resulting in the result in my second screenshot.
image
image

…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
@MustafaJafar MustafaJafar requested a review from moonyuet January 8, 2025 18:19
@MustafaJafar MustafaJafar requested a review from BigRoy January 8, 2025 21:27
@MustafaJafar MustafaJafar linked an issue Jan 8, 2025 that may be closed by this pull request
2 tasks
@MustafaJafar MustafaJafar changed the title Enhancement/130 ay 6965 rop default output path update pyblish to something less legacy 130 ay 6965 default output paths and support custom staging directories. Jan 8, 2025
Copy link
Contributor Author

@MustafaJafar MustafaJafar left a 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.

client/ayon_houdini/plugins/create/create_mantra_rop.py Outdated Show resolved Hide resolved
@@ -36,6 +61,10 @@ class GeneralSettingsModel(BaseSettingsModel):

DEFAULT_GENERAL_SETTINGS = {
"add_self_publish_button": False,
"rop_output": {
"enabled": True,
"default_output_dir": "$HIP/ayon/"
Copy link
Contributor Author

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.

Suggested change
"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
Copy link
Contributor

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.

Copy link
Member

@moonyuet moonyuet Jan 9, 2025

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.

Copy link
Contributor Author

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🤔?

Copy link
Contributor Author

@MustafaJafar MustafaJafar Jan 9, 2025

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

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor

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:

image

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 the instance 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:

  1. use get_staging_dir from super class and I've no idea what to do with HDAs and render creators.
  2. 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 pass product_name, product_type and folder_path but no idea about getting the version as get_staging_dir includes this call self.get_next_versions_for_instances([instance]).. the version is used as template data.
  3. 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)

Copy link
Member

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 the instance 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:

  1. use get_staging_dir from super class and I've no idea what to do with HDAs and render creators.
  2. 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 pass product_name, product_type and folder_path but no idea about getting the version as get_staging_dir includes this call self.get_next_versions_for_instances([instance]).. the version is used as template data.
  3. 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.

@MustafaJafar MustafaJafar requested a review from BigRoy January 9, 2025 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sponsored This is directly sponsored by a client or community member type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
3 participants