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.

Sign up for GitHub

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

USD contribution: Set up different default values based on profiles #973

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from
+63 −34
Draft

USD contribution: Set up different default values based on profiles #973

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8208bef
Allow to target not only `productType` by default with attributes, bu…
BigRoy Oct 25, 2024
1790f07
Draft to set defaults using `profiles`
BigRoy Oct 26, 2024
f7528cd
Merge branch 'develop' into enhancement/attributes_by_families
BigRoy Oct 29, 2024
0d5510a
Merge branch 'develop' of https://github.com/ynput/ayon-core into enh…
BigRoy Oct 29, 2024
c427f48
Merge branch 'develop' into enhancement/attributes_by_families
BigRoy Oct 30, 2024
6bc6af2
Merge branch 'develop' into enhancement/attributes_by_families
BigRoy Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions client/ayon_core/pipeline/publish/publish_plugins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,9 @@ def instance_matches_plugin_families(
if not cls.__instanceEnabled__:
return False

for _ in pyblish.logic.plugins_by_families(
[cls], [instance.product_type]
):
families = [instance.product_type]
Copy link
Member

Choose a reason for hiding this comment

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

To be honest I don't agree with this change. I still think we should not use families during create phase, but other data driven workflow. Either it would be is_usd: True on instance data or more complex structure, but it should be usd specific so it can be targeted and deterministic, with possibly more information, instead of arbitrary list of strings.

Example data on instance

{
    "usdData": {
        "someKey": "someValue",
        "renderer": "supaR2D3",
    }
}

If the values would not change during create phase and would be read only, they could be in transient data so we don't overfill stored data to scene.

This is my personal opinion, maybe caused by me not having full view into the issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly - most of these things I'm explaining here should also 1:1 define whether the plug-in should process the instance. Hence assigning families as tag is the way to go. But yes, there will be edge case where we may want to act on specific metadata and more complex values - but I'd say that's WAY harder to track in consistency, especially across multiple addon repositories instead of just checking "does it have this tag".

However, I just want to state that I'd like to PREFIX all these families, e.g. traits/reviewable or traits/frame_range or node/AlembicROP or whatever to differentiate clearly from the families that are intended as a product type. Also, because it allows us to transition smoother from the legacy system over time.

Also wanted to link this Community topic where I mentioned families versus product type as well: https://community.ynput.io/t/difference-between-family-and-product-type/1923

Copy link
Member

Choose a reason for hiding this comment

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

Be carefull about traits, as that will be new concept in representations, it might be confusin to mix those.

BTW does trait in this case make sense? Shouldn't be traits defined based on set up attributes instead of set up attributes based on traits?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW does trait in this case make sense? Shouldn't be traits defined based on set up attributes instead of set up attributes based on traits?

The idea is indeed that traits and attributes would be intertwined - in some case a toggle may enable/disable a trait, but to me that's essentially the same case with these families. A creator attribute may add/remove such a 'family'. But yes, whether traits is the best name for it I'm not sure. I just used it because it clarified the subject since there's huge overlap in that area (a much better overlap than "product type" is)

Copy link
Member

Choose a reason for hiding this comment

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

Like I've said, traits are dangerous, if you want to use families then use families, and don't call families traits.

BTW if there are families used only during create phase then I have another argument why not use families. Wouldn't make sense to add usdTraits instead of families to the instance data? So it is clear it is related to USD?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The concept I'm trying to portray is that this is not USD specific - the concept expands beyond that and is completely embedded into ayon. E.g. being able to target an instance to be farm compatible, or something to have a framerange or alike so that we can have a generic frame range validation (or something of the sort). The forum post I linked explains that well.

I understand your point regarding "traits" and the potential confusion. All I really mean to say is that I really want to differentiate from what 'families' are actually about.

I'd really hate these scattered attributes appearing:

isUsd: True
isFbx: True
usdTraits: a, b
etc.

Using e.g. file/usd as a family may at least set a standard so that whenever we target something to a filetype, that we prefix at such. So that whenever you see file/fbx you know what it's doing and that it is a file type being referred to. Personally I'd even like producttype/review (although bit lengthy) but it's way clearer that "review" family is being targeted based on a product type as opposed to some other scattered data.

Anyway, I've requested a discussion/call this week to discuss this more in-depth. Would be good to have you there too! :)

Copy link
Member

@iLLiCiTiT iLLiCiTiT Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Did you discuss the concept with anyone else before?

Copy link
Collaborator Author

@BigRoy BigRoy Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I discussed it with Milan - but more as "my proposal". And I mentioned it to Dee and Ondrej last week in a meeting. But mostly from the perspective that we should discuss this in a brainstorm - which I believe current indicative planning is Thursday morning. So this is indeed "to be discussed"

Also want to mention that to me @MustafaJafar 's PR mentioned here #973 (comment) shows the issue at hand. It's a good fix, but now suddenly we've flooded families with opengl and review - not knowing which refers to a product type, which refers to targeting a rop node in houdini, etc. Preferably we 'tag' that clearer so its absolutely obvious from reading the code - which could e.g. also be rop/opengl or rop.opengl.

families.extend(instance.data.get("families", []))
for _ in pyblish.logic.plugins_by_families([cls], families):
return True
return False

Expand Down
132 changes: 106 additions & 26 deletions client/ayon_core/plugins/publish/extract_usd_layer_contributions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
BoolDef,
UISeparatorDef,
UILabelDef,
EnumDef
EnumDef,
filter_profiles
)
try:
from ayon_core.pipeline.usdlib import (
Expand Down Expand Up @@ -458,7 +459,71 @@ def get_or_create_instance(self,
return new_instance

@classmethod
def get_attribute_defs(cls):
def get_attr_defs_for_instance(cls, create_context, instance):
# Filtering of instance, if needed, can be customized
if not cls.instance_matches_plugin_families(instance):
return []

# Set default target layer based on product type
# TODO: Define profiles in settings
profiles = [
{
"productType": "model",
"contribution_layer": "model",
"contribution_apply_as_variant": True,
"contribution_target_product": "usdAsset"
},
{
"productType": "look",
"contribution_layer": "look",
"contribution_apply_as_variant": True,
"contribution_target_product": "usdAsset"
},
{
"productType": "groom",
"contribution_layer": "groom",
"contribution_apply_as_variant": True,
"contribution_target_product": "usdAsset"
},
{
"productType": "rig",
"contribution_layer": "rig",
"contribution_apply_as_variant": True,
"contribution_target_product": "usdShot"
},
{
"productType": "usd",
"contribution_layer": "assembly",
"contribution_apply_as_variant": False,
"contribution_target_product": "usdShot"
},
]
profile = filter_profiles(profiles, {
"productType": instance.data["productType"]
})
if not profile:
profile = {}

# Define defaults
default_contribution_layer = profile.get(
"contribution_layer", None)
default_apply_as_variant = profile.get(
"contribution_apply_as_variant", False)
default_target_product = profile.get(
"contribution_target_product", "usdAsset")
default_init_as = (
"asset"
if profile.get("contribution_target_product") == "usdAsset"
else "shot")
init_as_visible = False

# Attributes logic
publish_attributes = instance["publish_attributes"].get(
cls.__name__, {})

visible = publish_attributes.get("contribution_enabled", True)
variant_visible = visible and publish_attributes.get(
"contribution_apply_as_variant", True)

return [
UISeparatorDef("usd_container_settings1"),
Expand All @@ -484,7 +549,8 @@ def get_attribute_defs(cls):
"the contribution itself will be added to the "
"department layer."
),
default="usdAsset"),
default=default_target_product,
visible=visible),
EnumDef("contribution_target_product_init",
label="Initialize as",
tooltip=(
Expand All @@ -495,7 +561,8 @@ def get_attribute_defs(cls):
"setting will do nothing."
),
items=["asset", "shot"],
default="asset"),
default=default_init_as,
visible=visible and init_as_visible),

# Asset layer, e.g. model.usd, look.usd, rig.usd
EnumDef("contribution_layer",
Expand All @@ -507,7 +574,8 @@ def get_attribute_defs(cls):
"the list) will contribute as a stronger opinion."
),
items=list(cls.contribution_layers.keys()),
default="model"),
default=default_contribution_layer,
visible=visible),
BoolDef("contribution_apply_as_variant",
label="Add as variant",
tooltip=(
Expand All @@ -518,13 +586,16 @@ def get_attribute_defs(cls):
"appended to as a sublayer to the department layer "
"instead."
),
default=True),
default=default_apply_as_variant,
visible=visible),
TextDef("contribution_variant_set_name",
label="Variant Set Name",
default="{layer}"),
default="{layer}",
visible=variant_visible),
TextDef("contribution_variant",
label="Variant Name",
default="{variant}"),
default="{variant}",
visible=variant_visible),
BoolDef("contribution_variant_is_default",
label="Set as default variant selection",
tooltip=(
Expand All @@ -535,31 +606,40 @@ def get_attribute_defs(cls):
"The behavior is unpredictable if multiple instances "
"for the same variant set have this enabled."
),
default=False),
default=False,
visible=variant_visible),
UISeparatorDef("usd_container_settings3"),
]


class CollectUSDLayerContributionsHoudiniLook(CollectUSDLayerContributions):
"""
This is solely here to expose the attribute definitions for the
Houdini "look" family.
"""
# TODO: Improve how this is built for the look family
hosts = ["houdini"]
families = ["look"]
label = CollectUSDLayerContributions.label + " (Look)"
@classmethod
def register_create_context_callbacks(cls, create_context):
create_context.add_value_changed_callback(cls.on_values_changed)

@classmethod
def get_attribute_defs(cls):
defs = super(CollectUSDLayerContributionsHoudiniLook,
cls).get_attribute_defs()
def on_values_changed(cls, event):
"""Update instance attribute definitions on attribute changes."""

# Update default for department layer to look
layer_def = next(d for d in defs if d.key == "contribution_layer")
layer_def.default = "look"
# Update attributes if any of the following plug-in attributes
# change:
keys = ["contribution_enabled", "contribution_apply_as_variant"]

return defs
for instance_change in event["changes"]:
instance = instance_change["instance"]
if not cls.instance_matches_plugin_families(instance):
continue
value_changes = instance_change["changes"]
plugin_attribute_changes = (
value_changes.get("publish_attributes", {})
.get(cls.__name__, {}))

if not any(key in plugin_attribute_changes for key in keys):
continue

# Update the attribute definitions
new_attrs = cls.get_attr_defs_for_instance(
event["create_context"], instance
)
instance.set_publish_plugin_attr_defs(cls.__name__, new_attrs)


class ValidateUSDDependencies(pyblish.api.InstancePlugin):
Expand Down