Create: Enhance instance & context changes

[iLLiCiTiT]

Brief description

Changes of instances and context have complex, hard to get structure. The structure did not change but instead of complex dictionaries are used objected data.

Description

This is poposal of changes data improvement for creators. Implemented TrackChangesItem which handles the changes for us. The item is creating changes based on old and new value and can provide information about changed keys or access to full old or new value. Can give the values on any "sub-dictionary".

Used this new approach to fix change in houdini and 3ds max and also modified one aftereffects plugin using changes.

Additional info

Each object of item is creating deepcopy of both old and new data so subdictionaries are calculated dynamically on demand.

[BigRoy]

I think this is an improvement, but not entirely sure.

I feel the code of the ChangedItem class is still too complex. Just looking at the class itself I'm not entirely sure what it's doing and why it needs to be this lengthy. What it actually is - in essence it's a dataclass storing the old value and the new value. But then at the same time there are "changed keys" and "available keys" and things might be a dict or they might not be.

It could be also that the code is so undocumented making it just hard to follow why it needs to be this lengthy. Some things that stood out:

• What is available_keys()? It's old_keys() + new_keys()? If they are the old keys they don't sound available? :)
• The mixture of terminology children and keys seem a bit confusing for how we're trying to mimic a dict like. Maybe items makes more sense then children?
• What's the reason we need to explicitly pass along the 'old' values too? If the changes would just return the new values where removed values might have a special class type value "REMOVED" then it'd be up to the implementation to decide what to do with the values? Or why not even disregard values for something that was removed? Just return the json structure of new values only?

I feel that somehow it should just be way more trivial to get and store and track a value difference.

[iLLiCiTiT]

What is available_keys()? It's old_keys() + new_keys()? If they are the old keys they don't sound available? :)

It's object having "changes" it's not "new value" focused.

The mixture of terminology children and keys seem a bit confusing for how we're trying to mimic a dict like. Maybe items makes more sense then children?

Children are not publicly available. The items method returns changes by changed keys {key: (old, new)} -> this is what I guess would be used at Houdini.

What's the reason we need to explicitly pass along the 'old' values too? If the changes would just return the new values where removed values might have a special class type value "REMOVED" then it'd be up to the implementation to decide what to do with the values? Or why not even disregard values for something that was removed? Just return the json structure of new values only?

You can use changes.new_value instead of instance.data_to_store(). New value at 1st dictionary level can be different than new value on 2nd level, the 1st level should return whole new values. Why old value is needed? If you'll get only new values you can't get information about "removed" keys. That's why old value must be available and old values/keys have it's point. Also without old value you can't prepare changes for 2nd -> nth level of dictionary.

feel that somehow it should just be way more trivial to get and store and track a value difference.

Like we've discussed. If you'll tell me a way which fit more then single use-case, I'd be happy. This implementation gives ability to "only get new value" (most of current usecases), "only get changes of n-th hierarchical level" (with full old/new value), "get removed keys" (probably could be added as property), "get any changes in whatever level of values" (for massochist), ...

Sometimes you may need to know the old value (e.g. because of type etc.).

changes = ChangedItem(..., ...)

# Use new value as it is
changes.new_value

# Handle changes key by key
removed_attrs = []
attr_values = {}
for key in changes.changed_keys:
    value = changes[key].new_value
    if value is None:
        removed_attrs.append(key)
    else:
        attr_values[key] = value

[BigRoy]

Ok, let's step back for a bit. Can you explain why we need the nested values to begin with? Why can't we just have {key}: {value}?

[iLLiCiTiT]

Ok, let's step back for a bit. Can you explain why we need the nested values to begin with? Why can't we just have {key}: {value}?

Not sure I understand {key}: {value}? You may want to store it per nuke/houdini/maya attribute (which is type based). Creator may want to check nested changes before data are stored (and trigger a callback). There can be other use-cases when you need to know the old value. There should be some way how to easily check if the value changed (changes["creator_attributes"]["my_attr"].changed). You should also be able to know which keys were removed. All of that this approach supports.

[BigRoy]

I think there's too little information for me as to what data we're actually dealing with. I assumed something like this:

{
    instance_attributes: {
        "subset": (old, new),
        "variant": (old, new),
        "asset": (old, new),
        "task": (old, new),
    },
    creator_attributes: {
        "attrx": (old, new),
        "attry": (old, new),
        "attrz": (old, new),
    },
    plugin_attributes: {
        pluginx: {
            "attrx": (old, new),
            "attry": (old, new),
            "attrz": (old, new),
       }
        pluginy:  {
            "attrx": (old, new),
            "attry": (old, new),
            "attrz": (old, new),
       }  
    } 

*Where originally 'instance_attributes' wasn't its own key but instead I believe those just ended up directly in the top dictionary.

The original comment I made was to simplify managing changes for developers in the Creator's update_instances() method so that any changes to any of the attributes from the above list are to be easily persisted in the scene.

Some of the attributes might be special custom attributes, but some, e.g. a frameStart and frameEnd creator attribute might be a DCC built-in attribute on a node or alike so we might want to customize per specific attribute how we'd go about persisting that so that it aligns with the DCC.

The simplest implementation for a DCC I could then think of would to be implement this:

class Creator:

    def update_changes(self, instance_changes):

        for instance, changes in instance_changes:
            
            # Store the instance attributes
            for attr, change in changes["instance_attributes"].items():
                old, new = change
                
                if old is None:
                    # New attribute
                    dcc.add_attribute(instance, attr, value=new)
                elif new is None:
                    # Deleted attribute
                    dcc.delete_attribute(instance, attr)
                else:
                    # Changed attribute
                    if type(old) != type(new):
                        # Attribute type changed, recreate attribute
                        dcc.delete_attribute(instance, attr)
                        dcc.add_attribute(instance, attr, value=new)
                    else:
                        # value changed
                        dcc.set_attribute(instance, attr, value=new)
                    
            
            # Store the creator attributes
            for attr, change in changes["creator_attributes"].items():
                old, new = change
                
                # Same dcc changes as above but maybe with a differen attr
                # prefix?
            
            # Store the plug-in attributes
            for plugin, plugin_changes in changes["plugin_attributes"].items():
                for attr, change in plugin_changes.items():
                    old, new = change
                
                    # Same dcc changes as above but maybe with a different attr
                    # prefix?

I feel that's quite lengthy one and it's not managing different datatypes and not managing that e.g. frameStart might need to be stored under a different attribute name in the dcc because there it might be a Vector2 attribute for time range or alike.

This example code here assumes a completely flat data structure for old and new value, there's no nesting of dictionary values, etc. I can't even fathom how to do this if the old, new values might even be nested datastructures. The DCC code would become crazy to pull off if those were nested values even.

I think if this is the length of code we'd need to write for a creator we've outdone ourselves and development will become much harder and bug-ridden because. We should think hard on how we can simplify this API and usage as much as possible before it'll become a dread to work with it and maintain.

Again, it might be just my frustration with not understanding what it is we're dealing with. I can't find the documentation and there's something about this area of the code that makes it hard to parse for me. Docstrings often leave me hungry for better information - if they are there to begin with.

[iLLiCiTiT]

I think there's too little information for me as to what data we're actually dealing with. I assumed something like this:

There is the same issue as before. If you remove publish plugin from publish attributes you have no way how to keep the structure and pass the information.

Again, it might be just my frustration with not understanding what it is we're dealing with. I can't find the documentation and there's something about this area of the code that makes it hard to parse for me. Docstrings often leave me hungry for better information - if they are there to begin with.

For houdini usecase. With ChangedItem you can skip the separated handling of "instance_attributes" and "creator_attributes" because in houdini are used only top level keys (top level key == attribute -> nested values are not needed).

# Keep in mind I just made the code based on what I remember from top of my head and based on your code snippet
def update_changes(self, instance_changes):
    for instance, changes in instance_changes:
        for key, changed_values in changes.changes():
            old_value, new_value = changed_values
            # I think this is what is happening in houdini, or?
            if isinstance(new_value, (dict, list)):
                new_value = "JSON::{}".format(json.dumps(new_value))
            if isinstance(old_value, (dict, list)):
                old_value = "JSON::{}".format(json.dumps(old_value))

            if new_value is None:
                dcc.delete_attribute(instance, key)

            elif old_value is None:
                dcc.add_attribute(instance, key, value=new_value)

            # Changed attribute
            elif type(old_value) != type(new_value):
                # Attribute type changed, recreate attribute
                dcc.delete_attribute(instance, key)
                dcc.add_attribute(instance, key, value=new_value)
            else:
                # value changed
                dcc.set_attribute(instance, key, value=new_value)

[BigRoy]

There is the same issue as before. If you remove publish plugin from publish attributes you have no way how to keep the structure and pass the information.

I didn't remove those though - the example include it, yes?

Even from your last example I don't see why there'd be any more nesting than my initial top example data here. Yes, it would be some dicts inside for changes for plug-in attributes and for changes of creator attributes, but the attributes themselves aren't nested.

I might need to step away from this for a bit and get back to it later - the frustration just seems to be a bit too high currently. 😵‍💫

---

Would be great if there were more parties using the new publisher to get some decent inputs from others who didn't build the system itself. That'd at least pinpoint whether it's just me failing to understand it or whether there's just a general lack of documentation for this to be ready for development.

I feel like @maxpareschi (Houdini + USD rendering) @Tilix4 (Blender Integration) @ddesmond (Clarisse integration) might be candidates to touching that codebase

[iLLiCiTiT]

Even from your last example I don't see why there'd be any more nesting than my initial #4375 (comment).

At the end is the object of ChangeItem is the same thing but on steroids with more prepared attributes and full values that are usable even on places where are not used at this moment.

Your (based on current) changes is dictionary which contain "only differences" -> not full value. So you can't just get full new value of whole "instance_attributes" or "publish_attributes" and store it, you have to call instance.data_to_store() once you find out that the key has changed and get the full value from that.

EDITED:
This is what current implementation changes does

# Old value
{"plugin_attributes": {
    "PluginX": {
        "attrx": 0
    },
    "PluginY":  {
        "attrx": 0,
        "attry": 0,
        "attrz": 0,
    }  
}}

# New value
{"plugin_attributes": {
    "PluginY": {
        "attrx": 255,
        "attry": 0,
        "attrz":0,
    },
    "PluginZ":  {
        "enabled": True
    }  
}}

# Changes (if it would work as was intended)
{"plugin_attributes": {
    # Removed plugin
    "PluginX": ({"attrx": 0}, None), 
    # Changed value of one plugin attribute
    "PluginY": {
        "attrx": (0, 255) 
    },
    # Added new plugin
    "PluginZ": (None, {"enabled": True}), 
}}

[maxpareschi]

I feel like @maxpareschi (Houdini + USD rendering) @Tilix4 (Blender Integration) @ddesmond (Clarisse integration) might be candidates to touching that codebase

we’re not really using that much of context stuff in Houdini, just instance subset, framerange, and instance_node name.
Other stuff is derived from the Houdini file.

As of today our usd render submitter is working functionally with some caveats:

• implemented deadline parameters under instance info in “extra” tab, most of them are still not checked against settings though. If you want the submitted job not to fail preemptively you need to write manually the pools to use.
• uses husk hardcoded.
• produces usd intermediates for rendering, maybe we can put them in a better spot or actually integrate them alongside the renders as a repre.
• one instance per RenderProduct. This is enforced.

Aside for this everything seem to work fine!

maybe take a look at my repo before I submit a pr, seems there could be places to improve.

My point is not to have too much flexibility on context and instance, it’s to have a preset data layout that can be documented and it would actually stick. Having other custom keys to contain specific plugin data should be separated from everything that is built in. So it’s easier to know what to expect.

Anyways, I’m all about discussion but I need to understand what you’re doing here because right now seems like another complication on top of an already complicated structure :)

[iLLiCiTiT]

Made some changes, added docstrings and changed usages of changes in current codebase.

[antirotor]

Tested it 3dsmax and Houdini and it seems to work

[kalisp]

Tested in AE, works