Glyphs GSLayer vertOrigin and vertWidth aren't being saved

[punchcutter]

For vertical layout vertOrigin and vertWidth can be set in Glyphs, but these values aren't being used in glyphsLib. When going to defcon we need verticalOrigin to be set properly so that the vmtx table can be built correctly.

Also, in Glyphs if these values aren't explicitly set on some glyphs we still get values in the vmtx table. In both cases (explicitly set or not set) we are not getting the right values when running fontmake so vertical layout is completely mangled.

[punchcutter]

@anthrotype @schriftgestalt I'm not entirely sure what Glyphs is doing here, but making this "work" in glyphsLib gives completely different results with the calculations.

Here's what I added to glyphsLib/builder/builders.py to save these values in the UFO:

    def masters(self):
        """Get an iterator over master UFOs that match the given family_name.
        """
        if self._sources:
            for source in self._sources.values():
                yield source.font
            return

        # Store set of actually existing master (layer) ids. This helps with
        # catching dangling layer data that Glyphs may ignore, e.g. when
        # copying glyphs from other fonts with, naturally, different master
        # ids. Note: Masters have unique ids according to the Glyphs
        # documentation and can therefore be stored in a set.
        master_layer_ids = {m.id for m in self.font.masters}

        # stores background data from "associated layers"
        supplementary_layer_data = []

        # TODO(jamesgk) maybe create one font at a time to reduce memory usage
        # TODO: (jany) in the future, return a lazy iterator that builds UFOs
        #     on demand.
        self.to_ufo_font_attributes(self.family_name)

        # Generate the main (master) layers first.
        for glyph in self.font.glyphs:
            for layer in glyph.layers.values():
                if layer.associatedMasterId != layer.layerId:
                    # The layer is not the main layer of a master
                    # Store all layers, even the invalid ones, and just skip
                    # them and print a warning below.
                    supplementary_layer_data.append((glyph, layer))
                    continue

                ufo_layer = self.to_ufo_layer(glyph, layer)
                ufo_glyph = ufo_layer.newGlyph(glyph.name)
               +ufo_glyph.verticalOrigin = layer.vertOrigin
               +ufo_glyph.height = layer.vertWidth
                self.to_ufo_glyph(ufo_glyph, layer, glyph)

The calculation for the vmtx advanceHeight and topSideBearing is then done in ufo2ft

    def setupTable_vmtx(self):
        """
        Make the vmtx table.

        **This should not be called externally.** Subclasses
        may override or supplement this method to handle the
        table creation in a different way if desired.
        """
        if "vmtx" not in self.tables:
            return

        self.otf["vmtx"] = vmtx = newTable("vmtx")
        vmtx.metrics = {} 
        for glyphName, glyph in self.allGlyphs.items():
            height = otRound(glyph.height)
            if height < 0: 
                raise ValueError(
                    "The height should not be negative: '%s'" % (glyphName)
                )    
            verticalOrigin = _getVerticalOrigin(self.otf, glyph)
            bounds = self.glyphBoundingBoxes[glyphName]
            top = bounds.yMax if bounds else 0
            vmtx[glyphName] = (height, verticalOrigin - top) 

This gives completely different results from what Glyphs produces. I can't share the file here right now, but I can email it to both of you.

[schriftgestalt]

The vertOrigin is a delta to the default origin (the ascender). I decided this a long time ago and didn't change it since to not disturb existing files.

[punchcutter]

@schriftgestalt Ok, so in ufo2ft verticalOrigin shouldn't be the same as Glyphs vertOrigin, right? So verticalOrigin should actually be calculated as ascender - vertOrigin? Then topSideBearing would be verticalOrigin - top, right?

[schriftgestalt]

Then topSideBearing would be verticalOrigin - top, right?

Probably. It is a bit confusing as makeOTF expects different numbers as what goes in the vmtx table.

[punchcutter]

Ok, so Glyphs writes the vmtx in the feature file like

VertOriginY = typoAscender - vertOrigin
VertAdvanceY = vertWidth

The actual vmtx table leaves VertAdvanceY in advanceHeight, but the value for topSideBearing is VertOriginY - top of bounding box.

So to get the equivalent values in ufo2ft we need verticalOrigin to be passed from glyphsLib as typoAscender - vertOrigin. Then the existing ufo2ft code will generate the same vmtx as Glyphs...except for glyphs that don't have explicit settings. For those the advanceHeight is getting set to typoAscender - typoDescender which is what the FDK spec says. In ufo2ft that's not happening so we don't get identical vmtx tables.

[anthrotype]

@punchcutter Thanks for investigating this issue. Yes, support for vertical typesetting in fontmake is poor. I hope to find the time to look into this in the coming weeks.
But If you want to work on PR I or Nikolaus can help with reviewing.

[anthrotype]

...except for glyphs that don't have explicit settings. For those the advanceHeight is getting set to typoAscender - typoDescender which is what the FDK spec says. In ufo2ft that's not happening...

the problem is defcon Glyph defaults to height = 0.

https://github.com/robotools/defcon/blob/f39457c79b90b680ca07ed8fc511bc1fd7d3c2fc/Lib/defcon/objects/glyph.py#L123

ufo2ft is simply taking that at face value. If the default glyph height is 0, how can one distinguish between a true 0 height from one which was not explicitly set and thus should default to typoAscender - typoDescender?

[anthrotype]

then i guess glyphsLib should set the UFO glyph height accordingly. That is, when vertWidth is present, use that; if not present, then it should store it as (typoAscender - typoDescender)

[anthrotype]

argh... we have the same problem when parsing vertWidth into a GSFont in glyphsLib. When vertWidth is missing in the .glyphs file, it currently defaults to 0, so we can't tell a real vertWidth of 0 from one that is not set and should default to typoAscender - typoDescender...

[anthrotype]

when vertOrigin and vertWidth are not set in a GSLayer they should defaut to None. I'll fix that.

[anthrotype]

@schriftgestalt how does Glyphs.app decide whether to produce vhea and vmtx tables? What are the minimum pieces of data required?
For ufo2ft, we look at whether openTypeVheaVertTypo(Ascender|Descender|LineGap) are defined in the UFO fontinfo.plist, and if so we build vhea and vmtx (googlefonts/fontmake#586).

[anthrotype]

It seems that Glyphs.app produces a vhea and vmtx any time there is at least one glyph containing a master layer where either vertOrigin or vertWidth is explicitly set to a non-default value.

[anthrotype]

I've thought about this again. I think we don't have to change the UFO spec default value for height, that can stay 0 as it is now.
What would be sufficient for implementing vertical typesetting support from .glyphs sources in fontmake is:

1. in glyphsLib, upon parsing a GSLayer vertWidth and vertOrigin, if these are missing in the source, default to None and not to 0.
2. upon exporting .glyphs -> UFO, check if any glyph master layer contain either vertOrigin or vertWidth explicitly set; if so, consider this font suitable for vertical typesetting
3. if .glyphs font contain such "vertical" glyph metrics, in the generated UFO export verticalOrigin (lib key) and height attributes for all the glyphs; as @schriftgestalt confirmed above, we need to set the UFO glyph.lib["public.verticalOrigin"] => typoAscender - GSLayer.vertOrigin to match Glyphs.app meaning of vertOrigin; the UFO glyph.height we should set to the explicit vertWidth (if any), or if None to typoAscender - typoDescender (like Glyphs.app and FDK use as default height).

Then ufo2ft should be able to build vhea and vmtx and VORG tables from the generated UFOs containing the above metrics data (and Noto Mongolian https://github.com/googlefonts/noto-fonts/issues/1414 can be fixed).

[aaronbell]

I am working to prep a CJK font for release on Google Fonts (https://github.com/Fontworks-TechService/Reggae – I have requested that it be made public but may not be yet) that requires support for Glyphs' vertWidth and vertOrigin values in order to build the vmtx table to align with the version in the AFDKO version.

This branch contains the updated .glyphs file, as well as the build.py file that I am using to generate the finalized .ttf file, if it would be useful for testing.

Right now, this bug is blocking me from completing work on this release :).

[m4rc1e]

@aaronbell the branch url 404s since it's a private repo. Can you send me the font? I'm working on a fix.

[aaronbell]

Yeah, I'd requested that they open it up but I think they want it to be perfect first. Anyway, I'll email you the font.

[kokiabe]

I have glyphsLib v5.3.1 installed, but if vertWidth is 0, it will not be reflected in vmtx.AdvancedHeight. Is 'vertWidth=0' not allowed?
https://github.com/fontworks-fonts/Train/tree/fix_outline

[anthrotype]

@kokiabe thanks for the report.

I suspect it might be this line where it checks if layer.vertWidth:

glyphsLib/Lib/glyphsLib/builder/glyph.py

Line 280 in 8422053

if layer.vertWidth:

which would return False if vertWidth == 0, and thus the if branch is not executed in that case. I think the fix should be to check if layer.vertWidth is not None: instead, but I haven't tried.

/cc @m4rc1e

[anthrotype]

I applied that change in #659
@kokiabe could you try with glyphsLib from the fix-vert-zero branch and see if that fixes it?

Unfortunately I don't have time right now to write proper tests or check that the logic works when going the other way around (from .glyphs to UFO).

[kokiabe]

@anthrotype Thank you for your support! The fix-vert-zero branch is working as I expected.

[schriftgestalt]

I should add support for those vertical values when writing .ufo file. My initial approach would be to just write out the values what are set in the .glyphs file. But I suspect that would not be enough? So do I need to write default values for all glyphs (if at least one glyph has them set)?

[anthrotype]

just write out the values what are set in the .glyphs file

are you talking about glyph.height and "public.verticalOrigin" key? Well, the problem is that those have differing default values in Glyphs.app and UFO, so you can't "just" write them out, you'd have to follow the translation logic encoded in glyphsLib. See #629 and #659