Maya: Yeti Validate Rig Input - OP-3454

[tokejepsen]

Brief description

Fix Yeti Validate Rig Input

Description

Existing workflow was broken due to this #3297.

Testing notes:

1. Follow documentation in PR for Yeti Rigs.

[ynbot]

Task linked: OP-3454 Maya: Yeti Validate Rig Input

[LiborBatek]

First test done and successfully published yeti_rig instance from workfile.

it also output all publish data:

When using Asset Loader user being able to load it as yeti_cache and yeti_Rig which brings into scene not just pgyetiMaya node but also full character rig with corresponding meshes and pgyetiMaya node(s).

There is a neccessity to use manual approach to connect yeti_cache to the corresponding asset in the new scene into which we brought yeti_cache as a hair setup. Otherwise the yeti sits in the scene as separate imovable asset not connected anyhow.

I will also do more testing regarding contents of the yeti rig before publish (multiple yeti nodes and/or grooms) to fully test the functionality.

As now it seems the core functionality is ok. Will give Review as soon as done with those tests.

[LiborBatek]

Also spotted that in the doc section for Yeti usage is a mistake when speaking about Yeti Instance creation...

Instead of creating Yeti Rig there is written Yeti Cache which is incorrect and misleading.

As seeing the Outliner window at the bottom which clearly states Yeti Rig Default Instance has been created not Yeti Cache.

[tokejepsen]

As seeing the Outliner window at the bottom which clearly states Yeti Rig Default Instance has been created not Yeti Cache.

Good spot! Fixed.

[LiborBatek]

Following testing steps / OP user docs with these findings:

1. Publish of the Yeti_rig correctly outputs yeti data which can be then loaded via Asset Loader

2. Publish doesnt output Sources folder with textures used in Yeti Graph into Publish folder

It should happen according to the last sentence in the user guides for Yeti:

3. When published Yeti asset loaded via Load Yeti Cache in Asset Loader it doesnt have correctly set on pgYetiMaya1Shape attrib for Other/Image Search Path to Publish Folder instead being set to orig file path to workfile area. * Note: I have inserted the file path manually when creating Yeti Setup as I was used to do it from production pov. I assume it would be left empty by OpenPype right now...no need to look after where it came from :)

There is also question how to handle publish/load of the Yeti setup according to Pergerine workflow guides. Will post more on this topic.

But generally it should be just:

• Select Yeti Setup and Publish it
• Select destination asset and Load Yeti Published Asset (will get connected to new selected Asset)
• Publish Yeti cache for animated sequence
• Load Yeti cache into lighting workfile scene for rendering (apply Look via Look Manager too)

Done.

[BigRoy]

Watch out.

maya.cmds.sets returns None if the set is empty.

from maya import cmds
my_set = cmds.sets(empty=True)
print(cmds.sets(my_set, query=True))
# None

[BigRoy]

Also, the get_all_children function itself does not filter to DAG nodes. So if the set in any way contains non dag nodes, like e.g. another set or dependency node the get_all_children function will likely error on sel.getDagPath() logic.

You'd need to make sure to pass only dagNode types.

members = cmds.sets("input_SET", query=True) or []
children = get_all_children(cmds.ls(members, type="dagNode", long=True))
instance.extend(members)
instance.extend(children)

[BigRoy]

And another thing to watch out for. If the instance already contained any of the nodes that are in the input_SET then now the instance will have these nodes duplicated (or at least added more than once to the instance).

As such validators will check these nodes twice, and will likely report them more than once too.
It'll be slower + harder to read for the end user.

We might want to ensure uniqueness of what ends up in the instance here?

[BigRoy]

And actually reading in this now - why do we require all children to be collected of the input_SET. I recall that by original design that usage of that would not require that at all and wasn't intended like that at all. There is something fishy here.

A lot of discussion related to that I think is here: #3404
Or wait - it seems you're referring to this comment specifically talking about the issue for the validator.

Reading that comment - this change here is clearly breaking the validator logic. Because with this change whatever is in the input_SET will ALWAYS be in the instance - so the validator makes no sense.

The idea is that:

• In the instance should be whatever you want to extract (your yeti rig)
• In the input_SET should be whatever you want to collect 'inputs' for. (So they are the destinations receiving inputs for which you want to collect the inputs. They are not the inputs themselves)
• The validator checks whether what we're collecting inputs for is actually part of the instance.

As such the change here is incorrect.

[tokejepsen]

In the input_SET should be whatever you want to collect 'inputs' for. (So they are the destinations receiving inputs for which you want to collect the inputs. They are not the inputs themselves)

Could you elaborate on this? Give an example?

[BigRoy]

Yes. I'd say the the docs are wrong unless we change the logic of collecting the input set around.

At least currently the code logic doesn't match what the docs describe.

[tokejepsen]

Tried giving this workflow a go and something seems off. The publish works well and it produces a .rigsettings file with correct IDs and attributes but when loading, this information is taken into account. From the code it looks like IDs of the source and destination nodes need to be same and even then it'll only connect outMesh > inMesh; https://github.com/ynput/OpenPype/blob/develop/openpype/hosts/maya/plugins/load/load_yeti_rig.py#L65-L77

[BigRoy]

That logic was implemented on OpenPype's side here and admittedly it's logic we (Colorbleed) thus had never used on our end on the intended implementation since it wasn't built to do so.

Instead it should be using the sourceAttr and destAttr stored in the rigSettings. I'd actually even go as far to by default NOT connect it on load unless someone ticks some "auto-connect" checkbox somewhere. We originally prototyped in our previous pipeline with a 'Connection Manager' which would show the possibilities of connections between A and B by the detected cbId entries.

Our simple rig loader was as basic as this and note how it's mentioning the connection manager haha! :)

[tokejepsen]

Ahh cool. Atm the loader only compares with select nodes in the scene. Since neither the code or the documentation is correct, we dont really need to be "backwards" compatible for any clients but rather just get a working solution going.

What do you think of using the same workflow as the Xgen implementation, where the "connection manager" is the inventory and the user selects which containers to connect together?

[BigRoy]

What do you think of using the same workflow as the Xgen implementation, where the "connection manager" is the inventory and the user selects which containers to connect together?

I think that's a solid first step after which we can look into the limitations of that workflow.

[tokejepsen]

There is also question how to handle publish/load of the Yeti setup according to Pergerine workflow guides. Will post more on this topic.

@LiborBatek think this need to be a separate issue cause this PR is just for fixing the broken pipeline. It'll need client support as well.

[LiborBatek]

This is what happens when using Asset Loader and loading Yeti Rig asset in current state of things:

There is a whole character hierarchy and Yeti setup now. I guess its not desired to load it all despite missing shaders too.

[tokejepsen]

@BigRoy I've updated the docs now. Could you have a read through and let me know if that sounds correct?

[BigRoy]

Nice, definitely getting better with the docs now. Did have some pointers/questions.

[LiborBatek]

Ok update here, it started to work with new published version! here it is with old one:

and then when updated to ver02:

Note: I didnt change any settings when doing ver02. Man soo weird to spent so much time realizing it suddenly works as expected!!!

seems working but need more testing for other use cases

[BigRoy]

Interesting. Does that mean that currently the PR is working as expected?

[LiborBatek]

Did run all test all round from YetiRig publish including 2 Grooms on the mesh and ran through yeticaching and rendering it out.

All works as expected!

[tokejepsen]

@BigRoy you happy with the code? Can merge this sucker then.

[BigRoy]

Some small notes - and I still wonder whether we should mention in the docs that a yetiCache and yetiRig publish can contain multiple yeti nodes - just for clarity?

[LiborBatek]

Some small notes - and I still wonder whether we should mention in the docs that a yetiCache and yetiRig publish can contain multiple yeti nodes - just for clarity?

Definetely yes (I think @tokejepsen already added it into docs), what comes to my mind is pgYeti node can have also multiple input geometries besides multiple groom nodes (which I have succesfully tested already). Are we ok with these scenarios too? Maybe I should make more stress tests with more complex setups like this...at least using multiple objects for yeti (rig_GRP)

[LiborBatek]

Interesting. Does that mean that currently the PR is working as expected?

To your question: yes after that yeti cache publish v002 all worked ok (no that weird frame offset at the frame 1001)! I have then tested yeti_rig with multiple groom nodes and worked well too

...but as I have mentioned above, I should also probably test scenario when more than one geometry is connected for grooming (which is perfectly ok in Yeti workflows)

[tokejepsen]

Some small notes - and I still wonder whether we should mention in the docs that a yetiCache and yetiRig publish can contain multiple yeti nodes - just for clarity?

@BigRoy I mention it here. Got anything to add to it?

[BigRoy]

on it here. Got anything to add to it?

Thanks - had missed that before. Looks good to me!

[BigRoy]

Solid work - code changes look good to me. However, have not done a test-run myself with the latest commits.

Also, I'd expect this PR to resolve this issue #3404 - Correct?

[tokejepsen]

Also, I'd expect this PR to resolve this issue #3404 - Correct?

Correct. Fixes #3404

[tokejepsen]

Solid work - code changes look good to me. However, have not done a test-run myself with the latest commits.

@LiborBatek would you mind doing one last test run on this?
My Yeti trial has just run out.

[LiborBatek]

Yeah, Im on it! Will do another test run...

[LiborBatek]

After testing round all seems fine even after newest commits....so go to go!