Starting to tidy up

src/nanoemoji/glyph_reuse.py

@@ -30,18 +30,6 @@
 from .fixed import fixed_safe
 
 
-# TODO delete me?
-class GlyphInstruction(NamedTuple):
-    glyph_name: str
-    path: str
-    transform: Affine2D
-    is_reuse_target: bool  # whether other shapes reuse this one
-
-    @property
-    def is_reuse(self) -> bool:
-        return not self.transform.almost_equals(Affine2D.identity())
-
-
 @dataclasses.dataclass
 class GlyphReuseCache:
     _reusable_parts: ReusableParts
@@ -72,30 +60,19 @@ def try_reuse(self, path: str) -> ReuseInstruction:
             )
         return maybe_reuse
 
-    # TODO delete me?
-    def for_path(self, path: str) -> GlyphInstruction:
-        # TODO we are doing an awful lot of str <> SVGPath
-        assert path[0].upper() == "M", path
-        maybe_reuse = self.try_reuse(path)
-        if maybe_reuse.shape not in self._shape_to_glyph:
-            raise ValueError(
-                f"Must associate glyph names with paths, what is {maybe_reuse.shape}"
-            )
-        glyph_name = self._shape_to_glyph[maybe_reuse.shape]
-        return GlyphInstruction(
-            glyph_name,
-            maybe_reuse.shape,
-            maybe_reuse.affine,
-            self._reusable_parts.is_reused(SVGPath(d=path)),
-        )
-
     def set_glyph_for_path(self, glyph_name: str, path: str):
         norm = self._reusable_parts.normalize(path)
         assert norm in self._reusable_parts.shape_sets, f"No shape set for {path}"
         shape = parts.as_shape(SVGPath(d=path))
         assert (
             shape in self._reusable_parts.shape_sets[norm]
         ), f"Not present in shape set: {path}"
+
+        if self._shape_to_glyph.get(shape, glyph_name) != glyph_name:
+            raise ValueError(f"{shape} cannot be associated with glyphs")
+        if self._glyph_to_shape.get(glyph_name, shape) != shape:
+            raise ValueError(f"{glyph_name} cannot be associated with multiple shapes")
+
         self._shape_to_glyph[shape] = glyph_name
         self._glyph_to_shape[glyph_name] = shape
 

src/nanoemoji/svg.py

@@ -24,7 +24,7 @@
 from nanoemoji.color_glyph import ColorGlyph
 from nanoemoji.config import FontConfig
 from nanoemoji.disjoint_set import DisjointSet
-from nanoemoji.glyph_reuse import GlyphReuseCache, GlyphInstruction
+from nanoemoji.glyph_reuse import GlyphReuseCache
 from nanoemoji.paint import (
     _BasePaintTransform,
     CompositeMode,
@@ -79,21 +79,17 @@ class ReuseCache:
     glyph_elements: MutableMapping[str, etree.Element] = dataclasses.field(
         default_factory=dict
     )
-    glyph_instructions: MutableMapping[str, GlyphInstruction] = dataclasses.field(
-        default_factory=dict
-    )
     gradient_ids: MutableMapping[GradientReuseKey, str] = dataclasses.field(
         default_factory=dict
     )
 
     def add_glyph(
         self,
         glyph_name: str,
-        glyph_instruction: GlyphInstruction,
+        glyph_path: str,
     ):
         assert glyph_name not in self.glyph_elements, f"Second addition of {glyph_name}"
-        self.glyph_elements[glyph_name] = to_element(SVGPath(d=glyph_instruction.path))
-        self.glyph_instructions[glyph_name] = glyph_instruction
+        self.glyph_elements[glyph_name] = to_element(SVGPath(d=glyph_path))
 
     def reuse_spans_glyphs(self, path: str) -> bool:
         return (
@@ -142,15 +138,9 @@ def _glyph_groups(
 ) -> Tuple[Tuple[str, ...]]:
     """Find glyphs that need to be kept together by union find."""
 
-    # for the purpose of grouping, assign every path a name associated with the containing color glyph
+    # This cache is solely to help us group
     glyph_cache = GlyphReuseCache(reusable_parts)
 
-    paint_glyphs_for_color_glyphs = [list(_paint_glyphs(cg)) for cg in color_glyphs]
-    for color_glyph, paint_glyphs in zip(color_glyphs, paint_glyphs_for_color_glyphs):
-        for i, paint_glyph in enumerate(paint_glyphs):
-            glyph_name = _paint_glyph_name(color_glyph, i)
-            glyph_cache.set_glyph_for_path(glyph_name, paint_glyph.glyph)
-
     # Make sure we keep together color glyphs that share shapes
     reuse_groups = DisjointSet()  # ensure glyphs sharing shapes are in the same doc
     for color_glyph in color_glyphs:
@@ -163,17 +153,22 @@ def _glyph_groups(
                     continue
 
                 paint_glyph = cast(PaintGlyph, context.paint)
-                glyph_instruction = glyph_cache.for_path(paint_glyph.glyph)
-                assert (
-                    glyph_instruction.glyph_name
-                ), "All paths should be associated with a glyph by now"
+                maybe_reuse = glyph_cache.try_reuse(paint_glyph.glyph)
 
-                if glyph_instruction.is_reuse:
-                    # This entire color glyph and the one we share a shape with go in one svg doc
+                if glyph_cache.is_known_path(maybe_reuse.shape):
+                    # we've seen this exact path before, join the union with other consumers
                     reuse_groups.union(
                         color_glyph.ufo_glyph_name,
-                        _color_glyph_name(glyph_instruction.glyph_name),
+                        _color_glyph_name(
+                            glyph_cache.get_glyph_for_path(maybe_reuse.shape)
+                        ),
                     )
+                else:
+                    # I claim this path in the name of myself!
+                    # Use a path-specific name so each color glyph can register multiple paths
+                    paint_glyph_name = _paint_glyph_name(color_glyph, nth_paint_glyph)
+                    glyph_cache.set_glyph_for_path(paint_glyph_name, maybe_reuse.shape)
+
                 nth_paint_glyph += 1
 
     return reuse_groups.sorted()
@@ -557,10 +552,7 @@ def _add_glyph(svg: SVG, color_glyph: ColorGlyph, reuse_cache: ReuseCache):
                     reuse_cache.glyph_cache.set_glyph_for_path(
                         glyph_name, maybe_reuse.shape
                     )
-                    glyph_instruction = reuse_cache.glyph_cache.for_path(
-                        maybe_reuse.shape
-                    )
-                    reuse_cache.add_glyph(glyph_name, glyph_instruction)
+                    reuse_cache.add_glyph(glyph_name, maybe_reuse.shape)
                     el = reuse_cache.glyph_elements[glyph_name]
 
                     _apply_paint(

tests/glyph_reuse_test.py

@@ -14,8 +14,8 @@
 
 
 from nanoemoji.config import _DEFAULT_CONFIG
-from nanoemoji.glyph_reuse import GlyphReuseCache, GlyphInstruction
-from nanoemoji.parts import ReusableParts
+from nanoemoji.glyph_reuse import GlyphReuseCache
+from nanoemoji.parts import as_shape, ReuseInstruction, ReusableParts
 from picosvg.svg_transform import Affine2D
 from picosvg.svg_types import SVGPath
 import pytest
@@ -63,27 +63,19 @@ def test_small_then_large_circle():
         (
             "M-2,-2 L0,2 L2,-2 z",
             "M-1,-1 L0,1 L1,-1 z",
-            GlyphInstruction(
-                glyph_name="A",
-                path="path_a",
-                transform=Affine2D.identity().scale(0.5),
-                is_reuse_target=False,
+            ReuseInstruction(
+                affine=Affine2D.identity().scale(0.5),
+                shape=as_shape(SVGPath(d="M-2,-2 L0,2 L2,-2 z")),
             ),
         ),
     ],
 )
 def test_glyph_reuse_cache(path_a, path_b, expected_result):
-    if expected_result.path == "path_a":
-        expected_result = expected_result._replace(path=path_a)
-    if expected_result.path == "path_b":
-        expected_result = expected_result._replace(path=path_b)
-
     parts = ReusableParts(_DEFAULT_CONFIG.reuse_tolerance)
     parts.add(SVGPath(d=path_a))
     parts.add(SVGPath(d=path_b))
     reuse_cache = GlyphReuseCache(parts)
     reuse_cache.set_glyph_for_path("A", path_a)
     reuse_cache.set_glyph_for_path("B", path_b)
 
-    gi = reuse_cache.for_path(path_a)
-    assert reuse_cache.for_path(path_b) == expected_result
+    assert reuse_cache.try_reuse(path_b) == expected_result