googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
151 stars 43 forks source link

Variable anchor positioning in sparse masters #640

Closed simoncozens closed 21 hours ago

simoncozens commented 2 years ago

This came up while I was testing #635. Consider a test which compiles this designspace file:

https://github.com/googlefonts/ufo2ft/blob/7d0a330626da6e4999349f3f416bfa508b2826ef/tests/conftest.py#L54-L93

So we have regular at 350, regular again at 450 and bold at 625. Fine. My question is, how does the positioning of the e glyph's top anchor vary over the designspace?

At 350 it is regular, so:

https://github.com/googlefonts/ufo2ft/blob/7d0a330626da6e4999349f3f416bfa508b2826ef/tests/data/LayerFont-Regular.ufo/glyphs/e.glif#L5

At 450 it is still regular, so the same.

At 625, it is bold, so:

https://github.com/googlefonts/ufo2ft/blob/7d0a330626da6e4999349f3f416bfa508b2826ef/tests/data/LayerFont-Bold.ufo/glyphs/e.glif#L5

So the X coordinate of the anchor goes 314 at wght=350, 314 at wght=450, and 315 at wght=650. The Y coordinate goes 556 at wght=350, 556 at wght=450, and 644 at wght=625.

Except that what we test for, the output TTX file, is:

https://github.com/googlefonts/ufo2ft/blob/7d0a330626da6e4999349f3f416bfa508b2826ef/tests/data/TestVariableFont-TTF.ttx#L320-L339

That's a linear progression from x=314, y=556 at wght=350 through to x=315, y=644 at wght=650. The medium stop has vanished. In fact, if you ask it to produce a debug feature file, the medium isn't there!

### LayerFont-Regular ###
feature liga {
    sub a e s s by s;
} liga;

markClass dotabovecomb <anchor -2 465> @MC_top;

feature mark {
    lookup mark2base {
        pos base e
            <anchor 314 556> mark @MC_top;
    } mark2base;

} mark;

### LayerFont-Bold ###
feature liga {
    sub a e s s by s;
} liga;

markClass dotabovecomb <anchor -2 465> @MC_top;

feature mark {
    lookup mark2base {
        pos base e
            <anchor 315 644> mark @MC_top;
    } mark2base;

} mark;

I think this is because the medium is the regular, and since we compiled that once, we don't compile it again. And so we end up with the wrong answer. With my PR, we get a debug feature file of:

feature liga {
    sub a e s s by s;
} liga;

markClass dotabovecomb <anchor -2 465> @MC_top;

feature mark {
    lookup mark2base {
        pos base e
            <anchor (wght=350:314 wght=450:314 wght=625:315) (wght=350:556 wght=450:556 wght=625:644)> mark @MC_top;
    } mark2base;

} mark;

which clearly looks better, and compiles to:

        <!-- RegionCount=2 -->
         <Region index="0">
           <VarRegionAxis index="0">
             <StartCoord value="0.0"/>
            <PeakCoord value="0.36365"/>
            <EndCoord value="1.0"/>
          </VarRegionAxis>
        </Region>
        <Region index="1">
          <VarRegionAxis index="0">
            <StartCoord value="0.36365"/>
             <PeakCoord value="1.0"/>
             <EndCoord value="1.0"/>
           </VarRegionAxis>
        </Region> 
       </VarRegionList> 
       <!-- VarDataCount=1 --> 
       <VarData index="0"> 
         <!-- ItemCount=2 --> 
         <NumShorts value="0"/> 
         <!-- VarRegionCount=1 --> 
         <VarRegionIndex index="0" value="1"/> 
         <Item index="0" value="[1]"/>
         <Item index="1" value="[88]"/>
       </VarData>

which I believe to be correct - but which doesn't pass tests.

anthrotype commented 2 years ago

we have regular at 350, regular again at 450

no, in the conftest.py::designspace fixture, the second source is not the same as the first, the filename is the same "LayerFont-Regular.ufo", but note how the second has the layerName = "Medium" attribute which the first omits. The first source is a normal one, the second is a "sparse layer". I'm not sure what's going on in the rest of your test, just wanted to clarify that.

simoncozens commented 2 years ago

Ohhhh. So the "medium" isn't a second master, and shouldn't affect layout?

anthrotype commented 2 years ago

hm.. Well, currently we don't compile features for sparse layers since they can't have features.fea of their own, we only use them for the outlines..

simoncozens commented 2 years ago

Right, but what about anchors...?

anthrotype commented 2 years ago

don't know, what do you expect to happen?

anthrotype commented 2 years ago

maybe with your variable features.fea you can take those into account, the anchors are still in the glyph, just need to get them from the named layer

simoncozens commented 2 years ago

The issue is that the variable feature build is taking into account the anchor positions of anchors in sparse master layers, and the current HEAD does not take into account the anchor positions of anchors in sparse master layers, and I don't know which is the right behaviour.

anthrotype commented 2 years ago

I would say taking them into account would be the right behavior, so yours is the correct one. We currently don't because sparse layer masters have no kerning.plist or features.fea of their own, they are just containers of glyph shapes (and yes anchors, so it happens), so we can't/don't write/compile OT features for the master TTFs that are produced off those

anthrotype commented 2 years ago

does your code handle the possibility that given anchors are missing from a sparse layer master and treat them as non-participating in the interpolation? that would allow a font developer to control whether they want or not the glyph anchors in sparse layers to contribute to variable mark/mkmk features. Right now they seem to be ignored.

khaledhosny commented 21 hours ago

This should be fixed now.