font-patcher: Correct mono scaling of thin glyphs #749

Finii · 2022-01-01T11:09:55Z

[why]
Some glyphs that are tall and thin, are too big in the resulting patched
font, i.e. are higher than our 'line'.

At least for --mono fonts. The non-mono fonts do not rescale the
inserted glyphs at all, so there is no definition of 'too tall/wide'.

[how]
We want all glyphs to fit into the box defined by *_dim['height'] and
*_dim['widths'], as it also defines our powerline-glyph scaling and
horizontal and vertical advance widths.

So we need to take that value (instead of EM) for the scaling
calculation. The history of the use of EM here is a bit obscure.

Requirements / Checklist

Read the Contributing Guidelines
Verified the license of any newly added font, glyph, or glyph set

What does this Pull Request (PR) do?

Base mono-patched font glyph size on height and width and not EM and width.

How should this be manually tested?

Any background context you can provide?

What are the relevant tickets (if any)?

Screenshots (if appropriate or helpful)

More in the comments below.

[why] Some glyphs that are tall and thin, are too big in the resulting patched font, i.e. are higher than our 'line'. At least for --mono fonts. The non-mono fonts do not rescale the inserted glyphs at all, so there is no definition of 'too tall/wide'. [how] We want all glyphs to fit into the box defined by *_dim['height'] and *_dim['widths'], as it also defines our powerline-glyph scaling and horizontal and vertical advance widths. So we need to take that value (instead of EM) for the scaling calculation. The history of the use of EM here is a bit obscure, more explanations in the PR. Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

Finii · 2022-01-01T11:16:01Z

The comment raised my curiosity, and I needed an answer for #748 where the scaling operations are modernized/refactured.

# font_dim['height'] represents total line height, keep our symbols sized based upon font's em
# NOTE: is this comment correct? font_dim['height'] isn't used here
scale_ratio_y = self.sourceFont.em / sym_dim['height']

So the questions were

Who introduced the question
Who introduced the code with .em
Examine which glyphs would be affected by a change from EM to height
Which is correct?

Finii · 2022-01-01T12:21:28Z

History

Origin of comment-question

The question comment has been introduced with 0a480bb

-def get_scale_factor(font_dim, sym_dim):
-    scale_ratio = 1
-    # We want to preserve x/y aspect ratio, so find biggest scale factor that allows symbol to fit
-    scale_ratio_x = font_dim['width'] / sym_dim['width']
-    # font_dim['height'] represents total line height, keep our symbols sized based upon font's em
-    scale_ratio_y = sourceFont.em / sym_dim['height']
-    if scale_ratio_x > scale_ratio_y:
-        scale_ratio = scale_ratio_y
-    else:
-        scale_ratio = scale_ratio_x
-    return scale_ratio
+    def get_scale_factor(self, sym_dim):
+        scale_ratio = 1
+
+        # We want to preserve x/y aspect ratio, so find biggest scale factor that allows symbol to fit
+        scale_ratio_x = self.font_dim['width'] / sym_dim['width']
+
+        # font_dim['height'] represents total line height, keep our symbols sized based upon font's em
+        # NOTE: is this comment correct? font_dim['height'] isn't used here
+        scale_ratio_y = self.sourceFont.em / sym_dim['height']
+        if scale_ratio_x > scale_ratio_y:
+            scale_ratio = scale_ratio_y
+        else:
+            scale_ratio = scale_ratio_x
+        return scale_ratio

There is no explanation (well, that would probably not have resulted in a question then), and Jimmy Haas just noticed that while refactoring and added the comment 👍

Origin of code that uses `.em`

Maybe there is an explanation when the actual code has been added, with commit ec419a8
Before that commit height has been used and not EM!

-                # Handle glyph stretching
-                scale_ratio_x = 1
-                scale_ratio_y = 1
-                if sym_attr['stretch'] == 'pa':
-                    # We want to preserve x/y aspect ratio, so find biggest scale factor that allows
-                    scale_ratio_x = font_dim['width'] / sym_dim['width']
-                    scale_ratio_y = font_dim['height'] / sym_dim['height']
-                    if scale_ratio_x > scale_ratio_y:
-                        scale_ratio_x = scale_ratio_y
-                    else:
-                        scale_ratio_y = scale_ratio_x
-                else:
-                    if 'x' in sym_attr['stretch']:
-                        # Stretch the glyph horizontally
-                        scale_ratio_x = font_dim['width'] / sym_dim['width']
-                    if 'y' in sym_attr['stretch']:
+            # Handle glyph stretching
+            scale_ratio_x = 1
+            scale_ratio_y = 1
+            if sym_attr['stretch'] == 'pa':
+                # We want to preserve x/y aspect ratio, so find biggest scale factor that allows symbol to fit
+                scale_ratio_x = font_dim['width'] / sym_dim['width']
+                # font_dim['height'] represents total line height, keep our symbols sized based upon font's em
+                scale_ratio_y = sourceFont.em / sym_dim['height']
+                if scale_ratio_x > scale_ratio_y:
+                    scale_ratio_x = scale_ratio_y
+                else:
+                    scale_ratio_y = scale_ratio_x
+            else:
+                if 'x' in sym_attr['stretch']:
+                    # Stretch the glyph horizontally
+                    scale_ratio_x = font_dim['width'] / sym_dim['width']
+                if 'y' in sym_attr['stretch']:

And no, there is nothing given, the comment even suggests no change at all: 😒

commit ec419a80ba0d219b00b5c44b8303526a8c2ed12e
Author: Marcus Kellerman <sharkus@gmail.com>
Date:   Thu Oct 13 17:48:30 2016 -0700

    Unified indents, auto-convert to otf when generating windows fonts.

Searched all PR and Issues in that time frame, but found nothing that explains the change.

Finii · 2022-01-01T14:11:28Z

Experimental findings

Add code that compares the scaling based on EM with based on height. Two fonts choosen, and the glyphs are not all the same...:

$ fontforge --script font-patcher --mono --powerline src/unpatched-fonts/ProggyClean/Regular/ProggyClean.ttf 2>/dev/null | grep ISSUE
ISSUE Glyph u_E610  height wins True  em wins False FACT 0.8362103426972418
ISSUE Glyph u_E763  height wins True  em wins False FACT 0.8376270545973712
ISSUE Glyph u_E7A4  height wins True  em wins False FACT 0.83379033307312
ISSUE Glyph u_E7C4  height wins True  em wins False FACT 0.8283919641695001
ISSUE Glyph u_E0A2  height wins True  em wins False FACT 0.940164080613519
$ fontforge --script font-patcher --mono --powerline src/unpatched-fonts/CascadiaCode/Regular/CascadiaCode-Regular.otf 2>/dev/null | grep ISSUE
ISSUE Glyph u_E60A  height wins False  em wins True FACT 1.0706268488052681
ISSUE Glyph u_E70D  height wins False  em wins True FACT 1.0764005533441507
ISSUE Glyph u_E737  height wins False  em wins True FACT 1.0100679916317992
ISSUE Glyph u_E0A2  height wins False  em wins True FACT 1.1574252466091246
$ fontforge --script font-patcher --mono --complete src/unpatched-fonts/ProggyClean/Regular/ProggyClean.ttf 2>/dev/null | grep ISSUE | wc -l
     58
$ fontforge --script font-patcher --mono --complete src/unpatched-fonts/CascadiaCode/Regular/CascadiaCode-Regular.otf 2>/dev/null | grep ISSUE | wc -l
     40

So total affected are in the 50 glyphs range.

Visual

This is ProggyClean Nerd Font Mono without this patch:

The comparsion is somehow rough, but all outlines are at 20% and clearly are higher than the powerline Triangular Thing that defines our full line height.

After this patch, the glyphs are correctly within the line..:

For other fonts it is not so obviously wrong, as Caskaydia, because here it scales too small rather than too big.

Caskaydia Cove Nerd Font Mono Regular without patch:

and with patch:

Note that most of the glyphs scale bigger, but are limited by the width now.

Finii · 2022-01-01T14:16:49Z

Here is my code change, the one for debugging purposes, to show which glyphs are affected.

For reproducing purposes.

Apply to font-patcher at HEAD (i.e. de13c66)

From 0353bc31f34959d3cd6eae0f4bfe90417fd8287e Mon Sep 17 00:00:00 2001
From: Fini Jastrow <ulf.fini.jastrow@desy.de>
Date: Sat, 1 Jan 2022 15:14:05 +0100
Subject: [PATCH] add debug stuff

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
---
 font-patcher | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/font-patcher b/font-patcher
index 9440b2a5..415f3a50 100755
--- a/font-patcher
+++ b/font-patcher
@@ -633,19 +633,37 @@ class font_patcher:
         self.font_dim['height'] = abs(self.font_dim['ymin']) + self.font_dim['ymax']
 
 
-    def get_scale_factor(self, sym_dim):
+    def get_scale_factor(self, sym_dim, code):
         scale_ratio = 1
 
         # We want to preserve x/y aspect ratio, so find biggest scale factor that allows symbol to fit
         scale_ratio_x = self.font_dim['width'] / sym_dim['width']
 
+        w_he = False
+        w_em = False
+
         # font_dim['height'] represents total line height, keep our symbols sized based upon font's em
         # NOTE: is this comment correct? font_dim['height'] isn't used here
+        scale_ratio_y_he = self.font_dim['height'] / sym_dim['height']
+        if scale_ratio_x > scale_ratio_y_he:
+            scale_ratio_he = scale_ratio_y_he
+            w_he = True
+        else:
+            scale_ratio_he = scale_ratio_x
+
         scale_ratio_y = self.sourceFont.em / sym_dim['height']
         if scale_ratio_x > scale_ratio_y:
             scale_ratio = scale_ratio_y
+            w_em = True
         else:
             scale_ratio = scale_ratio_x
+
+        if w_he != w_em:
+            print("\nISSUE Glyph u_{:X}  height wins {}  em wins {} FACT {}".format(code,w_he,w_em,scale_ratio_he/scale_ratio))
+            self.fini = True
+        else:
+            self.fini = False
+
         return scale_ratio
 
 
@@ -665,7 +683,7 @@ class font_patcher:
         scale_factor = 0
         if scaleGlyph:
             sym_dim = get_glyph_dimensions(symbolFont[scaleGlyph['ScaleGlyph']])
-            scale_factor = self.get_scale_factor(sym_dim)
+            scale_factor = self.get_scale_factor(sym_dim,0)
 
         # Create glyphs from symbol font
         #
@@ -752,7 +770,7 @@ class font_patcher:
                         scale_ratio_y = scale_factor
                     else:
                         # In this case, each glyph is sized independently to each other
-                        scale_ratio_x = self.get_scale_factor(sym_dim)
+                        scale_ratio_x = self.get_scale_factor(sym_dim,currentSourceFontGlyph)
                         scale_ratio_y = scale_ratio_x
                 else:
                     if 'x' in sym_attr['stretch']:
@@ -769,6 +787,8 @@ class font_patcher:
                 # Stretch the glyph vertically to total line height (good for powerline separators)
                 # Currently stretching vertically for both monospace and double-width
                 scale_ratio_y = self.font_dim['height'] / sym_dim['height']
+                if self.fini:
+                    print("ISSUE Redo!")
 
             if scale_ratio_x != 1 or scale_ratio_y != 1:
                 if 'overlap' in sym_attr['params']:
@@ -777,6 +797,7 @@ class font_patcher:
                 self.sourceFont[currentSourceFontGlyph].transform(psMat.scale(scale_ratio_x, scale_ratio_y))
 
             # Use the dimensions from the newly pasted and stretched glyph
+            self.fini = False
             sym_dim = get_glyph_dimensions(self.sourceFont[currentSourceFontGlyph])
             y_align_distance = 0
             if sym_attr['valign'] == 'c':
-- 
2.32.0

Finii · 2022-01-01T14:41:22Z

Of course Cascadia Code and ProggyClean are different, because they have these:

src/unpatched-fonts/ProggyClean/Regular/ProggyClean.ttf
EM 2048 height 1664

src/unpatched-fonts/CascadiaCode/Regular/CascadiaCode-Regular.otf
EM 2048 height 2380

Finii added the Bug fix label Jan 1, 2022

Finii mentioned this pull request Jan 1, 2022

Feature: Allow symbol scaling in nonmono (down to 2 'widths') #748

Merged

2 tasks

Finii added this to the v2.3.0 milestone Aug 22, 2022

Finii merged commit b2fa537 into master Sep 6, 2022

Finii deleted the bugfix/mono-vertical-scaling-slimglyph branch September 6, 2022 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

font-patcher: Correct mono scaling of thin glyphs #749

font-patcher: Correct mono scaling of thin glyphs #749

Uh oh!

Finii commented Jan 1, 2022

Uh oh!

Finii commented Jan 1, 2022

Uh oh!

Finii commented Jan 1, 2022

Uh oh!

Finii commented Jan 1, 2022

Uh oh!

Finii commented Jan 1, 2022

Uh oh!

Finii commented Jan 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

font-patcher: Correct mono scaling of thin glyphs #749

font-patcher: Correct mono scaling of thin glyphs #749

Uh oh!

Conversation

Finii commented Jan 1, 2022

Requirements / Checklist

What does this Pull Request (PR) do?

How should this be manually tested?

Any background context you can provide?

What are the relevant tickets (if any)?

Screenshots (if appropriate or helpful)

More in the comments below.

Uh oh!

Finii commented Jan 1, 2022

Uh oh!

Finii commented Jan 1, 2022

History

Origin of comment-question

Origin of code that uses .em

Uh oh!

Finii commented Jan 1, 2022

Experimental findings

Visual

Uh oh!

Finii commented Jan 1, 2022

Uh oh!

Finii commented Jan 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Origin of code that uses `.em`