Fix two problems: non-determinism and incorrect rule application in instanciator

[belluzj]

Hello, I pushed here a test that reproduces a bug with the instanciator when several designspace rules touch the same glyphs. I wrote a test designspace and a description of what should happen in the docstring of the test.

Instead of the expected result, there are two problems I would like to address in this PR:

• with the rules of the designspace provided as a test case, the output is wrong
• the output is also random, which can make it right sometimes by luck

The misbehaved code is below:

fontmake/Lib/fontmake/instantiator.py

Lines 376 to 383 in f07b193

	# Process rules
	glyph_names_list = self.glyph_mutators.keys()
	glyph_names_list_renamed = designspaceLib.processRules(
	self.designspace_rules, location, glyph_names_list
	)
	for name_old, name_new in zip(glyph_names_list, glyph_names_list_renamed):
	if name_old != name_new:
	swap_glyph_names(font, name_old, name_new)

Here is what I get if I put a breakpoint after line 380:

$ fontmake -m MyFont.designspace -o ttf -i
INFO:fontmake.font_project:Interpolating master UFOs from designspace
INFO:fontmake.font_project:Generating instance UFO for "My Font Style Black"
> c:\userslocal\jany.belluz\documents\code\fontmake\lib\fontmake\instantiator.py(382)generate_instance()
-> for name_old, name_new in zip(glyph_names_list, glyph_names_list_renamed):
(Pdb) glyph_names_list
dict_keys(['Q.ss01.alt', 'Q.ss01', 'Q.alt', 'Q'])
(Pdb) glyph_names_list_renamed
['Q.ss01.alt', 'Q.ss01.alt', 'Q.ss01.alt', 'Q.ss01.alt']

As you can see from the contents of glyph_names_list and glyph_names_list_renamed, the for loop will perform the following swaps:

1. Q.ss01 <-> Q.ss01.alt
2. Q.alt <-> Q.ss01.alt
3. Q <-> Q.ss01.alt

Initial

name	outlines
Q	Q
Q.alt	Q.alt
Q.ss01	Q.ss01
Q.ss01.alt	Q.ss01.alt

Step 1

name	outlines
Q	Q
Q.alt	Q.alt
Q.ss01	Q.ss01.alt
Q.ss01.alt	Q.ss01

Step 2

name	outlines
Q	Q
Q.alt	Q.ss01
Q.ss01	Q.ss01.alt
Q.ss01.alt	Q.alt

Step 3 = final

name	outlines
Q	Q.alt
Q.alt	Q.ss01
Q.ss01	Q.ss01.alt
Q.ss01.alt	Q

The end result is not right, because what we would want is that the Q gets the outlines of Q.ss01.alt.

And if you run the command again you get a different result, because glyph_name_list is keys from a dict comes from a set() at some point, so the order is random, see example below:

$ fontmake -m MyFont.designspace -o ttf -i
INFO:fontmake.font_project:Interpolating master UFOs from designspace
INFO:fontmake.font_project:Generating instance UFO for "My Font Style Black"
> c:\userslocal\jany.belluz\documents\code\fontmake\lib\fontmake\instantiator.py(382)generate_instance()
-> for name_old, name_new in zip(glyph_names_list, glyph_names_list_renamed):
(Pdb)  glyph_names_list
dict_keys(['Q.ss01.alt', 'Q', 'Q.alt', 'Q.ss01'])        # <--- different order here

Thinking about this by hand, here is what I think would make sense as a series of swaps (which is basically applying the designspace rules one after the other in order):

Initial

name	outlines
Q	Q
Q.alt	Q.alt
Q.ss01	Q.ss01
Q.ss01.alt	Q.ss01.alt

Apply rule 1: Q <-> Q.alt

name	outlines
Q	Q.alt
Q.alt	Q
Q.ss01	Q.ss01
Q.ss01.alt	Q.ss01.alt

Apply rule 2: Q.ss01 <-> Q.ss01.alt

name	outlines
Q	Q.alt
Q.alt	Q
Q.ss01	Q.ss01.alt
Q.ss01.alt	Q.ss01

Apply rule 3: Q <-> Q.ss01 and Q.alt <-> Q.ss01.alt

name	outlines
Q	Q.ss01.alt
Q.alt	Q.ss01
Q.ss01	Q.alt
Q.ss01.alt	Q

This gives a satisfying result for the main case (Q has the outlines of Q.ss01.alt) and also for the others, I feel, since the final swaps make sense (the style is changed so the ss01 has been swapped, and the weight is bold enough so the alt have been swapped). Do you agree that I should implement this result?

Next question: if I want to implement the result above, I need more information than what designspaceLib.processRules() returns, so I would need either to modify what that function returns (is that alright? Do other libraries rely on the previous output? I guess yes) or introduce another functions that returns what I want.

@anthrotype what do you think? Sorry for the long explanation!

[belluzj]

@anthrotype I mis-clicked to publish the PR before I had time to write the description, I wanted to make sure you got a notification now that I'm done writing.

@LettError I think you wrote designspaceLib.processRules(), what do you think about my proposed handling of the case above? Could that function be changed to return, for example, an ordered list of pairs of glyphs to swap? (or anything that allows me to implement the case above properly)

Thanks in advance for your advice.

[anthrotype]

Thanks Jany! I may not have time to look into this until the second week of September when I'll be back from holidays.

From a very quick check, I find a bit surprising that the dict keys are non deterministic, my understanding was that in Python >= 3.6 dictionaries keep items in insertion order. /cc @madig who authored this particular piece of code in fontmake

[anthrotype]

hm, this for loop that populates self.glyph_mutators iterates over a set (whose order is random even in python 3):

fontmake/Lib/fontmake/instantiator.py

Line 248 in f07b193

for glyph_name in glyph_names:

The glyph_names set is created here

fontmake/Lib/fontmake/instantiator.py

Line 199 in f07b193

glyph_names: Set[str] = set(default_font.keys())

[anthrotype]

processRules's docstrings emphasizes that "rules order matters", but I don't see why the order of the glyphNames should

https://github.com/fonttools/fonttools/blob/master/Lib/fontTools/designspaceLib/__init__.py#L205-L224

[belluzj]

Yes, I think the random ordering is purely a fontmake problem and has nothing to do with processRules()... I think I will fix the randomness here and also implement an alternative version of processRules here so that we can see how it could be different and comment on it, and later we can discuss if the alternative implementation should be upstreamed to designspaceLib somehow.

[anthrotype]

this looks good to me

[anthrotype]

Ideally we'd also have your processRulesSwaps in the upstream designspaceLib -- but I don't think that should block this.

I think ufoProcessor suffers from the same issue because it uses the current processRules where the order of glyphNames matters, however this glyphNames is created by turning a set into a list (hence order is random). /cc @LettError

[LettError]

What do you propose?

[anthrotype]

What do you propose?

the proposed processRulesSwaps in this PR solves the issue by returning a list of (oldName, newName) swaps ordered by rule older that doesn't depend on the ordering of the glyphNames list (which originally being a set has a random order). We should add a similar function to designspaceLib, or modify the current designspaceLib.processRules to do the same.

[belluzj]

Should I merge this?

[belluzj]

Oh actually I can't merge on this repository sorry! Please do if you're happy.

[anthrotype]

I can't merge on this repository

I just sent you an invite, please try again then -- thanks