ynput / ayon-houdini

Houdini addon for AYON
Apache License 2.0
11 stars 9 forks source link

Generic Loader OTLs #121

Closed MustafaJafar closed 4 weeks ago

MustafaJafar commented 1 month ago

Changelog Description

Hopefully, resolve #118 This PR mixes our solutions for LOPLoadAssetLoader and FilePathLoader resulting in the new generic loader otls.

The Generic Load OTL provides the following parameter UI to select an AYON product. image

The filepath loader is now using the generic loader instead of a null node. image

The generic Load OTL can be access via TAB menu. it comes with default parameter values (no products are selected at this moment). image

Users can also find a parameter action Load with AYON when right-click any parameter of type file on any non Rop nodes. image

When using Load with AYON action, a parameter editor pane will pop up, where artists can easily modify the parameters. Animation_21

The generic Load OTL can also show you the thumbnail of the loaded product Animation_27

The Generic Load OTL features:

Support contexts:

Additional Info

This PR extends our solution for LOPLoadAssetLoader and adds representation list so that artist can easily pick the representation to load. image

Additional Info 2 - Why using an HDA

An alternative solution to the Generic Loader OTL is to create a null node and add the parameters dynamically. But, that solution requires additional work to update nodes in any old scenes. Unlike using OTLs, OTLs make it easier to update the nodes automatically when opening the scenes.

Additional Info 3 - Promoting the Generic Loader parameters

When building a custom HDA that uses the generic loader, the recommended way to promote the generic loader parameters is by creating the parameters from nodes. image

But, you may hit an issue like this one if you promote a parameter with action, e.g. product parameter.

This issue is much similar when promoting some parameters (that have action callback) of the group sop node. image

The issue is discussed in SOP subnet parameter promotion issue | SideFx Forums and let me quote the easiest suggested solution:

The simplest is usually to promote the other parameters referenced by any such scripts as well,
making them invisible if they serve no other purpose on the promoted node. 
This is the easiest way to ensure the script is running similarly to how it does on the original node.

Here's a demo: I've project and folder_path parameters hidden, but they need to exist because the action script on product expects them to be found on the node.

Animation_28

Additional Info 4 - Expressions are so powerful

After using expressions instead of callbacks, This unlocked some powerful features.

e.g. I've these 3 products with different numbers image

I can load them in a for loop. and it works either or not I put my nodes inside a custom HDA. image image

Testing notes:

  1. Launch Houdini via AYON Launcher
  2. Try different load options:
    • Create the loader via tab menu.
    • right-click file parameter on file nodes to connect it to a generic loader.
    • use the loader tool to load using generic loader.
  3. Try loading different products (usd, caches, image sequences, textures with/without udims)
BigRoy commented 1 month ago

Nice!

HDA maintenance

I really dislike the fact that we now need to maintain multiple separate HDA definitions for something that is (or should essentially be) a node that has no uniqueness in behavior across the different contexts. It's just a node that takes input parameters to define the result on another parameter.

It's quite cumbersome to update the HDA definitions if we need to do this per context - not impossible, but very annoying. This is just two contexts - but there may be way more.

Maybe someone else knows how to better manage this or make the one HDA be able to be used across the different contexts without needing to handle the parameter configuration for each one separately.

Let's 'figure that out' but for now hold off creating all the other ones, because it'll just mean more maintenance. Let's continue the work on these two - and then just keep 'checking' how much effort this really is to maintain.

UDIM path

Currently it'll return the resolved filepath - but there are some cases where we need it to resolve to <UDIM> token. See here. Not required for MVP for this - but definitely high priority after.

So just noting that it may be on the todo list.

Generic Loader in Loader UI

In your screenshot it's ordered at the top, let's move it down. It's the "last resort" type of loader, not the primary one to use. This can be configured with the order attribute on the Loader Plug-in.

Houdini native focus

Loading from the Loader UI is fine - but our main focus with this HDA is:

MustafaJafar commented 1 month ago

HDA maintenance

I really dislike the fact that we now need to maintain multiple separate HDA definitions for something that is (or should essentially be) a node that has no uniqueness in behavior across the different contexts. It's just a node that takes input parameters to define the result on another parameter.

It's quite cumbersome to update the HDA definitions if we need to do this per context - not impossible, but very annoying. This is just two contexts - but there may be way more.

Yeah, I think our case should be similar to the subnet nodes. since HDAs are built on top of subnets. In asset manager, I can find multiple definitions of subnet, one for each Houdini context. I hope there's a solution for this.

image

MustafaJafar commented 1 month ago

UDIM path

Currently it'll return the resolved filepath - but there are some cases where we need it to resolve to <UDIM> token. See here. Not required for MVP for this - but definitely high priority after.

So just noting that it may be on the todo list.

I've ported the logic from the filepath loader. we may clean it later/move the logic to lib so that both file loader and hda_utils can share it. image

MustafaJafar commented 1 month ago

With the current state, I created an HDA with the loader node inside. I copied the parameters from the generic loader and I needed to do the following:

  1. remove callbacks.
  2. change the menu script in version and representation parameters to import the functions instead of using hou.phm(). no need for that https://github.com/ynput/ayon-houdini/pull/121/commits/bccc305d952993e55e69176d613a50cb28540181

image


For reference, loading objects from different projects leads to this result. which I think is not related to this PR. image

MustafaJafar commented 1 month ago

I was able to use the generic loader in a for loop to load multiple products with the same name and different postfix number (although, I had to use a python node to change the product name and trigger the callback). I don't know how to make it easy like https://github.com/ynput/ayon-houdini/pull/112#issuecomment-2420768027

image

BigRoy commented 1 month ago

Thanks - I see why this commit https://github.com/ynput/ayon-houdini/pull/121/commits/bccc305d952993e55e69176d613a50cb28540181 would fix that. However, I'd love actually for somehow the menu script to stay attached to the 'inner' node in an HDA instead of also promoted.

Because, that way - one wouldn't need to promote ALL relevant attributes to the parent HDA but could even (for whatever reason) promote e.g. only "Version" or whatever. The question becomes - how do you do that?

In this case, how did you promote the attributes to the parent HDA? Did you copy/paste the parms? Or did you promote them upwards (which would then actually be referencing the inner parms I'd imagine?)

Because we're now calling it on the "outer node" but it'd be great if we can keep all the interactions to the inner node - keeping the logic on the Generic Loader node itself.

MustafaJafar commented 1 month ago

In this case, how did you promote the attributes to the parent HDA? Did you copy/paste the parms? Or did you promote them upwards (which would then actually be referencing the inner parms I'd imagine?)

Yes, I copied/pasted these parms to the parent node and then copied them by reference to the inner node. Cool thing that the parameter actions work fine.

This commit https://github.com/ynput/ayon-houdini/commit/bccc305d952993e55e69176d613a50cb28540181 allows me to copy version and representation parms without errors.

e.g. I copied these parms to a null node and removed their callback actions.

Animation_9

BigRoy commented 1 month ago

This commit bccc305 allows me to copy version and representation parms without errors.

Correct - but it basically means the logic runs on that node. Which is what we'd like to avoid. We want it to run on the inner node preferably so that it's still contained all as logic there. Because now you need ALL those attributes exposed on the parent HDA. I'd prefer it it could also work if e.g. we ONLY expose e.g. the representations attribute.

The question is - how can we easily make it do that without needing to be super-specific when making HDAs to remap those callbacks.

MustafaJafar commented 1 month ago

Here's a demo for my last two commits. (I wish if I were able to avoid the noise in my street :" )

https://github.com/user-attachments/assets/01980fc2-4219-428f-a5c9-017a296db8e1

MustafaJafar commented 1 month ago

Because now you need ALL those attributes exposed on the parent HDA. I'd prefer it it could also work if e.g. we ONLY expose e.g. the representations attribute.

Good catch. my custom HDA still works eve if I only expose the representation. But the menu button doesn't work anymore. image image


I made another test. my custom HDA has its own representation menu with hard corded values abc, usd and exr. My custom HDA still works. tbh, I'm not yet sure how to expose/mirror the generic loader parameters on the parent HDA while relying on the generic loader to run the logic. should it be the responsibility of the parent HDA creator? I'll keep digging around finding ideas to make it easier to expose these parameters.

image

Another test, only exposing representation. I modified the menu script where I passed the loader node. The logic doesn't run in the loader node. but, it uses the actual node with the hda_utils function where I get the same results as if I'm clicking the menu button in the generic loader node.

from ayon_houdini.api.hda_utils import get_available_representations

node = hou.node(f'./generic_loader')
representations_names = get_available_representations(node)

result = []
for name in representations_names:
    result.append(name)
    result.append(name)

return result

image

BigRoy commented 1 month ago

I made another test. my custom HDA has its own representation menu with hard corded values abc, usd and exr. My custom HDA still works. tbh, I'm not yet sure how to expose/mirror the generic loader parameters on the parent HDA while relying on the generic loader to run the logic. should it be the responsibility of the parent HDA creator? I'll keep digging around finding ideas to make it easier to expose these parameters.

The issue here is with how you created the HDA and exposed the parameters the subnet.

This is how it works absolutely fine:

  1. Create a subnet
  2. Add the Generic Loader inside
  3. Go to Edit Parameters of the subnet
  4. Go top left to the "From nodes" tab
  5. Drag 'n' drop the parms onto your parameter list.

So drag 'n' drop e.g. from here to root: parm1

Then the menu script becomes: parm2

Which just means - use the inner node's menu.

So the issue was solely due to you 'copy pasting' the parameters instead of how you should be exposing the inner node's parameters upstream.

So basically we shouldn't need to change anything, we can keep the phm() calls etc.

We'll just need to document or provide pointers for non-senior Houdini artists that this is how they should expose these. In that case, you can e.g. just pick only the version field and it works absolutely fine.


I've played with this for a while - and it's really nice.

I think we just need:

MustafaJafar commented 1 month ago

So basically we shouldn't need to change anything, we can keep the phm() calls etc.

I've reverted the changes in https://github.com/ynput/ayon-houdini/pull/121/commits/f7d719c5830aa3f54694ca396d0b12a201f9b9ef

MustafaJafar commented 1 month ago

This commit https://github.com/ynput/ayon-houdini/pull/121/commits/baa2082c89555054186a91f31d593b26cbd129b0 replaces the list of operator paths with operator list.

Although, I like the operator list less but it complies with mark tucker's comment on the side fx forums:

You should really not be "doing things" as a result of evaluating expressions.


I also tried making an expression for the operator path parameter but it will be something like

references = hou.parm('./file').parmsReferencingThis()
num = int(hou.expandString("$CH").split("_")[-1])
parm = references[num-1]
parm.node().path()
moonyuet commented 1 month ago

I have tested the generic loader. It's really nice as you can update and switch the context easily with the loader itself, and it shows up in the scene inventory. The updating and switching function works as expected and the file parameter changes to the path with the correct version. Overall, it is a starting point for all integration being "generic". One thing I would think is it would be more artist-friendly if the file parameter being optional to be exposed on the generic loaders? (Like we did in nodes) Or we can have a setting in AYON (mainly for HDA) to allow users to expose the parameters, like we did in Nuke? Exposed parameter would be like the screenshot below. image

BigRoy commented 1 month ago

One thing I would think is it would be more artist-friendly if the file parameter being optional to be exposed on the generic loaders? (Like we did in nodes) Or we can have a setting in AYON (mainly for HDA) to allow users to expose the parameters, like we did in Nuke? Exposed parameter would be like the screenshot below.

Sorry, could you elaborate on this? I don't understand. If you're saying you want to expose additional parameters on the node from AYON settings I'm not sure I understand why we'd want that? What attributes?

My gut feeling is saying "no" we should not do that. The building block is supposed to be generic. It should preferably have the least amount of stuff that generates a pleasant user experience to pick the product to load whilst maintaining the right level of access from a Houdini perspective (e.g. allowing certain choices to be based of off references or other expressions). If someone wants the node to do more and expose more - that starts to sound like something studio specific which then should become their own HDA that contains the generic loader; not something that's driven by settings to configure the Load node itself. But I may be mistaken what you're describing to do.

moonyuet commented 1 month ago

One thing I would think is it would be more artist-friendly if the file parameter being optional to be exposed on the generic loaders? (Like we did in nodes) Or we can have a setting in AYON (mainly for HDA) to allow users to expose the parameters, like we did in Nuke? Exposed parameter would be like the screenshot below.

Sorry, could you elaborate on this? I don't understand. If you're saying you want to expose additional parameters on the node from AYON settings I'm not sure I understand why we'd want that? What attributes?

My gut feeling is saying "no" we should not do that. The building block is supposed to be generic. It should preferably have the least amount of stuff that generates a pleasant user experience to pick the product to load whilst maintaining the right level of access from a Houdini perspective (e.g. allowing certain choices to be based of off references or other expressions). If someone wants the node to do more and expose more - that starts to sound like something studio specific which then should become their own HDA that contains the generic loader; not something that's driven by settings to configure the Load node itself. But I may be mistaken what you're describing to do.

you didn't misunderstand me. I agree that it is more studio specific. The reason I come up with this contributes to the possibility of the HDAs containing some user-defined parameters. With the option of exposing the parameters, the users no need to always dive into the loader and reach to HDA to set all the parameters.

MustafaJafar commented 1 month ago

The next TODO is to try creating custom HDAs that include generic loader.

MustafaJafar commented 1 month ago

Based on our last discussion, it'd be better to open the parameters panel for the created generic loader. Also, it'd be cool to use better names for the generic loader, so we came up with {filenode_node}_{fileparm_name}_loader I borrowed some code from #140 and here's how it looks like:

image

BigRoy commented 1 month ago

Based on our last discussion, it'd be better to open the parameters panel for the created generic loader. Also, it'd be cool to use better names for the generic loader, so we came up with {filenode_node}_{fileparm_name}_loader I borrowed some code from #140 and here's how it looks like:

image

@MustafaJafar I've made some commits to your branch - in particular it now 'replaces' the "Load filepath to node" loader.

Aside of that I can't seem to get it to work that it shows me the popup panel?

MustafaJafar commented 1 month ago

in particular it now 'replaces' the "Load filepath to node" loader.

Thank you.

Aside of that I can't seem to get it to work that it shows me the popup panel?

we only need to add:

show_generic_loader_parmpanel(node)

I've already add it here https://github.com/ynput/ayon-houdini/pull/121/commits/5ed135752035970cac54b843b1f990d71adb7780 ^^

MustafaJafar commented 1 month ago

Promoting the Generic Loader parameters

When building a custom HDA that uses the generic loader, the recommended way to promote the generic loader parameters is by creating the parameters from nodes. image

But, you may hit an issue like this one if you promote a parameter with action, e.g. product parameter.

This issue is much similar when promoting some parameters (that have action callback) of the group sop node. image

The issue is discussed in SOP subnet parameter promotion issue | SideFx Forums and let me quote the easiest suggested solution:

The simplest is usually to promote the other parameters referenced by any such scripts as well,
making them invisible if they serve no other purpose on the promoted node. 
This is the easiest way to ensure the script is running similarly to how it does on the original node.

Here's a demo: I've project and folder_path parameters hidden, but they need to exist because the action script on product expects them to be found on the node. This demo is from https://github.com/ynput/ayon-houdini/pull/112 that implements expressions. Without implementing expressions we either set the generic loader node as editable node inside the HDA or we will get the following permission error

permission error ```python Traceback (most recent call last): File "ayon::Sop/generic_loader::1.0/product_name", line 2, in File "E:\Ynput\ayon-houdini\client\ayon_houdini\api\hda_utils.py", line 395, in on_representation_parms_changed node.parm("representation").set(representation_id) File "C:\PROGRA~1/SIDEEF~1/HOUDIN~1.805/houdini/python3.9libs\houpythonportion\Parm.py", line 91, in set self._set(value) File "C:\PROGRA~1/SIDEEF~1/HOUDIN~1.805/houdini/python3.9libs\hou.py", line 65711, in _set return _hou.Parm__set(self, *args) hou.PermissionError: Failed to modify node or parameter because of a permission error. Possible causes include locked assets, takes, product permissions or user specified permissions ```

Animation_20

MustafaJafar commented 1 month ago

Show the parameter editor, set it to the node path without selecting the node. By default the hscript commands create the pane on the lower left of the window and we may fix it using -P ‹x› ‹y› flag, refernce. Animation_21 Animation_22

MustafaJafar commented 4 weeks ago

After using expressions instead of callbacks, This unlocked some powerful features.

e.g. I've these 3 products with different numbers image

I can load them in a for loop. and it works either or not I put my nodes inside a custom HDA. image image


Also, here's the a demo for updating the version, the representation. Animation_26 here's the a demo for updating the thumbnail Animation_27