diff --git a/meshroom/core/attribute.py b/meshroom/core/attribute.py index 613d3bd52a..780acafd90 100644 --- a/meshroom/core/attribute.py +++ b/meshroom/core/attribute.py @@ -4,6 +4,7 @@ import copy import os import re +import threading import weakref import logging import inspect @@ -185,8 +186,17 @@ def _getValue(self): raise RuntimeError(f"Cannot get value of {self._getFullName()}, the attribute is keyable.") if self.isLink: return self._getInputLink().value + self._resolveValue() return self._value + def _resolveValue(self): + """ + Hook for subclasses to resolve pending values before returning _value. + Called by _getValue before returning self._value. + Default implementation is a no-op. + """ + pass + def _setValue(self, value): """ Set the attribute value from a given value, a given function or a given attribute. @@ -820,6 +830,9 @@ def getSerializedValue(self): class ListAttribute(Attribute): + # Sentinel to distinguish 'no pending dynamic value' from 'pending reset to empty' + _NO_PENDING_VALUE = object() + def __init__(self, node, attributeDesc: desc.ListAttribute, isOutput: bool, root=None, parent=None): super().__init__(node, attributeDesc, isOutput, root, parent) @@ -861,13 +874,14 @@ def insert(self, index, value): self._value.insert(index, attrs) self.valueChanged.emit() self._applyExpr() - self.requestGraphUpdate() + if self.isInput: + self.requestGraphUpdate() @raiseIfLink def remove(self, index, count=1): if self._value is None: return - if self.node.graph: + if self.node.graph and self.isInput: from meshroom.core.graph import GraphModification with GraphModification(self.node.graph): # remove potential links @@ -877,15 +891,41 @@ def remove(self, index, count=1): # delete edge if the attribute is linked self.node.graph.removeEdge(attr) self._value.removeAt(index, count) - self.requestGraphUpdate() + if self.isInput: + self.requestGraphUpdate() self.valueChanged.emit() # Override def _initValue(self): + self._dynamicValueLock = threading.Lock() + self._dynamicValue = ListAttribute._NO_PENDING_VALUE self.resetToDefaultValue() # Override def _setValue(self, value): + if self.isOutput: + # For output attributes (set during processChunk in a worker thread, + # or during loadOutputAttr in the TaskThread), store raw values without + # creating QObject children to avoid cross-thread parenting issues. + # The raw values are: + # - serialized by saveOutputAttr via getPrimitiveValue + # - materialized into QObjects lazily by _getValue on the main thread + with self._dynamicValueLock: + if value is None: + self._dynamicValue = None # pending reset + else: + self._dynamicValue = self._desc.validateValue(value) + return + + # Input attribute path: handle None + if value is None: + if self.node.graph and self._value is not None and len(self._value) > 0: + self.remove(0, len(self)) + if self._value is None: + self._value = ListModel(parent=self) + self.valueChanged.emit() + return + if self.node.graph: self.remove(0, len(self)) if self._handleLinkValue(value): @@ -897,7 +937,8 @@ def _setValue(self, value): self._value = ListModel(parent=self) newValue = self._desc.validateValue(value) self.extend(newValue) - self.requestGraphUpdate() + if self.isInput: + self.requestGraphUpdate() # Override def _applyExpr(self): @@ -907,6 +948,20 @@ def _applyExpr(self): for value in self._value: value._applyExpr() + def _populateFromDynamicValue(self, value): + """Store raw dynamic values for lazy materialization. + + Does NOT create QObject children — safe to call from any thread. + The actual ListModel population happens lazily in _getValue() + when the main thread (e.g. QML) reads the value. + """ + with self._dynamicValueLock: + if value is None: + self._dynamicValue = None # pending reset + else: + self._dynamicValue = self._desc.validateValue(value) + self.valueChanged.emit() + # Override def resetToDefaultValue(self): self._value = ListModel(parent=self) @@ -922,8 +977,57 @@ def getSerializedValue(self): return self._getInputLink().asLinkExpr() return [attr.getSerializedValue() for attr in self._value] + value = Property(Variant, Attribute._getValue, _setValue, notify=Attribute.valueChanged) + + # Override + def _resolveValue(self): + """ + Lazily materialize QObject children from pending raw dynamic values. + Called by Attribute._getValue (base) before returning self._value. + This hook dispatches via normal Python MRO, bypassing PySide Property + getter dispatch limitations. + Must only create QObjects on the main thread to avoid cross-thread issues. + """ + if self._dynamicValue is not ListAttribute._NO_PENDING_VALUE: + if threading.current_thread() is threading.main_thread(): + self._materializeDynamicValue() + + def _materializeDynamicValue(self): + """ + Create QObject children in the ListModel from pending raw dynamic values. + Must only be called on the main thread. + """ + + # Thread proof reading of dynamic value + with self._dynamicValueLock: + pendingValue = self._dynamicValue + self._dynamicValue = ListAttribute._NO_PENDING_VALUE + + # Create an empty list if the value is None + if self._value is None: + self._value = ListModel(parent=self) + elif len(self._value) > 0: + # Erase all items before reassigning + self._value.removeAt(0, len(self._value)) + + # Effectively create the objects from the raw data + if pendingValue: + attrs = [attributeFactory(self._desc.elementDesc, v, self.isOutput, self.node, self) + for v in pendingValue] + self._value.insert(0, attrs) + + self.valueChanged.emit() + # Override def getPrimitiveValue(self, exportDefault=True): + # If there is a pending dynamic value (set or reset), return it directly + # without touching the ListModel (which may be on a different thread). + with self._dynamicValueLock: + if self._dynamicValue is not ListAttribute._NO_PENDING_VALUE: + if self._dynamicValue is None: + return [] + return list(self._dynamicValue) + if exportDefault: return [attr.getPrimitiveValue(exportDefault=exportDefault) for attr in self._value] return [attr.getPrimitiveValue(exportDefault=exportDefault) for attr in self._value diff --git a/meshroom/core/desc/attribute.py b/meshroom/core/desc/attribute.py index 4a41c3d4e6..5023baaeba 100644 --- a/meshroom/core/desc/attribute.py +++ b/meshroom/core/desc/attribute.py @@ -181,15 +181,17 @@ class ListAttribute(Attribute): """ A list of Attributes """ @deprecated.depreciateParam("group", "Param 'group' on {name} should not be used anymore. Please use 'commandLineGroup' instead") def __init__(self, elementDesc, name, label=None, description=None, group="allParams", commandLineGroup=_setParamSentinel, - advanced=False, semantic="", enabled=True, joinChar=" ", visible=True, exposed=False): + advanced=False, semantic="", enabled=True, joinChar=" ", visible=True, exposed=False, value=None): """ :param elementDesc: the Attribute description of elements to store in that list + :param value: default value. Use None to declare a dynamic output ListAttribute + whose content is set during processChunk. """ self._elementDesc = elementDesc self._joinChar = joinChar commandLineGroup = commandLineGroup if commandLineGroup is not _setParamSentinel else group - super(ListAttribute, self).__init__(name=name, label=label, description=description, value=[], + super(ListAttribute, self).__init__(name=name, label=label, description=description, value=value, invalidate=False, commandLineGroup=commandLineGroup, advanced=advanced, semantic=semantic, enabled=enabled, visible=visible, exposed=exposed) diff --git a/meshroom/core/node.py b/meshroom/core/node.py index 583fc6fa5d..ea939a4350 100644 --- a/meshroom/core/node.py +++ b/meshroom/core/node.py @@ -1788,22 +1788,35 @@ def resetOutputAttr(self): def loadOutputAttr(self): """ Load output attributes with dynamic values from a values.json file. """ + + # This does not apply to non dynamic output if not self.nodeDesc.hasDynamicOutputAttribute: return + + # Check existence of values.json file valuesFile = self.valuesFile if not os.path.exists(valuesFile): logging.warning(f"No output attr file: {valuesFile}") return - # logging.warning("load output attr: {}, value: {}".format(self.name, valuesFile)) + # Open json file and parse with open(valuesFile) as jsonFile: data = json.load(jsonFile) - # logging.warning(data) + # loop over all output attributes in the node description for output in self.nodeDesc.outputs: + # Only consider dynamic values if output.isDynamicValue: if self.hasAttribute(output.name) and output.name in data: - self.attribute(output.name).value = data[output.name] + attr = self.attribute(output.name) + + # Use _populateFromDynamicValue for compatible classes + # (E.g. for ListAttributes) to properly + # create QObject children on the main thread + if hasattr(attr, '_populateFromDynamicValue'): + attr._populateFromDynamicValue(data[output.name]) + else: + attr.value = data[output.name] else: if not self.hasAttribute(output.name): logging.warning(f"loadOutputAttr: Missing dynamic output attribute. Node={self.name}, " @@ -1821,7 +1834,8 @@ def saveOutputAttr(self): for output in self.nodeDesc.outputs: if output.isDynamicValue: if self.hasAttribute(output.name): - data[output.name] = self.attribute(output.name).value + # Store the primitive value and not the value itself + data[output.name] = self.attribute(output.name).getPrimitiveValue() else: logging.warning(f"saveOutputAttr: Missing dynamic output attribute: {self.name}.{output.name}") diff --git a/tests/test_attributeDescDefaults.py b/tests/test_attributeDescDefaults.py index 704b311bf1..013358f832 100644 --- a/tests/test_attributeDescDefaults.py +++ b/tests/test_attributeDescDefaults.py @@ -95,14 +95,6 @@ def test_param_no_value_is_dynamic(attrDesc): assert attrDesc.isDynamicValue is True -def test_list_and_group_attributes_not_dynamic(): - """ListAttribute and GroupAttribute always have a non-None default value.""" - la = desc.ListAttribute(desc.StringParam(name="elem"), name="items") - ga = desc.GroupAttribute([], name="group") - assert la.isDynamicValue is False - assert ga.isDynamicValue is False - - def test_label_auto_generated_from_camel_case(): """Label should be auto-generated from camelCase attribute names.""" assert desc.File(name="outputFile").label == "Output File" diff --git a/tests/test_nodeDynamicOutputs.py b/tests/test_nodeDynamicOutputs.py index 6c6c931880..7060e4ee08 100644 --- a/tests/test_nodeDynamicOutputs.py +++ b/tests/test_nodeDynamicOutputs.py @@ -85,6 +85,34 @@ def process(self, node): node.floatOutput.value = node.floatInput.value * 2.0 +class NodeWithDynamicListOutput(desc.Node): + """Node with a dynamic output ListAttribute set during processChunk.""" + inputs = [ + desc.ListAttribute( + name="listInput", + label="List Input", + description="A list of strings as input.", + elementDesc=desc.StringParam(name="value", label="Value", description="", value=""), + ), + ] + + outputs = [ + desc.ListAttribute( + name="listOutput", + label="List Output", + description="A dynamic list output set during processing.", + elementDesc=desc.StringParam(name="value", label="Value", description="", value=""), + value=None, + ), + ] + + def processChunk(self, chunk): + # Read input list and produce an output list with uppercased values + inputValues = [attr.value for attr in chunk.node.listInput.value] + outputValues = [v.upper() for v in inputValues] + chunk.node.listOutput.value = outputValues + + class InputNodeWithDynamicOutputs(desc.InputNode): inputs = [ desc.File( @@ -220,3 +248,77 @@ def test_registerInputNodeWithDynamicOutputsV2(self): # InputDynamicOutputs has the same description as InputNodeWithDynamicOutputs: had it been valid, it would # have been loaded and registered by the plugin manager at the upper level of the test suite. graph.addNewNode("InputDynamicOutputs") + + +class TestDynamicListOutputs: + """Tests for dynamic output ListAttribute support.""" + + @classmethod + def setup_class(cls): + registerNodeDesc(NodeWithDynamicListOutput) + + @classmethod + def teardown_class(cls): + unregisterNodeDesc(NodeWithDynamicListOutput) + + def test_dynamicListOutputDescriptor(self): + """Check that a ListAttribute with value=None is correctly flagged as dynamic.""" + nodeDesc = NodeWithDynamicListOutput() + assert nodeDesc.hasDynamicOutputAttribute + listOutputDesc = nodeDesc.outputs[0] + assert listOutputDesc.isDynamicValue + + def test_processWithDynamicListOutput(self, graphSavedOnDisk): + """Process a node that sets a dynamic output ListAttribute during processChunk.""" + graph: Graph = graphSavedOnDisk + node = graph.addNewNode(NodeWithDynamicListOutput.__name__) + + node.listInput.value = ["hello", "world"] + + # Execute the node + node.process(inCurrentEnv=True) + + # After processChunk, raw values are stored for serialization (thread-safe). + # getPrimitiveValue returns the raw values without requiring QObject children. + assert node.listOutput.getPrimitiveValue() == ["HELLO", "WORLD"] + + # loadOutputAttr materializes QObject children (normally on the main thread) + node.loadOutputAttr() + outputValues = [attr.value for attr in node.listOutput.value] + assert outputValues == ["HELLO", "WORLD"] + + def test_loadGraphWithComputedDynamicListOutput(self, graphSavedOnDisk): + """Check that dynamic list output values are persisted and reloaded correctly.""" + graph: Graph = graphSavedOnDisk + node = graph.addNewNode(NodeWithDynamicListOutput.__name__) + name = node.name + + node.listInput.value = ["foo", "bar", "baz"] + graph.save() + + # Execute the node + node.process(inCurrentEnv=True) + + # Verify raw output was stored + assert node.listOutput.getPrimitiveValue() == ["FOO", "BAR", "BAZ"] + + # Reload the graph — loadOutputAttr populates the ListModel on the main thread + loadedGraph = loadGraph(graph.filepath) + loadedNode = loadedGraph.node(name) + + assert loadedNode + loadedOutputValues = [attr.value for attr in loadedNode.listOutput.value] + assert loadedOutputValues == ["FOO", "BAR", "BAZ"] + + def test_loadGraphWithUncomputedDynamicListOutput(self, graphSavedOnDisk): + """Check that an uncomputed dynamic list output is empty after loading.""" + graph: Graph = graphSavedOnDisk + node = graph.addNewNode(NodeWithDynamicListOutput.__name__) + graph.save() + + loadedGraph = loadGraph(graph.filepath) + loadedNode = loadedGraph.node(node.name) + + assert loadedNode + # Uncomputed dynamic list output should be empty + assert len(loadedNode.listOutput) == 0