Repository: googlefonts/glyphsLib
Pull request #629 (2020-11-03 20:45:14)

Improve vertical typesetting
This pr will solve #597 

My implementation is based on Cosimo's https://github.com/googlefonts/glyphsLib/issues/557#issuecomment-667074856.



Patch:
diff --git a/Lib/glyphsLib/builder/builders.py b/Lib/glyphsLib/builder/builders.py
index 78b626bd..30b1c374 100644
--- a/Lib/glyphsLib/builder/builders.py
+++ b/Lib/glyphsLib/builder/builders.py
@@ -130,6 +130,13 @@ class UFOBuilder(_LoggerMixin):
         self._designspace = self.designspace_module.DesignSpaceDocument()
         self._designspace_is_complete = False
 
+        # If any glyph layer which is associated with a GSFontMaster has the
+        # properties "vertWidth" or "vertOrigin" set, Glyphsapp will assume
+        # the font is going to be used for vertical typesetting. When
+        # Glyphsapp generates these fonts, it will include the vhea, vmtx,
+        # VORG tables. VORG will only be included if the font is an otf.
+        self.is_vertical = self._is_vertical()
+
         # check that source was generated with at least stable version 2.3
         # https://github.com/googlefonts/glyphsLib/pull/65#issuecomment-237158140
         if int(font.appVersion) < 895:
@@ -160,6 +167,16 @@ class UFOBuilder(_LoggerMixin):
             # instances with matching 'familyName' custom parameter
             self._do_filter_instances_by_family = True
 
+    def _is_vertical(self):
+        master_ids = {m.id for m in self.font.masters}
+        for glyph in self.font.glyphs:
+            for layer in glyph.layers:
+                if layer.layerId not in master_ids:
+                    continue
+                if layer.vertWidth is not None or layer.vertOrigin is not None:
+                    return True
+        return False
+
     @property
     def masters(self):
         """Get an iterator over master UFOs that match the given family_name.
@@ -500,7 +517,6 @@ class UFOBuilder(_LoggerMixin):
     from .custom_params import to_ufo_custom_params
     from .features import to_ufo_features
     from .font import to_ufo_font_attributes
-    from .glyph import to_ufo_glyph, to_ufo_glyph_background
     from .groups import to_ufo_groups
     from .guidelines import to_ufo_guidelines
     from .hints import to_ufo_hints
@@ -511,6 +527,11 @@ class UFOBuilder(_LoggerMixin):
     from .names import to_ufo_names
     from .paths import to_ufo_paths
     from .sources import to_designspace_sources
+    from .glyph import (
+        to_ufo_glyph,
+        to_ufo_glyph_background,
+        to_ufo_glyph_height_and_vertical_origin,
+    )
     from .user_data import (
         to_designspace_family_user_data,
         to_ufo_family_user_data,
@@ -749,19 +770,6 @@ class GlyphsBuilder(_LoggerMixin):
                         )
                     )
                     setattr(source, name, getattr(source.font.info, name))
-        # An axis without a mapping will see its range information (min and max
-        # values) lost when converted to a Glyps.app file. To combat this we
-        # add an explicit identity mapping.
-        for axis in copy.axes:
-            if axis.map:
-                continue
-            if axis.minimum == axis.maximum:
-                axis.map = [(axis.minimum, axis.minimum)]
-            else:
-                axis.map = [
-                    (axis.minimum, axis.minimum),
-                    (axis.maximum, axis.maximum),
-                ]
         return copy
 
     def _fake_designspace(self, ufos):
@@ -838,7 +846,7 @@ class GlyphsBuilder(_LoggerMixin):
     from .custom_params import to_glyphs_custom_params
     from .features import to_glyphs_features
     from .font import to_glyphs_font_attributes, to_glyphs_ordered_masters
-    from .glyph import to_glyphs_glyph
+    from .glyph import to_glyphs_glyph, to_glyphs_glyph_height_and_vertical_origin
     from .groups import to_glyphs_groups
     from .guidelines import to_glyphs_guidelines
     from .hints import to_glyphs_hints
diff --git a/Lib/glyphsLib/builder/custom_params.py b/Lib/glyphsLib/builder/custom_params.py
index 78578924..b82f8ff5 100644
--- a/Lib/glyphsLib/builder/custom_params.py
+++ b/Lib/glyphsLib/builder/custom_params.py
@@ -322,6 +322,9 @@ GLYPHS_UFO_CUSTOM_PARAMS = (
     # ('widthClass', 'openTypeOS2WidthClass'),
     # ('winAscent', 'openTypeOS2WinAscent'),
     # ('winDescent', 'openTypeOS2WinDescent'),
+    ("vheaVertAscender", "openTypeVheaVertTypoAscender"),
+    ("vheaVertDescender", "openTypeVheaVertTypoDescender"),
+    ("vheaVertLineGap", "openTypeVheaVertTypoLineGap"),
     ("vheaVertTypoAscender", "openTypeVheaVertTypoAscender"),
     ("vheaVertTypoDescender", "openTypeVheaVertTypoDescender"),
     ("vheaVertTypoLineGap", "openTypeVheaVertTypoLineGap"),
diff --git a/Lib/glyphsLib/builder/glyph.py b/Lib/glyphsLib/builder/glyph.py
index cb9f1744..f3a355e0 100644
--- a/Lib/glyphsLib/builder/glyph.py
+++ b/Lib/glyphsLib/builder/glyph.py
@@ -26,7 +26,7 @@ ORIGINAL_WIDTH_KEY = GLYPHLIB_PREFIX + "originalWidth"
 BACKGROUND_WIDTH_KEY = GLYPHLIB_PREFIX + "backgroundWidth"
 
 
-def to_ufo_glyph(self, ufo_glyph, layer, glyph):
+def to_ufo_glyph(self, ufo_glyph, layer, glyph):  # noqa: C901
     """Add .glyphs metadata, paths, components, and anchors to a glyph."""
     ufo_font = self._sources[layer.associatedMasterId or layer.layerId].font
 
@@ -138,6 +138,8 @@ def to_ufo_glyph(self, ufo_glyph, layer, glyph):
     self.to_ufo_paths(ufo_glyph, layer)
     self.to_ufo_components(ufo_glyph, layer)
     self.to_ufo_glyph_anchors(ufo_glyph, layer.anchors)
+    if self.is_vertical:
+        self.to_ufo_glyph_height_and_vertical_origin(ufo_glyph, layer)
 
 
 def to_glyphs_glyph(self, ufo_glyph, ufo_layer, master):  # noqa: C901
@@ -256,6 +258,40 @@ def to_glyphs_glyph(self, ufo_glyph, ufo_layer, master):  # noqa: C901
     self.to_glyphs_paths(ufo_glyph, layer)
     self.to_glyphs_components(ufo_glyph, layer)
     self.to_glyphs_glyph_anchors(ufo_glyph, layer)
+    self.to_glyphs_glyph_height_and_vertical_origin(ufo_glyph, master, layer)
+
+
+def to_ufo_glyph_height_and_vertical_origin(self, ufo_glyph, layer):
+    # implentation based on:
+    # https://github.com/googlefonts/glyphsLib/issues/557#issuecomment-667074856
+    assert self.is_vertical
+
+    ascender, descender = _get_typo_ascender_descender(layer.master)
+
+    if layer.vertWidth:
+        ufo_glyph.height = layer.vertWidth
+    else:
+        ufo_glyph.height = ascender - descender
+
+    if layer.vertOrigin:
+        ufo_glyph.verticalOrigin = ascender - layer.vertOrigin
+    else:
+        ufo_glyph.verticalOrigin = ascender
+
+
+def _get_typo_ascender_descender(master):
+    # Glyphsapp will use the typo metrics to set the verOrigin and
+    # vertWidth. If typo metrics are not present, the master
+    # ascender and descender are used instead.
+    if "typoAscender" in master.customParameters:
+        ascender = master.customParameters["typoAscender"]
+    else:
+        ascender = master.ascender
+    if "typoDescender" in master.customParameters:
+        descender = master.customParameters["typoDescender"]
+    else:
+        descender = master.descender
+    return ascender, descender
 
 
 def to_ufo_glyph_background(self, glyph, layer):
@@ -291,3 +327,12 @@ def _to_glyphs_color(color):
     # round-trip the actual value in later versions.
     # https://github.com/googlefonts/glyphsLib/pull/363#issuecomment-390418497
     return [round(float(component) * 255) for component in color.split(",")]
+
+
+def to_glyphs_glyph_height_and_vertical_origin(self, ufo_glyph, master, layer):
+    if ufo_glyph.height:
+        layer.vertWidth = ufo_glyph.height
+
+    if ufo_glyph.verticalOrigin:
+        ascender, _ = _get_typo_ascender_descender(master)
+        layer.vertOrigin = ascender - ufo_glyph.verticalOrigin
diff --git a/Lib/glyphsLib/builder/masters.py b/Lib/glyphsLib/builder/masters.py
index 2a8682e4..6c3e4f3f 100644
--- a/Lib/glyphsLib/builder/masters.py
+++ b/Lib/glyphsLib/builder/masters.py
@@ -71,6 +71,26 @@ def to_ufo_master_attributes(self, source, master):
                 user_loc = axis.get_user_loc(master)
                 axis.set_ufo_user_loc(ufo, user_loc)
 
+    # Set vhea values to glyphsapp defaults if they haven't been declared.
+    # ufo2ft needs these set in order for a ufo to be recognised as
+    # vertical. Glyphsapp uses the font upm, not the typo metrics
+    # for these.
+    if self.is_vertical:
+        custom_params = master.customParameters
+        font_upm = self.font.upm
+        if not any(
+            k in custom_params for k in ("vheaVertAscender", "vheaVertTypoAscender")
+        ):
+            ufo.info.openTypeVheaVertTypoAscender = int(font_upm / 2)
+        if not any(
+            k in custom_params for k in ("vheaVertDescender", "vheaVertTypoDescender")
+        ):
+            ufo.info.openTypeVheaVertTypoDescender = -int(font_upm / 2)
+        if not any(
+            k in custom_params for k in ("vheaVertLineGap", "vheaVertTypoLineGap")
+        ):
+            ufo.info.openTypeVheaVertTypoLineGap = font_upm
+
     self.to_ufo_blue_values(ufo, master)
     self.to_ufo_guidelines(ufo, master)
     self.to_ufo_master_user_data(ufo, master)
diff --git a/Lib/glyphsLib/classes.py b/Lib/glyphsLib/classes.py
index 5958d13a..0ef8d64a 100755
--- a/Lib/glyphsLib/classes.py
+++ b/Lib/glyphsLib/classes.py
@@ -3003,8 +3003,8 @@ class GSLayer(GSBase):
         "leftMetricsKey": None,
         "rightMetricsKey": None,
         "widthMetricsKey": None,
-        "vertWidth": 0,
-        "vertOrigin": 0,
+        "vertWidth": None,
+        "vertOrigin": None,
     }
     _wrapperKeysTranslate = {"guideLines": "guides", "background": "_background"}
     _keyOrder = (


Test patch:
diff --git a/tests/builder/axes_test.py b/tests/builder/axes_test.py
index a3f49ccf..06423ac4 100644
--- a/tests/builder/axes_test.py
+++ b/tests/builder/axes_test.py
@@ -13,7 +13,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from copy import deepcopy
 import os.path
 
 import pytest
@@ -82,9 +81,6 @@ def _make_designspace_with_axes(axes, ufo_module):
         axis = doc.newAxisDescriptor()
         axis.tag = tag
         axis.name = name
-        axis.minimum = 0
-        axis.default = 0
-        axis.maximum = 100
         doc.addAxis(axis)
 
         extreme = doc.newSourceDescriptor()
@@ -351,60 +347,8 @@ def test_axis_mapping(ufo_module):
     assert doc.axes[1].map != wdth_mapping
     assert doc.axes[1].map == []
 
+    # When we convert back to glyphs, the wdth mapping isn't present because
+    # it was removed during the designspace conversion.
     font = to_glyphs(doc)
+    axis_mappings["wdth"] = {}
     assert font.customParameters["Axis Mappings"] == axis_mappings
-
-
-def test_axis_with_no_mapping_does_not_error_in_roundtrip(ufo_module):
-    """Tests that a custom axis without a mapping and without sources on its
-    extremes does not generate an error during roundtrip. Also tests that
-    during a to_glyphs, to_designspace roundtrip the min and max axis
-    information is not lost.
-    """
-    doc = designspaceLib.DesignSpaceDocument()
-
-    # Add a "Regular" source
-    regular = doc.newSourceDescriptor()
-    regular.font = ufo_module.Font()
-    regular.location = {"Style": 0}
-    doc.addSource(regular)
-
-    axis = doc.newAxisDescriptor()
-    axis.tag = "styl"
-    axis.name = "Style"
-    doc.addAxis(axis)
-
-    # This axis spans a range of 0 to 1 but only has a source at {"Style": 0}
-    # and no explicit mapping. The point of this test is to see if the min and
-    # max are still the same after round tripping.
-    doc.axes[0].minimum = 0
-    doc.axes[0].maximum = 1
-    doc.axes[0].default = 0
-    doc.axes[0].map = []
-
-    doc2 = deepcopy(doc)
-    font = to_glyphs(doc2)
-    doc_rt = to_designspace(font)
-
-    assert doc_rt.axes[0].serialize() == doc.axes[0].serialize()
-
-
-def test_axis_with_no_mapping_does_not_error_in_roundtrip_with_2_axes(ufo_module):
-    """Tests that a designspace with 2 axis, one with a mapping and one
-    without a mapping, roundtrips correctly without error. The axis without a
-    mapping should generate an identity mapping on the fly so that the
-    Glyphs.app customParameter field does not lose min/max infromation about
-    the axis.
-    """
-    doc = _make_designspace_with_axes(
-        [("wght", "Weight with mapping"), ("wdth", "Width without mapping")], ufo_module
-    )
-    # Add mapping to weight axis
-    doc.axes[0].map = [(0, 0), (50, 350), (100, 1000)]
-
-    doc2 = deepcopy(doc)
-    font = to_glyphs(doc2)
-    doc_rt = to_designspace(font)
-
-    assert doc_rt.axes[0].serialize() == doc.axes[0].serialize()
-    assert doc_rt.axes[1].serialize() == doc.axes[1].serialize()
diff --git a/tests/builder/to_glyphs_test.py b/tests/builder/to_glyphs_test.py
index 40e3267d..0fc5b946 100644
--- a/tests/builder/to_glyphs_test.py
+++ b/tests/builder/to_glyphs_test.py
@@ -252,6 +252,56 @@ def test_glyph_color(ufo_module):
     assert ufo["b"].markColor == b.markColor
 
 
+def test_glyph_height_and_vertical_origin(ufo_module):
+    ufo = ufo_module.Font()
+    ufo.info.ascender = 1000
+    ufo.info.descender = -200
+    ufo.info.unitsPerEm = 1000
+
+    a = ufo.newGlyph("a")
+    a.height = 1024
+    a.verticalOrigin = 100
+    ufo.newGlyph("b")
+
+    font = to_glyphs([ufo])
+
+    assert font.glyphs["a"].layers[0].vertWidth == ufo["a"].height
+    assert font.glyphs["a"].layers[0].vertOrigin == 900
+    assert font.glyphs["b"].layers[0].vertWidth is None
+    assert font.glyphs["b"].layers[0].vertOrigin is None
+
+    (ufo,) = to_ufos(font)
+
+    assert ufo["a"].height == font.glyphs["a"].layers[0].vertWidth
+    assert ufo["a"].verticalOrigin == 100
+    assert ufo["b"].height == (ufo.info.ascender - ufo.info.descender)
+    assert ufo["b"].verticalOrigin == 1000
+
+
+def test_glyph_height_and_vertical_origin_trigger_vhea_metrics(ufo_module):
+    ufo = ufo_module.Font()
+    ufo.info.ascender = 1000
+    ufo.info.descender = -200
+    ufo.info.unitsPerEm = 1000
+
+    a = ufo.newGlyph("a")
+    a.height = 1024
+    a.verticalOrigin = 100
+
+    font = to_glyphs([ufo])
+
+    assert all(
+        "vhea" not in custom_param.name
+        for custom_param in font.masters[0].customParameters
+    )
+
+    (ufo,) = to_ufos(font)
+
+    assert ufo.info.openTypeVheaVertTypoAscender == 500
+    assert ufo.info.openTypeVheaVertTypoDescender == -500
+    assert ufo.info.openTypeVheaVertTypoLineGap == 1000
+
+
 def test_bad_ufo_date_format_in_glyph_lib(ufo_module):
     ufo = ufo_module.Font()
     a = ufo.newGlyph("a")