Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Maya: Xgen #4256

Merged
merged 69 commits into from
Feb 3, 2023
Merged

Maya: Xgen #4256

merged 69 commits into from
Feb 3, 2023

Conversation

tokejepsen
Copy link
Member

@tokejepsen tokejepsen commented Dec 21, 2022

Brief description

Initial Xgen implementation.

Description

Client request of Xgen pipeline.

Additional info

Currently have publishing and loading, but with two issues:

  • Critical: Duplicate collection errors on launching workfile
  • Non-critical: errors on opening workfile and failed opening of published xgen.

The critical issue can be worked around by opening the workfile again once Maya is launched. It might be a better solution to provide a way for Maya to be launched without workfile as argument but load the workfile when Maya is initialized to work around these two issues.

Testing notes:

  1. Enable project_settings/maya/open_workfile_post_initialization, to work around the mentioned non-critical and critical errors.
  2. Create Xgen collection and instance, then publish.
  3. Load Xgen from Loader.
  4. Load geometry animation from Loader (needs publishing of animation or pointcache of the animated geometry used to generate the xgen on).
  5. Select the animation and xgen container in the inventory > Actions > Connect Geometry.
  6. Load curves animation/fx from Loader (needs publishing of animation or pointcache of the animated curves used as guides in the xgen).
  7. Select the animation/fx and xgen container in the inventory > Actions > Connect Xgen.
  8. Create render instance and publish to Deadline.

@ynbot
Copy link
Contributor

ynbot commented Dec 21, 2022

Task linked: OP-3074 Xgen classic

@tokejepsen
Copy link
Member Author

Duplicate error might be due to taking all files from workfile xgen to publish resources. Will investigate more.

# Duplicate_transform subd patch geometry.
duplicate_transform = pc.duplicate(transform_name)[0]
pc.parent(duplicate_transform, world=True)
duplicate_transform.name(stripNamespace=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This query is unused. .name() as far as I know returns a value and doesn't change the actual node. So stripNamespace=True here does nothing to the node it's acting upon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

Comment on lines 57 to 60
# Duplicate_transform subd patch geometry.
duplicate_transform = pc.duplicate(transform_name)[0]
pc.parent(duplicate_transform, world=True)
duplicate_transform.name(stripNamespace=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we include in a comment why there is a need to duplicate the input transforms, etc? Why do we need that. Couldn't we just export a maya file with preserveReferences = false since duplicating breaks the reference connection anyway? What's the need for duplicating it?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to strip out any connections the geometry has to the current scene. For example when using some rig geometry that is nested deep in the rig.
Up for suggestions on how to solve this without duplicating and parent to the world.

Copy link
Collaborator

@BigRoy BigRoy Dec 21, 2022

Choose a reason for hiding this comment

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

We need to strip out any connections the geometry has to the current scene. For example when using some rig geometry that is nested deep in the rig.
Up for suggestions on how to solve this without duplicating and parent to the world.

I see, thanks. I believe you should be able to add these arguments to the cmds file export channels=False and constructionHistory=False. However, whenever you do so any connections you want to actively persist in the export would need to be included in the "selection" that you've set to export.

So basically if you're able to identify which actual objects (including its history) you want to have end up in the maya file you should be able to add those arguments and have the file ONLY contain those nodes (make sure preserveReference=False otherwise any reference in your selection would still fully be included).

I believe if the selection actively includes animation curves (if you ever need them) then channels=False and constructionHistory=False will still include it in the export and connect them as they were as long as they are part of the active selection in an exportSelected=True export.

However, you might need to double test this. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

But this includes the namespaces of nodes, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

But this includes the namespaces of nodes, right?

Yes, you are correct. It doesn't remove namespaces as far as I'm aware.

@BigRoy
Copy link
Collaborator

BigRoy commented Dec 21, 2022

Critical: Duplicate collection errors on launching workfile
Non-critical: errors on opening workfile and failed opening of published xgen.

For completeness sake - could you share the errors that pop up here? And if they are 'odd moments' when they occur a screen capture would be nice if you think that helps to explain what is going on.

Edge case of loading xgen while existing xgen in cache.
Resetting Xgen attributes after incremental save.
@tokejepsen
Copy link
Member Author

@LiborBatek this is ready for a test run through. Anything you can find before we push this to the client.

@LiborBatek
Copy link
Member

Its all tested and working fine. Also finally used guided sim so nothing left for testing and can be merged.

All is OK.

@tokejepsen
Copy link
Member Author

@BigRoy @antirotor last comments before merging?

@BigRoy
Copy link
Collaborator

BigRoy commented Jan 31, 2023

last comments before merging?

Are you really sure you want to be asking me this? Just before a merge! Let me get all over this bad boy!

Copy link
Collaborator

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

Quick pass over the code. Haven't proofread the documentation yet.

openpype/hosts/maya/api/lib.py Outdated Show resolved Hide resolved
openpype/hosts/maya/api/lib.py Outdated Show resolved Hide resolved
openpype/hosts/maya/api/plugin.py Outdated Show resolved Hide resolved
openpype/hosts/maya/plugins/inventory/connect_geometry.py Outdated Show resolved Hide resolved
openpype/hosts/maya/plugins/publish/extract_xgen.py Outdated Show resolved Hide resolved
openpype/hosts/maya/plugins/publish/extract_xgen.py Outdated Show resolved Hide resolved
openpype/hosts/maya/plugins/publish/validate_xgen.py Outdated Show resolved Hide resolved
openpype/hosts/maya/plugins/publish/validate_xgen.py Outdated Show resolved Hide resolved
@tokejepsen
Copy link
Member Author

@BigRoy think I've addressed all your feedback now.

Copy link
Collaborator

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

Much improved I think! Some minor notes that stood out to me this time as I quickly skimmed over the code.

Comment on lines 140 to 141
os.path.dirname(instance.data["resourcesDir"]),
os.path.basename(source.replace(basename, published_basename))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reading this made me think "so wait, basename and publish_basename themselves aren't actually basenames"? They didn't go through os.path.basename but are the root of os.path.splitext. Do we have naming confusion here?

See this example of os.path.splitext:

splitext('/foo/bar.exe')
# ('/foo/bar', '.exe')

Copy link
Member Author

Choose a reason for hiding this comment

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

Have you got a suggestions of names/code?

website/docs/artist_hosts_maya_xgen.md Outdated Show resolved Hide resolved
@mkolar
Copy link
Member

mkolar commented Feb 3, 2023

Gentlemen I believe this has gone through enough review torment now, and is ready for merge.

@mkolar mkolar merged commit eae6d5f into ynput:develop Feb 3, 2023
@github-actions github-actions bot added this to the next-patch milestone Feb 3, 2023
@tokejepsen tokejepsen deleted the feature/OP-3074_xgen branch February 3, 2023 10:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
host: Maya type: feature Larger, user affecting changes and completely new things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants