From d0d8039bd3aef42b9385a2bf3b77a832dc623233 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Thu, 31 Mar 2022 19:14:09 -0600 Subject: [PATCH 1/9] [varLib.interpolatable] Check for wrong contour starting point This seems to work already. Detects the example in the issue. I also ran this on master-compatible UFOs built from Noto Sans, and detected several issues. Confirmed visuall in AxisPraxis that theta.sc for example has wrong starting point in that font: Glyph theta.sc was not compatible: Contour start point differs: NotoSans-DisplayRegular, NotoSans-DisplaySemiBoldCondensed Contour start point differs: NotoSans-DisplayRegular, NotoSans-DisplaySemiBoldCondensed Contour start point differs: NotoSans-DisplaySemiBoldCondensed, NotoSans-DisplaySemiBold Contour start point differs: NotoSans-DisplaySemiBoldCondensed, NotoSans-DisplaySemiBold There's a TODO item left to be done, which is to check for mirrored contours and rotations thereof. Towards fixing https://github.com/fonttools/fonttools/issues/1801 --- Lib/fontTools/varLib/interpolatable.py | 92 +++++++++++++++++++++++++- 1 file changed, 89 insertions(+), 3 deletions(-) diff --git a/Lib/fontTools/varLib/interpolatable.py b/Lib/fontTools/varLib/interpolatable.py index cff76eceb..2d359339c 100644 --- a/Lib/fontTools/varLib/interpolatable.py +++ b/Lib/fontTools/varLib/interpolatable.py @@ -7,6 +7,7 @@ $ fonttools varLib.interpolatable font1 font2 ... """ from fontTools.pens.basePen import AbstractPen, BasePen +from fontTools.pens.pointPen import SegmentToPointPen from fontTools.pens.recordingPen import RecordingPen from fontTools.pens.statisticsPen import StatisticsPen from fontTools.pens.momentsPen import OpenContourError @@ -14,6 +15,14 @@ from collections import OrderedDict import itertools import sys +def _rot_list(l, k): + """Rotate list by k items forward. Ie. item at position 0 will be + at position k in returned list. Negative k is allowed.""" + n = len(l) + k %= n + if not k: return l + return l[n-k:] + l[:n-k] + class PerContourPen(BasePen): def __init__(self, Pen, glyphset=None): @@ -55,6 +64,21 @@ class PerContourOrComponentPen(PerContourPen): self.value[-1].addComponent(glyphName, transformation) +class RecordingPointPen(BasePen): + + def __init__(self): + self.value = [] + + def beginPath(self, identifier = None, **kwargs): + pass + + def endPath(self) -> None: + pass + + def addPoint(self, pt, segmentType=None): + self.value.append((pt, False if segmentType is None else True)) + + def _vdiff(v0, v1): return tuple(b - a for a, b in zip(v0, v1)) @@ -65,6 +89,12 @@ def _vlen(vec): v += x * x return v +def _complex_vlen(vec): + v = 0 + for x in vec: + v += abs(x) * abs(x) + return v + def _matching_cost(G, matching): return sum(G[i][j] for i, j in enumerate(matching)) @@ -125,6 +155,7 @@ def test(glyphsets, glyphs=None, names=None): try: allVectors = [] allNodeTypes = [] + allContourIsomorphisms = [] for glyphset, name in zip(glyphsets, names): # print('.', end='') if glyph_name not in glyphset: @@ -140,13 +171,16 @@ def test(glyphsets, glyphs=None, names=None): del perContourPen contourVectors = [] + contourIsomorphisms = [] nodeTypes = [] allNodeTypes.append(nodeTypes) allVectors.append(contourVectors) + allContourIsomorphisms.append(contourIsomorphisms) for ix, contour in enumerate(contourPens): - nodeTypes.append( - tuple(instruction[0] for instruction in contour.value) - ) + + nodeVecs = tuple(instruction[0] for instruction in contour.value) + nodeTypes.append(nodeVecs) + stats = StatisticsPen(glyphset=glyphset) try: contour.replay(stats) @@ -168,6 +202,31 @@ def test(glyphsets, glyphs=None, names=None): contourVectors.append(vector) # print(vector) + # Check starting point + if nodeVecs[0] == 'addComponent': + continue + assert nodeVecs[0] == 'moveTo' + assert nodeVecs[-1] == 'closePath' + points = RecordingPointPen() + converter = SegmentToPointPen(points, False) + contour.replay(converter) + # points.value is a list of pt,bool where bool is true if on-curve and false if off-curve; + # now check all rotations and mirror-rotations of the contour and build list of isomorphic + # possible starting points. + #print(points.value) + bits = 0 + for pt,b in points.value: + bits = (bits << 1) | b + n = len(points.value) + mask = (1 << n ) - 1 + isomorphisms = [] + contourIsomorphisms.append(isomorphisms) + for i in range(n): + b = ((bits << i) & mask) | ((bits >> (n - i))) + if b == bits: + isomorphisms.append((i, _rot_list ([complex(*pt) for pt,bl in points.value], i))) + # TODO Add mirrors + # Check each master against the next one in the list. for i, (m0, m1) in enumerate(zip(allNodeTypes[:-1], allNodeTypes[1:])): if len(m0) != len(m1): @@ -253,6 +312,25 @@ def test(glyphsets, glyphs=None, names=None): }, ) + for i, (m0, m1) in enumerate(zip(allContourIsomorphisms[:-1], allContourIsomorphisms[1:])): + if not m0: + assert not m1 + continue + for contour0,contour1 in zip(m0,m1): + c0 = contour0[0][1] + costs = [v for v in (_complex_vlen(_vdiff(c0, c1)) for i1,c1 in contour1)] + min_cost = min(costs) + first_cost = costs[0] + if min_cost < first_cost: + add_problem( + glyph_name, + { + "type": "wrong_start_point", + "master_1": names[i], + "master_2": names[i + 1], + }, + ) + except ValueError as e: add_problem( glyph_name, @@ -351,6 +429,14 @@ def main(args=None): p["master_2"], ) ) + if p["type"] == "wrong_start_point": + print( + " Contour start point differs: %s, %s" + % ( + p["master_1"], + p["master_2"], + ) + ) if p["type"] == "high_cost": print( " Interpolation has high cost: cost of %s to %s = %i, threshold %i" From 3a846a5389897b064adc828930372bea710b5fd3 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Thu, 31 Mar 2022 19:37:00 -0600 Subject: [PATCH 2/9] [interpolatable] Remove the empirical high-cost error This was very empirical and has no theoretical background. Experiment shows that this is mostly false-positive. --- Lib/fontTools/varLib/interpolatable.py | 28 -------------------------- 1 file changed, 28 deletions(-) diff --git a/Lib/fontTools/varLib/interpolatable.py b/Lib/fontTools/varLib/interpolatable.py index 2d359339c..174aa33e1 100644 --- a/Lib/fontTools/varLib/interpolatable.py +++ b/Lib/fontTools/varLib/interpolatable.py @@ -213,7 +213,6 @@ def test(glyphsets, glyphs=None, names=None): # points.value is a list of pt,bool where bool is true if on-curve and false if off-curve; # now check all rotations and mirror-rotations of the contour and build list of isomorphic # possible starting points. - #print(points.value) bits = 0 for pt,b in points.value: bits = (bits << 1) | b @@ -294,23 +293,6 @@ def test(glyphsets, glyphs=None, names=None): }, ) break - upem = 2048 - item_cost = round( - (matching_cost / len(m0) / len(m0[0])) ** 0.5 / upem * 100 - ) - hist.append(item_cost) - threshold = 7 - if item_cost >= threshold: - add_problem( - glyph_name, - { - "type": "high_cost", - "master_1": names[i], - "master_2": names[i + 1], - "value_1": item_cost, - "value_2": threshold, - }, - ) for i, (m0, m1) in enumerate(zip(allContourIsomorphisms[:-1], allContourIsomorphisms[1:])): if not m0: @@ -437,16 +419,6 @@ def main(args=None): p["master_2"], ) ) - if p["type"] == "high_cost": - print( - " Interpolation has high cost: cost of %s to %s = %i, threshold %i" - % ( - p["master_1"], - p["master_2"], - p["value_1"], - p["value_2"], - ) - ) if problems: return problems From 745631d16b3d4e08cbeefc74bec6671dbd2e8c81 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Thu, 31 Mar 2022 19:52:48 -0600 Subject: [PATCH 3/9] [interpolatable] In diff-contour-order, allow for 5% cost difference Reduces false-positives, as in this one in NotoSansArabic: Glyph asteriskArt-ar was not compatible: Contour order differs: [0, 1, 2, 3, 4, 5] in NotoSansArabic-CondensedBold, [0, 3, 2, 1, 4, 5] in NotoSansArabic-CondensedLight --- Lib/fontTools/varLib/interpolatable.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/fontTools/varLib/interpolatable.py b/Lib/fontTools/varLib/interpolatable.py index 174aa33e1..e57ded6f4 100644 --- a/Lib/fontTools/varLib/interpolatable.py +++ b/Lib/fontTools/varLib/interpolatable.py @@ -281,7 +281,9 @@ def test(glyphsets, glyphs=None, names=None): continue costs = [[_vlen(_vdiff(v0, v1)) for v1 in m1] for v0 in m0] matching, matching_cost = min_cost_perfect_bipartite_matching(costs) - if matching != list(range(len(m0))): + identity_matching = list(range(len(m0))) + identity_cost = sum(costs[i][i] for i in range(len(m0))) + if matching != identity_matching and matching_cost < identity_cost * .95: add_problem( glyph_name, { From b705bcdc8fccab7bbf8d4dca3ffe8186c2073c99 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Thu, 31 Mar 2022 20:16:55 -0600 Subject: [PATCH 4/9] [interpolatable] When checking for contour start point error allow for 5% error --- Lib/fontTools/varLib/interpolatable.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/fontTools/varLib/interpolatable.py b/Lib/fontTools/varLib/interpolatable.py index e57ded6f4..5abd3cb1a 100644 --- a/Lib/fontTools/varLib/interpolatable.py +++ b/Lib/fontTools/varLib/interpolatable.py @@ -305,7 +305,7 @@ def test(glyphsets, glyphs=None, names=None): costs = [v for v in (_complex_vlen(_vdiff(c0, c1)) for i1,c1 in contour1)] min_cost = min(costs) first_cost = costs[0] - if min_cost < first_cost: + if min_cost < first_cost * .95: add_problem( glyph_name, { From 9e96b954678fc4f550016a0d56f724bf6ce3a6d1 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Thu, 31 Mar 2022 20:18:44 -0600 Subject: [PATCH 5/9] [interpolatable] Don't keep contour-start-point index --- Lib/fontTools/varLib/interpolatable.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/fontTools/varLib/interpolatable.py b/Lib/fontTools/varLib/interpolatable.py index 5abd3cb1a..b79f313f0 100644 --- a/Lib/fontTools/varLib/interpolatable.py +++ b/Lib/fontTools/varLib/interpolatable.py @@ -223,7 +223,7 @@ def test(glyphsets, glyphs=None, names=None): for i in range(n): b = ((bits << i) & mask) | ((bits >> (n - i))) if b == bits: - isomorphisms.append((i, _rot_list ([complex(*pt) for pt,bl in points.value], i))) + isomorphisms.append(_rot_list ([complex(*pt) for pt,bl in points.value], i)) # TODO Add mirrors # Check each master against the next one in the list. @@ -301,8 +301,8 @@ def test(glyphsets, glyphs=None, names=None): assert not m1 continue for contour0,contour1 in zip(m0,m1): - c0 = contour0[0][1] - costs = [v for v in (_complex_vlen(_vdiff(c0, c1)) for i1,c1 in contour1)] + c0 = contour0[0] + costs = [v for v in (_complex_vlen(_vdiff(c0, c1)) for c1 in contour1)] min_cost = min(costs) first_cost = costs[0] if min_cost < first_cost * .95: From 3165cc132ac4967f43c3512d31a9c4cf0d4da7a7 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Thu, 31 Mar 2022 20:21:51 -0600 Subject: [PATCH 6/9] [interpolatable] Add mirrored rotated contour for starting-point check This further found an issue in NotoSansArabic which I visually verified: Glyph qafLamAlefMaksuraabove-ar was not compatible: Contour start point differs: NotoSansArabic-CondensedLight, NotoSansArabic-CondensedSemiBold Contour start point differs: NotoSansArabic-CondensedLight, NotoSansArabic-CondensedSemiBold Contour start point differs: NotoSansArabic-CondensedSemiBold, NotoSansArabic-Condensed Contour start point differs: NotoSansArabic-CondensedSemiBold, NotoSansArabic-Condensed --- Lib/fontTools/varLib/interpolatable.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/Lib/fontTools/varLib/interpolatable.py b/Lib/fontTools/varLib/interpolatable.py index b79f313f0..2954a15cb 100644 --- a/Lib/fontTools/varLib/interpolatable.py +++ b/Lib/fontTools/varLib/interpolatable.py @@ -224,7 +224,15 @@ def test(glyphsets, glyphs=None, names=None): b = ((bits << i) & mask) | ((bits >> (n - i))) if b == bits: isomorphisms.append(_rot_list ([complex(*pt) for pt,bl in points.value], i)) - # TODO Add mirrors + # Add mirrored rotations + mirrored = list(reversed(points.value)) + reversed_bits = 0 + for pt,b in mirrored: + reversed_bits = (reversed_bits << 1) | b + for i in range(n): + b = ((reversed_bits << i) & mask) | ((reversed_bits >> (n - i))) + if b == bits: + isomorphisms.append(_rot_list ([complex(*pt) for pt,bl in mirrored], i)) # Check each master against the next one in the list. for i, (m0, m1) in enumerate(zip(allNodeTypes[:-1], allNodeTypes[1:])): From 6b4e2e71474b02460169bd5b9936813361c83643 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Fri, 1 Apr 2022 13:14:39 -0600 Subject: [PATCH 7/9] [ufoLib / interpolatable] Wire up outputImpliedClosingLine parameter ufoLib's glyph draw() was passing outputImpliedClosingLine=False to PointToSegmentPen(). This was causing incompatible nodes in interpolatable tool for certain fonts, like this in NotoSansDevanagari: Glyph dabhadeva was not compatible: Node count differs in path 1: 23 in NotoSansDevanagari-Bold, 24 in NotoSansDevanagari-CondensedBold Node count differs in path 1: 24 in NotoSansDevanagari-CondensedBold, 23 in NotoSansDevanagari-CondensedLight Because a final lineto before a closepath was being elided or not in some masters but not others. Wire up the parameter and control it from interpolatable tool to fix this. --- Lib/fontTools/ufoLib/glifLib.py | 4 ++-- Lib/fontTools/varLib/interpolatable.py | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Lib/fontTools/ufoLib/glifLib.py b/Lib/fontTools/ufoLib/glifLib.py index 44622a148..89c9176a7 100755 --- a/Lib/fontTools/ufoLib/glifLib.py +++ b/Lib/fontTools/ufoLib/glifLib.py @@ -95,11 +95,11 @@ class Glyph: self.glyphName = glyphName self.glyphSet = glyphSet - def draw(self, pen): + def draw(self, pen, outputImpliedClosingLine=False): """ Draw this glyph onto a *FontTools* Pen. """ - pointPen = PointToSegmentPen(pen) + pointPen = PointToSegmentPen(pen, outputImpliedClosingLine=outputImpliedClosingLine) self.drawPoints(pointPen) def drawPoints(self, pointPen): diff --git a/Lib/fontTools/varLib/interpolatable.py b/Lib/fontTools/varLib/interpolatable.py index 2954a15cb..03a8ca807 100644 --- a/Lib/fontTools/varLib/interpolatable.py +++ b/Lib/fontTools/varLib/interpolatable.py @@ -166,7 +166,10 @@ def test(glyphsets, glyphs=None, names=None): perContourPen = PerContourOrComponentPen( RecordingPen, glyphset=glyphset ) - glyph.draw(perContourPen) + try: + glyph.draw(perContourPen, outputImpliedClosingLine=True) + except TypeError: + glyph.draw(perContourPen) contourPens = perContourPen.value del perContourPen From 86b5d7103db0ceca1f4526573333c261817d3aa0 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Fri, 1 Apr 2022 14:00:22 -0600 Subject: [PATCH 8/9] [interpolatable] Downgrade assert --- Lib/fontTools/varLib/interpolatable.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/fontTools/varLib/interpolatable.py b/Lib/fontTools/varLib/interpolatable.py index 03a8ca807..204a2f36f 100644 --- a/Lib/fontTools/varLib/interpolatable.py +++ b/Lib/fontTools/varLib/interpolatable.py @@ -308,8 +308,10 @@ def test(glyphsets, glyphs=None, names=None): break for i, (m0, m1) in enumerate(zip(allContourIsomorphisms[:-1], allContourIsomorphisms[1:])): + if len(m0) != len(m1): + # We already reported this + continue if not m0: - assert not m1 continue for contour0,contour1 in zip(m0,m1): c0 = contour0[0] From f0214415cc0db94cd5f406a7db4f606f43152b84 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Fri, 1 Apr 2022 14:36:03 -0600 Subject: [PATCH 9/9] [interpolatable] Fix assert --- Lib/fontTools/varLib/interpolatable.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/fontTools/varLib/interpolatable.py b/Lib/fontTools/varLib/interpolatable.py index 204a2f36f..a9583a18d 100644 --- a/Lib/fontTools/varLib/interpolatable.py +++ b/Lib/fontTools/varLib/interpolatable.py @@ -209,7 +209,7 @@ def test(glyphsets, glyphs=None, names=None): if nodeVecs[0] == 'addComponent': continue assert nodeVecs[0] == 'moveTo' - assert nodeVecs[-1] == 'closePath' + assert nodeVecs[-1] in ('closePath', 'endPath') points = RecordingPointPen() converter = SegmentToPointPen(points, False) contour.replay(converter)