diff --git a/src/nanoemoji/glyph_reuse.py b/src/nanoemoji/glyph_reuse.py index 4f4a1c12..7184f7c4 100644 --- a/src/nanoemoji/glyph_reuse.py +++ b/src/nanoemoji/glyph_reuse.py @@ -72,23 +72,6 @@ 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}" @@ -96,6 +79,12 @@ def set_glyph_for_path(self, glyph_name: str, path: str): 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 diff --git a/src/nanoemoji/svg.py b/src/nanoemoji/svg.py index 383ed146..f5827a55 100644 --- a/src/nanoemoji/svg.py +++ b/src/nanoemoji/svg.py @@ -79,9 +79,6 @@ 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 ) @@ -89,11 +86,10 @@ class ReuseCache: 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( diff --git a/tests/glyph_reuse_test.py b/tests/glyph_reuse_test.py index f69b30e1..0e942712 100644 --- a/tests/glyph_reuse_test.py +++ b/tests/glyph_reuse_test.py @@ -15,7 +15,7 @@ from nanoemoji.config import _DEFAULT_CONFIG from nanoemoji.glyph_reuse import GlyphReuseCache, GlyphInstruction -from nanoemoji.parts import ReusableParts +from nanoemoji.parts import as_shape, ReuseInstruction, ReusableParts from picosvg.svg_transform import Affine2D from picosvg.svg_types import SVGPath import pytest @@ -63,21 +63,14 @@ 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)) @@ -85,5 +78,4 @@ def test_glyph_reuse_cache(path_a, path_b, expected_result): 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