Variation alternates not processed for statics

[yanone]

I'm not sure whether this is intentional, but it feels a lot like an oversight:
ufo2ft (both stable release as well as repo version) isn’t processing the alternates for static fonts, see below (static vs. variable).

The master UFO do contain the relevant substitutions (and they work in variable):

    <rule name="BRACKET.Weight_150_220">
      <conditionset>
        <condition name="Weight" minimum="150" maximum="220"/>
      </conditionset>
      <sub name="peso" with="peso.BRACKET.varAlt01"/>
      <sub name="won" with="won.BRACKET.varAlt01"/>
      <sub name="naira" with="naira.BRACKET.varAlt01"/>
      <sub name="dollar" with="dollar.BRACKET.varAlt01"/>
      <sub name="cent" with="cent.BRACKET.varAlt01"/>
      <sub name="cedi" with="cedi.BRACKET.varAlt01"/>
      <sub name="gstroke.sc" with="gstroke.sc.BRACKET.varAlt01"/>
      <sub name="gstroke" with="gstroke.BRACKET.varAlt01"/>
      <sub name="slashlongcomb.sc" with="slashlongcomb.sc.BRACKET.varAlt01"/>
      <sub name="slashlongcomb" with="slashlongcomb.BRACKET.varAlt01"/>
      <sub name="slashshortcomb" with="slashshortcomb.BRACKET.varAlt01"/>
      <sub name="oslashacute.sc" with="oslashacute.sc.BRACKET.varAlt01"/>
      <sub name="oslash.sc" with="oslash.sc.BRACKET.varAlt01"/>
      <sub name="oslashacute" with="oslashacute.BRACKET.varAlt01"/>
      <sub name="oslash" with="oslash.BRACKET.varAlt01"/>
      <sub name="Oslashacute" with="Oslashacute.BRACKET.varAlt01"/>
      <sub name="Oslash" with="Oslash.BRACKET.varAlt01"/>
    </rule>

Always happy to help with further debugging. It's a commercial font so I can't post a source but can invite you to my repo.

[anthrotype]

interpolated static fonts are not handled inside ufo2ft actually, but in fontmake.instatiator module, see:

fontmake/Lib/fontmake/instantiator.py

Lines 425 to 432 in 6a8b290

	# Process rules
	glyph_names_list = self.glyph_mutators.keys()
	# The order of the swaps below is independent of the order of glyph names.
	# It depends on the order of the <sub>s in the designspace rules.
	swaps = process_rules_swaps(self.designspace_rules, location, glyph_names_list)
	for name_old, name_new in swaps:
	if name_old != name_new:
	swap_glyph_names(font, name_old, name_new)

if you are using fontmake to build the fonts, it should work.

[anthrotype]

I moved the issue to fontmake repository where I think it belongs. If you could, it'd be good to upload a test file that reproduces the issue.

[yanone]

It's something in my source file, but I can't say what.

I created a test file, removed features, some font info metadata (nothing axis-related) and all glyphs except $ and S (as a component), and it works.

Generating from the original source fails the swap.

Generated with fontmake -g tests/data/GlyphSwapWorks.glyphs -i -o ttf.

I added the two test files (tests/data/GlyphSwapWorks.glyphs and tests/data/GlyphSwapFails.glyphs) to a branch here: https://github.com/googlefonts/fontmake/tree/fix-glyph-swap/tests/data

I'm gonna need an adult here.

[yanone]

Update: The pair ('dollar', 'dollar.BRACKET.varAlt01') does show up correctly in the list returned by process_rules_swaps(), so the error must be in swap_glyph_names(). I'm trying to dig deeper

[yanone]

Update:

I kind of got to the bottom: process_rules_swaps() adds the swap twice, leading to no changed appearance after two swaps.

Personally, I would just add this and be done with it.

Understandably though, you may want to find out why it's getting added more than once.

Printing self.designspace_rules reveals that the dollar swap shows up a total of five times, for all locations with wght>120:

self.designspace_rules [RuleDescriptor(
    name='BRACKET.Weight_155_220',
    conditionSets=[[{'name': 'Weight', 'minimum': 155, 'maximum': 220}]],
    subs=[('Gstroke', 'Gstroke.BRACKET.varAlt01'), ('guarani', 'guarani.BRACKET.varAlt01'), ('peso', 'peso.BRACKET.varAlt01'), ('won', 'won.BRACKET.varAlt01'), ('naira', 'naira.BRACKET.varAlt01'), ('dollar', 'dollar.BRACKET.varAlt01'), ('cent', 'cent.BRACKET.varAlt01'), ('cedi', 'cedi.BRACKET.varAlt01'), ('gstroke.sc', 'gstroke.sc.BRACKET.varAlt01'), ('gstroke', 'gstroke.BRACKET.varAlt01'), ('slashlongcomb.sc', 'slashlongcomb.sc.BRACKET.varAlt01'), ('slashlongcomb', 'slashlongcomb.BRACKET.varAlt01'), ('slashshortcomb', 'slashshortcomb.BRACKET.varAlt01'), ('oslashacute.sc', 'oslashacute.sc.BRACKET.varAlt01'), ('oslash.sc', 'oslash.sc.BRACKET.varAlt01'), ('oslashacute', 'oslashacute.BRACKET.varAlt01'), ('oslash', 'oslash.BRACKET.varAlt01'), ('Oslashacute', 'Oslashacute.BRACKET.varAlt01'), ('Oslash', 'Oslash.BRACKET.varAlt01')],
), RuleDescriptor(
    name='BRACKET.Weight_150_155',
    conditionSets=[[{'name': 'Weight', 'minimum': 150, 'maximum': 155}]],
    subs=[('Gstroke', 'Gstroke.BRACKET.varAlt01'), ('peso', 'peso.BRACKET.varAlt01'), ('won', 'won.BRACKET.varAlt01'), ('naira', 'naira.BRACKET.varAlt01'), ('dollar', 'dollar.BRACKET.varAlt01'), ('cent', 'cent.BRACKET.varAlt01'), ('cedi', 'cedi.BRACKET.varAlt01'), ('gstroke.sc', 'gstroke.sc.BRACKET.varAlt01'), ('gstroke', 'gstroke.BRACKET.varAlt01'), ('slashlongcomb.sc', 'slashlongcomb.sc.BRACKET.varAlt01'), ('slashlongcomb', 'slashlongcomb.BRACKET.varAlt01'), ('slashshortcomb', 'slashshortcomb.BRACKET.varAlt01'), ('oslashacute.sc', 'oslashacute.sc.BRACKET.varAlt01'), ('oslash.sc', 'oslash.sc.BRACKET.varAlt01'), ('oslashacute', 'oslashacute.BRACKET.varAlt01'), ('oslash', 'oslash.BRACKET.varAlt01'), ('Oslashacute', 'Oslashacute.BRACKET.varAlt01'), ('Oslash', 'Oslash.BRACKET.varAlt01')],
), RuleDescriptor(
    name='BRACKET.Weight_150_220',
    conditionSets=[[{'name': 'Weight', 'minimum': 150, 'maximum': 220}]],
    subs=[('peso', 'peso.BRACKET.varAlt01'), ('won', 'won.BRACKET.varAlt01'), ('naira', 'naira.BRACKET.varAlt01'), ('dollar', 'dollar.BRACKET.varAlt01'), ('cent', 'cent.BRACKET.varAlt01'), ('cedi', 'cedi.BRACKET.varAlt01'), ('gstroke.sc', 'gstroke.sc.BRACKET.varAlt01'), ('gstroke', 'gstroke.BRACKET.varAlt01'), ('slashlongcomb.sc', 'slashlongcomb.sc.BRACKET.varAlt01'), ('slashlongcomb', 'slashlongcomb.BRACKET.varAlt01'), ('slashshortcomb', 'slashshortcomb.BRACKET.varAlt01'), ('oslashacute.sc', 'oslashacute.sc.BRACKET.varAlt01'), ('oslash.sc', 'oslash.sc.BRACKET.varAlt01'), ('oslashacute', 'oslashacute.BRACKET.varAlt01'), ('oslash', 'oslash.BRACKET.varAlt01'), ('Oslashacute', 'Oslashacute.BRACKET.varAlt01'), ('Oslash', 'Oslash.BRACKET.varAlt01')],
), RuleDescriptor(
    name='BRACKET.Weight_130_150',
    conditionSets=[[{'name': 'Weight', 'minimum': 130, 'maximum': 150}]],
    subs=[('Gstroke', 'Gstroke.BRACKET.varAlt01'), ('peso', 'peso.BRACKET.varAlt01'), ('won', 'won.BRACKET.varAlt01'), ('naira', 'naira.BRACKET.varAlt01'), ('dollar', 'dollar.BRACKET.varAlt01'), ('cent', 'cent.BRACKET.varAlt01'), ('cedi', 'cedi.BRACKET.varAlt01'), ('gstroke.sc', 'gstroke.sc.BRACKET.varAlt01'), ('gstroke', 'gstroke.BRACKET.varAlt01')],
), RuleDescriptor(
    name='BRACKET.Weight_120_150',
    conditionSets=[[{'name': 'Weight', 'minimum': 120, 'maximum': 150}]],
    subs=[('peso', 'peso.BRACKET.varAlt01'), ('won', 'won.BRACKET.varAlt01'), ('naira', 'naira.BRACKET.varAlt01'), ('dollar', 'dollar.BRACKET.varAlt01'), ('cent', 'cent.BRACKET.varAlt01'), ('cedi', 'cedi.BRACKET.varAlt01'), ('gstroke.sc', 'gstroke.sc.BRACKET.varAlt01'), ('gstroke', 'gstroke.BRACKET.varAlt01')],
), RuleDescriptor(
    name='BRACKET.Weight_100_120',
    conditionSets=[[{'name': 'Weight', 'minimum': 100, 'maximum': 120}]],
    subs=[('peso', 'peso.BRACKET.varAlt01')],
)]

which leads me to believe that the error is actually in designspaceLib.evaluateRule()

[anthrotype]

I think that logically the substitution should only ever be applied once, but because of the way this is implemented in fontmake.instatiator, literally swapping outlines while keeping both glyphs around, if one attempts to do it again, it has the effect of restoring the initial state. Maybe it'd make sense to patch it like you proposed above..

[anthrotype]

yeah, I think that makes sense. If this were a GSUB kind of substitution, and multiple consecutive rules wanted to replace the same glyph to something else, only the first one would actually trigger, subsequent ones would be no-op. So it makes sense for the process_rules_swaps() to work the same way for interpolated statics.

[yanone]

Do you want me to write a test for it? I don't really know how to design it, especially given that my sparse source works and the full source fails.

Or shall I just PR the one little change as is?

[anthrotype]

looks like many of the rules overlap one another in the DS file generated from your glyphs source, which may be the reason the same glyphs get swapped multiple times..

If I look at the glyphsLib code that is responsible for producing this, I see that it calls fontTools.featureVars' overlayFeatureVariations method which is exactly meant for splitting rules into non-overlapping boxes, but that # Reduce the mappings to a single mapping loop looks very suspicious:

https://github.com/googlefonts/glyphsLib/blob/8c0157e4fba7a97fc9fbd4e4b532bcf92796c06f/Lib/glyphsLib/builder/bracket_layers.py#L48-L57

@simoncozens I think you wrote that piece of code. Instead of unioning the mappings, should this not add one DS rule per (box, mapping) as returned from overlay method?

[anthrotype]

@yanone yeah it's no immediately obvious how to come up with a minimal reproducer, but the fix looks correct from a logical point of view. Perhaps just check if oldName is in replaced and keep a set of oldNames that were already swapped, it doesn't matter the newName if oldName has been replaced already.
Yes please send a PR

[yanone]

-> #1043