review comments fixed

Co-authored-by: Casper Lamboo <c.lamboo@ultimaker.com>

CURA-7951

cura/Arranging/ArrangeObjectsJob.py

@@ -16,7 +16,7 @@ i18n_catalog = i18nCatalog("cura")
 
 class ArrangeObjectsJob(Job):
     def __init__(self, nodes: List[SceneNode], fixed_nodes: List[SceneNode], min_offset = 8,
-                 grid_arrange: bool = False) -> None:
+                *, grid_arrange: bool = False) -> None:
         super().__init__()
         self._nodes = nodes
         self._fixed_nodes = fixed_nodes
@@ -33,13 +33,7 @@ class ArrangeObjectsJob(Job):
         status_message.show()
 
         try:
-
+            found_solution_for_all = arrange(self._nodes, Application.getInstance().getBuildVolume(), self._fixed_nodes, grid_arrange= self._grid_arrange)
-            if self._grid_arrange:
-                grid_arrange = GridArrange(self._nodes, Application.getInstance().getBuildVolume(), self._fixed_nodes)
-                found_solution_for_all = grid_arrange.arrange()
-
-            else:
-                found_solution_for_all = arrange(self._nodes, Application.getInstance().getBuildVolume(), self._fixed_nodes)
 
         except:  # If the thread crashes, the message should still close
             Logger.logException("e", "Unable to arrange the objects on the buildplate. The arrange algorithm has crashed.")

cura/Arranging/GridArrange.py

@@ -1,6 +1,11 @@
 import math
 from typing import List, TYPE_CHECKING, Optional, Tuple, Set
 
+
+
+if TYPE_CHECKING:
+    from UM.Scene.SceneNode import SceneNode
+
 from UM.Application import Application
 from UM.Math import AxisAlignedBox
 from UM.Math.Vector import Vector
@@ -10,23 +15,16 @@ from UM.Operations.TranslateOperation import TranslateOperation
 
 
 class GridArrange:
-    offset_x: float = 10
+    def __init__(self, nodes_to_arrange: List["SceneNode"], build_volume: "BuildVolume", fixed_nodes: List["SceneNode"] = None):
-    offset_y: float = 10
+        if fixed_nodes is None:
-
+            fixed_nodes = []
-    _grid_width: float
-    _grid_height: float
-
-    _nodes_to_arrange: List["SceneNode"]
-    _fixed_nodes: List["SceneNode"]
-    _build_volume: "BuildVolume"
-    _build_volume_bounding_box: AxisAlignedBox
-
-    def __init__(self, nodes_to_arrange: List["SceneNode"], build_volume: "BuildVolume", fixed_nodes: List["SceneNode"] = []):
         self._nodes_to_arrange = nodes_to_arrange
         self._build_volume = build_volume
         self._build_volume_bounding_box = build_volume.getBoundingBox()
         self._fixed_nodes = fixed_nodes
 
+        self._offset_x: float = 10
+        self._offset_y: float = 10
         self._grid_width = 0
         self._grid_height = 0
         for node in self._nodes_to_arrange:
@@ -40,11 +38,6 @@ class GridArrange:
         self._initial_leftover_grid_x = math.floor(self._initial_leftover_grid_x)
         self._initial_leftover_grid_y = math.floor(self._initial_leftover_grid_y)
 
-    def arrange(self)-> bool:
-        grouped_operation, not_fit_count = self.createGroupOperationForArrange()
-        grouped_operation.push()
-        return not_fit_count == 0
-
     def createGroupOperationForArrange(self) -> Tuple[GroupedOperation, int]:
         # Find grid indexes that intersect with fixed objects
         fixed_nodes_grid_ids = set()
@@ -96,8 +89,8 @@ class GridArrange:
 
     def moveNodeOnGrid(self, node: "SceneNode", grid_x: int, grid_y: int) -> "Operation.Operation":
         coord_grid_x, coord_grid_y = self.gridSpaceToCoordSpace(grid_x, grid_y)
-        center_grid_x = coord_grid_x + (0.5 * (self._grid_width + self.offset_x))
+        center_grid_x = coord_grid_x + (0.5 * (self._grid_width + self._offset_x))
-        center_grid_y = coord_grid_y + (0.5 * (self._grid_height + self.offset_y))
+        center_grid_y = coord_grid_y + (0.5 * (self._grid_height + self._offset_y))
 
         bounding_box = node.getBoundingBox()
         center_node_x = (bounding_box.left + bounding_box.right) * 0.5
@@ -134,13 +127,13 @@ class GridArrange:
         return grid_idx
 
     def gridSpaceToCoordSpace(self, x: float, y: float) -> Tuple[float, float]:
-        grid_x = x * (self._grid_width + self.offset_x) + self._build_volume_bounding_box.left
+        grid_x = x * (self._grid_width + self._offset_x) + self._build_volume_bounding_box.left
-        grid_y = y * (self._grid_height + self.offset_y) + self._build_volume_bounding_box.back
+        grid_y = y * (self._grid_height + self._offset_y) + self._build_volume_bounding_box.back
         return grid_x, grid_y
 
     def coordSpaceToGridSpace(self, grid_x: float, grid_y: float) -> Tuple[float, float]:
-        coord_x = (grid_x - self._build_volume_bounding_box.left) / (self._grid_width + self.offset_x)
+        coord_x = (grid_x - self._build_volume_bounding_box.left) / (self._grid_width + self._offset_x)
-        coord_y = (grid_y - self._build_volume_bounding_box.back) / (self._grid_height + self.offset_y)
+        coord_y = (grid_y - self._build_volume_bounding_box.back) / (self._grid_height + self._offset_y)
         return coord_x, coord_y
 
     def checkGridUnderDiscSpace(self, grid_x: int, grid_y: int) -> bool:

cura/Arranging/Nest2DArrange.py

@@ -15,7 +15,7 @@ from UM.Operations.AddSceneNodeOperation import AddSceneNodeOperation
 from UM.Operations.GroupedOperation import GroupedOperation
 from UM.Operations.RotateOperation import RotateOperation
 from UM.Operations.TranslateOperation import TranslateOperation
-
+from cura.Arranging.GridArrange import GridArrange
 
 if TYPE_CHECKING:
     from UM.Scene.SceneNode import SceneNode
@@ -27,6 +27,7 @@ def findNodePlacement(
         build_volume: "BuildVolume",
         fixed_nodes: Optional[List["SceneNode"]] = None,
         factor: int = 10000,
+        *,
         lock_rotation: bool = False
 ) -> Tuple[bool, List[Item]]:
     """
@@ -124,30 +125,36 @@ def createGroupOperationForArrange(nodes_to_arrange: List["SceneNode"],
                                    build_volume: "BuildVolume",
                                    fixed_nodes: Optional[List["SceneNode"]] = None,
                                    factor: int = 10000,
+                                   *,
                                    add_new_nodes_in_scene: bool = False,
-                                   lock_rotation: bool = False) -> Tuple[GroupedOperation, int]:
+                                   lock_rotation: bool = False,
-    scene_root = Application.getInstance().getController().getScene().getRoot()
+                                   grid_arrange: bool = False) -> Tuple[GroupedOperation, int]:
-    found_solution_for_all, node_items = findNodePlacement(nodes_to_arrange, build_volume, fixed_nodes, factor,
+    if grid_arrange:
-                                                           lock_rotation)
+        grid = GridArrange(nodes_to_arrange, build_volume, fixed_nodes)
+        return grid.createGroupOperationForArrange()
+    else:
+        scene_root = Application.getInstance().getController().getScene().getRoot()
+        found_solution_for_all, node_items = findNodePlacement(nodes_to_arrange, build_volume, fixed_nodes, factor,
+                                                               lock_rotation = lock_rotation)
 
-    not_fit_count = 0
+        not_fit_count = 0
-    grouped_operation = GroupedOperation()
+        grouped_operation = GroupedOperation()
-    for node, node_item in zip(nodes_to_arrange, node_items):
+        for node, node_item in zip(nodes_to_arrange, node_items):
-        if add_new_nodes_in_scene:
+            if add_new_nodes_in_scene:
-            grouped_operation.addOperation(AddSceneNodeOperation(node, scene_root))
+                grouped_operation.addOperation(AddSceneNodeOperation(node, scene_root))
 
-        if node_item.binId() == 0:
+            if node_item.binId() == 0:
-            # We found a spot for it
+                # We found a spot for it
-            rotation_matrix = Matrix()
+                rotation_matrix = Matrix()
-            rotation_matrix.setByRotationAxis(node_item.rotation(), Vector(0, -1, 0))
+                rotation_matrix.setByRotationAxis(node_item.rotation(), Vector(0, -1, 0))
-            grouped_operation.addOperation(RotateOperation(node, Quaternion.fromMatrix(rotation_matrix)))
+                grouped_operation.addOperation(RotateOperation(node, Quaternion.fromMatrix(rotation_matrix)))
-            grouped_operation.addOperation(TranslateOperation(node, Vector(node_item.translation().x() / factor, 0,
+                grouped_operation.addOperation(TranslateOperation(node, Vector(node_item.translation().x() / factor, 0,
-                                                                           node_item.translation().y() / factor)))
+                                                                               node_item.translation().y() / factor)))
-        else:
+            else:
-            # We didn't find a spot
+                # We didn't find a spot
-            grouped_operation.addOperation(
+                grouped_operation.addOperation(
-                TranslateOperation(node, Vector(200, node.getWorldPosition().y, -not_fit_count * 20), set_position = True))
+                    TranslateOperation(node, Vector(200, node.getWorldPosition().y, -not_fit_count * 20), set_position = True))
-            not_fit_count += 1
+                not_fit_count += 1
 
     return grouped_operation, not_fit_count
 
@@ -158,7 +165,8 @@ def arrange(
         fixed_nodes: Optional[List["SceneNode"]] = None,
         factor=10000,
         add_new_nodes_in_scene: bool = False,
-        lock_rotation: bool = False
+        lock_rotation: bool = False,
+        grid_arrange: bool = False
 ) -> bool:
     """
     Find placement for a set of scene nodes, and move them by using a single grouped operation.
@@ -174,6 +182,6 @@ def arrange(
     """
 
     grouped_operation, not_fit_count = createGroupOperationForArrange(nodes_to_arrange, build_volume, fixed_nodes,
-                                                                      factor, add_new_nodes_in_scene, lock_rotation)
+                                                                      factor, add_new_nodes_in_scene = add_new_nodes_in_scene, lock_rotation = lock_rotation, grid_arrange = grid_arrange)
     grouped_operation.push()
     return not_fit_count == 0

cura/CuraActions.py

@@ -18,7 +18,6 @@ from UM.Operations.RemoveSceneNodeOperation import RemoveSceneNodeOperation
 from UM.Operations.TranslateOperation import TranslateOperation
 
 import cura.CuraApplication
-from cura.Arranging.GridArrange import GridArrange
 from cura.Operations.SetParentOperation import SetParentOperation
 from cura.MultiplyObjectsJob import MultiplyObjectsJob
 from cura.Settings.SetObjectExtruderOperation import SetObjectExtruderOperation
@@ -239,8 +238,7 @@ class CuraActions(QObject):
             if node.callDecoration("isSliceable"):
                 fixed_nodes.append(node)
         # Add the new nodes to the scene, and arrange them
-        grid_arrange = GridArrange(nodes, application.getBuildVolume(), fixed_nodes)
+        group_operation, not_fit_count = createGroupOperationForArrange(nodes, application.getBuildVolume(), fixed_nodes, grid_arrange = True)
-        group_operation, not_fit_count = grid_arrange.createGroupOperationForArrange()
         group_operation.push()
 
         # deselect currently selected nodes, and select the new nodes

cura/CuraApplication.py

@@ -1442,13 +1442,13 @@ class CuraApplication(QtApplication):
     # Single build plate
     @pyqtSlot()
     def arrangeAll(self) -> None:
-        self._arrangeAll(False)
+        self._arrangeAll(grid_arrangement = False)
 
     @pyqtSlot()
     def arrangeAllInGrid(self) -> None:
-        self._arrangeAll(True)
+        self._arrangeAll(grid_arrangement = True)
 
-    def _arrangeAll(self, grid_arrangement: bool) -> None:
+    def _arrangeAll(self, *, grid_arrangement: bool) -> None:
         nodes_to_arrange = []
         active_build_plate = self.getMultiBuildPlateModel().activeBuildPlate
         locked_nodes = []
@@ -1478,18 +1478,17 @@ class CuraApplication(QtApplication):
                         locked_nodes.append(node)
                     else:
                         nodes_to_arrange.append(node)
-        self.arrange(nodes_to_arrange, locked_nodes, grid_arrangement)
+        self.arrange(nodes_to_arrange, locked_nodes, grid_arrangement = grid_arrangement)
 
-    def arrange(self, nodes: List[SceneNode], fixed_nodes: List[SceneNode], grid_arrangement: bool = False) -> None:
+    def arrange(self, nodes: List[SceneNode], fixed_nodes: List[SceneNode], *,  grid_arrangement: bool = False) -> None:
         """Arrange a set of nodes given a set of fixed nodes
 
         :param nodes: nodes that we have to place
         :param fixed_nodes: nodes that are placed in the arranger before finding spots for nodes
         :param grid_arrangement: If set to true if objects are to be placed in a grid
         """
-
         min_offset = self.getBuildVolume().getEdgeDisallowedSize() + 2  # Allow for some rounding errors
-        job = ArrangeObjectsJob(nodes, fixed_nodes, min_offset=max(min_offset, 8), grid_arrange =grid_arrangement)
+        job = ArrangeObjectsJob(nodes, fixed_nodes, min_offset = max(min_offset, 8), grid_arrange = grid_arrangement)
         job.start()
 
     @pyqtSlot()

cura/MultiplyObjectsJob.py

@@ -14,14 +14,13 @@ from UM.Operations.TranslateOperation import TranslateOperation
 from UM.Scene.Iterator.DepthFirstIterator import DepthFirstIterator
 from UM.Scene.SceneNode import SceneNode
 from UM.i18n import i18nCatalog
-from cura.Arranging.GridArrange import GridArrange
+from cura.Arranging.Nest2DArrange import createGroupOperationForArrange
-from cura.Arranging.Nest2DArrange import arrange, createGroupOperationForArrange
 
 i18n_catalog = i18nCatalog("cura")
 
 
 class MultiplyObjectsJob(Job):
-    def __init__(self, objects, count: int, min_offset: int = 8, grid_arrange: bool = False):
+    def __init__(self, objects, count: int, min_offset: int = 8 ,* , grid_arrange: bool = False):
         super().__init__()
         self._objects = objects
         self._count: int = count
@@ -78,16 +77,12 @@ class MultiplyObjectsJob(Job):
         found_solution_for_all = True
         group_operation = GroupedOperation()
         if nodes:
-            if(self._grid_arrange):
+            group_operation, not_fit_count = createGroupOperationForArrange(nodes,
-                grid_arrange = GridArrange(nodes,Application.getInstance().getBuildVolume(),fixed_nodes)
+                                                                            Application.getInstance().getBuildVolume(),
-                group_operation, not_fit_count = grid_arrange.createGroupOperationForArrange()
+                                                                            fixed_nodes,
-
+                                                                            factor=10000,
-            else:
+                                                                            add_new_nodes_in_scene=True,
-                group_operation, not_fit_count = createGroupOperationForArrange(nodes,
+                                                                            grid_arrange=self._grid_arrange)
-                                                                                Application.getInstance().getBuildVolume(),
-                                                                                fixed_nodes,
-                                                                                factor=10000,
-                                                                                add_new_nodes_in_scene=True)
 
         if nodes_to_add_without_arrange:
             for nested_node in nodes_to_add_without_arrange:

resources/qml/Menus/ContextMenu.qml

@@ -109,18 +109,7 @@ Cura.Menu
         height: UM.Theme.getSize("small_popup_dialog").height
         minimumWidth: UM.Theme.getSize("small_popup_dialog").width
         minimumHeight: UM.Theme.getSize("small_popup_dialog").height
-
+        onAccepted: gridPlacementSelected.checked? CuraActions.multiplySelectionToGrid(copiesField.value) : CuraActions.multiplySelection(copiesField.value)
-        onAccepted: {
-            if (gridPlacementSelected.checked)
-            {
-                CuraActions.multiplySelectionToGrid(copiesField.value)
-            }
-            else
-            {
-                 CuraActions.multiplySelection(copiesField.value)
-            }
-        }
-
         buttonSpacing: UM.Theme.getSize("thin_margin").width
 
         rightButtons:
@@ -169,7 +158,17 @@ Cura.Menu
             {
                 id: gridPlacementSelected
                 text: catalog.i18nc("@label", "Grid Placement")
+
+                UM.ToolTip
+                {
+                    visible: parent.hovered
+                    targetPoint: Qt.point(parent.x + Math.round(parent.width / 2), parent.y)
+                    x: 0
+                    y: parent.y + parent.height + UM.Theme.getSize("default_margin").height
+                    tooltipText: catalog.i18nc("@info", "Multiply selected item and place them in a grid of build plate.")
+                }
             }
+
         }
     }
 }