OpenPype icon indicating copy to clipboard operation
OpenPype copied to clipboard

Houdini: Enhance ABC publishing

Open antirotor opened this issue 3 years ago • 2 comments

Add automatic path attribute on creation, automatic output node connection.

  • If s@path attribute does not exist, or not present on all primitives, generate automatic path where missing (on creation) - auto path would be constructed like this: {parent_node_name}/{output_node_name} (this behavior will have to be a bit more refined in the future)
  • if Output_SOP connector does not exist, create one, connecting it to the node with render flag on, and display it (display flag on).

[cuID:OP-1798]

antirotor avatar Jun 29 '22 10:06 antirotor

If s@path attribute does not exist, or not present on all primitives, generate automatic path where missing (on creation) - auto path would be constructed like this: {parent_node_name}/{output_node_name} (this behavior will have to be a bit more refined in the future)

Would this be a Validator that adds in the extra nodes in Repair? Or when would this occur?

if Output_SOP connector does not exist, create one, connecting it to the node with render flag on, and display it (display flag on).

Could you elaborate slightly more about why this is needed?

BigRoy avatar Jun 29 '22 10:06 BigRoy

Would this be a Validator that adds in the extra nodes in Repair? Or when would this occur?

Yeah, I think Repair action si the right place for it.

Could you elaborate slightly more about why this is needed?

This is currently required by validators (so again, it would be probably Repair action). I think Output is good to have to bring some order into what's going out - at least visually.

antirotor avatar Jun 29 '22 10:06 antirotor

@BigRoy @moonyuet
Can we move the discussion here about the details of Houdini point cache enhancement ? I'm sorry I should have done that earlier.

Here are the details I can think of : this code related to these files

  • houdini\plugins\create\create_pointcache.py
  • houdini\plugins\publish\validate_primitive_hierarchy_paths.py
  • houdini\plugins\publish\validate_sop_output_node.py

Current behavior

  • on creation: it will set sop_path parm of the abc ROP to selection whether it's an ObjNode or SopNode
  • if selection is SopNode
    • sop_path will be set to a SopNode path
    • on publishing: if no path attribute (path_attrib of the abc ROP to be specific ), these validators will log.error
      • validate_abc_primitive_to_detail.py
      • validate_primitive_hierarchy_paths.py
  • if selection is ObjNode and it has a child of type "output"
    • sop_path will be set to that "output" sop node path which yields to the previous case
  • if selection is ObjNode
    • sop_path will be set to an ObjNode path
    • on publishing: these validators will raise this error AttributeError: 'ObjNode' object has no attribute 'geometry'
      • validate_abc_primitive_to_detail.py
      • validate_primitive_hierarchy_paths.py
    • After applying quick fix (checking not hasattr(output_node, "geometry"))
    • on publishing: validate_sop_output_node.py will log.error
      • "Output node %s is not a SOP node. "
      • Note that it doesn't force a specific SOP node type

Desired behavior

Disclaimer: I personally don't know what could deliver a better user experience for artists, and I will appreciate any recommendations

  • on creation:
  • if selection is SopNode - if it's not a node of type "output", an output node will be created and set its input to the selected node - on publishing: if no path attribute (path_attrib of the abc ROP to be specific ) - a repair action on validate_primitive_hierarchy_paths.py will - create a name node with the same attribute as path_attrib in ROP - add a default value {parent_node_name}/{output_node_name} or {parent_node_name}/{name_node_name} - it can be connected before or after the assigned node in sop_path, it depends on the node whether it's of type "output" or any other type. - ANY RECOMMENDATIONS
  • if selection is ObjNode and it has a child of type "output"
    • sop_path will be set to that "output" sop node path which yields to the previous case
  • if selection is ObjNode
    • sop_path will be set to an ObjNode path
    • Assuming I made the quick fix mentioned above
    • on publishing: validate_sop_output_node.py will log.error - a repair action to fix it, which will set the sop_path of the ROP to the selected node (which is a general solution for all cases) - which yields to the first case

Conclusion

See things from this perspective, I can understand why a sop "output" node is not a mandatory.

Connecting the name node before won't be a good thing if the node in sop_path is a multi-input node like "merge" node if it's a sop "output" node, then the name node must be connected before it

Using the 'output' sop-node and attaching the name-node before it satisfies the problem description above, which is what I was originally trying to achieve.

Let me know what do you think ?

MustafaJafar avatar Jun 30 '23 08:06 MustafaJafar

For desired behavior:

Other than sopNode, it will be great to also include dopNode for selection although user usually cache as bgeo and use file cache to get the related bgeo into SOP level to do the alembic export. Adding dopNode for selection just in case that users may want to export the alembic within the DOP network. You can read this reddit post: https://www.reddit.com/r/Houdini/comments/f6oefh/i_have_a_simple_dop_simulation_that_i_want_to/

It can show that you can do alembic export for the DOP network.

Some comments on Output Node Not quite sure about the standard workflow but I personally dont use output nodes except for rendering. Usually I use null nodes at the end of all creation. If the purpose of the point cache are to export and do comparison between several same objects with different fracturing levels, it is okay to create output node and hook it into the node..(but you can preview these fracturing objects quickly with exploded view node for sure)

Path Attribute Repair Action As more I look at the issue (and PR), more questions come in my mind regarding to repair action(It can be that I overthink too). As mentioned in the PR, the path attributes can be created with different attribute-related nodes. Not only attribute-create node shown in the PR, you can create path attributes with string data by connectivity node.

image

Many options can be choosed for storing the attributes data in Houdini, meaning that the nodes and the parameter which links to the string datacan be varied. E.g. The relative reference of parm is chs("prefix") for connectivity, while chs("string1") in attribute create and chs("name1") in the name attribute. In more complicated case, I am not sure if users use attribute promote (promoting point to primitive etc.) too. The repair action is only helpful for the occassion when there is no path attribute in the scene. By this, it may be better idea to have one invalid options to show the user doesn't include string data in the path attribute, recommend what to use for the string data: i.e. 'opname("..")/opoutput(".", 0)'

moonyuet avatar Jun 30 '23 11:06 moonyuet

As far as I understood, Here are the suggested edits:

  1. houdini\plugins\create\create_pointcache.py

    It's better to leave it as it was, plus we may do this

    • if selection is ObjNode , if output node wasn't present, the node with the active display flag will be used
  2. houdini\plugins\publish\validate_primitive_hierarchy_paths.py

    adding a repair action to add a node with a default path value

    • node name AUTO_PATH there are 4 possibilities: name, attribute create, connectivity, vex wrangle connectivity is better as it will increment prefix and all connected primitives will share the same number

    • attribute value opname('..')/opname('..')Shape_

    • node comment
      something similar to this, so that it will be clear for artists

        auto path node created automatically by repair action
        Feel free to modify or replace it.
      
    • node connection
      connect before if the output sop node of type output or null connect afterwards for other types, set sop_path to it

  3. houdini\plugins\publish\validate_sop_output_node.py

    adding a repair action to set sop_path to the selected node

    • if SopNode is selected, set its path to sop_path
    • if ObjNode is selected, follow same logic as create_pointcache
  4. applying quick fix (checking not hasattr(output_node, "geometry"))

    • validate_abc_primitive_to_detail.py
    • validate_primitive_hierarchy_paths.py

MustafaJafar avatar Jun 30 '23 13:06 MustafaJafar

Regarding including dopNode I think it can be possible during the creation process

if selected is dopNode then I would do this:

  • create a new geo node
  • create dop import
  • create unpack node
  • use unpack instead of the selected node, continue default execution flow

MustafaJafar avatar Jun 30 '23 13:06 MustafaJafar

Creator I agree that we should leave it as it is for current stage.

Validator For validate_primitive_hierarchy_paths.py, there is repair action for sure but there are more than four possibilities to create primitive attribute. So we need to do accurate and more general checks on it. If there is no primitive attribute named path in the network, it wont pass the validation. Rather than doing a check on just four certain nodes. Repair action can't repair anything as the relative references of parameters to store attribute values vary from node to nodes. But still it needs to be inside the validator for creating a new name node for storing the path attribute and connects to the node if the selected network doesn't have path attributes. For current stage, maybe try not to touch validate_sop_output_node.py as it is not only pointcache using but other families like vdbcache involving.

DopNode I dont have so much objection on that. but I think you can check the see if the dop nerwork from the dop import has the pack nodes.

Any thoughts? @BigRoy @antirotor

moonyuet avatar Jul 03 '23 08:07 moonyuet

We are currently supporting SOPs level exports to Alembic (if we ignore the camera export that is). The alembic exporter additionally supports exporting Object level nodes but we do not support that yet in the publishing - adding support for that should be done but should definitely be an issue/PR of its own because it'll require different validators all together (e.g. path parm and use sop path parm must be disabled)

So we support the "Use SOP paths" enabled route only - that's what we are working to enhance with this issue and PR #5189

adding a repair action to set sop_path to the selected node if SopNode is selected, set its path to sop_path if ObjNode is selected, follow same logic as create_pointcache

I thought that whenever Use SOP Path is enabled on the ROP node you should not set it to an ObjNode. However it seems that this does work: image

Which to me is confusing Houdini design because that's not a SOP path, hehe. Anyway, for our validations whenever and OBJ node is found as output node we'd likely need to isntead validate against:

  1. Output node with the lowest index (usually 0) (or maybe that's ObjNode.outputForViewFlag?) I'm not sure what the code is to get the output node for an ObjNode. It seems that ObjNode.subnetOutputs() outputs at least the output nodes so might be the way to go to find the lowest index one?
  2. If no output node, then the node with the display flag set. -> ObjNode.displayNode()

It doesn't change the validations itself, but only how we retrieve which node should be validated. I'm pretty sure that currently does not work during publish.

DopNode I dont have so much objection on that. but I think you can check the see if the dop nerwork from the dop import has the pack nodes.

I'm not too knowledgable about DOPs, but if the alembic rop doesn't support that natively with Use SOP path I'd say let's not try and automagically convert it.

As such, if this means we need to create extra nodes to import it to SOPs then I'd say, no. Let's not do that. It's up to the Houdini artist to create SOPs geometry for the alembic export to work. If they have something in DOPs, they convert it to SOPs and export the SOPs. If they have something in LOPs, they convert it to SOPs, etc. We're exposing the export feature to Alembic that they are familiar with from Houdini itself - we're not designing new export conversion features. Let's try and touch that as little as possible so we give the Houdini artist as much freedom as possible to do what they are great at.

Repair action can't repair anything as the relative references of parameters to store attribute values vary from node to nodes. But still it needs to be inside the validator for creating a new name node for storing the path attribute and connects to the node if the selected network doesn't have path attributes.

@moonyuet not sure what you're saying here. Why couldn't it?


I have to admit that I have a relatively hard time grasping what's being discussed here. There's a lot of information for something that initially seemed so trivial. I think we've lost somewhere what we are actually trying to solve and why. It would be great if for each mentioned changed that we want to do we could add a succinct explanation as to why that change is needed and what it solves.

BigRoy avatar Jul 03 '23 08:07 BigRoy

@BigRoy,I admit that it is getting more and more confusing. I am actually talking about the string value where stores the opname('..')/opname('..')Shape. So basically the nodes such as attribute_create or connectvity can store path primitive attributes with string value. But the relative references of the string value are different. e.g. the one from the attribute_create is chs("string1") while that from connectivity is chs("prefix"). Maybe the screenshot can explain more than words. Repair action may not fix all the errors as the relative references from nodes to nodes are different. image

image

moonyuet avatar Jul 03 '23 08:07 moonyuet

I am actually talking about the string value where stores the opname('..')/opname('..')Shape. So basically the nodes such as attribute_create or connectvity can store path primitive attributes with string value. But the relative references of the string value are different. e.g. the one from the attribute_create is chs("string1") while that from connectivity is chs("prefix"). Maybe the screenshot can explain more than words. Repair action may not fix all the errors as the relative references from nodes to nodes are different.

I still don't see the issue unfortunately. We never validate those particular nodes - we only validate the output and whether it's geometry is ok or not. If the geometry is invalid, then I believe currently this issue proposes to add repair actions which "insert" some nodes above the output node that add a path entry. That repair has full control over the nodes it generates and thus it should be trivial to set the right values. Right?

So we'd add repair actions like:

  • Set single name
  • Name using connectivity

Then whatever we use as node to set the actual name, either wrangle, name or attributecreate doesn't really matter. We just pick the one that we feel is the fastest node to use (which performs best), is the clearest for the artist in their graph visually and is easiest in code to maintain.

BigRoy avatar Jul 03 '23 09:07 BigRoy

Let's take a few steps back: To clear up some misconceptions, correct me if I'm wrong the original objective of this issue was to:

  1. add path repair action
  2. create output sop node if not present

ouptut sop node + my ignorance, they added few complexity my apologies.


let me summarize this discussion:

enhancement 1 : path repair action

if no path attribute, A connectivity node with

  • attribname same as path_attrib in the ROP node
  • prefix like this opname('..')/opname('..')Shape_
  • Connected as follows
    • before output node if the output node's is type null or output
    • otherwise after output node and set sop_path accordingly

    because output node may be a merge or copy node and connecting before will lead to un-desired results

P.S. I prefer connectivity as it will increment for each connected piece In maya it will be one object with many shapes inside it, we can discuss if this will be sufficient for now.

enhancement 2 : output repair action

because Validate Output Node requires a sop node set in sop_path in the ROP node
this validator will error for many reasons a general repair action to set it to the selected sop node I suggest to extend it allow selecting obj nodes as well and using the same logic to get the correct output node

this repair action will affect these families pointcache, vdbcache

enhancement 3 : create point cache

bear in mind that Validate Output Node requires a sop node this code doesn't correctly detect the output node whether it's an output node with the smallest outputidx or the node with the display flag

also, it allows all node types e.g. I can create an alembic subset while selecting a dop node, an artist may wonder why it's allowed despite publisher will error

# try to find output node
for child in self.selected_nodes[0].children():
    if child.type().name() == "output":
        parms["sop_path"] = child.path()
        break

bug fix : validators error (crash)

it happens when this line is executed as for many reasons, the output node may not have geometry geo = output_node.geometryAtFrame(frame) this bug happens in

  • validate_abc_primitive_to_detail.py
  • validate_primitive_hierarchy_paths.py

a quick fix to add a check like this: to skip if not hasattr(output_node, "geometry")

this fix may be irrelevant to this issue however I needed to fix it to test these enhancements

Let me know that do you think ? @moonyuet @BigRoy @antirotor

MustafaJafar avatar Jul 03 '23 11:07 MustafaJafar

Thanks for the breakdown

enhancement 1 : path repair action

I think this can be a nice feature. Definitely a PR on its own... but there are so many edge cases that I'm still wondering whether it's worth it.

In many cases, Connectivity is NOT the way to do it. In many case, single name is NOT the way to do it. Etc. what if connectivity changes over time? etc.

The highest priority is that the Validator explains the issue to the artist in a way that is understandable. What data is missing, why and how can I fix it? Then having a repair is secondary to that. If you feel artists get stuck 90% of the time, our priority is improving the reports output so they know what to do.

Once that's done, then we can look into tooling that can provide some out of the box options which will be INVALID 90% of the time: Set path by connectivity, Set single path, Set path by attribute X, etc. but we will need to keep in mind that these are just "helpers" and not a full blown repair since there are so many cases where it's NOT the intended export design for your particular case in production.

enhancement 2 : output repair action

this repair action will affect these families pointcache, vdbcache

Since this influences VDBCaches as well we can't use the same logic for both unfortunately - since afaik we should never set an object level path. Which actually makes me think allowing obj level path (as described in enhancement 3 below) should just be disallowed from OP point of view. It's just much more complex, and by still allowing explicit SOP level paths we still allow it to export the same things. (We're not losing features by requiring a SOP path instead - we're solely making it more explicit)

Having a repair action that updates the SOP path using the selection can be nice. However, we'll need to make sure that the action's label is not "Repair" but something like "Update sop path from selection" or alike because with "Repair" you wouldn't know it'll be using your selection. Which makes me think, isn't it much clearer if we just LOG very clearly what the user should end up doing (setting a valid SOP path in the SOP Path parameter) and maybe just having a "Select invalid" action on the plugin. The log then clearly mentions:

"The SOP Path is not set to an existing SOP path, make sure to update it on the ROP node." and then you can just select the ROP node to do it yourself. I'm not sure a "Set from selection" is going to be that much clearer for the artist especially compared to how much code might need to be written to write it and maintain it over time. Is it worth it?

enhancement 3 : create point cache

bear in mind that Validate Output Node requires a sop node

Yes.

this code doesn't correctly detect the output node whether it's an output node with the smallest outputidx or the node with the display flag

This might be a language barrier in your wording, but it doesn't have to be an output node with the smallest id NOR does it need to have the display flag. The preferred way pipeline-wise would be to have it point to a SOP path on the ROP node, but apparently Houdini Alembic ROP supports putting an OBJ path in the SOP path parm - confusing, but hey, let's support it.

As such, output SOP node becomes:

output_node = = instance.data["output_node"]
if isinstance(sop_node, hou.SopNode):
    # all ok
    sop_node = output_node
elif isinstance(sop_node, hou.ObjNode):
    # convert to relevant sop path
    sop_node = get_obj_output(output_node)
    if not sop_node:
        sop_node = get_obj_display_output(output_node)
        # Technically this could still be None if there are no nodes in the geometry node to begin with

However, as I mentioned in Enhancement 2 we're likely better of disallowing ObjNode paths to begin with and forcing SopNode paths.

As you can clearly see - even a simple case like this becomes way more complicated now ;) As such I'd recommend taking each of the enhancement you mention here into separate PRs since each likely involves code changes quite isolated plus already quite lengthy each.

also, it allows all node types e.g. I can create an alembic subset while selecting a dop node, an artist may wonder why it's allowed despite publisher will error

I think logic wise maybe it just makes sense to set any SOP path from selection if found, if not found set the OBJ node path (since apparently that works in Houdini) and if not found then set nothing from selection with maybe logging a warning that no valid selection was found.

selection = self.selected_nodes
path = None
preferred_node_types = [hou.SopNode, hou.ObjNode]
for type in preferred_node_types:
    if path is None:
        for node in selection:
            if isinstance(node, type):
                path = node.path()
                break

Not the prettiest code, but it shows the idea. We don't need to redirect to "output" nodes, etc.


bug fix : validators error (crash)

Yes, let's add the fix. Could even be a PR of its own since it doesn't change much about the enhancements.

BigRoy avatar Jul 03 '23 12:07 BigRoy

So, what about:

enhancement 1: Validate Prims Hierarchy Path(ABC)

It's already logs info to artist, I find it useful. however for this issue, I can add a create default path which uses name node to make a single default path with a comment on the node to provide more guidance for artists.

 Auto path node created automatically by "create default path" action
 Feel free to modify or replace it.

enhancement 2 : Validate Output Node

log already mentions that SOP Path must point to a SOP node I think adding actions like Select Invalid ROP so that artist can fix sop_path themselves Set selected SopNode as output node that only set sop_path if artists selected a SOP node

I like the first one more as it will be clear which ROP node to be fixed the second can be confusing when publishing many point caches

enhancement 3 : create point cache

I like forcing SopNode paths on creation which is going to set the sop_path to

  • the selected sop node
  • or the output of the select obj node
  • or keep it empty + logging

bug fix : validators error (crash)

as discussed above

@BigRoy

MustafaJafar avatar Jul 03 '23 13:07 MustafaJafar

Sounds good. Let's draft each of those and go from there.

BigRoy avatar Jul 03 '23 14:07 BigRoy

There are many PRs related to this issue!

enhancement 1: Validate Prims Hierarchy Path(ABC)

Enhancement/houdini add path action for abc validator

enhancement 2 : Validate Output Node

Houdini: add select invalid action for ValidateSopOutputNode

enhancement 3 : create point cache

Houdini: better selection on pointcache creation

bug fix : validators error (crash)

Houdini: Add geometry check for pointcache family

Houdini: Add geometry check for VDB family

it's not related to this issue however it shares the same bug as the point cache.

MustafaJafar avatar Jul 04 '23 08:07 MustafaJafar