Add support for onAttributeNameChanged callbacks for nested Attributes

[cbentejac]

Description

This PR adds the support of the on{AttributeName}Changed callbacks for attributes that are nested in GroupAttributes. Prior to it, callbacks could not be used for nested attributes; either the callback was defined for the highest parent GroupAttribute or ListAttribute (and was thus triggered for every single change occurring in that Group or List), or no change could be tracked for that attribute.

Elements within ListAttributes still cannot use these callbacks; the callback can be setup for the list itself, meaning it will be triggered for any insertion/deletion in that list, but not for element value changes.

To determine whether an attribute with a parent can be bound to a callback, we introduce a new isInsideList property, that is evaluated once upon the attribute's creation, and determines whether there is a ListAttribute in the hierarchy of the current attribute.

Tests are added for both the binding of callbacks and the isInsideList property.

Features list

• Catch ValueError exception in _getRootName when it is called for a list element while the element is being added to the list.
• Add a new isInsideList property to determine whether an attribute is, directly or not, part of a ListAttribute.
• Add a unit test to ensure isInsideList always evaluates correctly.
• Support the binding of on{AttributeName}Changed callbacks for attributes that are nested within GroupAttributes.
• Add unit tests to ensure the on{AttributeName}Changed callbacks are bound correctly depending on their attribute's status.

[codecov]

Codecov Report

❌ Patch coverage is 97.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.50%. Comparing base (6ac15ec) to head (498353c).
⚠️ Report is 71 commits behind head on develop.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
meshroom/core/attribute.py	85.71%	2 Missing ⚠️
tests/test_nodeAttributeChangedCallback.py	98.24%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3115      +/-   ##
===========================================
+ Coverage    85.39%   85.50%   +0.10%     
===========================================
  Files           73       73              
  Lines        11403    11498      +95     
===========================================
+ Hits          9738     9831      +93     
- Misses        1665     1667       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Alxiice]

So I have a picky remark concerning performances.
I created a simple test node :

import inspect
from meshroom.core import desc

class TestNodeA(desc.Node):
    category = 'TestPlugin'
    
    inputs = [
        desc.GroupAttribute(
            name="parentGrp",
            exposed=True,
            items=[
                desc.GroupAttribute(
                    name="nestedGrp",
                    exposed=True,
                    items=[
                        desc.IntParam(
                            name="x",
                            value=0,
                            exposed=True
                        ),
                        desc.IntParam(
                            name="y",
                            value=0,
                            exposed=True
                        ),
                    ],
                ),
            ],
        ),
    ]

    def display_func_infos(self, fname, node):
        print(f"[Func {fname}] (node {node.name})")
    
    def onParentGrpChanged(self, node):
        self.display_func_infos(inspect.stack()[0][3], node)
    
    def onParentGrpNestedGrpChanged(self, node):
        self.display_func_infos(inspect.stack()[0][3], node)
    
    def onParentGrpNestedGrpXChanged(self, node):
        self.display_func_infos(inspect.stack()[0][3], node)
    
    def onParentGrpNestedGrpYChanged(self, node):
        self.display_func_infos(inspect.stack()[0][3], node)
    
    def process(self, node):
        self.display_func_infos(inspect.stack()[0][3], node)

Here is the update execution order

Before (on develop) :

# TestA_1.parentGrp.nestedGrp.x -> cannot call callback because of nested attr
# TestB_1.parentGrp.nestedGrp.x -> cannot call callback because of nested attr
# TestB_1.parentGrp.nestedGrp   -> cannot call callback because of nested attr
[Func onParentGrpChanged] (node TestB_1)
# TestA_1.parentGrp.nestedGrp   -> cannot call callback because of nested attr
# TestB_1.parentGrp.nestedGrp   -> cannot call callback because of nested attr
[Func onParentGrpChanged] (node TestB_1)
[Func onParentGrpChanged] (node TestA_1)
[Func onParentGrpChanged] (node TestB_1)

After (on this branch) :

[Func onParentGrpNestedGrpXChanged] (node TestA_1)
[Func onParentGrpNestedGrpXChanged] (node TestB_1)
[Func onParentGrpNestedGrpChanged] (node TestB_1)
[Func onParentGrpChanged] (node TestB_1)
[Func onParentGrpNestedGrpChanged] (node TestA_1)
[Func onParentGrpNestedGrpChanged] (node TestB_1)
[Func onParentGrpChanged] (node TestB_1)
[Func onParentGrpChanged] (node TestA_1)
[Func onParentGrpChanged] (node TestB_1

It bugs me a little that we don't have a way to batch the updates on the parent node and then execute the updates on the children.

I extracted a stack trace to explain how the signals are called:

Change TestA.parentGrp.nestedGrp.x
    SetAttributeCommand.redoImpl
        Attribute._setValue
            Attribute._onValueChanged
                BaseNode._onAttributeChanged
                |   get the callback (BaseNode._getAttributeChangedCallback)
                |   callback is onParentGrpNestedGrpXChanged
                |   call callback
                |   for each edge on the graph:
                |       emit valueChanged
                |           [TestB.parentGrp.nestedGrp.x]
                |           |   valueChanged signal received on attribute TestB.parentGrp.nestedGrp.x
                |           |   Attribute._onValueChanged
                |           |       BaseNode._onAttributeChanged
                |           |           get the callback (BaseNode._getAttributeChangedCallback)
                |           |           callback is onParentGrpNestedGrpXChanged
                |           |           call callback
                |           |   Then because the TestB.parentGrp.nestedGrp _value is driven by the
                |           |   content inside the GroupAttribute, the signal valueChanged is called
                |           |   on the attribute TestB.parentGrp.nestedGrp
                |           |   Execute BaseNode._onAttributeChanged TestB.parentGrp.nestedGrp
                |           `-  Then execute BaseNode._onAttributeChanged TestB.parentGrp
                The attribute TestA.parentGrp.nestedGrp detects its content have changed 
                so it will trigger an update
                [On TestA.parentGrp.nestedGrp]
                |   BaseNode._onAttributeChanged
                |       get and call the callback -> onParentGrpNestedGrpChanged
                |       emit valueChanged on linked attributes
                |       -> onValueChanged on TestB.parentGrp.nestedGrp
                |          Then : onValuChanged on TestB.parentGrp
                [On TestA.parentGrp]
                |   BaseNode._onAttributeChanged
                |       get and call the callback -> onParentGrpChanged
                |       emit valueChanged on linked attributes
                `-       -> onValueChanged on TestB.parentGrp

Technically it's a "new" issue because we didn't see it before, but I would uderstand if you want to tackle this in another PR because this require probably a huge update on the signals... So maybe at least we could open an issue for this ?

[Alxiice]

It's just an idea but in this implementation if an element of the list change we don't even call the list update callback.
It's a draft but here's a snippet that make it partially work :

    def _onAttributeChanged(self, attr: Attribute):
        """
        When an attribute value has changed, a specific function can be defined in the descriptor
        and be called.

        Args:
            attr: The Attribute that has changed.
        """

        if self.isCompatibilityNode:
            # Compatibility nodes are not meant to be updated.
            return

        if attr.isOutput and not self.isInputNode:
            # Ignore changes on output attributes for non-input nodes
            # as they are updated during the node's computation.
            # And we do not want notifications during the graph processing.
            return

        if not attr.keyable and attr.value is None:
            # Discard dynamic values depending on the graph processing.
            return

        if self.graph and self.graph.isLoading:
            # Do not trigger attribute callbacks during the graph loading.
            return

        callback = self._getAttributeChangedCallback(attr)
        
        if not callback and attr.root is not None and attr.isInsideList:
            def getLastCallable(attr):
                if not attr.root or not attr.isInsideList:
                    return attr
                return getLastCallable(attr._root())
            attr = getLastCallable(attr)  # Important to replace attr for the call on edges
            callback = self._getAttributeChangedCallback(attr)

        if callback:
            callback(self)

        self.hasInvalidAttributeChanged.emit()

        if self.graph:
            # If we are in a graph, propagate the notification to the connected output attributes
            for edge in self.graph.outEdges(attr):
                edge.dst.valueChanged.emit()

Once again my test :

class TestNodeB(desc.Node):
    category = 'TestPlugin'
    
    inputs = [
        desc.GroupAttribute(
            name="parentGrp",
            exposed=True,
            items=[
                desc.ListAttribute(
                    name="listAttr",
                    exposed=True,
                    elementDesc=desc.IntParam(
                        name="listItem",
                        value=0,
                    )
                )
            ]
        )
    ]

    def display_func_infos(self, fname, node):
        print(f"[Func {fname}] (node {node.name})")
    
    def onParentGrpChanged(self, node):
        self.display_func_infos(inspect.stack()[0][3], node)

    def onParentGrpListAttrChanged(self, node):
        self.display_func_infos(inspect.stack()[0][3], node)
    
    def onParentGrpListAttrListItemChanged(self, node):
        self.display_func_infos(inspect.stack()[0][3], node)
    
    def process(self, node):
        self.display_func_inf

And the output :

[Func onParentGrpListAttrChanged] (node TestC_1)  # Update the closest attr that we can update
[Func onParentGrpListAttrChanged] (node TestD_1)  # Call update on child param
[Func onParentGrpChanged] (node TestD_1)          # Update parent on child param
# MISSING : Call onParentGrpChanged on node TestC_1