Maya: add loaded containers to published instance #2837
Conversation
antirotor
added
type: enhancement
| @@ -57,10 +58,22 @@ def process(self, instance): | |||
| else: | |||
| members = instance[:] | |||
|
|
|||
| loaded_containers = None | |||
| if {f.lower() for f in self.add_for_families}.intersection( | |||
There was a problem hiding this comment.
Why should this be non case sensitive in the check even though families in Pyblish plugins are case sensitive, right?
Also, are we missing self.add_for_families attribute being explicitly set in the code? (Or am I just missing it in the GitHub view somehow?)
Instead of adding these in the extractor - shouldn't these originally be "collected" along instead?
There was a problem hiding this comment.
add_for_families is added there directly from Settings, no need to manually specify.
With case sensitivity - I am not a fan of case sensitive family names as I belive it just adds to the chaos - there is no reason why layout should be different from Layout even more when you allow user to set family name by typing it somewhere in the Settings.
There was a problem hiding this comment.
I'd avoid the case-insensitivity here. Pyblish plug-in families are case-sensitive. This logic is tied very close to those exact families. There are more OpenPype settings exposed that related to Pyblish families and those are also case sensitive.
It's less code/logic to maintain, less cases to test and also more explicit. We're only making things more complicated by allowing the other cases. Plus I'd personally rather not see code with LaYouT and have it considered valid too.
To me plug-in families are constants. If we want to avoid human error in the settings then it should be an enum with a list to specify from. User might just as well mistype with a different character as recently was found with aftereffects being mistyped.
add_for_families is added there directly from Settings, no need to manually specify.
A code linter will likely not detect that? And most plug-ins that I've seen that specify custom settings also define the attribute on the class for readability. We should find a consistency between those two - I'd prefer the explicit method for readability.
I'd even go as far as not allowing Settings to set attributes on the plug-ins that do not exist yet to enforce this explicit code style. It could log a clear warning for when that happens to aid developers.
There was a problem hiding this comment.
@mkolar Any preference on this from your end? ☝️
There was a problem hiding this comment.
@mkolar I saw you approved this PR - could you check in regarding this question above?
There was a problem hiding this comment.
and yes, after giving it more thought than it deserves :) I agree with your point. But I believe with this we really need to implement some central way of managing families - we are currently unable to present (for example in Settings) all families we have, so in many places you just need to write it down manually and that is source of errors.
|
What's the reason this setting is configurable? When would you NOT want this for e.g. for layout, etc.? |
Reason is that it allows you the decision between old behaviour and new one, even if the old one was somewhat limited. It should stay there imho for a while as deprecated feature problably (and I should state it somewhere that it is so). |
Somehow I feel this should and could be simplified but I'm having a hard time thinking of a clean solution now. Adding a setting that we already know would be considered "deprecated" behavior just doesn't feel right and we're only introducing more technical/code debt that way, no? |
I agree and we should probably log an issue or discussion so it doesn't get covered by a dust of time, but for now, this works well enough for production use-cases we have. |
…d-containers' into enhancement/OP-2825_attach-loaded-containers
|
Good with me. let's merge it, it's needed in production |
| def load(self, context, name=None, namespace=None, options=None): | ||
| container = super(ReferenceLoader, self).load( | ||
| context, name, namespace, options) | ||
| # clean containers if present to AVALON_CONTAINERS | ||
| self._organize_containers(self[:], container[0]) | ||
|
|
There was a problem hiding this comment.
This can now be removed completely now the logic is implemented in the parent class. Correct?
| loaded_containers = None | ||
| if set(self.add_for_families).intersection( | ||
| set(instance.data.get("families")), | ||
| set(instance.data.get("family").lower())): |
There was a problem hiding this comment.
set(instance.data.get("family").lower())this is incorrect? Family is not a list and thus this would turn into e.g.set("model".lower())which turns intoset("model")which becomes{"m", "o", "d", "e", "l"}:).lower()can be removed?
There was a problem hiding this comment.
Also, are you sure you require the family to also be present in families which is what the current logic is doing because a.intersection(b, c) returns a set with items that is present in all 3 sets.
I'd rather detect whether it's at least present in either as in my pseudocode mockup
| selection = members | ||
| if loaded_containers: | ||
| self.log.info(loaded_containers) | ||
| selection += loaded_containers |
There was a problem hiding this comment.
This if statement can be moved into the if statement above.
# psuedocode
families = set([instance.data.get("family", "")] + instance.data.get("families", []))
if families.intersection(set(self.add_for_families)):
members += self._get_loaded_containers(members) # note that I also refactored this method to a more logical name| refs_to_include = [ | ||
| cmds.referenceQuery(ref, referenceNode=True) | ||
| for ref in members | ||
| if cmds.referenceQuery(ref, isNodeReferenced=True) | ||
| ] | ||
|
|
||
| refs_to_include = set(refs_to_include) | ||
|
|
||
| obj_sets = cmds.ls("*.id", long=True, type="objectSet", recursive=True, | ||
| objectsOnly=True) | ||
|
|
||
| loaded_containers = [] | ||
| for obj_set in obj_sets: | ||
|
|
||
| if not cmds.attributeQuery("id", node=obj_set, exists=True): | ||
| continue | ||
|
|
||
| id_attr = "{}.id".format(obj_set) | ||
| if cmds.getAttr(id_attr) != AVALON_CONTAINER_ID: | ||
| continue | ||
|
|
||
| set_content = set(cmds.sets(obj_set, query=True)) | ||
| if set_content.intersection(refs_to_include): | ||
| loaded_containers.append(obj_set) | ||
|
|
||
| return loaded_containers |
There was a problem hiding this comment.
I think this logic would fail to detect e.g. loaded gpuCache containers, yetiCache containers, etc. and others that aren't referenced. So this logic would not be correct?
members_with_refs= set(members) + set(refs_to_include) # line 108
...
if set_content.intersection(members_with_refs): # line 124Likely you'd need something like that?
Ouch. Please see my notes above. :) |
Enhancement
Adding ability to include loaded containers to published instance.
This is mainly useful for Layouts and Setdress families. When you create them from loaded models for example, this will pull their containers along published Layout so OpenPype will retain information about them when Layout is loaded into the new scene.
This can be set in the Settings (
project_settings/maya/publish/ExtractMayaSceneRaw/add_for_families):How to test:
In the new scene load bunch of models with OpenPype, create Layout and publish it. When you load this layout, you should see all content in the layout in Manager.
Close #2830