googlefonts / fontmake

Compile fonts from sources (UFO, Glyphs) to binary (OpenType, TrueType).
Apache License 2.0
771 stars 93 forks source link

No kerning in instances #625

Open weiweihuanghuang opened 4 years ago

weiweihuanghuang commented 4 years ago

I am using fontmake 2.0.8, and there are no errors returning, however there is no kerning in the instance .UFOs (or kerning exists but it's all 0, or +1).

I ran fontmake -m ProblemItalic-Static.designspace -i -o ttf --expand-features-to-instances

Please see a sample of the project, I have scrambled all the data including the kerning values, and the problem persists.

Problem.zip

weiweihuanghuang commented 4 years ago

I can see that now when I remove the intermediate masters (which has this empty array) the kerning interpolates.

madig commented 4 years ago

This is a weird one. Does the instance in Problem-Italic.ufo.zip look sane to you? Empty kerning dicts were pruned before interpolation and instances rounded.

If you want to test it yourself, modify the fontmake/instantiator.py file in your project venv:

def collect_kerning_masters(
    designspace: designspaceLib.DesignSpaceDocument, axis_bounds: AxisBounds
) -> List[Tuple[Location, FontMathObject]]:
    """Return master kerning objects wrapped by MathKerning."""
    locations_and_masters = []
    for source in designspace.sources:
        if source.layerName is not None:
            continue  # No kerning in source layers.

        if not source.font.kerning:
            continue

        # This assumes that groups of all sources are the same.
        normalized_location = varLib.models.normalizeLocation(
            source.location, axis_bounds
        )
        locations_and_masters.append(
            (
                normalized_location,
                fontMath.MathKerning(source.font.kerning, source.font.groups),
            )
        )

    return locations_and_masters
arrowtype commented 4 years ago

@madig Thanks so much for suggesting this solution!

Indeed, I was having a similar issue. More precisely:

If I had letters in an support/partial source such as r and a, the kerning in that pair would be treated as 0 at that spot.

But, after adding this to collect_kerning_masters, it fixed it (just trying to make this patch easy for others):

if not source.font.kerning:
            continue

Do you suspect this might cause unintended consequences? If not, would it be helpful for me to PR it into the repo?

anthrotype commented 4 years ago

yeah, that could work..

However I'd like to better understand the use case for using a whole UFO as Designspace source, as opposed to a non-default layer, when specifying intermediate sparse or partial masters.

There is an assumption currently in our pipeline that DS sources with layerName not None (None being the default layer) are treated as "sparse" and the glyphs that are present in them are included in the interpolation, whereas those that are missing are excluded; since they don't have kerning of their own, given kerning.plist refers to the default glyph set, they are not considered when interpolating kerning. Similarly they do not have a fontinfo.plist of their own, so they are not included in interpolation of font-wide data.

In @weiweihuanghuang example, the intermediate masters are distinct UFOs, not layers within the same UFO as another "whole" master. As such, they do have their own fontinfo.plist and kerning.plist. These may well be empty, perhaps deliberately so. But there's no way to tell whether one should interpret an empty kerning dict as a the intention to "mute" a master's kerning or treat the missing kern pairs as 0 valued.

mutatorMath/ufoProcessor have this <kerning mute="1/> attribute (parsed as source.muteKerning attribute) to indicate that the kerning data from a source needs to be muted. There's also one for info and for individual glyphs. None of these mute attributes are currently supported by fontmake.instantiator. varLib.build does not support these either when building a VF, also because "kerning" or "info" don't mean the same thing when when DS sources are UFOs, or TTFs.

It's a bit tricky.. We need to think more about this, and how the interpolation of statics vs VF from the same designspace data plays out in relation to this. We want to make sure that the static UFO instances and the instances produced at runtime from a VF at the same location can be seamlessly swapped with one another.

Now I wonder, if your intention is to mute the info and kerning for your intermediate masters, why then not use a layer within one of the whole UFO masters?

madig commented 4 years ago

E.g. Adobe Source Sans Pro is set up with full UFOs used as sparse sources: https://github.com/adobe-fonts/source-sans-pro/tree/master/Roman/Masters.

arrowtype commented 3 years ago

I ran into this issue again and was baffled why my static fonts had different kerning that my variable font. I guess I must have updated fontmake or been using another venv. šŸ˜…

image

To @anthrotypeā€™s question:

Now I wonder, if your intention is to mute the info and kerning for your intermediate masters, why then not use a layer within one of the whole UFO masters?

As a designer, it is more intuitive to use separate UFOs for support sources.

My general workflow, in RoboFont, is that the foreground layer is used for the current design of a given glyph, and then other layers are basically a place to store previous designs that I have improved upon, but want to keep around for reference or if I need to easily move backwards in a certain glyph.

I have also seen the background layer used for overlapped glyphs, then the foreground used for overlap-removed (and manually corrected) glyphs. Or, in making Italic fonts in the near future, I am planning to do this by skewing glyphs mathematically, storing them in a background layer, then correcting them manually in the foreground layer.

Layers are most often used for workflow purposes, but only the foreground layer is what I think of as "The Current Design" of any given style.

So, adding a layer just for intermediate sources would get pretty confusing, because it would be interacting with my ā€œjunk drawerā€ of scrapped or in-progress drawings.

Additionally, it is useful to have an overview of all my support glyphs for a given weight. So, in my project Name Sans I currently have full sources at wght 1, 700, and 1000, and support sources at wght 200. I have these support sources to control contrast, as strokes get too thin at joints or certain areas of a few glyphs. When I want to create a support source, I follow this basic process:

  1. Generate an interpolated instance
  2. Copy only required glyphs into the support source
  3. Edit that set of glyphs

So, as part of this process, it is very helpful to be able to have an overview of the glyphs in a support source. At least in RoboFont, I donā€™t think there is a built-in way to show only glyphs that have something in a certain layer. But, itā€™s still important for me to be able to view various sources together, an have an overview of all glyphs within each of them, like this:

image

My support sources will eventually have a few more glyphs, but never at all as many as the main sources have. This is why support sources are so nice! But, I find it very helpful to be able to manage them as specific entities/UFOs, rather than hard-to-see add-ons to a main source (as would be the case with support layers).

As one extra example: most type designers always draw a ā€œregularā€ source for their fonts, even though it tends to really be redundant data for many glyphs, if the project also has lighter & bolder sources. I am considering trying to draw my next project with full sources at ExtraLight & ExtraBold, then drawing just the core latin alphabet at Regular. This way, I could draw & space the font as though it were any other font (e.g. using the space center, making random strings & correcting spacing, etc), without having to draw everything over again. However, I would prefer to just use interpolated kerning at regular, if it works well. I donā€™t want to add more kerning to a type design workflow if itā€™s not necessary. So, in this case, it would be another reason to treat a sparse source as a UFO, rather than just in layers.

In @weiweihuanghuang example, the intermediate masters are distinct UFOs, not layers within the same UFO as another "whole" master. As such, they do have their own fontinfo.plist and kerning.plist. These may well be empty, perhaps deliberately so. But there's no way to tell whether one should interpret an empty kerning dict as a the intention to "mute" a master's kerning or treat the missing kern pairs as 0 valued.

Didnā€™t you give a solution for this in another issue? ā€œTry to add a com.github.googlei18n.ufo2ft.featureWriters key to the sparse UFO's lib.plist containing an empty array of feature writers.`

This is what Iā€™ve assumed does indicate that kerning values are deliberately empty, and it seems to be working to interpolate kerning in this way for the variable font.

If for some reason there would be another, better way to indicate that kerning is deliberately empty, I would be happy to use that instead.

anthrotype commented 3 years ago

The workaround you use to disable automatic features for sparse UFO masters is correct. When an interpolatable TTF master has no GPOS, it'll be ignored by varLib.build when creating the variable font. Similarly sparse "layers" are compiled to such intermediate master TTFs with no OTL tables (only sparse glyf, hmtx, etc.).

arrowtype commented 3 years ago

Oh my. Ha, now I am trying to influence kerning in one of my sparse sources, but not the others.

Specifically, Iā€™m hoping to let me wght=200 sources continue to not influence kerning, but I also need to fix the kerning of pairs like Tu, Ty, etc at wght=800+. Basically, Iā€™m hoping to solve this visual problem, shown by Just:

https://twitter.com/justvanrossum/status/811481272333778944?s=20

But, if I leave out the featureWriters lib key in hopes to preserve kerning pairs in the 800 partial source, I run into this build error:

fontmake: Error: In 'source/name_sans-varfontprep-2020_12_24-14_06_02/name_sans-wght_1_1000-opsz_12_96--w_sparse_supports.designspace': Generating fonts from Designspace failed: ((3, [3, 3, 1, 3, 3, 3, 3]), 'int', '.FeatureCount', 'FeatureList', '.FeatureList', 'GPOS', '.table', 'table_G_P_O_S_')

To get around this, I suppose I could try the following?

But, is there some better way Iā€™m missing to maybe do all my supports in UFOs, but only influence kerning with some of them but not all? E.g. could I somehow set a lib key to preserve vs ignore kerning?

arrowtype commented 3 years ago

Small update: I have attempted to just include interpolated kerning in sparse UFOs, and leave out the com.github.googlei18n.ufo2ft.featureWriters font lib item.

I have not yet tried exactly my idea above, of using sparse layers for contour adjustment and sparse sources for contour + kern adjustments. However, I have tested this in a simpler way: commenting out the 200 wght sparse supports (where I donā€™t want kern adjustments) in the designspace, and only leaving the 800 sparse source. This failed, again with the same error as above.

In fact, this makes me wonder whether kern adjustments are possible at all with sparse supports in a standard-ish FontMake workflow.

According to https://github.com/googlefonts/fontmake/issues/607#issuecomment-560521672, it seems that it may not be.

So, maybe I will have to do something clever with a post-build kerning adjustment?

Or, probably the more achievable solution: I can add an alt T, and kern this more loosely against lowercase y, n, x, etc. I can then swap to this with a rule at the heaviest weights.

Later Update: I took the alt T route, and it has been working well.

arrowtype commented 1 year ago

Hi @anthrotype, Iā€™m contending with this again, so Iā€™m hoping to revisit the earlier question, and maybe finally get this into the code base for the next time I update FontMake and use sparse UFO sources.

If I had letters in an support/partial source such as r and a, the kerning in that pair would be treated as 0 at that spot.

But, after adding this to collect_kerning_masters, it fixed it (just trying to make this patch easy for others):

if not source.font.kerning:
            continue

Do you suspect this might cause unintended consequences? If not, would it be helpful for me to PR it into the repo?

Part of your earlier response was:

We want to make sure that the static UFO instances and the instances produced at runtime from a VF at the same location can be seamlessly swapped with one another.

As far as I have experienced, keeping the kerning in generated static instances is the way to make sure the locations can be seamlessly swapped with the same locations in the VF.

Of course, I do fear there could be unintended consequences, as this codebase is way bigger than I understand. However, I would guess that this addition seems fairly low risk?

anthrotype commented 1 year ago

ok so you would like that an empty kerning dict in a designspace source's ufo be understood as an implicit signal to skip considering that source when collecting kerning masters. While this sounds sensible in general, it is still a change and I'm worried that there could be projects that rely on the current behavior where an empty kerning dict does participates in the interpolation (implicitly as all zero kerning pairs).

Now I just remembered that MutatorMath already had an attribute to "mute" kerning from a given source:

https://fonttools.readthedocs.io/en/latest/designspaceLib/xml.html#kerning-element-source

Perhaps we could use that? /cc @madig

anthrotype commented 1 year ago

looks like SourceDescriptor objects in designSpaceLib can have a muteKerning (bool) attribute (translated in the xml format as a <kerning mute="1"/>). The docs say it's currently only used by the old MutatorMath, but I can see this being useful here as well.

The problem, as I already stated ealier, is we want to make sure the static UFO instances outputted from fontmake.instantiator behave exactly (and can be used as drop-in replacement) as the VF built from the same designspace. That might mean ufo2ft.compileVariable methods and/or fontTools.varLib may also need to be made aware of this muteKerning and do the right thing..

madig commented 1 year ago

Uh, yeah, so I think this is another instance of "is missing kerning a zero-kern or just sparse?", where we still have to figure out what varLib is doing during font merging. An explicit attribute might help us here, but also: what Cosimo said.

anthrotype commented 1 year ago

This might mean ufo2ft.compileVariable methods and/or fontTools.varLib

I say "might" because maybe we don't need to change those, maybe these sparse UFO sources with no/empty kerning.plist already produce no GPOS table in the master interpolatable TTFs and they end up "not participating" when a VF is built from these? I haven't tried but @arrowtype might have

anthrotype commented 1 year ago

.. there is another workaround/hack which I would not really recommend, and relies on the fact that fontmake.instantiator already assumes that "sparse layers" (i.e. those sources whose layerName attribute is not None) do not participate in the info/kerning interpolation (only in the glyph interpolation)...

        if source.layerName is not None:
            continue  # No kerning in source layers.

so you might get away with setting the layer attribute for the sources which you do not want to have info/kerning participating, even setting it explicitly to the name of the default layer (layer="public.default" or layer="foreground" whichever your default layer is). But as I said, this would be a hack and should not be relied upon.

madig commented 1 year ago

They also won't have feature participation then.

arrowtype commented 1 year ago

looks like SourceDescriptor objects in designSpaceLib can have a muteKerning (bool) attribute (translated in the xml format as a ). The docs say it's currently only used by the old MutatorMath, but I can see this being useful here as well.

Yes, Iā€™d be happy if there were an attribute I could add to the source descriptor in a designspace, and that were used to mute the kerning!

I say "might" because maybe we don't need to change those, maybe these sparse UFO sources with no/empty kerning.plist already produce no GPOS table in the master interpolatable TTFs and they end up "not participating" when a VF is built from these? I haven't tried but @arrowtype might have

If I understand you correctly... yes, without any fontmake code hacks, those sparse UFO sources with no kerning do not zero out kerning at their locations in a variable font ā€“ the zero kerning only shows up in static font outputs, which makes this issue really easy to miss when building fonts.

They also won't have feature participation then.

I keep all my features the same across sources, as I want the variable font to have one set of features. This may be a silly question, but are there cases in which you would use different features between sources for a single variable font?

arrowtype commented 1 year ago

So... what is the recommended action to take here? What might the next steps be? Itā€™s stressful to need to hack fontmake/instantiator.py each time I pip install a fontmake update... I especially worry about the long run, where I may go for longer without doing a build, and then forget this hacking step if Iā€™m on a new computer, etc.

madig commented 1 year ago

Until a PR comes in that changes all the places for consistency, you could vendor instantiator.py into your projects. It's made for that.

anthrotype commented 1 year ago

setting the layer attribute for the sources which you do not want to have info/kerning participating, even setting it explicitly to the name of the default layer (layer="public.default" or layer="foreground" whichever your default layer is).

Have you tried with the workaround I suggested above?

arrowtype commented 1 year ago

Until a PR comes in that changes all the places for consistency, you could vendor instantiator.py into your projects. It's made for that.

Oh, hmm, I had considered forking fontmake to make this change, then installing from my fork, but... is that what you mean here, or is there a more formal approach? Is this Stack Overflow answer describing a method you would recommend? Please excuse my newness to this subject, and thank you for any guidance, if youā€™re willing to suggest something!

Have you tried with the workaround I suggested above?

Oh, sure enough, that does seem to work! I didnā€™t quite understand your suggestion at first, but (in case it helps someone else), I just added that layer attribute to the source tags for sparse sources:

    <source filename="sparse--Name_Sans-Display_500.ufo" familyname="AT Name Sans" stylename="Display Medium" layer="foreground">
      <location>
        <dimension name="Weight" xvalue="500"/>
        <dimension name="Optical Size" xvalue="72"/>
        <dimension name="Italic" xvalue="0"/>
      </location>
    </source>

You almost seem to recommend not doing that, that. "this would be a hack and should not be relied upon." Do you think this behavior is likely to change, or what are the risks of doing it this way?

anthrotype commented 1 year ago

That's definitely better than forking fontmake. It will continue to work until we find a better way.

arrowtype commented 1 year ago

Makes sense to me! Thanks for the suggestion.

madig commented 1 year ago

Please excuse my newness to this subject, and thank you for any guidance, if youā€™re willing to suggest something!

Specifically, the fontmake.instantiator module started life as a standalone module, and you should still be able to copy it out and talk to it directly.