Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2979 +/- ##
===========================================
+ Coverage 81.09% 81.37% +0.28%
===========================================
Files 66 67 +1
Lines 8858 9022 +164
===========================================
+ Hits 7183 7342 +159
- Misses 1675 1680 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds support for backdrop nodes in the graph editor, enabling users to visually group and organize nodes with resizable backdrop containers. It replaces PR #2574.
Changes:
- Introduces a new BackdropNode type at both the core (Python) and UI (QML) levels
- Adds resizing functionality for backdrop nodes with drag handles for all four sides
- Implements automatic child node selection when backdrop nodes are selected or dragged
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| meshroom/ui/qml/GraphEditor/qmldir | Registers the new Backdrop.qml component |
| meshroom/ui/qml/GraphEditor/Backdrop.qml | New component implementing the visual representation and interaction logic for backdrop nodes |
| meshroom/ui/qml/GraphEditor/GraphEditor.qml | Updates node instantiation to use Loader pattern supporting both Node and Backdrop components; removes semicolons for consistency |
| meshroom/ui/qml/GraphEditor/NodeEditor.qml | Adds conditional UI handling for backdrop nodes (hides attributes tab, shows only notes/documentation) |
| meshroom/ui/qml/GraphEditor/Node.qml | Adds isBackdropNode property for type checking |
| meshroom/ui/qml/Controls/DelegateSelectionBox.qml | Adds logic to automatically select backdrop children when backdrop is selected |
| meshroom/ui/graph.py | Implements resizeNode and resizeAndMoveNode methods for backdrop manipulation; adds BackdropNode import |
| meshroom/ui/components/geom2D.py | Adds rectRectFullIntersect method for checking if one rectangle fully contains another |
| meshroom/nodes/general/Backdrop.py | Defines the Backdrop node plugin |
| meshroom/core/nodeFactory.py | Adds getNodeConstructor function to select appropriate node class (BackdropNode vs Node) |
| meshroom/core/node.py | Implements BackdropNode class with resizing properties (nodeWidth, nodeHeight, fontSize) |
| meshroom/core/graph.py | Updates node creation to use getNodeConstructor |
| meshroom/core/desc/node.py | Adds BACKDROP MrNodeType, BackdropNode descriptor, and InternalAttributesFactory for managing node internal attributes |
| meshroom/core/desc/init.py | Exports new BackdropNode and InternalAttributesFactory classes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2fac1c7 to
78941e3
Compare
78941e3 to
7025ce9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Great work! Honestly on the UI side I didn't have any issue, it's really great to have this too !
I have though some small remarks and bugs to report :
- The NodeActions disappeared :(((
- I would love to be able to make a horizontal+vertical resize (like the diagonal control)
- To select nodes with the ctrl+click (so only nodes without backdrop) we need to click twice
- If the text is too long it goes outside of the backdrop, maybe this is okay or we want to clip the text ouside of the backdrop
- I think we should put the backdrop on top of the menu and not on the Utils submenu
- The compute/submit doesn't work well it should remove the Backdrop node from the list of nodes to create, now it raises an issue because of the chunk creation process
- Same issues with cut/copy : there are either issues in the QML or directly bugs in the python side (see comments for the code review that will come next)
- It would be cool to have some additional features (to do in a future PRs though) :
- a "fit nodes" button so that the bckdrop is resized to fir the nodes inside
- if we have selected nodes when we create the backdrop the backdrop could be created in a way so it fits the nodes
- We could use ctrl+click+drag to draw the backdrop : if no node is selected, we press ctrl and click on the graph editor then drag and it draws the rectangle for the backdrop
7025ce9 to
b3f8124
Compare
…invalidating attribute Following the changes regarding the internal inputs, `InputNodes` now do not have the `invalidation` internal attribute anymore. If their inputs/outputs are not taken into account in the UID computation, their UID may not be serialized, which leaves them open to an automatic upgrade if they ever are in Compatibility Mode.
This allows to distinguish between the regular `Node` constructor, which was used up until now, and the `BackdropNode` constructor, which is to be used if the node type is "Backdrop".
Co-authored-by: waaake <vivek_ve@outlook.com>
Co-authored-by: waaake <vivek_ve@outlook.com>
…nodes Co-authored-by: waaake <vivek_ve@outlook.com>
- Remove useless `isBackdropNode` property in Node.qml - Remove useless `isBackdrop` property in Backdrop.qml - Use `getItemAt` accessor when relevant - Fix typos in property names and comments - Remove semicolons when they are not needed
b3f8124 to
796d5a7
Compare
`BackdropNodes` have no input/output attributes, only internal ones. Before attempting to serialize the input attributes, we thus need to check that the node possesses any.
Instead of using the `Toggle` mode, which would select then unselect the backdrop on every click, we use `Clear`, which correctly selects the nodes without ever selecting the backdrop. This allows to get the expected behaviour.
|
Could be useful to add a text color parameter (or handle the color based on luminance), to handle cases where user choose a dark background color ? |
This is done! A "Font Color" internal attribute has been added so the user can set the font color at the same time they set the font size (it is also easier to handle than checking the luminance and adapting the color to it).
All the remarks have been addressed except for the one suggesting to move the |
The QML component for `ColorParam` attribute was mistakenly based on the node's `color` internal attribute rather than the attribute it was instantiated for.
310726c to
d1f0f83
Compare
Alxiice
left a comment
There was a problem hiding this comment.
Small bug :
ERROR:root:Error while undoing command 'Disconnect 'SleepNode_9.output' -> 'SleepNode_8.inputs[0]'':
Traceback (most recent call last):
File "/s/apps/users/sonoleta/packages/meshroom/asdev2/repo/meshroom/ui/commands.py", line 36, in undo
self.undoImpl()
File "/s/apps/users/sonoleta/packages/meshroom/asdev2/repo/meshroom/ui/commands.py", line 486, in undoImpl
edge[0].connectTo(edge[1])
File "/s/apps/users/sonoleta/packages/meshroom/asdev2/repo/meshroom/core/attribute.py", line 551, in connectTo
connectedEdge, deletedEdge = graph.addEdge(self, dstAttribute)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/s/apps/users/sonoleta/packages/meshroom/asdev2/repo/meshroom/core/graph.py", line 157, in decorator
result = func(self, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/s/apps/users/sonoleta/packages/meshroom/asdev2/repo/meshroom/core/graph.py", line 937, in addEdge
if not srcAttr.node.graph == dstAttr.node.graph == self:
^^^^^^^^^^^^^^^^^^
AttributeError: 'PySide6.QtCore.Property' object has no attribute 'graph'
So I have both good and bad news about this: the good one is that it is not related to this specific PR 😀 The bad one is that it is an issue that's already on the "develop" branch (and should thus preferably be fixed with its own PR). |


Description
This PR is a replacement for #2574.
It introduces a new "Backdrop" node type to Meshroom's core and UI, allowing users to visually group nodes in the graph editor.
Backdrops are a specific type of node that can be instantiated as any other node type, but have no input or output attributes. They only possess internal attributes, including their size (width and height) as they can be resized. Backdrops can be given custom names, colors, and display any comment that was added by the user (with an adjustable font size).
Visually, backdrops are always placed behind any non-backdrop node they may be overlapping with. Whenever a node is fully contained within a backdrop node, it is automatically "attached" to it: moving the backdrop will move the node as well. Conversely, moving a node that is contained within a backdrop will not move the backdrop. As selecting a backdrop adds the nodes that are contained in it to the selection, two new controls are added when clicking on a backdrop:
Features list
BackdropNodetype of node which is non-computable and only has internal attributes.Backdropnode description to make backdrops instantiable.BackdropQML component.Implementation remarks
Backdrop Node Support:
BackdropNodeclass in bothdesc.nodeandcore.node, representing a non-computable node for grouping other nodes visually. The Backdrop node has its own internal attributes and serialization logic. (F2d2396aL336R336, [1] [2]getNodeConstructorutility. [1] [2] [3] [4] [5] [6]MrNodeTypeenum and related type-handling logic to recognize the new BACKDROP node type. [1] [2] [3] [4] [5] [6]Node Internal Attributes & Display Properties:
InternalAttributesFactory, with attribute sets for basic, invalidation, and resizable (font size, width, height) properties. [1] [2]NodeforfontSize,nodeWidth, andnodeHeight, enabling UI customization for nodes, especially Backdrop nodes. [1] [2]isBackdropNodeproperty. [1] [2] [3]