Clones now also clone commands attached to element and its children. (#3032, #3121) (#3086)

7 files changed, 178 insertions, 43 deletions

diff --git a/lib/commands.py b/lib/commands.py
index 86abe52a..9a352cfe 100644
--- a/lib/commands.py
+++ b/lib/commands.py
@@ -7,13 +7,16 @@ import os
 import sys
 from copy import deepcopy
 from random import random
+from typing import List
 
 import inkex
 from shapely import geometry as shgeo
+from shapely import get_coordinates
 
 from .i18n import N_, _
 from .svg import (apply_transforms, generate_unique_id,
                   get_correction_transform, get_document, get_node_transform)
+from .svg.svg import copy_no_children, point_upwards
 from .svg.tags import (CONNECTION_END, CONNECTION_START, CONNECTOR_TYPE,
                        INKSCAPE_LABEL, INKSTITCH_ATTRIBS, SVG_SYMBOL_TAG,
                        SVG_USE_TAG, XLINK_HREF)
@@ -113,7 +116,7 @@ class BaseCommand(object):
 
 class Command(BaseCommand):
     def __init__(self, connector):
-        self.connector = connector
+        self.connector: inkex.Path = connector
         self.svg = self.connector.getroottree().getroot()
 
         self.parse_command()
@@ -132,7 +135,8 @@ class Command(BaseCommand):
             (self.get_node_by_url(self.connector.get(CONNECTION_END)), path[0][-1][1])
         ]
 
-        if neighbors[0][0].tag != SVG_USE_TAG:
+        self.symbol_is_end = neighbors[0][0].tag != SVG_USE_TAG
+        if self.symbol_is_end:
             neighbors.reverse()
 
         if neighbors[0][0].tag != SVG_USE_TAG:
@@ -143,12 +147,41 @@ class Command(BaseCommand):
         self.symbol = self.get_node_by_url(neighbors[0][0].get(XLINK_HREF))
         self.parse_symbol()
 
-        self.target = neighbors[1][0]
+        self.target: inkex.BaseElement = neighbors[1][0]
         self.target_point = neighbors[1][1]
 
     def __repr__(self):
         return "Command('%s', %s)" % (self.command, self.target_point)
 
+    def clone(self, new_target: inkex.BaseElement) -> inkex.BaseElement:
+        """
+            Clone this command and point it to the new target, positioning it relative to the new target the same as the target
+        """
+        relative_transform = new_target.composed_transform() @ -self.target.composed_transform()
+
+        # Clone group
+        cloned_group = copy_no_children(self.connector.getparent())
+        cloned_group.transform = relative_transform @ cloned_group.transform
+        new_target.getparent().append(cloned_group)
+
+        symbol = copy_no_children(self.use)
+        cloned_group.append(symbol)
+        point_upwards(symbol)
+
+        # Copy connector
+        connector = copy_no_children(self.connector)
+        cloned_group.append(connector)
+        if self.symbol_is_end:
+            symbol_attr = CONNECTION_END
+            target_attr = CONNECTION_START
+        else:
+            symbol_attr = CONNECTION_START
+            target_attr = CONNECTION_END
+        connector.set(symbol_attr, f"#{symbol.get_id()}")
+        connector.set(target_attr, f"#{new_target.get_id()}")
+
+        return cloned_group
+
 
 class StandaloneCommand(BaseCommand):
     def __init__(self, use):
@@ -175,11 +208,11 @@ class StandaloneCommand(BaseCommand):
         return Point(*pos)
 
 
-def get_command_description(command):
+def get_command_description(command: str):
     return COMMANDS[command]
 
 
-def find_commands(node):
+def find_commands(node: BaseCommand) -> List[Command]:
     """Find the symbols this node is connected to and return them as Commands"""
 
     # find all paths that have this object as a connection
@@ -307,10 +340,24 @@ def add_group(document, node, command):
 
 
 def add_connector(document, symbol, command, element):
-    # I'd like it if I could position the connector endpoint nicely but inkscape just
-    # moves it to the element's center immediately after the extension runs.
+    # "I'd like it if I could position the connector endpoint nicely but inkscape just
+    # moves it to the element's center immediately after the extension runs." - Lex Neva, rev. 4baced7085
+    # "Maybe we should have the target point be a seperately-moveable node? Sometimes moving the command
+    # node so the line drawn from the command to the centroid of the target is awkward anyway?" - CapellanCitizen
+
+    # Inkscape will draw this connector line from the bounding box center of the two nodes, but
+    # will stop at the first intersection with the path it's pointing to. It is necessary to
+    # compute the target point accurately to what inkscape will do.
+    # If not, then the target position will change when the document is loaded by inkscape and break.
+    # For example, not doing this caused issues when implementing commands attached to clones.
     start_pos = (symbol.get('x'), symbol.get('y'))
-    end_pos = element.shape.centroid
+    centroid_pos = element.node.bounding_box(inkex.Transform(get_node_transform(element.node.getparent()))).center
+    connector_line = shgeo.LineString([start_pos, centroid_pos])
+    if connector_line.intersects(element.shape):
+        end_pos = get_coordinates(connector_line.intersection(element.shape))[0]
+    else:
+        # Sometimes the line won't intersect anything and will go straight to the centroid.
+        end_pos = centroid_pos
 
     # Make sure the element's XML node has an id so that we can reference it.
     if element.node.get('id') is None:
@@ -318,10 +365,10 @@ def add_connector(document, symbol, command, element):
 
     path = inkex.PathElement(attrib={
         "id": generate_unique_id(document, "command_connector"),
-        "d": "M %s,%s %s,%s" % (start_pos[0], start_pos[1], end_pos.x, end_pos.y),
+        "d": f"M {start_pos[0]},{start_pos[1]} {end_pos[0]},{end_pos[1]}",
         "style": "stroke:#000000;stroke-width:1px;stroke-opacity:0.5;fill:none;",
-        CONNECTION_START: "#%s" % symbol.get('id'),
-        CONNECTION_END: "#%s" % element.node.get('id'),
+        CONNECTION_START: f"#{symbol.get('id')}",
+        CONNECTION_END: f"#{element.node.get('id')}",
 
         # l10n: the name of the line that connects a command to the object it applies to
         INKSCAPE_LABEL: _("connector")
diff --git a/lib/elements/clone.py b/lib/elements/clone.py
index 9a89b6ff..91c69740 100644
--- a/lib/elements/clone.py
+++ b/lib/elements/clone.py
@@ -5,18 +5,20 @@
 
 from math import degrees
 from contextlib import contextmanager
-from typing import Generator, List
+from typing import Generator, List, Dict
 
 from inkex import Transform, BaseElement
 from shapely import MultiLineString
 
 from ..stitch_plan.stitch_group import StitchGroup
 
-from ..commands import is_command_symbol
+from ..commands import is_command_symbol, find_commands
 from ..i18n import _
+from ..svg.svg import copy_no_children
 from ..svg.path import get_node_transform
 from ..svg.tags import (EMBROIDERABLE_TAGS, INKSTITCH_ATTRIBS, SVG_USE_TAG,
-                        XLINK_HREF, SVG_GROUP_TAG)
+                        XLINK_HREF, CONNECTION_START, CONNECTION_END,
+                        SVG_GROUP_TAG)
 from ..utils import cache
 from .element import EmbroideryElement, param
 from .validation import ValidationWarning
@@ -88,7 +90,8 @@ class Clone(EmbroideryElement):
             stitch_groups = []
 
             for element in elements:
-                element_stitch_groups = element.to_stitch_groups(last_stitch_group)
+                # Using `embroider` here to get trim/stop after commands, etc.
+                element_stitch_groups = element.embroider(last_stitch_group)
                 if len(element_stitch_groups):
                     last_stitch_group = element_stitch_groups[-1]
                     stitch_groups.extend(element_stitch_groups)
@@ -104,40 +107,40 @@ class Clone(EmbroideryElement):
         Could possibly be refactored into just a generator - being a context manager is mainly to control the lifecycle of the elements
         that are cloned (again, for testing convenience primarily)
         """
-        parent: BaseElement = self.node.getparent()
-        cloned_node = self.resolve_clone()
+        cloned_nodes = self.resolve_clone()
         try:
             # In a try block so we can ensure that the cloned_node is removed from the tree in the event of an exception.
             # Otherwise, it might be left around on the document if we throw for some reason.
-            yield self.clone_to_elements(cloned_node)
+            yield self.clone_to_elements(cloned_nodes[0])
         finally:
             # Remove the "manually cloned" tree.
-            parent.remove(cloned_node)
+            for cloned_node in cloned_nodes:
+                cloned_node.delete()
 
-    def resolve_clone(self, recursive=True) -> BaseElement:
+    def resolve_clone(self, recursive=True) -> List[BaseElement]:
         """
         "Resolve" this clone element by copying the node it hrefs as if unlinking the clone in Inkscape.
         The node will be added as a sibling of this element's node, with its transform and style applied.
         The fill angles for resolved elements will be rotated per the transform and clone_fill_angle properties of the clone.
 
         :param recursive: Recursively "resolve" all child clones in the same manner
-        :returns: The "resolved" node
+        :returns: A list where the first element is the "resolved" node, and zero or more commands attached to that node
         """
         parent: BaseElement = self.node.getparent()
         source_node: BaseElement = self.node.href
         source_parent: BaseElement = source_node.getparent()
-        cloned_node = source_node.copy()
+        cloned_node = clone_with_fixup(parent, source_node)
 
         if recursive:
             # Recursively resolve all clones as if the clone was in the same place as its source
             source_parent.add(cloned_node)
 
             if is_clone(cloned_node):
-                cloned_node = cloned_node.replace_with(Clone(cloned_node).resolve_clone())
+                cloned_node = cloned_node.replace_with(Clone(cloned_node).resolve_clone()[0])
             else:
                 clones: List[BaseElement] = [n for n in cloned_node.iterdescendants() if is_clone(n)]
                 for clone in clones:
-                    clone.replace_with(Clone(clone).resolve_clone())
+                    clone.replace_with(Clone(clone).resolve_clone()[0])
 
             source_parent.remove(cloned_node)
 
@@ -153,12 +156,18 @@ class Clone(EmbroideryElement):
         # Compute angle transform:
         # Effectively, this is (local clone transform) * (to parent space) * (from clone's parent space)
         # There is a translation component here that will be ignored.
-        source_transform = source_parent.composed_transform()
-        clone_transform = self.node.composed_transform()
+        source_transform: Transform = source_parent.composed_transform()
+        clone_transform: Transform = self.node.composed_transform()
         angle_transform = clone_transform @ -source_transform
         self.apply_angles(cloned_node, angle_transform)
 
-        return cloned_node
+        ret = [cloned_node]
+
+        # We need to copy all commands that were attached directly to the href'd node
+        for command in find_commands(source_node):
+            ret.append(command.clone(cloned_node))
+
+        return ret
 
     def apply_angles(self, cloned_node: BaseElement, transform: Transform) -> None:
         """
@@ -174,6 +183,11 @@ class Clone(EmbroideryElement):
             if node.tag not in EMBROIDERABLE_TAGS:
                 continue
 
+            # Only need to adjust angles on fill elements.
+            element = EmbroideryElement(node)
+            if not (element.get_style("fill", "black") and not element.get_style('fill-opacity', 1) == "0"):
+                continue
+
             # Normally, rotate the cloned element's angle by the clone's rotation.
             if self.clone_fill_angle is None:
                 element_angle = float(node.get(INKSTITCH_ATTRIBS['angle'], 0))
@@ -212,3 +226,38 @@ def is_clone(node):
     if node.tag == SVG_USE_TAG and node.get(XLINK_HREF) and not is_command_symbol(node):
         return True
     return False
+
+
+def clone_with_fixup(parent: BaseElement, node: BaseElement) -> BaseElement:
+    """
+    Clone the node, placing the clone as a child of parent, and fix up
+    references in the cloned subtree to point to elements from the clone subtree.
+    """
+    # A map of "#id" -> "#corresponding-id-in-the-cloned-subtree"
+    id_map: Dict[str, str] = {}
+
+    def clone_children(parent: BaseElement, node: BaseElement) -> BaseElement:
+        # Copy the node without copying its children.
+        cloned = copy_no_children(node)
+        parent.append(cloned)
+        id_map[f"#{node.get_id()}"] = f"#{cloned.get_id()}"
+
+        for child in node.getchildren():
+            clone_children(cloned, child)
+
+        return cloned
+
+    ret = clone_children(parent, node)
+
+    def fixup_id_attr(node: BaseElement, attr: str):
+        # Replace the id value for this attrib with the corresponding one in the clone subtree, if applicable.
+        val = node.get(attr)
+        if val is not None:
+            node.set(attr, id_map.get(val, val))
+
+    for n in ret.iter():
+        fixup_id_attr(n, XLINK_HREF)
+        fixup_id_attr(n, CONNECTION_START)
+        fixup_id_attr(n, CONNECTION_END)
+
+    return ret
diff --git a/lib/elements/element.py b/lib/elements/element.py
index 5cc13174..8884bc2a 100644
--- a/lib/elements/element.py
+++ b/lib/elements/element.py
@@ -9,8 +9,10 @@ from copy import deepcopy
 import inkex
 import numpy as np
 from inkex import bezier, BaseElement
+from typing import List, Optional
 
-from ..commands import find_commands
+from ..commands import Command, find_commands
+from ..stitch_plan import StitchGroup
 from ..debug.debug import debug
 from ..exceptions import InkstitchException, format_uncaught_exception
 from ..i18n import _
@@ -436,19 +438,19 @@ class EmbroideryElement(object):
 
     @property
     @cache
-    def commands(self):
+    def commands(self) -> List[Command]:
         return find_commands(self.node)
 
     @cache
-    def get_commands(self, command):
+    def get_commands(self, command: str) -> List[Command]:
         return [c for c in self.commands if c.command == command]
 
     @cache
-    def has_command(self, command):
+    def has_command(self, command: str) -> bool:
         return len(self.get_commands(command)) > 0
 
     @cache
-    def get_command(self, command):
+    def get_command(self, command: str) -> Optional[Command]:
         commands = self.get_commands(command)
 
         if commands:
@@ -495,7 +497,7 @@ class EmbroideryElement(object):
 
         return lock_start, lock_end
 
-    def to_stitch_groups(self, last_stitch_group):
+    def to_stitch_groups(self, last_stitch_group: Optional[StitchGroup]) -> List[StitchGroup]:
         raise NotImplementedError("%s must implement to_stitch_groups()" % self.__class__.__name__)
 
     @debug.time
@@ -517,7 +519,7 @@ class EmbroideryElement(object):
 
         return stitch_groups
 
-    def uses_previous_stitch(self):
+    def uses_previous_stitch(self) -> bool:
         """Returns True if the previous stitch can affect this Element's stitches.
 
         This function may be overridden in a subclass.
@@ -589,7 +591,7 @@ class EmbroideryElement(object):
 
         return cache_key
 
-    def embroider(self, last_stitch_group):
+    def embroider(self, last_stitch_group: Optional[StitchGroup]) -> List[StitchGroup]:
         debug.log(f"starting {self.node.get('id')} {self.node.get(INKSCAPE_LABEL)}")
 
         with self.handle_unexpected_exceptions():
@@ -606,8 +608,10 @@ class EmbroideryElement(object):
                 apply_patterns(stitch_groups, self.node)
 
                 if stitch_groups:
-                    stitch_groups[-1].trim_after = self.has_command("trim") or self.trim_after
-                    stitch_groups[-1].stop_after = self.has_command("stop") or self.stop_after
+                    # In some cases (clones) the last stitch group may have trim_after or stop_after already set,
+                    # and we shouldn't override that with this element's values, hence the use of or-equals
+                    stitch_groups[-1].trim_after |= self.has_command("trim") or self.trim_after
+                    stitch_groups[-1].stop_after |= self.has_command("stop") or self.stop_after
 
                 for stitch_group in stitch_groups:
                     stitch_group.min_jump_stitch_length = self.min_jump_stitch_length
diff --git a/lib/extensions/unlink_clone.py b/lib/extensions/unlink_clone.py
index 0c187808..e94bf0ea 100644
--- a/lib/extensions/unlink_clone.py
+++ b/lib/extensions/unlink_clone.py
@@ -5,9 +5,10 @@
 
 from inkex import Boolean, errormsg, BaseElement
 
-from ..elements import Clone
+from ..elements import Clone, EmbroideryElement
 from ..i18n import _
 from .base import InkstitchExtension
+from ..svg.tags import CONNECTION_END, CONNECTION_START
 
 from typing import List, Tuple
 
@@ -34,8 +35,14 @@ class UnlinkClone(InkstitchExtension):
         for element in self.elements:
             if isinstance(element, Clone):
                 resolved = element.resolve_clone(recursive=recursive)
-                clones_resolved.append((element.node, resolved))
+                clones_resolved.append((element.node, resolved[0]))
 
         for (clone, resolved) in clones_resolved:
-            clone.getparent().remove(clone)
-            resolved.set_id(clone.get_id())
+            clone.delete()
+            orig_id = resolved.get_id()
+            new_id = clone.get_id()
+            # Fix up command backlinks - note this has to happen before we rename so they can actually be found.
+            for command in EmbroideryElement(resolved).commands:
+                backlink_attrib = CONNECTION_START if command.connector.get(CONNECTION_START) == ("#"+orig_id) else CONNECTION_END
+                command.connector.set(backlink_attrib, "#"+new_id)
+            resolved.set_id(new_id)
diff --git a/lib/svg/path.py b/lib/svg/path.py
index 878d2a7c..9d92058b 100644
--- a/lib/svg/path.py
+++ b/lib/svg/path.py
@@ -31,7 +31,7 @@ def compose_parent_transforms(node, mat):
     return mat
 
 
-def get_node_transform(node):
+def get_node_transform(node: inkex.BaseElement):
     """
     if getattr(node, "composed_transform", None):
         return node.composed_transform()
diff --git a/lib/svg/svg.py b/lib/svg/svg.py
index 2c521098..99f5bf26 100644
--- a/lib/svg/svg.py
+++ b/lib/svg/svg.py
@@ -3,7 +3,9 @@
 # Copyright (c) 2010 Authors
 # Licensed under the GNU GPL version 3.0 or later.  See the file LICENSE for details.
 
-from inkex import NSS
+import math
+
+from inkex import NSS, BaseElement, Transform
 from lxml import etree
 
 from ..utils import cache
@@ -29,3 +31,28 @@ def find_elements(node, xpath):
     document = get_document(node)
     elements = document.xpath(xpath, namespaces=NSS)
     return elements
+
+
+def copy_no_children(node: BaseElement) -> BaseElement:
+    return type(node)(attrib=node.attrib)
+
+
+def point_upwards(node: BaseElement) -> None:
+    """
+    Given a node, adjust the transform such that it is in the same spot, but pointing upwards (e.g. for command symbols)
+    """
+    # Adjust the transform of the node so it's face-up and the right way around.
+    node_transform = node.composed_transform()
+    compensation = -Transform((node_transform.a, node_transform.b, node_transform.c, node_transform.d, 0, 0))
+    scale_vector = compensation.capply_to_point(1+1j)
+    scale_factor = math.sqrt(2)/math.sqrt(scale_vector.real*scale_vector.real + scale_vector.imag*scale_vector.imag)
+    compensation.add_scale(scale_factor, scale_factor)
+
+    node_correction = Transform().add_translate(float(node.get('x', 0)), float(node.get('y', 0)))
+    node_correction @= compensation
+    # Quick hack to compute the rotational angle - node_transform @ (1,0) = (a, b)
+
+    node.transform = node.transform @ node_correction
+    # Clear the x and y coords, they've been incorporated to the transform above
+    node.set('x', None)
+    node.set('y', None)
diff --git a/lib/utils/__init__.py b/lib/utils/__init__.py
index b24257be..60a0bd36 100644
--- a/lib/utils/__init__.py
+++ b/lib/utils/__init__.py
@@ -3,6 +3,7 @@
 # Copyright (c) 2010 Authors
 # Licensed under the GNU GPL version 3.0 or later.  See the file LICENSE for details.
 
+from . import cache as cache_module  # Slight hack to allow cache to be imported for monkeypatching
 from .cache import cache
 from .dotdict import DotDict
 from .geometry import *