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

Bugfix vertical lines in powerline prompts #780

Closed
wants to merge 2 commits into from
Closed

Conversation

Finii
Copy link
Collaborator

@Finii Finii commented Feb 7, 2022

Description

First part of solution:

[why]
For some powerline symbols we add a certain amount of overlap into the
previous or next character to cover up a small gap between the symbols
that otherwise can show up as ugly thin (usually colored) line.

But after we carefully design that glyph with a bit overlap (over-sized
and having negative bearings) we remove all bearings. That breaks of
course the glyph and no actual overlap on the left side happens.

[how]
Just do not remove negative bearings on overlap-enabled glyphs. As they
are rescaled in both directions anyhow all bearings are wanted and must
be kept.

Second part of solution:

[why]
With the previous commit the gap between powerline glyphs became much
better, but a faint line is still visible for LCD antialiasing (on my
machine).

Having seen how big the overlap is for Cascadia Code the overlap here
can be increase maybe a bit.

[how]
Increase the overlap to 3 % (from mostly 2 %) for the glyphs that have a
'full colored edge' on one side.

Fixes: #29

Requirements / Checklist

What does this Pull Request (PR) do?

Increase the overlap for some glyphs slightly and correct the needed negative left side bearings for glyphs that should have had an overlap on the left side but had not.

How should this be manually tested?

Any background context you can provide?

The issue comes up relatively often, I think. I was never bothered by the lines, so I dod not really follow that. Maybe I'm wrong. I have real list of Issues that are related.

What are the relevant tickets (if any)?

#29

Also see #779 for params future as dict.

Screenshots (if appropriate or helpful)

@MIvanchev used LiterationMono Nerd Font Mono and neovim-Qt, so I use that here as examples.
Note: Hinting full, Subpixel antialiasing, X11

Current behavior (before the PR):

overlap0
image

After fixing the code: (first part)

overlap1
image
Note more pronounced line for round things, as that has only 1%, the other have 2% overlap

After increasing the overlap: (second part)

overlap2
image

I guess 3% is enough, it is still very very faintly visible. Can be increased if users still complain

@Finii
Copy link
Collaborator Author

Finii commented Feb 7, 2022

Issue not completely fixed, needs probably 4% of overlap.
See #29 (comment)

Okay... let's pretend to be scientific...

Cascadia has this overlap:
image

Left negative bearing is 100, total width is 1200, this is well... 8% overlap horizontally.

But vertically, the glyph extends from 2226 to -480 while we have these metrics:

image

So there is virtually no overlap.

I will increase to 4% and limit vertical overlap to 1% as this will become ugly if stretched too much vertically.

@Finii
Copy link
Collaborator Author

Finii commented Feb 7, 2022

I see no problem with the 1% v-overlap.

Holy cow, the 'arrow up' glyph is tiny with that (i.e. Literation) font... (compare to underlying window with Cascadia)

image

Edit: Arrow is u_2191 and not patched in by us, it's just the font that has it so small 😅

@MIvanchev
Copy link

Maybe I should check out Cascadia... I do like Liberation a lot but some stuff are not OK.

@Finii
Copy link
Collaborator Author

Finii commented Mar 3, 2022

Just as that is kind of related...
When using Windows Terminal: microsoft/terminal#12601

Keep it here for reference if Issues turn up ;)

@musjj
Copy link

musjj commented Aug 24, 2022

Tested out this PR with JetBrainsMono in Neovide and it seem to mess up the offset of some glyphs.
For example, the half-circle looks like this:
image
(the character is supposed to be in the first, leftmost column)
The other pair () seems fine though:
image

@Finii
Copy link
Collaborator Author

Finii commented Oct 14, 2022

@musjj I guess you used the non-Mono version, i.e. JetBrainsMono Nerd Font (and not JetBrainsMono Nerd Font Mono).

Something seems to be wrong with the scaling in the non-Mono fonts.
I will try neovide, if it's not too complicated 😬 or maybe neovim-qt first, I believe I have that installed already.

@Finii
Copy link
Collaborator Author

Finii commented Oct 14, 2022

Hmm, neovim-qt does render it like this:

image

It allows the upper D shape thing to extend into the right cell, but cuts off the stuff that extrudes into the left cell for the inverse D thing on the lower line.

Hmm, and this is neovide

image

Both D shape things extend on both sided, as encoded in the font. (That is a bug in the font, but anyhow)

So I can not reproduce @musjj My version is freshly downloaded from https://github.com/neovide/neovide/releases/latest/download/neovide.tar.gz (reports Neovide 0.10.1)

Finii added a commit that referenced this pull request Oct 14, 2022
[why]
The vertical overlap has never been a problem (as far as I know). It is
maybe good to have some overlap for the terminal emulators that support
vertical overlap.
On terminals that truncate at the nominal cell border too much overlap
looks bad, i.e. the glyphs 'distorted'.

If we ever increase the overlap it is most likely be meant to be the
left-right overlap.

[note]
Originally this has been part of commit fecda6a of #780.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@MIvanchev
Copy link

What's the status here friends? Should this be merged or at least rebased? It aged a bit...

@LandonSchropp
Copy link

I'd still love to see this fixed. 🙂

@Finii
Copy link
Collaborator Author

Finii commented Jan 11, 2023

I'll rebase later, not sure if this should go into 2.3.0 or not 🤔

@MIvanchev
Copy link

A rebase would be enough for now as I can patch manually :}

Thanks I really appreciate it because it allows me to continue using Neovim-qt.

Finii added a commit that referenced this pull request Jan 12, 2023
[why]
The vertical overlap has never been a problem (as far as I know). It is
maybe good to have some overlap for the terminal emulators that support
vertical overlap.
On terminals that truncate at the nominal cell border too much overlap
looks bad, i.e. the glyphs 'distorted'.

If we ever increase the overlap it is most likely be meant to be the
left-right overlap.

[note]
Originally this has been part of commit fecda6a of #780.

[note2]
Originally this has been part of PR #967.
Although that had a bug 😬
It used max() instead of min() (T_T)

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
Finii added a commit that referenced this pull request Jan 12, 2023
[why]
The vertical overlap has never been a problem (as far as I know). It is
maybe good to have some overlap for the terminal emulators that support
vertical overlap.
On terminals that truncate at the nominal cell border too much overlap
looks bad, i.e. the glyphs 'distorted'.

If we ever increase the overlap it is most likely be meant to be the
left-right overlap.

[note]
Originally this has been part of commit fecda6a of #780.

[note2]
Originally this has been part of PR #967.
Although that had a bug 😬
It used max() instead of min() (T_T)

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii
Copy link
Collaborator Author

Finii commented Jan 12, 2023

Rebase on feature/allow-downscaling-nonmono (i.e. #748), which fixes a lot of scaling issues.

Force push.

@Finii
Copy link
Collaborator Author

Finii commented Jan 12, 2023

@MIvanchev

Hmm, I fixed some mathematical bug in the overlap formula (b3c079d), and now the overlap imposed on the glyphs by the same overla-percentage-value can be / is smaller. Maybe you can check if it is still good with 4%?

@MIvanchev
Copy link

On it! Thank you @Finii

Finii added a commit that referenced this pull request Jan 12, 2023
[why]
The vertical overlap has never been a problem (as far as I know). It is
maybe good to have some overlap for the terminal emulators that support
vertical overlap.
On terminals that truncate at the nominal cell border too much overlap
looks bad, i.e. the glyphs 'distorted'.

If we ever increase the overlap it is most likely be meant to be the
left-right overlap.

Note that the glyphs are usually valign='c' and the overlap is
distributed half top and half bottom. There are no other valign values
implemented (just 'not align' which is ... most likely bad).

[note]
Originally this has been part of commit fecda6a of #780.

[note2]
Originally this has been part of PR #967.
Although that had a bug 😬
It used max() instead of min() (T_T)

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

f
@Finii
Copy link
Collaborator Author

Finii commented Jan 12, 2023

Merged the other PR, rebase on master, force push.
It had yet another scaling and overlap fix 😬

All the changes (apart from increasing the overlap percentages) are merged now, so the changes here are trivial.
I'm not sure how Windows Terminal handles the (bigger) overlap, one would need to investigate that before we merge this.
But otoh, Cascadia Code also does have overlap through negative bearings for the powerline glyphs, so it should work.

@Finii
Copy link
Collaborator Author

Finii commented Jan 12, 2023

I have no recollection why we did not increase the other overlaps... Any hints, or should they also go to 4%?

image

@MIvanchev
Copy link

I have no idea as well, still testing :) Will be back shortly!

@MIvanchev
Copy link

Thanks for all your hard free work!

@MIvanchev
Copy link

I'm having some issues testing because the patcher sets weight to Book vs Regular. Is there a way to change this @Finii

@Finii
Copy link
Collaborator Author

Finii commented Jan 12, 2023

Which font do you patch?
Usually the weight is kept. If the patcher detects the correct one. But Book is only used if the original font somehow mentioned that.
You can try to --makegroups which uses the 'new' font naming engine that is a bit better.

@MIvanchev
Copy link

I used LiberationMono Regular from https://github.com/liberationfonts/liberation-fonts/files/7261482/liberation-fonts-ttf-2.1.5.tar.gz, will test --makegroups in a bit :)

@Finii
Copy link
Collaborator Author

Finii commented Jan 13, 2023

Hmm,

$ tar zxf liberation-fonts-ttf-2.1.5.tar.gz
$ find *ttf-2.1.5 -name '*.ttf' -exec fontforge ~/git/nerd-fonts/font-patcher {} \;
$ fontforge ~/git/nerd-fonts/bin/scripts/name_parser/query_names *.ttf 2>/dev/null
Examining 12 font files
 |Filename                                           | | Fullname                                                          | | Family                                                  | | Subfamily                      | | Typogr. Family                           | | Typogr. Subfamily
 |-------------------------------------------------- |-| ------------------------------------------------------------      |-| ------------------------------------------------------- |-| ------------------------------ |-| ---------------------------------------- |-| ----------------------------------------
 |Literation Mono Bold Italic Nerd Font.ttf          | | Literation Mono Bold Italic Nerd Font                             | | LiterationMono Nerd Font                                | | Bold Italic                    | | LiterationMono Nerd Font                 | |
 |Literation Mono Bold Nerd Font.ttf                 | | Literation Mono Bold Nerd Font                                    | | LiterationMono Nerd Font                                | | Bold                           | | LiterationMono Nerd Font                 | |
 |Literation Mono Italic Nerd Font.ttf               | | Literation Mono Italic Nerd Font                                  | | LiterationMono Nerd Font                                | | Italic                         | | LiterationMono Nerd Font                 | |
 |Literation Mono Nerd Font.ttf                      | | Literation Mono Nerd Font                                         | | LiterationMono Nerd Font                                | | Book                           | | LiterationMono Nerd Font                 | |
 |Literation Sans Bold Italic Nerd Font.ttf          | | Literation Sans Bold Italic Nerd Font                             | | LiterationSans Nerd Font                                | | Bold Italic                    | | LiterationSans Nerd Font                 | |
 |Literation Sans Bold Nerd Font.ttf                 | | Literation Sans Bold Nerd Font                                    | | LiterationSans Nerd Font                                | | Bold                           | | LiterationSans Nerd Font                 | |
 |Literation Sans Italic Nerd Font.ttf               | | Literation Sans Italic Nerd Font                                  | | LiterationSans Nerd Font                                | | Italic                         | | LiterationSans Nerd Font                 | |
 |Literation Sans Nerd Font.ttf                      | | Literation Sans Nerd Font                                         | | LiterationSans Nerd Font                                | | Book                           | | LiterationSans Nerd Font                 | |
 |Literation Serif Bold Italic Nerd Font.ttf         | | Literation Serif Bold Italic Nerd Font                            | | LiterationSerif Nerd Font                               | | Bold Italic                    | | LiterationSerif Nerd Font                | |
 |Literation Serif Bold Nerd Font.ttf                | | Literation Serif Bold Nerd Font                                   | | LiterationSerif Nerd Font                               | | Bold                           | | LiterationSerif Nerd Font                | |
 |Literation Serif Italic Nerd Font.ttf              | | Literation Serif Italic Nerd Font                                 | | LiterationSerif Nerd Font                               | | Italic                         | | LiterationSerif Nerd Font                | |
 |Literation Serif Nerd Font.ttf                     | | Literation Serif Nerd Font                                        | | LiterationSerif Nerd Font                               | | Book                           | | LiterationSerif Nerd Font                | |

Indeed what would be Regular is Book.

$ fontforge ~/git/nerd-fonts/bin/scripts/name_parser/query_names liberation-fonts-ttf-2.1.5/*.ttf 2>/dev/null
Examining 12 font files
 |Filename                                           | | Fullname                                                          | | Family                                                  | | Subfamily                      | | Typogr. Family                           | | Typogr. Subfamily
 |-------------------------------------------------- |-| ------------------------------------------------------------      |-| ------------------------------------------------------- |-| ------------------------------ |-| ---------------------------------------- |-| ----------------------------------------
 |LiberationMono-BoldItalic.ttf                      | | Liberation Mono Bold Italic                                       | | Liberation Mono                                         | | Bold Italic                    | |                                          | |
 |LiberationMono-Bold.ttf                            | | Liberation Mono Bold                                              | | Liberation Mono                                         | | Bold                           | |                                          | |
 |LiberationMono-Italic.ttf                          | | Liberation Mono Italic                                            | | Liberation Mono                                         | | Italic                         | |                                          | |
 |LiberationMono-Regular.ttf                         | | Liberation Mono                                                   | | Liberation Mono                                         | | Regular                        | |                                          | |
 |LiberationSans-BoldItalic.ttf                      | | Liberation Sans Bold Italic                                       | | Liberation Sans                                         | | Bold Italic                    | |                                          | |
 |LiberationSans-Bold.ttf                            | | Liberation Sans Bold                                              | | Liberation Sans                                         | | Bold                           | |                                          | |
 |LiberationSans-Italic.ttf                          | | Liberation Sans Italic                                            | | Liberation Sans                                         | | Italic                         | |                                          | |
 |LiberationSans-Regular.ttf                         | | Liberation Sans                                                   | | Liberation Sans                                         | | Regular                        | |                                          | |
 |LiberationSerif-BoldItalic.ttf                     | | Liberation Serif Bold Italic                                      | | Liberation Serif                                        | | Bold Italic                    | |                                          | |
 |LiberationSerif-Bold.ttf                           | | Liberation Serif Bold                                             | | Liberation Serif                                        | | Bold                           | |                                          | |
 |LiberationSerif-Italic.ttf                         | | Liberation Serif Italic                                           | | Liberation Serif                                        | | Italic                         | |                                          | |
 |LiberationSerif-Regular.ttf                        | | Liberation Serif                                                  | | Liberation Serif                                        | | Regular                        | |                                          | |

And it is not from the sourcefont.

Investigating

@Finii
Copy link
Collaborator Author

Finii commented Jan 13, 2023

At least the 'new' method does is as expected:

$ rm *.ttf
$ find *ttf-2.1.5 -name '*.ttf' -exec fontforge ~/git/nerd-fonts/font-patcher --makegroups {} \;
$ fontforge ~/git/nerd-fonts/bin/scripts/name_parser/query_names *.ttf 2>/dev/null
Examining 12 font files
 |Filename                                           | | Fullname                                                          | | Family                                                  | | Subfamily                      | | Typogr. Family                           | | Typogr. Subfamily
 |-------------------------------------------------- |-| ------------------------------------------------------------      |-| ------------------------------------------------------- |-| ------------------------------ |-| ---------------------------------------- |-| ----------------------------------------
 |Literation Mono Nerd Font Bold Italic.ttf          | | Literation Mono Nerd Font Bold Italic                             | | LiterationMono Nerd Font                                | | Bold Italic                    | |                                          | |
 |Literation Mono Nerd Font Bold.ttf                 | | Literation Mono Nerd Font Bold                                    | | LiterationMono Nerd Font                                | | Bold                           | |                                          | |
 |Literation Mono Nerd Font Italic.ttf               | | Literation Mono Nerd Font Italic                                  | | LiterationMono Nerd Font                                | | Italic                         | |                                          | |
 |Literation Mono Nerd Font.ttf                      | | Literation Mono Nerd Font                                         | | LiterationMono Nerd Font                                | | Regular                        | |                                          | |
 |Literation Sans Nerd Font Bold Italic.ttf          | | Literation Sans Nerd Font Bold Italic                             | | LiterationSans Nerd Font                                | | Bold Italic                    | |                                          | |
 |Literation Sans Nerd Font Bold.ttf                 | | Literation Sans Nerd Font Bold                                    | | LiterationSans Nerd Font                                | | Bold                           | |                                          | |
 |Literation Sans Nerd Font Italic.ttf               | | Literation Sans Nerd Font Italic                                  | | LiterationSans Nerd Font                                | | Italic                         | |                                          | |
 |Literation Sans Nerd Font.ttf                      | | Literation Sans Nerd Font                                         | | LiterationSans Nerd Font                                | | Regular                        | |                                          | |
 |Literation Serif Nerd Font Bold Italic.ttf         | | Literation Serif Nerd Font Bold Italic                            | | LiterationSerif Nerd Font                               | | Bold Italic                    | |                                          | |
 |Literation Serif Nerd Font Bold.ttf                | | Literation Serif Nerd Font Bold                                   | | LiterationSerif Nerd Font                               | | Bold                           | |                                          | |
 |Literation Serif Nerd Font Italic.ttf              | | Literation Serif Nerd Font Italic                                 | | LiterationSerif Nerd Font                               | | Italic                         | |                                          | |
 |Literation Serif Nerd Font.ttf                     | | Literation Serif Nerd Font                                        | | LiterationSerif Nerd Font                               | | Regular                        | |                                          

I would advise anyone to always use --makegroups, at least when patching more than one font in a group.
At some point in the future the 'old' mathod will be dropped as it has more than 2 and 1/2 known bugs.

@Finii
Copy link
Collaborator Author

Finii commented Jan 13, 2023

Lets separate the issues and discuss here only the vertical lines. Created new Issue #1046

@MIvanchev
Copy link

Possible to get another rebase to account for the naming issues? :))

[why]
With the previous commit the gap between powerline glyphs became much
better, but a faint line is still visible for LCD antialiasing (on my
machine).

Having seen how big the overlap is for Cascadia Code the overlap here
can be increase maybe a bit.

[how]
Increase the overlap to 3 % (from mostly 2 %) for the glyphs that have a
'full colored edge' on one side.

Fixes: #29

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
On some machine the gap is still visible, but with 4% overlap the
result looks good.

[how]
Increase the overlap from 3% to 4%.

Cascadia Code has 8% of overlap, but they have a special extension to
the overlap side, we will have a visual glitch if the overlap becomes
too big. So lets stay with 4% or we should change the basis for the
powerline glyphs.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii
Copy link
Collaborator Author

Finii commented Jan 13, 2023

Here you are.

Grump, it runs the PackSVG workflow ... sigh

@MIvanchev
Copy link

Thanks in case you're wondering why I don't rebase myself, there's a slight issue with the repo size ;)

@MIvanchev
Copy link

So all of my issues are resolved currently, I see no glitching.

@Finii
Copy link
Collaborator Author

Finii commented Jan 13, 2023

It's by far the biggest singe thing on my laptop:

image

(light red chunk)

:-D

@Finii
Copy link
Collaborator Author

Finii commented Mar 10, 2023

Increasing the overlap introduces issues with some terminal emulators.
The lines are usually an artifact from subpixel rendering and can be avoided if the user turns on greyscale-antialiasing.

Anyhow, as terminals behave differently, if we improve the situation for one it will get worse for another.

Maybe we can live with what we have now. Moving this to the Parallel Universe.

@Finii Finii closed this Mar 10, 2023
@Finii Finii mentioned this pull request Aug 11, 2023
3 tasks
LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 2023
[why]
The vertical overlap has never been a problem (as far as I know). It is
maybe good to have some overlap for the terminal emulators that support
vertical overlap.
On terminals that truncate at the nominal cell border too much overlap
looks bad, i.e. the glyphs 'distorted'.

If we ever increase the overlap it is most likely be meant to be the
left-right overlap.

Note that the glyphs are usually valign='c' and the overlap is
distributed half top and half bottom. There are no other valign values
implemented (just 'not align' which is ... most likely bad).

[note]
Originally this has been part of commit fecda6a of ryanoasis#780.

[note2]
Originally this has been part of PR ryanoasis#967.
Although that had a bug 😬
It used max() instead of min() (T_T)

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>

f
@Finii Finii mentioned this pull request Jan 22, 2024
2 tasks
@Finii Finii deleted the bugfix/overlap branch April 22, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Odd gap between glyphs and text
4 participants