[feaLib] Use otlLib for generating SinglePos tables

See https://github.com/behdad/fonttools/issues/471 for the change
to ValueRecords whose ValueFormat is zero; this may indicate a problem
in otlLib that needs to be fixed.

Resolves https://github.com/behdad/fonttools/issues/472.

Lib/fontTools/feaLib/builder.py

@@ -672,14 +672,7 @@ class Builder(object):
 
     def add_single_pos(self, location, glyph, valuerecord):
         lookup = self.get_lookup_(location, SinglePosBuilder)
-        curValue = lookup.mapping.get(glyph)
+        lookup.add_pos(location, glyph, valuerecord)
-        if curValue is not None and curValue != valuerecord:
-            otherLoc = valuerecord.location
-            raise FeatureLibError(
-                'Already defined different position for glyph "%s" at %s:%d:%d'
-                % (glyph, otherLoc[0], otherLoc[1], otherLoc[2]),
-                location)
-        lookup.mapping[glyph] = valuerecord
 
     def setGlyphClass_(self, location, glyph, glyphClass):
         oldClass, oldLocation = self.glyphClassDefs_.get(glyph, (None, None))
@@ -1313,77 +1306,28 @@ class ClassPairPosBuilder(LookupBuilder):
 class SinglePosBuilder(LookupBuilder):
     def __init__(self, font, location):
         LookupBuilder.__init__(self, font, location, 'GPOS', 1)
-        self.mapping = {}  # glyph -> ast.ValueRecord
+        self.locations = {}  # glyph -> (filename, line, column)
+        self.mapping = {}  # glyph -> otTables.ValueRecord
+
+    def add_pos(self, location, glyph, valueRecord):
+        otValueRecord, _ = makeOpenTypeValueRecord(valueRecord)
+        curValue = self.mapping.get(glyph)
+        if curValue is not None and curValue != otValueRecord:
+            otherLoc = self.locations[glyph]
+            raise FeatureLibError(
+                'Already defined different position for glyph "%s" at %s:%d:%d'
+                % (glyph, otherLoc[0], otherLoc[1], otherLoc[2]),
+                location)
+        if otValueRecord:
+            self.mapping[glyph] = otValueRecord
+        self.locations[glyph] = location
 
     def equals(self, other):
         return (LookupBuilder.equals(self, other) and
                 self.mapping == other.mapping)
 
     def build(self):
-        subtables = []
+        subtables = otl.buildSinglePos(self.mapping, self.glyphMap)
-
-        # If multiple glyphs have the same ValueRecord, they can go into
-        # the same subtable which saves space. Therefore, we first build
-        # a reverse mapping from ValueRecord to glyph coverage.
-        values = {}
-        for glyph, valuerecord in self.mapping.items():
-            values.setdefault(valuerecord, []).append(glyph)
-
-        # For compliance with the OpenType specification,
-        # we sort the glyph coverage by glyph ID.
-        for glyphs in values.values():
-            glyphs.sort(key=self.font.getGlyphID)
-
-        # Make a list of (glyphs, (otBase.ValueRecord, int valueFormat)).
-        # Glyphs with the same otBase.ValueRecord are grouped into one item.
-        values = [(glyphs, makeOpenTypeValueRecord(valrec))
-                  for valrec, glyphs in values.items()]
-
-        # Find out which glyphs should be encoded as SinglePos format 2.
-        # Format 2 is more compact than format 1 when multiple glyphs
-        # have different values but share the same integer valueFormat.
-        format2 = {}  # valueFormat --> [(glyph, value), (glyph, value), ...]
-        for glyphs, (value, valueFormat) in values:
-            if len(glyphs) == 1:
-                glyph = glyphs[0]
-                format2.setdefault(valueFormat, []).append((glyph, value))
-
-        # Only use format 2 if multiple glyphs share the same valueFormat.
-        # Otherwise, format 1 is more compact.
-        format2 = [(valueFormat, valueList)
-                   for valueFormat, valueList in format2.items()
-                   if len(valueList) > 1]
-        format2.sort()
-        format2Glyphs = set()  # {"A", "B", "C"}
-        for _, valueList in format2:
-            for (glyph, _) in valueList:
-                format2Glyphs.add(glyph)
-        for valueFormat, valueList in format2:
-            valueList.sort(key=lambda x: self.font.getGlyphID(x[0]))
-            st = otTables.SinglePos()
-            subtables.append(st)
-            st.Format = 2
-            st.ValueFormat = valueFormat
-            st.Coverage = otTables.Coverage()
-            st.Coverage.glyphs = [glyph for glyph, _value in valueList]
-            st.ValueCount = len(valueList)
-            st.Value = [value for _glyph, value in valueList]
-
-        # To make the ordering of our subtables deterministic,
-        # we sort subtables by the first glyph ID in their coverage.
-        # Not doing this would be OK for OpenType, but testing the
-        # compiler would be harder with non-deterministic output.
-        values.sort(key=lambda x: self.font.getGlyphID(x[0][0]))
-
-        for glyphs, (value, valueFormat) in values:
-            if len(glyphs) == 1 and glyphs[0] in format2Glyphs:
-                continue  # already emitted as part of a format 2 subtable
-            st = otTables.SinglePos()
-            subtables.append(st)
-            st.Format = 1
-            st.Coverage = otl.buildCoverage(glyphs, self.glyphMap)
-            st.Value, st.ValueFormat = value, valueFormat
-
         return self.buildLookup_(subtables)
 
 

Lib/fontTools/feaLib/builder_test.py

@@ -44,7 +44,7 @@ class BuilderTest(unittest.TestCase):
         Attach enum markClass language_required
         GlyphClassDef LigatureCaretByIndex LigatureCaretByPos
         lookup lookupflag feature_aalt
-        GPOS_1 GPOS_2 GPOS_2b GPOS_3 GPOS_4 GPOS_5 GPOS_6 GPOS_8
+        GPOS_1 GPOS_1_zero GPOS_2 GPOS_2b GPOS_3 GPOS_4 GPOS_5 GPOS_6 GPOS_8
         GSUB_2 GSUB_3 GSUB_6 GSUB_8
         spec4h1 spec5d1 spec5d2 spec5fi1 spec5fi2 spec5fi3 spec5fi4
         spec5h1 spec6d2 spec6e spec6f spec6h_ii spec8a

Lib/fontTools/feaLib/testdata/GPOS_1.ttx

@@ -40,8 +40,18 @@
       <Lookup index="0">
         <!-- LookupType=1 -->
         <LookupFlag value="0"/>
-        <!-- SubTableCount=8 -->
+        <!-- SubTableCount=7 -->
-        <SinglePos index="0" Format="2">
+        <SinglePos index="0" Format="1">
+          <Coverage>
+            <Glyph value="one"/>
+            <Glyph value="two"/>
+            <Glyph value="three"/>
+            <Glyph value="five"/>
+          </Coverage>
+          <ValueFormat value="5"/>
+          <Value XPlacement="-80" XAdvance="-160"/>
+        </SinglePos>
+        <SinglePos index="1" Format="2">
           <Coverage>
             <Glyph value="four"/>
             <Glyph value="six"/>
@@ -53,7 +63,16 @@
           <Value index="1" XAdvance="-200"/>
           <Value index="2" XAdvance="401"/>
         </SinglePos>
-        <SinglePos index="1" Format="2">
+        <SinglePos index="2" Format="1">
+          <Coverage>
+            <Glyph value="seven"/>
+            <Glyph value="eight"/>
+            <Glyph value="nine"/>
+          </Coverage>
+          <ValueFormat value="4"/>
+          <Value XAdvance="-100"/>
+        </SinglePos>
+        <SinglePos index="3" Format="2">
           <Coverage>
             <Glyph value="P"/>
             <Glyph value="Q"/>
@@ -65,7 +84,7 @@
           <Value index="1" XPlacement="1" XAdvance="801"/>
           <Value index="2" XPlacement="1" XAdvance="802"/>
         </SinglePos>
-        <SinglePos index="2" Format="2">
+        <SinglePos index="4" Format="2">
           <Coverage>
             <Glyph value="S"/>
             <Glyph value="T"/>
@@ -77,37 +96,12 @@
           <Value index="1" XPlacement="1" YPlacement="1" XAdvance="804"/>
           <Value index="2" XPlacement="1" YPlacement="1" XAdvance="805"/>
         </SinglePos>
-        <SinglePos index="3" Format="1">
-          <Coverage>
-            <Glyph value="zero"/>
-          </Coverage>
-          <ValueFormat value="0"/>
-        </SinglePos>
-        <SinglePos index="4" Format="1">
-          <Coverage>
-            <Glyph value="one"/>
-            <Glyph value="two"/>
-            <Glyph value="three"/>
-            <Glyph value="five"/>
-          </Coverage>
-          <ValueFormat value="5"/>
-          <Value XPlacement="-80" XAdvance="-160"/>
-        </SinglePos>
         <SinglePos index="5" Format="1">
-          <Coverage>
-            <Glyph value="seven"/>
-            <Glyph value="eight"/>
-            <Glyph value="nine"/>
-          </Coverage>
-          <ValueFormat value="4"/>
-          <Value XAdvance="-100"/>
-        </SinglePos>
-        <SinglePos index="6" Format="1">
           <Coverage>
             <Glyph value="A"/>
             <Glyph value="B"/>
           </Coverage>
-          <ValueFormat value="15"/>
+          <ValueFormat value="127"/>
           <Value XPlacement="1" YPlacement="2" XAdvance="3" YAdvance="4">
             <XPlaDevice>
               <StartSize value="11"/>
@@ -129,11 +123,11 @@
             </XAdvDevice>
           </Value>
         </SinglePos>
-        <SinglePos index="7" Format="1">
+        <SinglePos index="6" Format="1">
           <Coverage>
             <Glyph value="C"/>
           </Coverage>
-          <ValueFormat value="15"/>
+          <ValueFormat value="255"/>
           <Value XPlacement="1" YPlacement="2" XAdvance="3" YAdvance="4">
             <XPlaDevice>
               <StartSize value="11"/>

Lib/fontTools/feaLib/testdata/GPOS_1_zero.fea

@@ -0,0 +1,5 @@
+# https://github.com/behdad/fonttools/issues/471
+feature test {
+    pos zero 0;
+    pos four 500;
+} test;

Lib/fontTools/feaLib/testdata/GPOS_1_zero.ttx

@@ -0,0 +1,47 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<ttFont>
+ 
+  <GPOS>
+    <Version value="1.0"/>
+    <ScriptList>
+      <!-- ScriptCount=1 -->
+      <ScriptRecord index="0">
+        <ScriptTag value="DFLT"/>
+        <Script>
+          <DefaultLangSys>
+            <ReqFeatureIndex value="65535"/>
+            <!-- FeatureCount=1 -->
+            <FeatureIndex index="0" value="0"/>
+          </DefaultLangSys>
+          <!-- LangSysCount=0 -->
+        </Script>
+      </ScriptRecord>
+    </ScriptList>
+    <FeatureList>
+      <!-- FeatureCount=1 -->
+      <FeatureRecord index="0">
+        <FeatureTag value="test"/>
+        <Feature>
+          <!-- LookupCount=1 -->
+          <LookupListIndex index="0" value="0"/>
+        </Feature>
+      </FeatureRecord>
+    </FeatureList>
+    <LookupList>
+      <!-- LookupCount=1 -->
+      <Lookup index="0">
+        <!-- LookupType=1 -->
+        <LookupFlag value="0"/>
+        <!-- SubTableCount=1 -->
+        <SinglePos index="0" Format="1">
+          <Coverage>
+            <Glyph value="four"/>
+          </Coverage>
+          <ValueFormat value="4"/>
+          <Value XAdvance="500"/>
+        </SinglePos>
+      </Lookup>
+    </LookupList>
+  </GPOS>
+
+</ttFont>