Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patcher reduces the spacing between lines making fonts difficult to read. #850

Closed
3 tasks done
BoltsJ opened this issue Jun 27, 2022 · 44 comments · Fixed by #943
Closed
3 tasks done

Patcher reduces the spacing between lines making fonts difficult to read. #850

BoltsJ opened this issue Jun 27, 2022 · 44 comments · Fixed by #943

Comments

@BoltsJ
Copy link

BoltsJ commented Jun 27, 2022

🗹 Requirements

  • I have searched the issues for my issue and found nothing related and/or helpful
  • I have searched the FAQ for help
  • I have searched the Wiki for help

🎯 Subject of the issue

Experienced behavior:
After patching a font, the line height is reduced which makes

Expected behavior:
Line height should remain the same after patching.

Example symbols:
N/A

🔧 Your Setup

  • Which font are you using (e.g. Anonymice Powerline Nerd Font Complete.ttf)?
    • Comic Code Regular Nerd Font Complete.otf
    • Self-patched using the oci image.
  • gnome-terminal, Blackbox, gnome-text-editor
  • Fedora 36 Silverblue

★ Screenshots (Optional)

Unpatched font:
image

Patched font:
image

@bartdorsey
Copy link

The same thing happens to me when patching comic code ligatures

@Finii
Copy link
Collaborator

Finii commented Jul 26, 2022

image

I guess that is very easy to fix. Believe I know where the size is lost.

@plodocus
Copy link

plodocus commented Aug 6, 2022

image

I guess that is very easy to fix. Believe I know where the size is lost.

Could you share your insights?

@tusharsnx
Copy link

tusharsnx commented Sep 25, 2022

I am facing the same issue, is there a known fix yet ? I am trying to patch MonoLisa.

Edit:

I found a quick and dirty fix, Just comment these lines in font-patcher script:

self.sourceFont.hhea_linegap = 0
self.sourceFont.os2_typolinegap = 0 

The code is suppose to fix powerline related glyphs, however, if you don't care about them then commenting above lines should fix the issue.

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

@tusharsnn 👍

I'm not sure the font-patcher code is correct in this regard anyhow.

🤔 Obviously I commented already some time back 😬

This needs some research in the src/unpatched-fonts base, which might take me some time.
Feel free to ping me from time to time 😬

This is the commit that added the lnes you mention:
9770856 Zero out linegap values to allow power line glyphs to fill properly

And probably it is right, but we have to keep the gap in another property then, we can not just silently remove it without replacement.

@tusharsnx
Copy link

tusharsnx commented Sep 26, 2022

I should point out that MonoLisa is aligned towards the top instead of being centered, which means there is more gap on the bottom then it is on the top. Is that fonts with this characteristic is effected by mentioned code or is there are other cases aswell ?

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

Fonts that have a linegap set (only regular shown):

3270Medium                                         Line gap: 90 (90)     [x]
Arimo                                              Line gap: 67 (307)    [ ]
VeraMono                                           Line gap: 0 (410)     [ ]
DejaVuSansMono                                     Line gap: 0 (410)     [ ]
FantasqueSansMono                                  Line gap: 50 (100)    [x]
gohufont*                                          Line gap: 377 (377)   [x]
Go-Mono                                            Line gap: 0 (393)     [ ]
Hack                                               Line gap: 0 (410)     [ ]
heavy_data                                         Line gap: 67 (307)    [ ]
iAWriter*                                          Line gap: 0 (300)     [ ]
IBMPlexMono                                        Line gap: 0 (300)     [ ]
Inconsolata-LGC                                    Line gap: 205 (0)     [ ]
iosevka*                                           Line gap: 68 (0)      [x]
LiberationS*                                       Line gap: 67 (307)    [ ]
Lilex                                              Line gap: 0 (600)     [ ]
Meslo LG *                                         Line gap: 0 (1210)    [ ]
mononoki                                           Line gap: 0 (229)     [ ]
mplus-*                                            Line gap: 90 (90)     [ ]
RobotoMono                                         Line gap: 0 (102)     [ ]
Terminus                                           Line gap: 90 (90)     [x]
Tinos                                              Line gap: 87 (307)    [ ]
Ubuntu-*                                           Line gap: 28 (56)     [ ]
VictorMono*                                        Line gap: 200 (200)   [x]

Name ...                                           hhea_linegap (typolinegap) [use_typo_metrics]

https://www.high-logic.com/font-editor/fontcreator/tutorials/font-metrics-vertical-line-spacing
https://glyphsapp.com/learn/vertical-metrics
https://silnrsi.github.io/FDBP/en-US/Line_Metrics.html

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

Victor Mono:

image

VictorMono Nerd Font:

image

Hmm, for my eye the spacing is 'too loose' in the original font. If we fix this people might complain now the patched fonts are wrong 😬

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

Downloaded Comic Code from myfonts ...:

ComicCodeDemo                                      Line gap: 200 (200) [x]

image

After patching:

image

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

Maybe add option to 'keep line distance', but turned off for prepatched fonts?
Opinions?

Edit: Both instanced in the Issue are self-patched fonts

@tusharsnx
Copy link

IMO the tool (or at least the ./font-patcher) should not override original font properties by default. For already patched fonts this should be explicitly mentioned that spacing has been modified.

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

But people will complain if the visuals of their beloved patched fonts change with an Nerd Font update. They do not care what is 'right' or 'wrong' as long as it is the same as before the update 😬 ;-)

Thanks for the opinion!

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

IF we use the line gap, there is another thing to decide on:

image

You see that the Powerline Triangular Thing is exactly on the same height as the middle of X - etc...
If the Powerline glyph needs to fill further down, what will happen with that?

  • Move the tip further down, too low compared with - etc
  • Keep tip on middle X height, but clip (make unsymmetric)
  • Vary angles of the triangle (too much work probably)

@tusharsnx
Copy link

But people will complain if the visuals of their beloved patched fonts change with an Nerd Font update. They do not care what is 'right' or 'wrong' as long as it is the same as before the update 😬 ;-)

I agree with that, patched font should stay the same. And for self patching, 'keep line distance' flag should be enough.

@tusharsnx
Copy link

You see that the Powerline Triangular Thing is exactly on the same height as the middle of X - etc... If the Powerline glyph needs to fill further down, what will happen with that?

  • Move the tip further down, too low compared with - etc
  • Keep tip on middle X height, but clip (make unsymmetric)
  • Vary angles of the triangle (too much work probably)

I don't pretend to understand fontforge completely, but is it possible to centre the text AND divide the original line space evenly on top and bottom? that way we don't need to workaround glyphs to look right.

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

Patched without removing line gap...:

image

image

but is it possible to centre the text AND divide the original line space evenly on top and bottom?

Excellent idea!

Edit: Add detail image

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

AH! This font has no headroom! I have seen such patched fonts before, and they 'break' if people like to use diacritics like Ö (Big O with dots, Ö) because it has no room. I have an issue about that somewhere... that would be solved if we keep the gap.

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

Hmm....

Gap distributed to top and bottom, equally:

image

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

This looks ok-isch? I will clean up the commit and test with some of the other problematic (i.e. gap-having) fonts.

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

Here is the issue where the headroom is too small for umlauts.. tested with text Übelst...

@BoltsJ
Copy link
Author

BoltsJ commented Sep 26, 2022

Hmm....

Gap distributed to top and bottom, equally:

image

Maybe it's an optical illusion, but it looks like the font is being stretched out here.

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

Looks identical... (200% view)
Note the vertical spacing between the ? boxes.

Original

image

Patched, with HEAD

image

Patched, gap left intact

image

Patched, gap redistributed top/bottom

image

Next to each other

image

@Finii
Copy link
Collaborator

Finii commented Sep 26, 2022

Superimposed 1st (original) and 4th (split gap) variant at 400%:

image

@tusharsnx
Copy link

patched with equal gaps looks good to me 👍

Finii added a commit that referenced this issue Sep 26, 2022
[why]
We just ignore specified gaps in the source fonts (i.e. set them to
zero). This reduces the line spacing in the patched font (because the
gap is missing).

[how]
Distribute the gap INTO the cell, so that we can work with zero gap (we
need that for the powerline glyphs), and still keeping the powerline
glyphs centered about the regular glyphs AND keeping the total line
spacing.

Idea-by: Tushar Singh <tusharvickey1999@gmail.com>

Fixes: #850

Reported-by: Joe Bolts
Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii Finii closed this as completed in #943 Oct 7, 2022
Finii added a commit that referenced this issue Oct 7, 2022
[why]
We just ignore specified gaps in the source fonts (i.e. set them to
zero). This reduces the line spacing in the patched font (because the
gap is missing).

[how]
Distribute the gap INTO the cell, so that we can work with zero gap (we
need that for the powerline glyphs), and still keeping the powerline
glyphs centered about the regular glyphs AND keeping the total line
spacing.

Idea-by: Tushar Singh <tusharvickey1999@gmail.com>

Fixes: #850

Reported-by: Joe Bolts
Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@snpefk
Copy link

snpefk commented Feb 6, 2023

Sorry for necroposting, but I just patched my Comic Code and spacing between the lines has diminished. I used font-patcher version

Nerd Fonts Patcher v2.3.3 (3.5.1) executing
Nerd Fonts: font-patcher (2.3.3)

and alacritty

alacritty 0.11.0 (8dbaa0bb)

@paldepind
Copy link

I have the same issue as @snpefk. I patched Comic Code with font-patcher version v2.3.3 (3.5.1) and the line height is now much reduced and very cramped.

@Finii
Copy link
Collaborator

Finii commented Feb 15, 2023

@paldepind @snpefk Sorry to hear of your problems.

I introduced a bug in the line height calculations, and found that just a few days ago, let me check if your version(s) are what I believe fixed, or suffer that bug...

That were these commits

image

Here...

Refs: 2.2.0-RC-466-g62b972b43
Author:     Fini Jastrow <ulf.fini.jastrow@desy.de>
AuthorDate: Sun Feb 12 16:35:20 2023 +0100
Commit:     Fini Jastrow <ulf.fini.jastrow@desy.de>
CommitDate: Sun Feb 12 17:06:18 2023 +0100

    font-patcher: Fix: Use WIN metrics in all conflicting cases

    [why]
    With commit
      621008773  font-patcher: Use WIN metrics in all conflicting cases
    we intended to use the WIN metrics for the baseline to baseline
    calculations for fonts that have contradicting (i.e. broken) metrices.

    But we use the TYPO metrics instead.

    [how]
    This is obviously a typo in the code. To prevent such errors and improve
    the readability we use Enums now. I believe we silently dropped support
    for Python 2 some time back. And if not we drop it today :-}

    [note]
    Many thanks to Nathaniel Evan for again finding this bug!

    Mentioned in: #1116

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

diff --git a/font-patcher b/font-patcher
index b0c451358..a00eb7f37 100755
--- a/font-patcher
+++ b/font-patcher
@@ -6,7 +6,7 @@
 from __future__ import absolute_import, print_function, unicode_literals

 # Change the script version when you edit this script:
-script_version = "3.5.6"
+script_version = "3.5.7"

 version = "2.3.3"
 projectName = "Nerd Fonts"

So it should be re-fixed with version 3.5.7 while you have 3.5.1.

What do you use? Docker, zip-archive, ?

@Finii
Copy link
Collaborator

Finii commented Feb 15, 2023

@Finii
Copy link
Collaborator

Finii commented Feb 15, 2023

I did not initiate a bugfix 2.3.x release because I believe we are very near 3.0.0; lthough ... I keep being pushed back 😬

@paldepind
Copy link

Thanks a lot for the quick reply @Finii 🙂

I got the patch script through the ZIP archive. How do I get 3.5.7? Then I'll be happy to check if this fixes the issue.

@Finii
Copy link
Collaborator

Finii commented Feb 17, 2023

@paldepind would be great if you could check this out 👍

Thanks to @b- (#1044) we have now not only a always-up-do-date docker image, but also a zip archive:

https://github.com/ryanoasis/nerd-fonts/blob/-/FontPatcher.zip

@paldepind
Copy link

I just ran the script at version 3.5.10 and this fixed the issue 🙂 Thanks!

@snpefk
Copy link

snpefk commented Mar 22, 2023

Maybe I do something wrong, but this is my results:
Screenshot from 2023-03-22 20-24-42
Screenshot from 2023-03-22 20-24-38
Screenshot from 2023-03-22 20-24-33

Now the height between lines is too big.

I tried to patch with -cl args, my version is

Nerd Fonts Patcher v2.3.3 (3.6.1) (ff 20230101) executing
Nerd Fonts: font-patcher (2.3.3)

@Finii
Copy link
Collaborator

Finii commented Mar 22, 2023

Sorry to hear that.

Can you point me to the origin of the Comic Code Liga font?

@Finii
Copy link
Collaborator

Finii commented Mar 22, 2023

Hmm, you are on mac... It might be that the source font is one of the fonts with broken (i.e. contradicting) line spacing for windows and mac. If you can, maybe run font-line on the sourcefont?

https://github.com/source-foundry/font-line

Can be installed via pip for example.

@Finii
Copy link
Collaborator

Finii commented Mar 22, 2023

You are not on Mac 🤦‍♀️

@Finii
Copy link
Collaborator

Finii commented Mar 22, 2023

I found one, but that looks good:

Screenshot 2023-03-22 at 18 58 08

In this case font-patcher should not change the line height at all... Strange.

@Finii
Copy link
Collaborator

Finii commented Mar 22, 2023

That font comes out unchanged in height:

Screenshot 2023-03-22 at 19 03 13

[...]

Screenshot 2023-03-22 at 19 03 29

@snpefk
Copy link

snpefk commented Mar 24, 2023

Yeah, I'm on linux (Linux 5.15.96-1-MANJARO specifically).
I'm noticed this warning about font vertical, maybe this is why height is so big

❯ ./font-patcher ComicCodeLigatures-Regular.otf -cl
Nerd Fonts Patcher v2.3.3 (3.6.1) (ff 20230101) executing
Nerd Fonts: WARNING Font vertical metrics inconsistent (HHEA 1667 / TYPO 1200 / WIN 1667), using WIN
Redistributing line gap of 200 (100 top and 100 bottom)
Info: Extended glyphs wider bounding box than basic glyphs
Warning: Font has negative right side bearing in extended glyphs

@Finii
Copy link
Collaborator

Finii commented Mar 24, 2023

Ah, yes.

Different companies worked on the TrueType format, and they all had different ideas. That resulted in the baseline to baseline distance being encoded in 3 places. Apple's HHEA, Microsoft's WIN, and the newer TYPO.

Nerd Fonts: WARNING Font vertical metrics inconsistent (HHEA 1667 / TYPO 1200 / WIN 1667), using WIN

Unfortunately the font you use has contradicting values. Different OS's and even applications use different line heights with that font.
Because the original font has a gap between lines, and powerline symbols can have no gaps - you want them to touch line to line - we need to rewrite the metrices on patching. For that one metric is selected (in your case the WIN line height, which is equal to Apple's HHEA) and written to the other metrices. Your OS/client uses the TYPO value.

The one I tried is Comic Code Ligatures by Toshi Omagari - Tabular Type Foundry (Nov 2019 / version 1.0) [1].

You could use font-line to fix your font's line-height information, then patch it again.
Or use the same sourcefont I used.

[1] https://tosche.net/fonts

@snpefk
Copy link

snpefk commented Mar 24, 2023

Pardon me, but what is font-line?

@Finii
Copy link
Collaborator

Finii commented Mar 24, 2023

Pardon me, but what is font-line?

https://github.com/source-foundry/font-line

Can be installed via pip for example.

$ pip3 install font-line
$ font-line report my_cosmic_font.ttf

Example usage

@Sycha
Copy link

Sycha commented Apr 7, 2023

I am was having similar problems. Here is my font-line report for the unpatched 'Comic Code Regular.otf'

=== Comic Code Regular.otf ===
1.000
SHA1: 5e2a8bfcb29b617af8b7c748b220f2f4b5d47eda

::::::::::::::::::::::::::::::::::::::::::::::::::
  Metrics
::::::::::::::::::::::::::::::::::::::::::::::::::
[head] Units per Em:   1000
[head] yMax:           1096
[head] yMin:          -454
[OS/2] CapHeight:      700
[OS/2] xHeight:        550
[OS/2] TypoAscender:   800
[OS/2] TypoDescender: -200
[OS/2] WinAscent:      1136
[OS/2] WinDescent:     331
[hhea] Ascent:         1136
[hhea] Descent:       -331

[hhea] LineGap:        0
[OS/2] TypoLineGap:    200

::::::::::::::::::::::::::::::::::::::::::::::::::
  Ascent to Descent Calculations
::::::::::::::::::::::::::::::::::::::::::::::::::
[hhea] Ascent to Descent:              1467
[OS/2] TypoAscender to TypoDescender:  1000
[OS/2] WinAscent to WinDescent:        1467

::::::::::::::::::::::::::::::::::::::::::::::::::
  Delta Values
::::::::::::::::::::::::::::::::::::::::::::::::::
[hhea] Ascent to [OS/2] TypoAscender:       336
[hhea] Descent to [OS/2] TypoDescender:     131
[OS/2] WinAscent to [OS/2] TypoAscender:    336
[OS/2] WinDescent to [OS/2] TypoDescender:  131

::::::::::::::::::::::::::::::::::::::::::::::::::
  Baseline to Baseline Distances
::::::::::::::::::::::::::::::::::::::::::::::::::
hhea metrics: 1467
typo metrics: 1200
win metrics:  1467

[OS/2] fsSelection USE_TYPO_METRICS bit set: True

::::::::::::::::::::::::::::::::::::::::::::::::::
  Ratios
::::::::::::::::::::::::::::::::::::::::::::::::::
hhea metrics / UPM:  1.47
typo metrics / UPM:  1.2
win metrics  / UPM:  1.47

I used this command on the unpatched files:

font-line percent 20 *

Then patched them with the docker container and the -c argument and it all looks okay now

docker run --rm -v ./in12:/in -v ./out12:/out nerdfonts/patcher -c

image

image

Cheers for the pointer to font-line @Finii

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

This issue has been automatically locked since there has not been any recent activity (i.e. last half year) after it was closed. It helps our maintainers focus on the active issues. If you have found a problem that seems similar, please open a new issue, complete the issue template with all the details necessary to reproduce, and mention this issue as reference.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2023
LNKLEO pushed a commit to LNKLEO/Nerd that referenced this issue Nov 24, 2023
[why]
We just ignore specified gaps in the source fonts (i.e. set them to
zero). This reduces the line spacing in the patched font (because the
gap is missing).

[how]
Distribute the gap INTO the cell, so that we can work with zero gap (we
need that for the powerline glyphs), and still keeping the powerline
glyphs centered about the regular glyphs AND keeping the total line
spacing.

Idea-by: Tushar Singh <tusharvickey1999@gmail.com>

Fixes: ryanoasis#850

Reported-by: Joe Bolts
Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants