Fix remaining unit tests

gerbonara/aperture_macros/parse.py

@@ -49,18 +49,21 @@ def _parse_expression(expr):
 
 @dataclass(frozen=True, slots=True)
 class ApertureMacro:
-    name: str = None
+    name: str = field(default=None, hash=False, compare=False)
     primitives: tuple = ()
     variables: tuple = ()
-    comments: tuple = ()
+    comments: tuple = field(default=(), hash=False, compare=False)
 
     def __post_init__(self):
         if self.name is None or re.match(r'GNX[0-9A-F]{16}', self.name):
             # We can't use field(default_factory=...) here because that factory doesn't get a reference to the instance.
-            object.__setattr__(self, 'name', f'GNX{hash(self)&0xffffffffffffffff:016X}')
+            self._reset_name()
+
+    def _reset_name(self):
+        object.__setattr__(self, 'name', f'GNX{hash(self)&0xffffffffffffffff:016X}')
 
     @classmethod
-    def parse_macro(cls, name, body, unit):
+    def parse_macro(kls, name, body, unit):
         comments = []
         variables = {}
         primitives = []
@@ -86,11 +89,10 @@ class ApertureMacro:
             else: # primitive
                 primitive, *args = block.split(',')
                 args = [ _parse_expression(arg) for arg in args ]
-                primitive = ap.PRIMITIVE_CLASSES[int(primitive)](unit=unit, args=args)
-                primitives.append(primitive)
+                primitives.append(ap.PRIMITIVE_CLASSES[int(primitive)].from_arglist(unit, args))
 
-        variables = [variables.get(i+1) for i in range(max(variables.keys()))]
-        return kls(name, tuple(primitives), tuple(variables), tuple(primitives))
+        variables = [variables.get(i+1) for i in range(max(variables.keys(), default=0))]
+        return kls(name, tuple(primitives), tuple(variables), tuple(comments))
 
     def __str__(self):
         return f'<Aperture macro {self.name}, variables {str(self.variables)}, primitives {self.primitives}>'
@@ -110,6 +112,7 @@ class ApertureMacro:
         return replace(self, primitives=tuple(new_primitives))
 
     def to_gerber(self, unit=None):
+        """ Serialize this macro's content (without the name) into Gerber using the given file unit """
         comments = [ str(c) for c in self.comments ]
         variable_defs = [ f'${var}={str(expr)[1:-1]}' for var, expr in enumerate(self.variables, start=1) if expr is not None ]
         primitive_defs = [ prim.to_gerber(unit) for prim in self.primitives ]

gerbonara/aperture_macros/primitive.py

@@ -58,8 +58,8 @@ class Primitive:
         return str(self)
 
     @classmethod
-    def from_arglist(kls, arglist):
-        return kls(*arglist)
+    def from_arglist(kls, unit, arglist):
+        return kls(unit, *arglist)
 
     class Calculator:
         def __init__(self, instance, variable_binding={}, unit=None):
@@ -267,11 +267,11 @@ class Outline(Primitive):
             yield x, y
 
     @classmethod
-    def from_arglist(kls, arglist):
-        if len(arglist[3:]) % 2 == 0:
-            return kls(unit=arglist[0], exposure=arglist[1], length=arglist[2], coords=arglist[3:], rotation=0)
+    def from_arglist(kls, unit, arglist):
+        if len(arglist[2:]) % 2 == 0:
+            return kls(unit=unit, exposure=arglist[0], length=arglist[1], coords=arglist[2:], rotation=0)
         else:
-            return kls(unit=arglist[0], exposure=arglist[1], length=arglist[2], coords=arglist[3:-1], rotation=arglist[-1])
+            return kls(unit=unit, exposure=arglist[0], length=arglist[1], coords=arglist[2:-1], rotation=arglist[-1])
 
     def __str__(self):
         return f'<Outline {len(self.coords)} points>'

gerbonara/apertures.py

@@ -60,8 +60,8 @@ class Aperture:
     _ : KW_ONLY
     unit: LengthUnit = None
     attrs: tuple = None
-    original_number: int = None
-    _bounding_box: tuple = None
+    original_number: int = field(default=None, hash=False, compare=False)
+    _bounding_box: tuple = field(default=None, hash=False, compare=False)
 
     def _params(self, unit=None):
         out = []
@@ -351,7 +351,7 @@ class PolygonAperture(Aperture):
     hole_dia : Length(float) = None
 
     def __post_init__(self):
-        self.n_vertices = int(self.n_vertices)
+        object.__setattr__(self, 'n_vertices', int(self.n_vertices))
 
     def _primitives(self, x, y, unit=None, polarity_dark=True):
         return [ gp.ArcPoly.from_regular_polygon(x, y, self.unit.convert_to(unit, self.diameter)/2, self.n_vertices,

gerbonara/cli.py

@@ -494,7 +494,7 @@ def meta(path, force_zip, format_warnings):
         d[function] = {
                 'format': 'Gerber',
                 'path': str(layer.original_path),
-                'apertures': len(layer.apertures),
+                'apertures': len(list(layer.apertures())),
                 'objects': len(layer.objects),
                 'bounding_box': {'min_x': min_x, 'min_y': min_y, 'max_x': max_x, 'max_y': max_y},
                 'format_settings': format_settings,

gerbonara/excellon.py

@@ -46,8 +46,8 @@ class ExcellonContext:
 
     def select_tool(self, tool):
         """ Select the current tool. Retract drill first if necessary. """
-        current_id = self.tools.get(id(self.current_tool))
-        new_id = self.tools[id(tool)]
+        current_id = self.tools.get(self.current_tool)
+        new_id = self.tools[tool]
         if new_id != current_id:
             if self.drill_down:
                 yield 'M16' # drill up
@@ -270,17 +270,15 @@ class ExcellonFile(CamFile):
 
     def to_gerber(self, errros='raise'):
         """ Convert this excellon file into a :py:class:`~.rs274x.GerberFile`. """
-        apertures = {}
         out = GerberFile()
         out.comments = self.comments
 
+        apertures = {}
         for obj in self.objects:
-            if id(obj.tool) not in apertures:
-                apertures[id(obj.tool)] = CircleAperture(obj.tool.diameter)
+            if not (ap := apertures[obj.tool]):
+                ap = apertures[obj.tool] = CircleAperture(obj.tool.diameter)
 
-            out.objects.append(dataclasses.replace(obj, aperture=apertures[id(obj.tool)]))
-
-        out.apertures = list(apertures.values())
+            out.objects.append(dataclasses.replace(obj, aperture=ap))
 
     @property
     def generator(self):
@@ -373,7 +371,7 @@ class ExcellonFile(CamFile):
         yield 'METRIC' if settings.unit == MM else 'INCH'
 
         # Build tool index
-        tool_map = { id(obj.tool): obj.tool for obj in self.objects }
+        tool_map = { obj.tool: obj.tool for obj in self.objects }
         tools = sorted(tool_map.items(), key=lambda id_tool: (id_tool[1].plated, id_tool[1].diameter))
 
         mixed_plating = (len({ tool.plated for tool in tool_map.values() }) > 1)

gerbonara/graphic_objects.py

@@ -216,7 +216,8 @@ class Flash(GraphicObject):
 
     def bounding_box(self, unit=None):
         (min_x, min_y), (max_x, max_y) = self.aperture.bounding_box(unit)
-        return (min_x+self.x, min_y+self.y), (max_x+self.x, max_x+self.y)
+        x, y = self.unit.convert_to(unit, self.x), self.unit.convert_to(unit, self.y)
+        return (min_x+x, min_y+y), (max_x+x, max_y+y)
 
     @property
     def plated(self):
@@ -398,6 +399,9 @@ class Region(GraphicObject):
         yield from gs.set_current_point(self.outline[0], unit=self.unit)
 
         for point, arc_center in zip_longest(self.outline[1:], self.arc_centers):
+            if point is None and arc_center is None:
+                break
+
             if arc_center is None:
                 yield from gs.set_interpolation_mode(InterpMode.LINEAR)
 

gerbonara/layers.py

@@ -838,12 +838,12 @@ class LayerStack:
             if use_use:
                 layer.dedup_apertures()
                 for obj in layer.objects:
-                    if hasattr(obj, 'aperture') and obj.polarity_dark and id(obj.aperture) not in use_map:
+                    if hasattr(obj, 'aperture') and obj.polarity_dark and obj.aperture not in use_map:
                         children = [prim.to_svg(fg, bg, tag=tag)
                                     for prim in obj.aperture.flash(0, 0, svg_unit, polarity_dark=True)]
                         use_id = f'a{len(use_defs)}'
                         use_defs.append(tag('g', children, id=use_id))
-                        use_map[id(obj.aperture)] = use_id
+                        use_map[obj.aperture] = use_id
 
             objects = []
             for obj in layer.instance.svg_objects(svg_unit=svg_unit, fg=fg, bg=bg, aperture_map=use_map, tag=Tag):

gerbonara/rs274x.py

@@ -24,6 +24,7 @@ import math
 import warnings
 from pathlib import Path
 import dataclasses
+import functools
 
 from .cam import CamFile, FileSettings
 from .utils import MM, Inch, units, InterpMode, UnknownStatementWarning
@@ -58,7 +59,6 @@ class GerberFile(CamFile):
     :ivar layer_hints: Similar to ``generator_hints``, this is a list containing hints which layer type this file could
                        belong to. Usually, this will be empty, but some EDA tools automatically include layer
                        information inside tool-specific comments in the Gerber files they generate.
-    :ivar apertures: List of apertures used in this file. Make sure you keep this in sync when adding new objects.
     :ivar file_attrs: List of strings with Gerber X3 file attributes. Each list item corresponds to one file attribute.
     """
 
@@ -70,11 +70,87 @@ class GerberFile(CamFile):
         self.generator_hints = generator_hints or []
         self.layer_hints = layer_hints or []
         self.import_settings = import_settings
-        self.apertures = [] # FIXME get rid of this? apertures are already in the objects.
         self.file_attrs = file_attrs or {}
 
-    def sync_apertures(self):
-        self.apertures = list({id(obj.aperture): obj.aperture for obj in self.objects if hasattr(obj, 'aperture')}.values())
+    def apertures(self):
+        """ Iterate through all apertures in this layer. """
+        found = set()
+        for obj in self.objects:
+            if hasattr(obj, 'aperture'):
+                ap = obj.aperture
+                if ap not in found:
+                    found.add(ap)
+                    yield ap
+
+    def aperture_macros(self):
+        found = set()
+        for aperture in self.apertures():
+            if isinstance(aperture, apertures.ApertureMacroInstance):
+                macro = aperture.macro
+                if (macro.name, macro) not in found:
+                    found.add((macro.name, macro))
+                    yield macro
+
+    def map_apertures(self, map_or_callable, cache=True):
+        """ Replace all apertures in all objects in this layer according to the given map or callable.
+
+        When a map is passed, apertures that are not in the map are left alone. When a callable is given, it is called
+        with the old aperture as its argument.
+
+        :param map_or_callable: A dict-like object, or a callable mapping old to new apertures
+        :param cache: When True (default) and a callable is passed, caches the output of callable, only calling it once
+            for each old aperture.
+        """
+
+        if callable(map_or_callable):
+            if cache:
+                map_or_callable = functools.cache(map_or_callable)
+        else:
+            d = map_or_callable
+            map_or_callable = lambda ap: d.get(ap, ap)
+
+        for obj in self.objects:
+            if (aperture := getattr(obj, 'aperture', None)):
+                obj.aperture = map_or_callable(aperture)
+
+    def dedup_apertures(self, settings=None):
+        """ Merge all apertures and aperture macros in this layer that result in the same Gerber definition under the
+        given :py:class:~.FileSettings:.
+
+        When no explicit settings are given, uses Gerbonara's default settings.
+
+        :param settings: settings under which to de-duplicate the apertures.
+        """
+
+        if settings is None:
+            settings = FileSettings.defaults()
+
+        cache = {}
+        macro_cache = {}
+        macro_names = set()
+        def lookup(aperture):
+            nonlocal cache, settings
+            if isinstance(aperture, apertures.ApertureMacroInstance):
+                macro = aperture.macro
+                macro_def = macro.to_gerber(unit=settings.unit)
+                if macro_def not in cache:
+                    cache[macro_def] = macro
+
+                    if macro.name in macro_names:
+                        macro._reset_name()
+                    macro_names.add(macro.name)
+
+                else:
+                    macro = cache[macro_def]
+                    aperture = dataclasses.replace(aperture, macro=macro)
+
+            code = aperture.to_gerber(settings)
+            if code not in cache:
+                cache[code] = aperture
+
+            return cache[code]
+
+        self.map_apertures(lookup)
 
     def to_excellon(self, plated=None, errors='raise'):
         """ Convert this excellon file into a :py:class:`~.excellon.ExcellonFile`. This will convert interpolated lines
@@ -85,7 +161,7 @@ class GerberFile(CamFile):
         new_tools = {}
         for obj in self.objects:
             if not (isinstance(obj, go.Line) or isinstance(obj, go.Arc) or isinstance(obj, go.Flash)) or \
-                not isinstance(obj.aperture, apertures.CircleAperture):
+                not isinstance(getattr(obj, 'aperture', None), apertures.CircleAperture):
                     if errors == 'raise':
                         raise ValueError(f'Cannot convert {obj} to excellon.')
                     elif errors == 'warn':
@@ -96,9 +172,9 @@ class GerberFile(CamFile):
                     else:
                         raise ValueError('Invalid "errors" parameter. Allowed values: "raise", "warn" or "ignore".')
 
-            if not (new_tool := new_tools.get(id(obj.aperture))):
+            if not (new_tool := new_tools.get(obj.aperture)):
                 # TODO plating?
-                new_tool = new_tools[id(obj.aperture)] = apertures.ExcellonTool(obj.aperture.diameter, plated=plated, unit=obj.aperture.unit)
+                new_tool = new_tools[obj.aperture] = apertures.ExcellonTool(obj.aperture.diameter, plated=plated, unit=obj.aperture.unit)
             new_objs.append(dataclasses.replace(obj, aperture=new_tool))
             
         return ExcellonFile(objects=new_objs, comments=self.comments)
@@ -127,18 +203,6 @@ class GerberFile(CamFile):
             self.import_settings = None
         self.comments += other.comments
 
-        # dedup apertures
-        new_apertures = {}
-        replace_apertures = {}
-        mock_settings = FileSettings.defaults()
-        for ap in self.apertures + other.apertures:
-            gbr = ap.to_gerber(mock_settings)
-            if gbr not in new_apertures:
-                new_apertures[gbr] = ap
-            else:
-                replace_apertures[id(ap)] = new_apertures[gbr]
-        self.apertures = list(new_apertures.values())
-
         # Join objects
         if mode == 'below':
             self.objects = other.objects + self.objects
@@ -147,57 +211,23 @@ class GerberFile(CamFile):
         else:
             raise ValueError(f'Invalid mode "{mode}", must be one of "above" or "below".')
 
-        for obj in self.objects:
-            # If object has an aperture attribute, replace that aperture.
-            if (ap := replace_apertures.get(id(getattr(obj, 'aperture', None)))):
-                obj.aperture = ap
-
-        # dedup aperture macros
-        macros = { m.to_gerber(): m
-                for m in [ GenericMacros.circle, GenericMacros.rect, GenericMacros.obround, GenericMacros.polygon] }
-        for ap in new_apertures.values():
-            if isinstance(ap, apertures.ApertureMacroInstance):
-                macro_grb = ap.macro.to_gerber() # use native unit to compare macros
-                if macro_grb in macros:
-                    ap.macro = macros[macro_grb]
-                else:
-                    macros[macro_grb] = ap.macro
-
-        # make macro names unique
-        seen_macro_names = set()
-        for macro in macros.values():
-            i = 2
-            while (new_name := f'{macro.name}{i}') in seen_macro_names:
-                i += 1
-            macro.name = new_name
-            seen_macro_names.add(new_name)
+        self.dedup_apertures()
 
     def dilate(self, offset, unit=MM, polarity_dark=True):
         # TODO add tests for this
-        self.apertures = [ aperture.dilated(offset, unit) for aperture in self.apertures ]
+        self.map_apertures(lambda ap: ap.dilated(offset, unit))
 
         offset_circle = apertures.CircleAperture(offset, unit=unit)
-        self.apertures.append(offset_circle)
-
-        new_primitives = []
-        for p in self.primitives:
-
-            p.polarity_dark = polarity_dark
+        new_objects = []
+        for obj in self.objects:
+            obj.polarity_dark = polarity_dark
 
             # Ignore Line, Arc, Flash. Their actual dilation has already been done by dilating the apertures above.
-            if isinstance(p, Region):
-                ol = p.poly.outline
-                for start, end, arc_center in zip(ol, ol[1:] + ol[0], p.poly.arc_centers):
-                    if arc_center is not None:
-                        new_primitives.append(Arc(*start, *end, *arc_center,
-                            polarity_dark=polarity_dark, unit=p.unit, aperture=offset_circle))
-
-                    else:
-                        new_primitives.append(Line(*start, *end,
-                            polarity_dark=polarity_dark, unit=p.unit, aperture=offset_circle))
+            if isinstance(obj, Region):
+                new_objects.extend(obj.outline_objects(offset_circle))
 
         # it's safe to append these at the end since we compute a logical OR of opaque areas anyway.
-        self.primitives.extend(new_primitives)
+        self.objects.extend(new_objects)
 
     @classmethod
     def open(kls, filename, enable_includes=False, enable_include_dir=None, override_settings=None):
@@ -228,29 +258,8 @@ class GerberFile(CamFile):
         parser.parse(data, filename=filename)
         return obj
 
-    def dedup_apertures(self, settings=None):
-        settings = settings or FileSettings.defaults()
-
-        defined_apertures = {}
-        ap_map = {}
-        for obj in self.objects:
-            if not hasattr(obj, 'aperture'):
-                continue
-
-            if id(obj.aperture) in ap_map:
-                obj.aperture = ap_map[id(obj.aperture)]
-
-            ap_def = obj.aperture.to_gerber(settings)
-            if ap_def in defined_apertures:
-                ap_map[id(obj.aperture)] = obj.aperture = defined_apertures[ap_def]
-            else:
-                ap_map[id(obj.aperture)] = defined_apertures[ap_def] = obj.aperture
-
-        self.apertures = list(ap_map.values())
-
     def _generate_statements(self, settings, drop_comments=True):
         """ Export this file as Gerber code, yields one str per line. """
-        self.sync_apertures()
 
         yield 'G04 Gerber file generated by Gerbonara*'
         for name, value in self.file_attrs.items():
@@ -273,33 +282,15 @@ class GerberFile(CamFile):
             for cmt in self.comments:
                 yield f'G04{cmt}*'
 
-        # Always emit gerbonara's generic, rotation-capable aperture macro replacements for the standard C/R/O/P shapes.
-        # Unconditionally emitting these here is easier than first trying to figure out if we need them later,
-        # and they are only a few bytes anyway.
+        self.dedup_apertures()
+
         am_stmt = lambda macro: f'%AM{macro.name}*\n{macro.to_gerber(unit=settings.unit)}*\n%'
-        for macro in [ GenericMacros.circle, GenericMacros.rect, GenericMacros.obround, GenericMacros.polygon ]:
+        for macro in self.aperture_macros():
             yield am_stmt(macro)
 
-        processed_macros = set()
-        aperture_map = {}
-        defined_apertures = {}
-        number = 10
-        for aperture in self.apertures:
-            if isinstance(aperture, apertures.ApertureMacroInstance):
-                macro_def = am_stmt(aperture.macro)
-                if macro_def not in processed_macros:
-                    processed_macros.add(macro_def)
-                    yield macro_def
-
-            ap_def = aperture.to_gerber(settings)
-            if ap_def in defined_apertures:
-                aperture_map[id(aperture)] = defined_apertures[ap_def]
-
-            else:
-                yield f'%ADD{number}{ap_def}*%'
-                defined_apertures[ap_def] = number
-                aperture_map[id(aperture)] = number
-                number += 1
+        aperture_map = {ap: num for num, ap in enumerate(self.apertures(), start=10)}
+        for aperture, number in aperture_map.items():
+            yield f'%ADD{number}{aperture.to_gerber(settings)}*%'
 
         def warn(msg, kls=SyntaxWarning):
             warnings.warn(msg, kls)
@@ -312,7 +303,7 @@ class GerberFile(CamFile):
 
     def __str__(self):
         name = f'{self.original_path.name} ' if self.original_path else ''
-        return f'<GerberFile {name}with {len(self.apertures)} apertures, {len(self.objects)} objects>'
+        return f'<GerberFile {name}with {len(list(self.apertures()))} apertures, {len(self.objects)} objects>'
 
     def __repr__(self):
         return str(self)
@@ -348,17 +339,11 @@ class GerberFile(CamFile):
     def scale(self, factor, unit=MM):
         scaled_apertures = {}
 
-        for ap in self.apertures:
-            scaled_apertures[id(ap)] = ap.scaled(factor)
+        self.map_apertures(lambda ap: ap.scaled(factor))
 
         for obj in self.objects:
             obj.scale(factor)
 
-            if (obj_ap := getattr(obj, 'aperture', None)):
-                obj.aperture = scaled_apertures[id(obj_ap)]
-
-        self.apertures = list(scaled_apertures.values())
-
     def offset(self, dx=0,  dy=0, unit=MM):
         # TODO round offset to file resolution
         for obj in self.objects:
@@ -368,10 +353,7 @@ class GerberFile(CamFile):
         if math.isclose(angle % (2*math.pi), 0):
             return
 
-        # First, rotate apertures. We do this separately from rotating the individual objects below to rotate each
-        # aperture exactly once.
-        for ap in self.apertures:
-            ap.rotation += angle
+        self.map_apertures(lambda ap: ap.rotated(angle))
 
         for obj in self.objects:
             obj.rotate(angle, cx, cy, unit)
@@ -568,8 +550,8 @@ class GraphicsState:
             yield '%LPD*%' if polarity_dark else '%LPC*%'
 
     def set_aperture(self, aperture):
-        ap_id = self.aperture_map[id(aperture)]
-        old_ap_id = self.aperture_map.get(id(self.aperture), None)
+        ap_id = self.aperture_map[aperture]
+        old_ap_id = self.aperture_map.get(self.aperture, None)
         if ap_id != old_ap_id:
             self.aperture = aperture
             yield f'D{ap_id}*'
@@ -711,7 +693,6 @@ class GerberParser:
                 self.warn(f'Unknown statement found: "{self._shorten_line()}", ignoring.', UnknownStatementWarning)
                 self.target.comments.append(f'Unknown statement found: "{self._shorten_line()}", ignoring.')
         
-        self.target.apertures = list(self.aperture_map.values())
         self.target.import_settings = self.file_settings
         self.target.unit = self.file_settings.unit
         self.target.file_attrs = self.file_attrs
@@ -852,12 +833,12 @@ class GerberParser:
             if match['shape'] in 'RO' and (math.isclose(modifiers[0], 0) or math.isclose(modifiers[1], 0)):
                 self.warn('Definition of zero-width and/or zero-height rectangle or obround aperture. This is invalid according to spec.' )
 
-            new_aperture = kls(*modifiers, unit=self.file_settings.unit, attrs=self.aperture_attrs.copy(),
+            new_aperture = kls(*modifiers, unit=self.file_settings.unit, attrs=tuple(self.aperture_attrs.items()),
                     original_number=number)
 
         elif (macro := self.aperture_macros.get(match['shape'])):
-            new_aperture = apertures.ApertureMacroInstance(macro, modifiers, unit=self.file_settings.unit,
-                    attrs=self.aperture_attrs.copy(), original_number=number)
+            new_aperture = apertures.ApertureMacroInstance(macro, tuple(modifiers), unit=self.file_settings.unit,
+                    attrs=tuple(self.aperture_attrs.items()), original_number=number)
 
         else:
             raise ValueError(f'Aperture shape "{match["shape"]}" is unknown')