[ui] add support for selecting multiple nodes at once #1227

ChemicalXandco · 2021-01-15T20:33:28Z

Description

The ability to select multiple nodes at once improves the efficiency of using the graph editor. I have tried to make the node selection behave similar to blender. The box select widget is used by dragging with the left mouse button. The main selected node is slightly brighter than the other selected nodes and has a blue border.

Features list

select multiple nodes at once
use of control key modifier to select multiple nodes
use of box select widget to select multiple nodes
duplicate selected nodes
remove selected nodes
delete data of selected nodes
move selected nodes together

(Resolves some points on #269)

cursor is only 'closed hand' when scrolling the graph

fabiencastan · 2021-01-22T22:24:48Z

I often get this error when manipulating nodes in the GraphEditor:

Traceback (most recent call last):
  File "C:\dev\meshroom\meshroom\common\qt.py", line 190, in remove
    raise ValueError("QObjectListModel.remove(obj) : obj not in list")
ValueError: QObjectListModel.remove(obj) : obj not in list
[2021-01-22 23:22:10,394][WARNING] file:///C:/dev/meshroom/meshroom/ui/qml/GraphEditor/GraphEditor.qml:446: Error: QObjectListModel.remove(obj) : obj not in list

… selected

fabiencastan

Thanks for the really cool PR!
I have added few notes to clean the code before integration.

fabiencastan · 2021-01-25T09:38:29Z

meshroom/ui/graph.py

+            for node in self._selectedNodes:
+                position = Position(node.x + deltaX, node.y + deltaY)
+                self.push(commands.MoveNodeCommand(self._graph, node, position))
+
    @Slot(Node)
    def removeNode(self, node):


The check on locked makes sense, but should we really care about the notion of selection here?
The function is called removeNode and takes a node in parameter, so it should just do the action.

fabiencastan · 2021-01-25T09:39:13Z

meshroom/ui/graph.py

+                if not node.locked:
+                    self.push(commands.RemoveNodeCommand(self._graph, node))


Suggested change

if not node.locked:

self.push(commands.RemoveNodeCommand(self._graph, node))

self.removeNode(node)

fabiencastan · 2021-01-25T09:40:14Z

meshroom/ui/graph.py

+                if not node.locked:
+                    self.removeNode(node)


Suggested change

if not node.locked:

self.removeNode(node)

self.removeNode(node)

Already checked in the function.

fabiencastan · 2021-01-25T09:47:40Z

meshroom/ui/graph.py

@@ -569,19 +599,33 @@ def duplicateNode(self, srcNode, duplicateFollowingNodes=False):
        Returns:
            [Nodes]: the list of duplicated nodes
        """
-        title = "Duplicate Nodes from {}" if duplicateFollowingNodes else "Duplicate {}"
+        if duplicateFollowingNodes: title = "Duplicate Nodes from {}"
+        elif self.nodeSelection(srcNode): title = "Duplicate selected nodes"


We should not have links with the notion of node selection at this level. The UI use the selection to determine the list of nodes. Then we have the list of nodes and perform actions on it. But the low-level functions should not check the notion of selection again.

The function is explicit: duplicateNode(srcNode, duplicateFollowingNodes=False). We don't expect that to check the notion of selection.

fabiencastan · 2021-01-25T09:48:14Z

meshroom/ui/graph.py

        # enable updates between duplication and layout to get correct depths during layout
        with self.groupedGraphModification(title.format(srcNode.name), disableUpdates=False):
            # disable graph updates during duplication
            with self.groupedGraphModification("Node duplication", disableUpdates=True):
-                duplicates = self.push(commands.DuplicateNodeCommand(self._graph, srcNode, duplicateFollowingNodes))
+                if self.nodeSelection(srcNode) and not duplicateFollowingNodes:


same, no notion of selection here.

fabiencastan · 2021-01-25T09:48:43Z

meshroom/ui/graph.py

            # move nodes below the bounding box formed by the duplicated node(s)
            bbox = self._layout.boundingBox(duplicates)
            for n in duplicates:
                self.moveNode(n, Position(n.x, bbox[3] + self.layout.gridSpacing + n.y))

        return duplicates

+    @Slot(QObject)
+    def clearData(self, node):
+        if self.nodeSelection(node):


same, no notion of selection here.

fabiencastan · 2021-01-25T09:49:26Z

meshroom/ui/graph.py

    def clearNodeSelection(self):
        """ Clear node selection. """
        self.selectedNode = None

+    @Slot()
+    def clearNodesSelections(self):


Suggested change

def clearNodesSelections(self):

def clearNodesSelection(self):

fabiencastan · 2021-01-25T10:14:58Z

meshroom/ui/commands.py

-        for nodeName in self.duplicates:
-            self.graph.removeNode(nodeName)
+
+class DuplicateNodeListCommand(_DuplicateNodes):


Don't need to have 2 classes to do it with one of multiple nodes and inheritance, etc.
Either we keep DuplicateNodeListCommand and always use it (even for a single node) or keep DuplicateNodeCommand and group the commands for multiple nodes.

fabiencastan · 2021-04-13T19:54:27Z

Small behavior problem in this configuration (duplicate nodes and nodes behind):

It gives:

While the simple duplicate nodes:

Gives the correct result but with a strange selection:

fabiencastan · 2021-04-13T19:57:18Z

It would be great the replace the "Alt+click" to select all the nodes after (instead of duplicating the nodes).
So if we want to move a node and all nodes connected behind it (a fairly common need), it would be simple.

fabiencastan · 2021-04-13T23:06:30Z

Regarding the selection, I think we should clear the selection and select all the newly created nodes.

fabiencastan · 2021-04-13T23:09:32Z

Really great PR, this is a life changer!
Looking forward to be able to merge it.

…on alt + left click

ChemicalXandco · 2021-04-16T14:21:57Z

Small behavior problem in this configuration (duplicate nodes and nodes behind)

I agree that this does not behave right. Should I remove the options '_ from here' or should I rework them to include the other selected nodes? I think that removing those options and relying on alt + click to select the following nodes is more intuitive than having these options that operate on some nodes that are not selected. What do you think @fabiencastan?

fabiencastan · 2021-04-16T14:50:27Z

Good question. I agree that's redundant and makes two ways to do the same.
But, on the other hand, I think it's still useful to have the "from here" option as it's visual and do not require to know the keyboard shortcuts.
Also alt/shift/ctrl can be problematic in remote, or on some linux alt could be used by the os to move the window, etc.

…e' operations

natowi · 2021-04-20T20:38:49Z

Love it :)

Only a few minor layout issues #1389 (nodes overlap and auto layout problems), but it might be too much for this PR

fabiencastan · 2021-04-23T11:39:10Z

@ChemicalXandco Is it ready for review? Or are you still working on it?

ChemicalXandco · 2021-04-23T16:13:42Z

It is ready

fabiencastan · 2021-05-03T21:42:04Z

Great work!

ChemicalXandco added 2 commits January 15, 2021 18:16

[ui] add support for selecting multiple nodes at once

c253e7d

[ui] GraphEditor: fix cursor shape

6f50f65

cursor is only 'closed hand' when scrolling the graph

natowi added feature new feature (proposed as PR or issue planned by dev) UI labels Jan 16, 2021

ChemicalXandco mentioned this pull request Jan 18, 2021

Infinite drag #993

Closed

[ui] GraphEditor: fix error when node is clicked with control and not…

aef50d9

… selected

fabiencastan requested changes Jan 25, 2021

View reviewed changes

ChemicalXandco added 2 commits January 26, 2021 21:13

[ui] grapheditor: explicitly pass selected nodes argument

acf1bf2

[tests] update node duplication test

55b16bc

[ui] GraphEditor: select duplicated nodes and select following nodes …

e8a5491

…on alt + left click

natowi mentioned this pull request Apr 16, 2021

Preferences menu #964

Closed

[ui] GraphEditor: use entire node selection when handling '_ from her…

fdfabf0

…e' operations

This was referenced Apr 20, 2021

node duplication error in the pipeline #1389

Closed

Improvements: Graph Editor #269

Open

fabiencastan added this to the Meshroom 2021.2.0 milestone May 3, 2021

fabiencastan approved these changes May 3, 2021

View reviewed changes

fabiencastan merged commit 18be350 into alicevision:develop May 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ui] add support for selecting multiple nodes at once #1227

[ui] add support for selecting multiple nodes at once #1227

ChemicalXandco commented Jan 15, 2021

fabiencastan commented Jan 22, 2021

fabiencastan left a comment

fabiencastan Jan 25, 2021

fabiencastan Jan 25, 2021

fabiencastan Jan 25, 2021

fabiencastan Jan 25, 2021

fabiencastan Jan 25, 2021

fabiencastan Jan 25, 2021

fabiencastan Jan 25, 2021

fabiencastan Jan 25, 2021

fabiencastan commented Apr 13, 2021

fabiencastan commented Apr 13, 2021

fabiencastan commented Apr 13, 2021

fabiencastan commented Apr 13, 2021

ChemicalXandco commented Apr 16, 2021

fabiencastan commented Apr 16, 2021

natowi commented Apr 20, 2021 •

edited

Loading

fabiencastan commented Apr 23, 2021

ChemicalXandco commented Apr 23, 2021

fabiencastan commented May 3, 2021

		if not node.locked:
		self.push(commands.RemoveNodeCommand(self._graph, node))

	if not node.locked:
	self.push(commands.RemoveNodeCommand(self._graph, node))
	self.removeNode(node)

	def clearNodesSelections(self):
	def clearNodesSelection(self):

[ui] add support for selecting multiple nodes at once #1227

[ui] add support for selecting multiple nodes at once #1227

Conversation

ChemicalXandco commented Jan 15, 2021

Description

Features list

fabiencastan commented Jan 22, 2021

fabiencastan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiencastan commented Apr 13, 2021

fabiencastan commented Apr 13, 2021

fabiencastan commented Apr 13, 2021

fabiencastan commented Apr 13, 2021

ChemicalXandco commented Apr 16, 2021

fabiencastan commented Apr 16, 2021

natowi commented Apr 20, 2021 • edited Loading

fabiencastan commented Apr 23, 2021

ChemicalXandco commented Apr 23, 2021

fabiencastan commented May 3, 2021

natowi commented Apr 20, 2021 •

edited

Loading