Merge pull request #2586 from alicevision/fix/attributeValueChanged

Fix attribute value change propagation and callback handling

Author: cbentejac

meshroom/core/attribute.py

@@ -25,11 +25,14 @@ def attributeFactory(description, value, isOutput, node, root=None, parent=None)
         root: (optional) parent Attribute (must be ListAttribute or GroupAttribute)
         parent (BaseObject): (optional) the parent BaseObject if any
     """
-    attr = description.instanceType(node, description, isOutput, root, parent)
+    attr: Attribute = description.instanceType(node, description, isOutput, root, parent)
     if value is not None:
-        attr._set_value(value, emitSignals=False)
+        attr._set_value(value)
     else:
-        attr.resetToDefaultValue(emitSignals=False)
+        attr.resetToDefaultValue()
+
+    attr.valueChanged.connect(lambda attr=attr: node._onAttributeChanged(attr))
+
     return attr
 
 
@@ -67,7 +70,6 @@ def __init__(self, node, attributeDesc, isOutput, root=None, parent=None):
         self._value = None
         self.initValue()
 
-        self.valueChanged.connect(self.onChanged)
 
     @property
     def node(self):
@@ -180,22 +182,7 @@ def _get_value(self):
             return self.getLinkParam().value
         return self._value
 
-    def onChanged(self):
-        """ Called when the attribute value has changed """
-        if self.node.isCompatibilityNode:
-            # We have no access to the node's implementation,
-            # so we cannot call the custom method.
-            return
-        if self.isOutput and not self.node.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
-        # notify the node that the attribute has changed
-        # this will call the node descriptor "onAttrNameChanged" method
-        self.node.onAttributeChanged(self)
-
-    def _set_value(self, value, emitSignals=True):
+    def _set_value(self, value):
         if self._value == value:
             return
 
@@ -211,9 +198,6 @@ def _set_value(self, value, emitSignals=True):
             convertedValue = self.validateValue(value)
             self._value = convertedValue
 
-        if not emitSignals:
-            return
-
         # Request graph update when input parameter value is set
         # and parent node belongs to a graph
         # Output attributes value are set internally during the update process,
@@ -251,8 +235,8 @@ def initValue(self):
         if self.desc._valueType is not None:
             self._value = self.desc._valueType()
 
-    def resetToDefaultValue(self, emitSignals=True):
-        self._set_value(copy.copy(self.defaultValue()), emitSignals=emitSignals)
+    def resetToDefaultValue(self):
+        self._set_value(copy.copy(self.defaultValue()))
 
     def requestGraphUpdate(self):
         if self.node.graph:
@@ -538,14 +522,13 @@ def index(self, item):
         return self._value.indexOf(item)
 
     def initValue(self):
-        self.resetToDefaultValue(emitSignals=False)
+        self.resetToDefaultValue()
 
-    def resetToDefaultValue(self, emitSignals=True):
+    def resetToDefaultValue(self):
         self._value = ListModel(parent=self)
-        if emitSignals:
-            self.valueChanged.emit()
+        self.valueChanged.emit()
 
-    def _set_value(self, value, emitSignals=True):
+    def _set_value(self, value):
         if self.node.graph:
             self.remove(0, len(self))
         # Link to another attribute
@@ -558,8 +541,6 @@ def _set_value(self, value, emitSignals=True):
                 self._value = ListModel(parent=self)
             newValue = self.desc.validateValue(value)
             self.extend(newValue)
-        if not emitSignals:
-            return
         self.requestGraphUpdate()
 
     def upgradeValue(self, exportedValues):
@@ -696,7 +677,7 @@ def __getattr__(self, key):
             except KeyError:
                 raise AttributeError(key)
 
-    def _set_value(self, exportedValue, emitSignals=True):
+    def _set_value(self, exportedValue):
         value = self.validateValue(exportedValue)
         if isinstance(value, dict):
             # set individual child attribute values
@@ -734,7 +715,7 @@ def initValue(self):
             childAttr.valueChanged.connect(self.valueChanged)
         self._value.reset(subAttributes)
 
-    def resetToDefaultValue(self, emitSignals=True):
+    def resetToDefaultValue(self):
         for attrDesc in self.desc._groupDesc:
             self._value.get(attrDesc.name).resetToDefaultValue()
 

meshroom/core/node.py

@@ -14,6 +14,7 @@
 import uuid
 from collections import namedtuple
 from enum import Enum
+from typing import Callable, Optional
 
 import meshroom
 from meshroom.common import Signal, Variant, Property, BaseObject, Slot, ListModel, DictModel
@@ -929,25 +930,50 @@ def updateStatisticsFromCache(self):
     def _updateChunks(self):
         pass
 
-    def onAttributeChanged(self, attr):
-        """ When an attribute changed, a specific function can be defined in the descriptor and be called.
+    def _getAttributeChangedCallback(self, attr: Attribute) -> Optional[Callable]:
+        """Get the node descriptor-defined value changed callback associated to `attr` if any."""
+
+        # Callbacks cannot be defined on nested attributes.
+        if attr.root is not None:
+            return None
+
+        attrCapitalizedName = attr.name[:1].upper() + attr.name[1:]
+        callbackName = f"on{attrCapitalizedName}Changed"
+
+        callback = getattr(self.nodeDesc, callbackName, None)
+        return callback if callback and callable(callback) else None
+
+    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 (Attribute): attribute that has changed
+            attr: The Attribute that has changed.
         """
-        # Call the specific function if it exists in the node implementation
-        paramName = attr.name[:1].upper() + attr.name[1:]
-        methodName = f'on{paramName}Changed'
-        if hasattr(self.nodeDesc, methodName):
-            m = getattr(self.nodeDesc, methodName)
-            if callable(m):
-                m(self)
+
+        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 attr.value is None:
+            # Discard dynamic values depending on the graph processing.
+            return
+
+        callback = self._getAttributeChangedCallback(attr)
+
+        if callback:
+            callback(self)
 
         if self.graph:
             # If we are in a graph, propagate the notification to the connected output attributes
-            outEdges = self.graph.outEdges(attr)
-            for edge in outEdges:
-                edge.dst.onChanged()
+            for edge in self.graph.outEdges(attr):
+                edge.dst.node._onAttributeChanged(edge.dst)
 
     def onAttributeClicked(self, attr):
         """ When an attribute is clicked, a specific function can be defined in the descriptor and be called.

tests/conftest.py

@@ -0,0 +1,32 @@
+from pathlib import Path
+import tempfile
+
+import pytest
+
+from meshroom.core.graph import Graph
+
+
+@pytest.fixture
+def graphWithIsolatedCache():
+    """
+    Yield a Graph instance using a unique temporary cache directory.
+
+    Can be used for testing graph computation in isolation, without having to save the graph to disk.
+    """
+    with tempfile.TemporaryDirectory() as cacheDir:
+        graph = Graph("")
+        graph.cacheDir = cacheDir
+        yield graph
+
+    
+@pytest.fixture
+def graphSavedOnDisk():
+    """
+    Yield a Graph instance saved in a unique temporary folder.
+
+    Can be used for testing graph IO and computation in isolation.
+    """
+    with tempfile.TemporaryDirectory() as cacheDir:
+        graph = Graph("")
+        graph.save(Path(cacheDir) / "test_graph.mg")
+        yield graph