Victor Mono vertical alignment? #1116

nathanielevan · 2023-02-11T20:17:41Z

🗹 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 patched with options -s, -l and -c, Victor Mono's vertical alignment seems too low. Here's a comparison:

Patched:
Unpatched:

Metrics:

metrics	unpatched	3.5.1
Typo ascent	1100	1200
Typo descent	-250	-150
Typo line gap	200	0
HHEA ascent	1100	1200
HHEA descent	-250	-150
HHEA line gap	200	0

Expected behavior:
Patched font retains its vertical alignment

The text was updated successfully, but these errors were encountered:

[why] Instead of redistributing the line gap we remove it. At least when HHEA or TYPO metrics are used. It's ok with WIN metrics. [how] If we have negative numbers for a gap and want to add more to it, where 'add' means 'make it more', we must of course _subtract_ the value. But baseline-to-baseline code into function so we can check it after all our gymnastics for correctness. It means the metrics. [note] Also correct out-of-sync comment. Fixes: #1116 Reported-by: Nathaniel Evan <nathanielevan> Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

Finii · 2023-02-11T23:02:02Z

Thanks for reporting, and with the detailed metrics.

Turns out we drop the gap instead of redistributing, because one sign is wrong.
I have no clue how that bug crept in :-(

Btw, I use font-line to check that kind of stuff.

$ font-line report src/unpatched-fonts/VictorMono/Regular/VictorMono-Regular.ttf
=== src/unpatched-fonts/VictorMono/Regular/VictorMono-Regular.ttf ===
Version 1.410
SHA1: 3365cea003978e423ff95a479db2965f69942ce5

::::::::::::::::::::::::::::::::::::::::::::::::::
  Metrics
::::::::::::::::::::::::::::::::::::::::::::::::::
[head] Units per Em:   1100
[head] yMax:           1306
[head] yMin:          -529
[OS/2] CapHeight:      800
[OS/2] xHeight:        618
[OS/2] TypoAscender:   1100
[OS/2] TypoDescender: -250
[OS/2] WinAscent:      1100
[OS/2] WinDescent:     250
[hhea] Ascent:         1100
[hhea] Descent:       -250

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

::::::::::::::::::::::::::::::::::::::::::::::::::
  Baseline to Baseline Distances
::::::::::::::::::::::::::::::::::::::::::::::::::
hhea metrics: 1550
typo metrics: 1550
win metrics:  1550

$ font-line report patched-fonts/VictorMono/Regular/complete/Victor\ Mono\ Regular\ Nerd\ Font\ Complete.ttf
=== patched-fonts/VictorMono/Regular/complete/Victor Mono Regular Nerd Font Complete.ttf ===
Version 1.410;Nerd Fonts 2.3.3
SHA1: 9907eee46ccd8601bafccd0f36f88f24227e0609

::::::::::::::::::::::::::::::::::::::::::::::::::
  Metrics
::::::::::::::::::::::::::::::::::::::::::::::::::
[head] Units per Em:   1100
[head] yMax:           1306
[head] yMin:          -529
[OS/2] CapHeight:      800
[OS/2] xHeight:        618
[OS/2] TypoAscender:   1200
[OS/2] TypoDescender: -150
[OS/2] WinAscent:      1200
[OS/2] WinDescent:     150
[hhea] Ascent:         1200
[hhea] Descent:       -150

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

::::::::::::::::::::::::::::::::::::::::::::::::::
  Baseline to Baseline Distances
::::::::::::::::::::::::::::::::::::::::::::::::::
hhea metrics: 1350
typo metrics: 1350
win metrics:  1350

Finii · 2023-02-11T23:06:24Z

After PR:

$ font-line report Victor\ Mono\ Regular\ Nerd\ Font.ttf
=== Victor Mono Regular Nerd Font.ttf ===
Version 1.410;Nerd Fonts 2.3.3-36
SHA1: d27acb443db1a69400ecabb9ffe04c0f0389c818

::::::::::::::::::::::::::::::::::::::::::::::::::
  Metrics
::::::::::::::::::::::::::::::::::::::::::::::::::
[head] Units per Em:   1100
[head] yMax:           1306
[head] yMin:          -529
[OS/2] CapHeight:      800
[OS/2] xHeight:        618
[OS/2] TypoAscender:   1200
[OS/2] TypoDescender: -350
[OS/2] WinAscent:      1200
[OS/2] WinDescent:     350
[hhea] Ascent:         1200
[hhea] Descent:       -350

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

::::::::::::::::::::::::::::::::::::::::::::::::::
  Baseline to Baseline Distances
::::::::::::::::::::::::::::::::::::::::::::::::::
hhea metrics: 1550
typo metrics: 1550
win metrics:  1550

[why] Instead of redistributing the line gap we remove it. At least when HHEA or TYPO metrics are used. It's ok with WIN metrics. [how] If we have negative numbers for a gap and want to add more to it, where 'add' means 'make it more', we must of course _subtract_ the value. But baseline-to-baseline code into function so we can check it after all our gymnastics for correctness. It means the metrics. [note] Also correct out-of-sync comment. Fixes: #1116 Reported-by: Nathaniel Evan <nathanielevan> Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

Finii · 2023-02-11T23:17:45Z

@all-contributors Please add @nathanielevan also for bug ❤️

allcontributors · 2023-02-11T23:17:54Z

@Finii

I've put up a pull request to add @nathanielevan! 🎉

[why] Instead of redistributing the line gap we remove it. At least when HHEA or TYPO metrics are used. It's ok with WIN metrics. [how] If we have negative numbers for a gap and want to add more to it, where 'add' means 'make it more', we must of course _subtract_ the value. But baseline-to-baseline code into function so we can check it after all our gymnastics for correctness. It means the metrics. [note] Also correct out-of-sync comment. Fixes: #1116 Reported-by: Nathaniel Evan <nathanielevan> Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

nathanielevan · 2023-02-12T10:43:01Z

Many thanks for the patch, however Fantasque Sans Mono seems to have failed patching:

Nerd Fonts: WARNING Font vertical metrics inconsistent (HHEA 2298 / TYPO 2348 / WIN 2298), using WIN
Redistributing line gap of 100 (50 top and 50 bottom)
Nerd Fonts: Error in baseline to baseline code detected

It's one of those fonts with inconsistent vertical metrics. Did a little debugging and it appears that, at least specifically with this font, it fails at the final check on this line; here's what I got

check_win_btb: 2348 != our_btb: 2298

Here are the metrics of the stock font:

So as far as I can tell, if the calculations are correct, get_btb_metrics() seem to be doing exactly what it's supposed to do; our_btb takes the value of (the old) win_btb and metrics is set to 1, and then after distributing the line gap, the WIN metrics are set to the new Typo metrics, which explains the difference in value. So I suppose you can either change how get_btb_metrics() calculates things, or simply drop the last if-condition in the line I mentioned above -- your call.

Finii · 2023-02-12T10:56:40Z

Merde!

I'm currently on the ice, skating with the kids, will look/fix asap.

Thank for checking, reporting, etc ❤️

Finii · 2023-02-12T15:14:19Z

Well, get_btb_metrics() can not be changed because that is how the baseline distances need to be calculated.

And also the last check is ok, I guess.

But what is shocking and wrong, maybe is this

        # We use either TYPO (1) or WIN (2) and compare with HHEA
          # and use HHEA (0) if the fonts seems broken - no WIN, see #1056
          our_btb = typo_btb if use_typo else win_btb
          if our_btb == hhea_btb:
              metrics = 1 if use_typo else 2 # conforming font
          else:
              # We trust the WIN metric more, see experiments in #1056
              print("{}: WARNING Font vertical metrics inconsistent (HHEA {} / TYPO {} / WIN {}), using WIN".format(projectName, hhea_btb, typo_btb, win_btb))
              our_btb = win_btb
              metrics = 1

metrics can be 0, 1, or 2 (HHEA, TYPO, WIN).
We say we want to use WIN.
We set out_btb to WIN's value.
We set metrics to TYPO ??!

I need to read #1056 and find out why the code is wrong.
Also contradicting is (was) the comment in the top, 'use HHEA for broken fonts'.

Finii · 2023-02-12T15:52:13Z

Hmm, all fonts in src/unpatched-fonts/FantasqueSansMono/ patch without the 'metrics inconsistent' warning?
It is also listed as problematic in my table #1056 (comment)

Do you use a different Fantastique Sans Mono?

I see it with Hermit (tested just that one, because it is marked red on list above).

nathanielevan · 2023-02-12T16:03:57Z

Ah right, I forgot I was using the "large line height" variant -- it's available to download here. Indeed I'm not getting that error with the regular version

[why] With commit e69a025 font-patcher: Fix line gap redistribution we fixed the wrong adding instead of subtraction of the bottom gap part from the descenders. At least this was done for HHEA and TYPO values. With WIN values the descenders have positive (!) numbers, so the sign was not changed for the WIN case. But that is wrong, as we are already in the ymin xmax coordinate system (and took the negative of the WIN descenders). So of course here also we need to subtract and not add. Mentioned in: #1116 Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

[why] With commit 6210087 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>

Finii · 2023-02-12T16:07:50Z

Thank you so much for reporting and evaluating!

Fixed the issue in the two commits mentioned above.

👍 😍

Finii · 2023-02-12T17:39:28Z

I should resign... 😭

$ ./gotta-patch-em-all-font-patcher\!.sh -cj /NerdFontsSymbolsOnly
# [Nerd Fonts]  Filter given, limiting search and patch to pathname pattern '/NerdFontsSymbolsOnly'
# [Nerd Fonts]  Total source fonts found: 2
# [Nerd Fonts]  Release timestamp is Sun, 12 Feb 2023 18:37:03 +0100
# [Nerd Fonts]  Processing font 1/2
# [Nerd Fonts]  Processing font 2/2
Nerd Fonts Patcher v2.3.3-44 (3.5.7) (ff 20220308) executing
Nerd Fonts Patcher v2.3.3-44 (3.5.7) (ff 20220308) executing
PythonUI_Init()
copyUIMethodsToBaseTable()
Nerd Fonts: Error in baseline to baseline code detected
Patcher run aborted!

There are too many special cases in the code.

Well, I will fix this silently en passant with (PR to be filled in)

@@ -1039,6 +1039,7 @@ class font_patcher:
                 'width' : self.sourceFont.em,
                 'height': self.sourceFont.descent + self.sourceFont.ascent,
             }
+            our_btb = self.sourceFont.descent + self.sourceFont.ascent
         elif self.font_dim['height'] < 0:
             sys.exit("{}: Can not detect sane font height".format(projectName))

nathanielevan · 2023-02-12T19:07:56Z

What are the results of check_hhea_btb, check_typo_btb, check_win_btb and our_btb? I'll see if I can help somehow. I'm currently also thinking of adding a flag for font-patcher to simply set the gap to 0 and not redistribute it

Edit: just checked and it generates an our_btb value of 0. Interesting

Finii · 2023-02-12T20:14:24Z

Thank you, but, the fix is the diff above. See 49e7662

Not redistributing the gap is not an option? That will destroy the original font vertical metrics.
If someone want to change the gap there are tools to just do that.

The names are check_ + metric name + _btb. btb stands for baseline to baseline distance (i.e. "line height").

In the check_ we have the results for after the gap redistribution and general metrics correction (where needed). All fonts will / shall have the same btb, regardless which client uses the font (they sometimes prefer different metrices). The will habe the same btb, even though we removed the gap. We need to remove the gap because we want to have powerline glyphs that touch from line to line i.e. have no gap.

our_btb is our target-btb. Sometimes the different metrics do not match and then we need to decide which btb is the "real" btb.

[why] Instead of redistributing the line gap we remove it. At least when HHEA or TYPO metrics are used. It's ok with WIN metrics. [how] If we have negative numbers for a gap and want to add more to it, where 'add' means 'make it more', we must of course _subtract_ the value. But baseline-to-baseline code into function so we can check it after all our gymnastics for correctness. It means the metrics. [note] Also correct out-of-sync comment. Fixes: #1116 Reported-by: Nathaniel Evan <nathanielevan> Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

[why] With commit e69a025 font-patcher: Fix line gap redistribution we fixed the wrong adding instead of subtraction of the bottom gap part from the descenders. At least this was done for HHEA and TYPO values. With WIN values the descenders have positive (!) numbers, so the sign was not changed for the WIN case. But that is wrong, as we are already in the ymin xmax coordinate system (and took the negative of the WIN descenders). So of course here also we need to subtract and not add. Mentioned in: #1116 Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

[why] With commit 6210087 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>

[why] Instead of redistributing the line gap we remove it. At least when HHEA or TYPO metrics are used. It's ok with WIN metrics. [how] If we have negative numbers for a gap and want to add more to it, where 'add' means 'make it more', we must of course _subtract_ the value. But baseline-to-baseline code into function so we can check it after all our gymnastics for correctness. It means the metrics. [note] Also correct out-of-sync comment. Fixes: ryanoasis#1116 Reported-by: Nathaniel Evan <nathanielevan> Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

[why] With commit e69a025 font-patcher: Fix line gap redistribution we fixed the wrong adding instead of subtraction of the bottom gap part from the descenders. At least this was done for HHEA and TYPO values. With WIN values the descenders have positive (!) numbers, so the sign was not changed for the WIN case. But that is wrong, as we are already in the ymin xmax coordinate system (and took the negative of the WIN descenders). So of course here also we need to subtract and not add. Mentioned in: ryanoasis#1116 Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

[why] With commit 6210087 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: ryanoasis#1116 Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

gregbacchus · 2023-02-22T06:09:27Z

Just to add to this, as it wasn't mentioned specifically (but possibly related) many of the icons in Victor Mono are higher up than others.

e.g. U+E38A

in SauceCode Pro

vs

in VictorMono

Finii · 2023-02-22T06:55:50Z

Thanks for reporting / showing this @gregbacchus

To check I re-patched both fonts with the current master patcher, and this is how it comes out:

So I guess the problem is fixed 😬

gregbacchus · 2023-02-26T07:56:13Z

Thank you @Finii, and great work, by the way!

github-actions · 2023-08-28T00:11:12Z

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.

[why] Instead of redistributing the line gap we remove it. At least when HHEA or TYPO metrics are used. It's ok with WIN metrics. [how] If we have negative numbers for a gap and want to add more to it, where 'add' means 'make it more', we must of course _subtract_ the value. But baseline-to-baseline code into function so we can check it after all our gymnastics for correctness. It means the metrics. [note] Also correct out-of-sync comment. Fixes: ryanoasis#1116 Reported-by: Nathaniel Evan <nathanielevan> Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

Finii added the confirmed label Feb 11, 2023

Finii mentioned this issue Feb 11, 2023

font-patcher: Fix line gap redistribution #1117

Merged

2 tasks

Finii added the ⚠ bug label Feb 11, 2023

allcontributors bot mentioned this issue Feb 11, 2023

docs: add nathanielevan as a contributor for bug #1118

Merged

Finii closed this as completed in #1117 Feb 11, 2023

Finii mentioned this issue Feb 15, 2023

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

Closed

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Victor Mono vertical alignment? #1116

Victor Mono vertical alignment? #1116

nathanielevan commented Feb 11, 2023

Finii commented Feb 11, 2023 •

edited

Loading

Finii commented Feb 11, 2023

Finii commented Feb 11, 2023

allcontributors bot commented Feb 11, 2023

nathanielevan commented Feb 12, 2023 •

edited

Loading

Finii commented Feb 12, 2023

Finii commented Feb 12, 2023

Finii commented Feb 12, 2023

nathanielevan commented Feb 12, 2023

Finii commented Feb 12, 2023

Finii commented Feb 12, 2023 •

edited

Loading

nathanielevan commented Feb 12, 2023 •

edited

Loading

Finii commented Feb 12, 2023

gregbacchus commented Feb 22, 2023

Finii commented Feb 22, 2023

gregbacchus commented Feb 26, 2023

github-actions bot commented Aug 28, 2023

Victor Mono vertical alignment? #1116

Victor Mono vertical alignment? #1116

Comments

nathanielevan commented Feb 11, 2023

🗹 Requirements

🎯 Subject of the issue

Finii commented Feb 11, 2023 • edited Loading

Finii commented Feb 11, 2023

Finii commented Feb 11, 2023

allcontributors bot commented Feb 11, 2023

nathanielevan commented Feb 12, 2023 • edited Loading

Finii commented Feb 12, 2023

Finii commented Feb 12, 2023

Finii commented Feb 12, 2023

nathanielevan commented Feb 12, 2023

Finii commented Feb 12, 2023

Finii commented Feb 12, 2023 • edited Loading

nathanielevan commented Feb 12, 2023 • edited Loading

Finii commented Feb 12, 2023

gregbacchus commented Feb 22, 2023

Finii commented Feb 22, 2023

gregbacchus commented Feb 26, 2023

github-actions bot commented Aug 28, 2023

Finii commented Feb 11, 2023 •

edited

Loading

nathanielevan commented Feb 12, 2023 •

edited

Loading

Finii commented Feb 12, 2023 •

edited

Loading

nathanielevan commented Feb 12, 2023 •

edited

Loading