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

Load Asset LOP: Easy access to set primitive path to the folder name #54

Merged

Conversation

BigRoy
Copy link
Contributor

@BigRoy BigRoy commented Jul 29, 2024

Changelog Description

Working with the folder name as root primitive as name is crucial for asset build because we want to work within that correct default primitive (that in AYON USD contribution workflow) and hence the Asset Structure we have defined according to the USD Asset Structure guidelines must exist within the default prim {folder_name}.

  • This PR adds a hidden folder_name parameter so users can easily reference it using chs("folder_name").
  • This PR adds a "Presets" box next to the Primitive Root parameter to easily pick a nice default:

image

Additional info

Questions:

  • Should folder name be visible instead and locked maybe? So it's clear that the parm exists users can right click it to copy reference?-> ✅ Done with 0c0e433
  • Should the default value for Primitive Root be chs("folder_name") instead so that it has a sensible default for ASSET BUILD instead of a sensible default for SCENE ASSEMBLY? (Especially because scene assembly may be done via stage managers or whatever in Houdini anyway?) -> ⚠️ If we want this we'll do it in a separate PR because it'll be a Backwards Incompatible change for existing load asset lop nodes in the scene that have no explicit override set to primpath1 parm.

This PR took me way more time than I planned. Seemed like a SIMPLE thing to implement but I faced a Houdini bug where I was incapable of setting the value on primpath1 whenever it explicitly referenced another parm. (Even with hou.Parm.set(follow_parm_reference=False) it didn't work, nor the recommended Parm.deleteAllKeyframes() call would not help).

Houdini Bug: 139806 can't set string parameters that are referencing another parm currently

It's reported as Houdini bug: 139806 - for what it's worth, SideFX confirmed it's a bug and the only workaround for string parms is to use hscript then using the opparm command since it does NOT set it on the referenced parms.

Note that the follow_parm_references argument to hou.Parm.set() only works for the form used for setting channel references when the value is a hou.Parm (or hou.ParmTuple when working with parm tuples), so if that's not what you're doing, it's misleading to include it. The set methods that take an constant value always follow channel references because they do not actually replace channel expressions. There is definitely something missing here, but in the meantime, you can use the hscript equivalent as a workaround.

The opparm hscript command doesn't follow channel references by default, and it also deletes any channels by default prior to setting the value, so it can be used to work around this issue by invoking it with hou.hscript().

You could just always use it, or use the parm.deleteAllKeyframes() approach for everything but string parameters, depending on your needs.

Something along the lines of

if isinstance(parm.parmTemplate(), hou.StringParmTemplate):
    hou.hscript("opparm " + parm.node().path() + " " + parm.name() + "('" + "new_value" +  "')")
else:
    # the deleteAllKeyframes() approach

Testing notes:

  1. Create Load Asset LOP.
  2. Primitive Root presets should be clear and usable.

@BigRoy BigRoy added the type: enhancement Improvement of existing functionality or minor addition label Jul 29, 2024
@BigRoy BigRoy requested review from MustafaJafar and antirotor July 29, 2024 14:50
@BigRoy BigRoy self-assigned this Jul 29, 2024
@BigRoy
Copy link
Contributor Author

BigRoy commented Jul 29, 2024

@antirotor @Lypsolon @MustafaJafar could you check the "Questions" in the additional notes and maybe answer from your perspectives?

@Lypsolon
Copy link

Lypsolon commented Jul 29, 2024

Should the folder name be visible instead and locked, maybe? So it's clear that the parm exists. Users can right-click it to copy references.

My personal opinion is that everything important for automation should probably be hidden. My reason for this is that a user might accidentally delete something in the parm and then get confused about what is happening, and this could be a pain to debug.
However, the argument that copying relative references would be nice is good. I think it might be an option to store all the creations into one(dict, maybe ?) or a few parts that aren't available for the user but only for the developer.
then we could have a few "expose" parms that allow the copy reference options without risking deleting something important. (but this might be a bit overcomplicated)

Should the default value for Primitive Root be chs("folder_name") instead so that it has a sensible default for ASSET BUILD instead of a sensible default for SCENE ASSEMBLY? (Especially because scene assembly may be done via stage managers or whatever in Houdini anyway?)

That's a good question. And I kinda see two answers.
A: we could have a "template" in the loader where we have two options on right click one "asset load" and the second "asset shot import" or something like that. and then set the default from this. (might make the UI UX complicated)
B: if we assume that shot build will be handled mainly with tools other than "automation", then I think the Asset Build default might be the most straightforward way.

[Edit]: I didn't make this a review, so I can do a PR review tomorrow. I hope that workflow fits; if not, let me know.

…ancement/ayon_load_asset_lop_folder_name

# Conflicts:
#	client/ayon_houdini/startup/otls/ayon_lop_import.hda/ayon_8_8Lop_1lop__import_8_81.0/DialogScript
Copy link

@Lypsolon Lypsolon left a comment

Choose a reason for hiding this comment

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

tested on Almalinux 9 Houdini 20.0.710

Works and also updates already existing asset lops (not sure if that's wanted)

@BigRoy
Copy link
Contributor Author

BigRoy commented Jul 30, 2024

Works and also updates already existing asset lops (not sure if that's wanted)

It should be completely backwards compatible (it shouldn't break loaded content currently) so yes, that's the preferred behavior. Or did you encounter issues with it being updated?

Copy link
Contributor

@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's how it looks on my side.

image
image

As far as I'm able to tell:

  1. I have a terrible short term memory, and when selecting the preset, I can't find any info tells me to which preset the current value belongs (Until I read the preset selection properly and get use to it )
  2. Why would I want to copy the primitive root parameter ? As far as I know, the workflow (currently) expects exact hardcoded values to work properly, and copying that parameter may break something ?
  3. I find it confusing that primitive root uses a hidden parameter (until I get use to it and I'll be okay, it's hidden and it has the value of the last part of the folder path after the /).

@BigRoy
Copy link
Contributor Author

BigRoy commented Jul 30, 2024

Why would I want to copy the primitive root parameter ? As far as I know, the workflow (currently) expects exact hardcoded values to work properly, and copying that parameter may break something ?

Sorry, not sure what you're referring to? Are you referring to me asking about whether to make folder_name visible or not. That wasn't necessarily targetted at the Primitive Root parameter at all. But just so users have easy access to the folder name to do whatever they want with that.

I find it confusing that primitive root uses a hidden parameter (until I get use to it and I'll be okay, it's hidden and it has the value of the last part of the folder path after the /).

I guess that answers it maybe - I may just make folder_name visible but locked. -> ✅ Done with 0c0e433

@MustafaJafar
Copy link
Contributor

Why would I want to copy the primitive root parameter ? As far as I know, the workflow (currently) expects exact hardcoded values to work properly, and copying that parameter may break something ?

Sorry, not sure what you're referring to?

I was referring to ynput/ayon-core#295 (comment)
So, do I have to have primitive root path as folder_name ?

@BigRoy
Copy link
Contributor Author

BigRoy commented Jul 30, 2024

So, do I have to have primitive root path as folder_name ?

If you're doing "asset" build work, like adding geometry, shaders, grooms to the USD asset then you'll want to work within the asset's structure which is the default prim {folder_name}. In that case you want to use that as the primitive path that you're loading and also make sure that any shaders or other things you create in your Look scene or alike to be INSIDE that root primitive.

Does that make more sense?

If that means nothing is blocking this @MustafaJafar can you still approve?

Copy link
Contributor

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

It works as expected and it is cool!

@BigRoy BigRoy merged commit c6004f0 into ynput:develop Jul 30, 2024
1 check passed
@BigRoy BigRoy deleted the enhancement/ayon_load_asset_lop_folder_name branch July 30, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants