From 2cd724f95769601e85ce051ffb09305a78ef942f Mon Sep 17 00:00:00 2001 From: Yann Lanthony Date: Wed, 4 Apr 2018 17:29:04 +0200 Subject: [PATCH] [graph] ListAttribute: replace '__getitem__ by 'at' __getitem__ is spuriously called when the object is used inside a Qt model and exposed to QML, leading to unexpected behaviors --- meshroom/core/graph.py | 11 +++++++---- meshroom/ui/graph.py | 2 +- meshroom/ui/reconstruction.py | 6 +++--- tests/test_multiviewPipeline.py | 28 ++++++++++++++-------------- 4 files changed, 25 insertions(+), 22 deletions(-) diff --git a/meshroom/core/graph.py b/meshroom/core/graph.py index 68061cd9..be6ce070 100644 --- a/meshroom/core/graph.py +++ b/meshroom/core/graph.py @@ -285,12 +285,15 @@ class ListAttribute(Attribute): super(ListAttribute, self).__init__(node, attributeDesc, isOutput, root, parent) self._value = ListModel(parent=self) - def __getitem__(self, item): - return self._value.at(item) - def __len__(self): return len(self._value) + def at(self, idx): + """ Returns child attribute at index 'idx' """ + # implement 'at' rather than '__getitem__' + # since the later is called spuriously when object is used in QML + return self._value.at(idx) + def _set_value(self, value): self.desc.validateValue(value) self._value.clear() @@ -317,7 +320,7 @@ class ListAttribute(Attribute): def remove(self, index, count=1): if self.node.graph: for i in range(index, index + count): - attr = self[i] + attr = self._value.at(i) if attr.isLink: self.node.graph.removeEdge(attr) # delete edge if the attribute is linked self._value.removeAt(index, count) diff --git a/meshroom/ui/graph.py b/meshroom/ui/graph.py index 5b46cb32..e12b639d 100644 --- a/meshroom/ui/graph.py +++ b/meshroom/ui/graph.py @@ -259,7 +259,7 @@ class UIGraph(QObject): if isinstance(dst, graph.ListAttribute): with self.groupedGraphModification("Insert and Add Edge on {}".format(dst.fullName())): self.appendAttribute(dst) - self.push(commands.AddEdgeCommand(self._graph, src, dst[-1])) + self.push(commands.AddEdgeCommand(self._graph, src, dst.at(-1))) else: self.push(commands.AddEdgeCommand(self._graph, src, dst)) diff --git a/meshroom/ui/reconstruction.py b/meshroom/ui/reconstruction.py index a73f85eb..f6e4c5c1 100755 --- a/meshroom/ui/reconstruction.py +++ b/meshroom/ui/reconstruction.py @@ -122,7 +122,7 @@ class LiveSfmManager(QObject): def imagePathsInCameraInit(self, node): """ Get images in the given CameraInit node. """ assert node.nodeType == 'CameraInit' - return [vp.path.value for vp in node.viewpoints] + return [vp.path.value for vp in node.viewpoints.value] def imagesInStep(self): """ Get images in the current augmentation step. """ @@ -304,11 +304,11 @@ class Reconstruction(UIGraph): def allImagePaths(self): """ Get all image paths in the reconstruction. """ - return [vp.path.value for node in self._cameraInits for vp in node.viewpoints] + return [vp.path.value for node in self._cameraInits for vp in node.viewpoints.value] def allViewIds(self): """ Get all view Ids involved in the reconstruction. """ - return [vp.viewId.value for node in self._cameraInits for vp in node.viewpoints] + return [vp.viewId.value for node in self._cameraInits for vp in node.viewpoints.value] @Slot(QObject, graph.Node) def handleFilesDrop(self, drop, cameraInit): diff --git a/tests/test_multiviewPipeline.py b/tests/test_multiviewPipeline.py index 1b1e08d0..c85e3729 100644 --- a/tests/test_multiviewPipeline.py +++ b/tests/test_multiviewPipeline.py @@ -17,10 +17,10 @@ def test_multiviewPipeline(): {'path': '/non/existing/file2', 'intrinsicId': 55} ]) - assert graph1.findNode('CameraInit').viewpoints[0].path.value == '/non/existing/fileA' + assert graph1.findNode('CameraInit').viewpoints.at(0).path.value == '/non/existing/fileA' assert len(graph2.findNode('CameraInit').viewpoints) == 0 - assert graph3.findNode('CameraInit').viewpoints[0].path.value == '/non/existing/file1' - assert graph4.findNode('CameraInit').viewpoints[0].path.value == '/non/existing/file1' + assert graph3.findNode('CameraInit').viewpoints.at(0).path.value == '/non/existing/file1' + assert graph4.findNode('CameraInit').viewpoints.at(0).path.value == '/non/existing/file1' assert len(graph1.findNode('CameraInit').viewpoints) == 1 assert len(graph2.findNode('CameraInit').viewpoints) == 0 @@ -28,15 +28,15 @@ def test_multiviewPipeline(): assert len(graph4.findNode('CameraInit').viewpoints) == 2 viewpoints = graph3.findNode('CameraInit').viewpoints - assert viewpoints[0].path.value == '/non/existing/file1' + assert viewpoints.at(0).path.value == '/non/existing/file1' - assert viewpoints[0].path.value == '/non/existing/file1' - assert viewpoints[0].intrinsicId.value == -1 - assert viewpoints[1].path.value == '/non/existing/file2' - assert viewpoints[1].intrinsicId.value == -1 + assert viewpoints.at(0).path.value == '/non/existing/file1' + assert viewpoints.at(0).intrinsicId.value == -1 + assert viewpoints.at(1).path.value == '/non/existing/file2' + assert viewpoints.at(1).intrinsicId.value == -1 - assert viewpoints[0].path.isDefault == False - assert viewpoints[0].intrinsicId.isDefault == True + assert not viewpoints.at(0).path.isDefault + assert viewpoints.at(0).intrinsicId.isDefault assert viewpoints.getPrimitiveValue(exportDefault=False) == [ {"path": '/non/existing/file1'}, {"path": '/non/existing/file2'}, @@ -44,10 +44,10 @@ def test_multiviewPipeline(): for graph in (graph4, graph4b): viewpoints = graph.findNode('CameraInit').viewpoints - assert viewpoints[0].path.value == '/non/existing/file1' - assert viewpoints[0].intrinsicId.value == 50 - assert viewpoints[1].path.value == '/non/existing/file2' - assert viewpoints[1].intrinsicId.value == 55 + assert viewpoints.at(0).path.value == '/non/existing/file1' + assert viewpoints.at(0).intrinsicId.value == 50 + assert viewpoints.at(1).path.value == '/non/existing/file2' + assert viewpoints.at(1).intrinsicId.value == 55 # Ensure that all output UIDs are different as the input is different: # graph1 != graph2 != graph3 != graph4