Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions meshroom/core/node.py
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,15 @@ def getColor(self):
return self.internalAttribute("color").value.strip()
return ""

def setColor(self, color: str):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regards to what this PR is addressing, adding the setter for this property is not needed.

""" Sets the color of the Node.

Args:
color (str): The hex of the color to set on the node.
"""
if self.hasInternalAttribute("color"):
self.internalAttribute("color").value = color

def getInvalidationMessage(self):
"""
Returns:
Expand Down Expand Up @@ -967,7 +976,7 @@ def _onAttributeChanged(self, attr: Attribute):
if attr.value is None:
# Discard dynamic values depending on the graph processing.
return

if self.graph and self.graph.isLoading:
# Do not trigger attribute callbacks during the graph loading.
return
Expand Down Expand Up @@ -1356,7 +1365,7 @@ def hasSequenceOutputAttribute(self):
False otherwise.
"""
for attr in self._attributes:
if attr.enabled and attr.isOutput and (attr.desc.semantic == "sequence" or
if attr.enabled and attr.isOutput and (attr.desc.semantic == "sequence" or
attr.desc.semantic == "imageList"):
return True
return False
Expand Down Expand Up @@ -1386,7 +1395,7 @@ def has3DOutputAttribute(self):
internalAttributes = Property(BaseObject, getInternalAttributes, constant=True)
internalAttributesChanged = Signal()
label = Property(str, getLabel, notify=internalAttributesChanged)
color = Property(str, getColor, notify=internalAttributesChanged)
color = Property(str, getColor, setColor, notify=internalAttributesChanged)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same as above) Adding the setter is not needed in this scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been removed.

invalidation = Property(str, getInvalidationMessage, notify=internalAttributesChanged)
comment = Property(str, getComment, notify=internalAttributesChanged)
internalFolderChanged = Signal()
Expand Down Expand Up @@ -1909,7 +1918,7 @@ def nodeFactory(nodeDict, name=None, template=False, uidConflict=False):
# do not perform that check for internal attributes because there is no point in
# raising compatibility issues if their number differs: in that case, it is only useful
# if some internal attributes do not exist or are invalid
if not template and (sorted([attr.name for attr in nodeDesc.inputs
if not template and (sorted([attr.name for attr in nodeDesc.inputs
if not isinstance(attr, desc.PushButtonParam)]) != sorted(inputs.keys()) or
sorted([attr.name for attr in nodeDesc.outputs if not attr.isDynamicValue]) !=
sorted(outputs.keys())):
Expand Down
48 changes: 48 additions & 0 deletions meshroom/ui/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,54 @@ def undoImpl(self):
self.graph.removeNode(duplicate)


class UpdateNodeColorCommand(GraphCommand):
""" Command representing the work for Coloring nodes in the Graph.
"""

def __init__(self, graph, nodes, color, parent=None):
""" Command Constructor.

Args:
graph (meshroom.core.Graph): Current Graph instance.
nodes (list<Node>): Array of nodes.
color (str): Hex code for the color.

Keyword Args:
parent (QObject): Parent QObject instance.
"""
super(UpdateNodeColorCommand, self).__init__(graph, parent)
self._nodes = nodes
self._color = color

# Existing colors
# Map <Node: str>
# Holds the existing color of the node which can be applied on the node back in case of an undo
self._colorMap = {}

self.setText("Update Selected Node Color")

def redoImpl(self) -> bool:
""" Redo implementation.
"""
for node in self._nodes:
# Update the existing color for the node in the map
self._colorMap[node] = node.color

# Now update the color of the node with the provided one
node.color = self._color

return True

def undoImpl(self):
""" Undo Implementation.
"""
with GraphModification(self.graph):
# Revert the color for the nodes
for node in self._nodes:
# Get the color which was saved for the node
node.color = self._colorMap.get(node)


class PasteNodesCommand(GraphCommand):
"""
Handle node pasting in a Graph.
Expand Down
11 changes: 11 additions & 0 deletions meshroom/ui/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -952,6 +952,17 @@ def selectFollowing(self, node):
""" Select all the nodes the depend on 'node'. """
self.selectNodes(self._graph.dfsOnDiscover(startNodes=[node], reverse=True, dependenciesOnly=True)[0])

@Slot(str)
def updateNodeColor(self, color):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this method is working on the node selection (which makes sense), I would recommend:

Suggested change
def updateNodeColor(self, color):
def setSelectedNodesColor(self, color: str):

""" Sets the Provided color on the selected Nodes.

Args:
color (str): Hex code of the color to be set on the nodes.
"""
# Update the color attribute of the nodes which are currently selected
with self.groupedGraphModification("Update Color for Select Nodes"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the command name that appears in the undo stack:

Suggested change
with self.groupedGraphModification("Update Color for Select Nodes"):
with self.groupedGraphModification("Set Nodes Color"):

I think we could go with removing the notion of "selected" in those, as selection operations are not undoable - therefore, undoing "Set Selected Nodes Color" is a bit ambiguous if the selection has changed in between.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, makes sense, I probably did not notice this while working on the previous comments. Thanks for the suggestion.

self.push(commands.UpdateNodeColorCommand(self._graph, list(self._selectedNodes), color))

@Slot(QObject, QObject)
def boxSelect(self, selection, draggable):
"""
Expand Down
133 changes: 133 additions & 0 deletions meshroom/ui/qml/Controls/ColorSelector.qml
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
import QtQuick
import QtQuick.Controls

import Utils 1.0
import MaterialIcons 2.2

/**
* ColorSelector is a color picker based on a set of predefined colors.
* It takes the form of a ToolButton that pops-up its palette when pressed.
*/
MaterialToolButton {
id: root

text: MaterialIcons.palette

// Internal property holding when the popup remains visible and when is it toggled off
property var isVisible: false

property var colors: [
"#FF6600",
"#FFAD7D",
"#FFF82F",
"#FFFB8B",
"#BDFF07",
"#E8FF97",
"#0BFFCA",
"#8EFFF7",
"#1158FF",
"#77A7FF",
"#9318FF",
"#EAACFF",
"#B61518",
"#FF989A",
]

// When a color gets selected/choosen
signal colorSelected(var color)

// Toggles the visibility of the popup
onPressed: toggle()

function toggle() {
/*
* Toggles the visibility of the color palette.
*/
if (!isVisible) {
palettePopup.open()
isVisible = true
}
else {
palettePopup.close()
isVisible = false
}
}

// Popup for the color palette
Popup {
id: palettePopup

// The popup will not get closed unless explicitly closed
closePolicy: Popup.NoAutoClose

// Bounds
padding: 4
width: (root.height * 4) + (padding * 4)

// center the current color
y: -height
x: -width + root.width + padding

// Layout of the Colors
Grid {
// Allow only 4 columns and all the colors can be adjusted in row multiples of 4
columns: 4

spacing: 2
anchors.centerIn: parent

// Default -- Reset Colour button
ToolButton {
id: defaultButton
padding: 0
width: root.height
height: root.height

// Emit no color so the graph sets None as the color of the Node
onClicked: {
root.colorSelected("")
}

background: Rectangle {
color: "#FFFFFF"
// display border of current/selected item
border.width: defaultButton.hovered ? 1 : 0
border.color: "#000000"

// Another Rectangle
Rectangle {
color: "#FF0000"
width: parent.width + 8
height: 2
anchors.centerIn: parent
rotation: 135 // Diagonally creating a Red line from bottom left to top right
}
}
}

// Colors palette
Repeater {
model: root.colors
// display each color as a ToolButton with a custom background
delegate: ToolButton {
padding: 0
width: root.height
height: root.height

// Emit the model data as the color to update
onClicked: {
colorSelected(modelData)
}

// Model color as the background of the button
background: Rectangle {
color: modelData
// display border of current/selected item
border.width: hovered ? 1 : 0
border.color: "#000000"
}
}
}
}
}
}
1 change: 1 addition & 0 deletions meshroom/ui/qml/Controls/qmldir
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module Controls

ColorChart 1.0 ColorChart.qml
ColorSelector 1.0 ColorSelector.qml
ExpandableGroup 1.0 ExpandableGroup.qml
FloatingPane 1.0 FloatingPane.qml
Group 1.0 Group.qml
Expand Down
20 changes: 20 additions & 0 deletions meshroom/ui/qml/GraphEditor/GraphEditor.qml
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ Item {
Keys.onPressed: function(event) {
if (event.key === Qt.Key_F) {
fit()
} else if (event.key === Qt.Key_C) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This takes precedence over the copy shortcut (Ctrl+C).
We need better handling of keyboard shortcuts, but in the meantime, this should either test if no modifier are active, or be moved at after the copy action handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's my bad, I did not notice something as simple as this. Have updated the code to consider both cases.

colorSelector.toggle()
} else if (event.key === Qt.Key_Delete) {
if (event.modifiers === Qt.AltModifier) {
uigraph.removeNodesFrom(uigraph.selectedNodes)
Expand Down Expand Up @@ -1048,6 +1050,24 @@ Item {
}
}
}

// Separator
Rectangle {
Layout.fillHeight: true
Layout.margins: 2
implicitWidth: 1
color: activePalette.window
}

ColorSelector {
id: colorSelector
Layout.minimumWidth: colorSelector.width

// When a Color is selected
onColorSelected: (color)=> {
uigraph.updateNodeColor(color)
}
}
}
}

Expand Down
Loading