robotools / fontParts

The replacement for RoboFab
MIT License
135 stars 44 forks source link

height and topMargin #445

Open simoncozens opened 5 years ago

simoncozens commented 5 years ago

As I understand it, topMargin ("The glyph’s top margin") is meant to be the distance between the top of the glyph bounding box and the ink bounding box. This is implemented (for UFO) by subtracting yMax from height:

        xMin, yMin, xMax, yMax = bounds
        return self.height - yMax

self.height in turn comes from defcon. But height in the UFO glyph file is optional and is used for vertical layout ("The vertical advance. Optional if width is defined.") So it is normally zero:

>>> from fontParts.world import OpenFont
>>> f = OpenFont("Source Sans Pro-Regular.ufo")
>>> f.layers[0]["zeta"].height
0

This leads to topMargin and bottomMargin being nonsensical:

>>> f.layers[0]["zeta"].topMargin
-712
>>> f.layers[0]["zeta"].bottomMargin
-184

Shouldn't the height of a layer come instead from one of the vertical metrics in the fontinfo table? And if so (at risk of starting a major fight) which one?

anthrotype commented 5 years ago

fonttools uses the hhea.ascent when vmtx is not present to compute the vertical phantom points (and the head.unitsPerEm for the vertical advance)

anthrotype commented 5 years ago

topMargin should be the distance between the glyph's vertical origin and the top of the glyph's bounding box (yMax). The vertical origin is defined in public.verticalOrigin inside the glif lib (https://github.com/unified-font-object/ufo-spec/pull/41). defcon exposes that as verticalOrigin attribute of a glyph. https://github.com/robotools/defcon/blob/14f7b94d55b4bde841927da693d610d5ae8ead27/Lib/defcon/objects/glyph.py#L437-L439

i think the problem is here, when verticalOrigin is None, defcon is subtracting height - yMax to compute to topMargin: https://github.com/robotools/defcon/blob/14f7b94d55b4bde841927da693d610d5ae8ead27/Lib/defcon/objects/glyph.py#L353

instead it should use the font's ascender. Height is the vertical advance, it's needed to compute the bottomMargin, not the top one.