-
Notifications
You must be signed in to change notification settings - Fork 129
Enhancement: Houdini use instance transient data for instance_node
#5616
Enhancement: Houdini use instance transient data for instance_node
#5616
Conversation
@@ -56,7 +56,7 @@ def process(self, instance): | |||
layer_inst.data["subset"] = "__stub__" | |||
layer_inst.data["label"] = label | |||
layer_inst.data["asset"] = instance.data["asset"] | |||
layer_inst.data["instance_node"] = instance.data["instance_node"] | |||
layer_inst.data["instance_node"] = instance.data["transientData"]["instance_node"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (94 > 79 characters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instance_node = instance.data["transientData"]["instance_node"]
for layer in usdlib.get_configured_save_layers(rop_node):
...
layer_inst.data["instance_node"] = instance_node
...
@@ -38,7 +38,7 @@ def process(self, instance): | |||
validate_nodes = [] | |||
|
|||
if len(instance) > 0: | |||
validate_nodes.append(hou.node(instance.data.get("instance_node"))) | |||
validate_nodes.append(instance.data["transientData"]["instance_node"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (82 > 79 characters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be:
instance_node = instance.data["transientData"].get("instance_node")
if instance_node is not None:
validate_nodes.append(instance_node)
@@ -98,7 +98,7 @@ def process(self, instance): | |||
"Invalid VDB content: {}".format(message), | |||
formatting_data={ | |||
"message": message, | |||
"rop_path": instance.data.get("instance_node"), | |||
"rop_path": instance.data["transientData"]["instance_node"].path(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (87 > 79 characters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instance_node = instance.data["transientData"]["instance_node"]
formatting_data={
"message": message,
"rop_path": instance_node.path(),
"sop_path": output_path
}
Will this PR be still useful if #5490 is merged ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested as a User these product types work fine
- camer
- imsage sequence
- ABC point cache
- BGEO point cache
- VDB cache
- Review
These families (that submits to farm) error
- Mantra
- Karma
After applying this fix, it worked
OpenPype/openpype/modules/deadline/plugins/publish/submit_houdini_render_deadline.py
Line 125 in 23e9f5e
driver = hou.node(instance.data["instance_node"]) |
driver = instance.data["transientData"]["instance_node"]
I was Misled and thought that
So, This PR
where |
The idea is that
How do you mean? The Creators set it. They don't need to be 'maintained/updated' because it always represents the physical
Yes, I mean. Not by choice really ;) I'd prefer it |
Let me rephrase my questions:
|
Transient DataTransient Data was once added because all instance data generated by the Creator had to be json serializable - the It got added with this PR: #3897 where at first it was referred to as lifetime data but later got renamed to transient data. It seems that Transient Data unfortunately was never added to the documentation: https://openpype.io/docs/dev_publishing - probably best to create an issue about that.
It will still refer to the ROP node, but by It's likely however that some conflicts need to be resolved since that PR - but before anyone would do so I'd rather first know whether this PR is even remotely worth the effort and whether it's a good design choice. As a reference, Fusion also uses the |
yeah, I will add that to the community guide AYON / Openpype Publish process - Development guide When I started learning about Openpype 4 or 5 months ago, I didn't expect all these details! |
The Creator collects the Personally I see it like this: The only reason for me that But transient data tells a bit more as well - it says 'do not store these on the node' - which happens to be the case for non-JSON data (because we can't serialize it well) but also for this instance node. We don't need to store it on the node, because it is the node. Not sure how to better explain it. I know @iLLiCiTiT has a view of his own on it and likely can explain it better in a different way.
Exactly - the Creator already knows how to find the nodes, that logic never relied on the node's path being stored on the node, etc. All the Creator does was "find all nodes with attribute The fact that Similarly we can have |
I think I realized how it works, Thanks! So, there are two ways to create instances in Houdini
and this snippet some where inside calls HoudiniCreator(...).collect_instances() which recreate instances from existing openpype/ayon rop nodes from openpype.pipeline.create import CreateContext
from openpype.pipeline import registered_host
host = registered_host()
context = CreateContext(host)
for instance in context.instances:
print(instance.subset_name, instance.transient_data) Eureka ! OpenPype/openpype/pipeline/create/context.py Line 2141 in 12f4128
|
I like the idea introduced by this PR, I'm amazed that same approach is followed in Nuke, Fusion and also Maya. |
Would you just need me to resolve the conflicts, or are there other things to implement? |
Nah, It's a casual PR revisiting during PRs cleaning up. |
Is it worth our while to refactor like this. Since it doesn't change anything end-user wise it doesn't bring any new features. So, biggest question is - do we want this? If so, I can resolve conflicts and prepare it for testing. |
I'm afraid I can't answer this question, I'll leave it to @moonyuet or @antirotor when dealing with contexts So, my opinion this PR is helpful and I like it. |
Is this PR still relevant? I think most of the changes are, but not sure if are up to date. |
Not sure how to deal with this one right now. Lot's of conflicts, but probably a reasonable change. @antirotor I might need your guidance here. Seems that it should be OP and AYON compatible out of the box though |
Because we're splitting OpenPype into ayon-core and individual host addons, this PR would have to be re-created to target one of those. We're closing it down, but we'll he happy for a new PR to ynput/ayon-core or the host addon repository once it's up. |
Changelog Description
Use transient data to store the actual houdini node object in the instance as opposed to the path so
hou.node()
isn't needed to retrieve the node per plugin.Also removed:
Additional notes
👇 Please consider this question
I wasn't sure whether I should open this as a Draft PR or not. I'm not sure whether everyone agrees this is a design improvement or whether it's worse. Thoughts?
☝️
Testing notes: