ryanoasis / nerd-fonts

Iconic font aggregator, collection, & patcher. 3,600+ icons, 50+ patched fonts: Hack, Source Code Pro, more. Glyph collections: Font Awesome, Material Design Icons, Octicons, & more
https://NerdFonts.com
Other
52.6k stars 3.59k forks source link

[Bug] Curved separators do not extend to full height of character box #661

Closed LandonSchropp closed 1 year ago

LandonSchropp commented 2 years ago

I'm trying to set up a status line plugin in NeoVim, and I'm running into an issue with the curved separator characters (, , , and ). While configuring my status line, I noticed that these characters do not extend the full height of the character. The chevron separator characters (, , , and ) don't seem to share this problem.

Screen Shot 2021-09-06 at 12 51 06 AM Screen Shot 2021-09-06 at 12 53 26 AM Screen Shot 2021-09-06 at 12 50 47 AM

Unfortunately, this ruins the look of my status line:

Screen Shot 2021-09-06 at 12 40 05 AM

I'm sure this issue is related to the line height of 120% I'm using for my characters. However, the font is so much more readable at that line height that I hesitate to change it, and since the chevron characters seem to work fine, I thought I'd open up an issue.

Here are some exaggerated views of the problem at 200% line height:

Screen Shot 2021-09-06 at 1 00 04 AM Screen Shot 2021-09-06 at 1 00 36 AM

🔧 Your Setup

Which font are you using?

I'm using the lastest version of Sauce Code Pro.

Which terminal emulator are you using?

iTerm 2 3.4.8 with the following profile text settings:

Screen Shot 2021-09-06 at 12 44 53 AM

I'm running macOS 11.5.2.

★ Optional

Thanks for the awesome fonts!

Bahnschrift commented 2 years ago

I've also run into this issue when patching multiple other fonts, and I'm not using any sort of changed line height.

Bahnschrift commented 2 years ago

I may have just managed to fix this by changing line 743 in font-patcher from scale_ratio_y = self.font_dim['height'] / sym_dim['height'] to scale_ratio_y = sym_dim["height"] / self.font_dim["height"] I'll test this some more and submit a PR if it works for other fonts.

LandonSchropp commented 2 years ago

@Bahnschrift Awesome! Thank you. 🙂

Finii commented 2 years ago

Although I have no terminal that scales the glyphs like yours, I can see that the curvies are smaller:

image

Top-Left point: 0 ; 997 | 0 ; 991 Bottom-Left: 0 ; -285 | 0 ; -279

Finii commented 2 years ago

from scale_ratio_y = self.font_dim['height'] / sym_dim['height'] to scale_ratio_y = sym_dim["height"] / self.font_dim["height"]

Well, later the scale_ratio_y is used on the sym and it should become self.font (symbolically speaking).

Like sym_dim["height"] * scale_ratio_y shall be desired output font height self.font_dim["height"]

So the original formula is correct.

The differences come in later with 'overlap', lets see... fiddling around


Here the concrete numbers (left is the triangular thing, right is the D shape, all values: height):

Beginning (sym_dim): 1135 / 1257 y-scale-fact: 1.10749 / 1.06616 After scale: 1257 / 1257

Edit: Typo and add numbers

Finii commented 2 years ago

Overlap is differently defined for triangular and roundish things: 0.01 vs 0.02, and that is specified deliberately so:

    def setup_patch_set(self):
          """ Creates list of dicts to with instructions on copying glyphs from each symbol font into self.sourceFont """
          # Supported params: overlap | careful
          # Powerline dividers
          SYM_ATTR_POWERLINE = {
              'default': {'align': 'c', 'valign': 'c', 'stretch': 'pa', 'params': ''},

              # Arrow tips
              0xe0b0: {'align': 'l', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.02}},
              0xe0b1: {'align': 'l', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.02}},
              0xe0b2: {'align': 'r', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.02}},
              0xe0b3: {'align': 'r', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.02}},

              # Rounded arcs
              0xe0b4: {'align': 'l', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.01}},
              0xe0b5: {'align': 'l', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.01}},
              0xe0b6: {'align': 'r', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.01}},
              0xe0b7: {'align': 'r', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.01}},

This is the fact since it has been introduced with commit 1edd4ca9 Fixed final issues with careful mode, parameterized overlap.

No explanation is given in the commit, but I guess it helps to get the visual height equal. Usually with fonts peaky stuff protrudes more than roundish or even horizontal structures, because how people see things.


Well, because this is deliberate, but unwanted obviously for you use case, I do not know what to do now. This is something for @ryanoasis to decide ;-D

LandonSchropp commented 2 years ago

@Finii That's quite the explanation. 🙂

LandonSchropp commented 2 years ago

@ryanoasis @Finii Did you guys ever get a chance to loop back on this? From a pure user standpoint, the current behavior I mentioned in the original Issue explanation isn't ideal.

Looking at the commit @Finii mentioned, it looks like many changes were made. Maybe this was unintended? Is it possible that those specific issues affected these particular characters in an unintended way?

I'd love to get this fixed if possible. Please let me know!

Happy holidays. 🙂

Finii commented 2 years ago

The commit I mentioned means 1edd4ca? This looks like intentional. It was back in 2016 as you will have noticed.

The git discipline was not high at that time: there are two changes in one commit. But half the commit is there to introduce / change from just one uniform overlap to two different overlaps. Unfortunately there is no reasoning given in the commit message. Feel free to dive into the Issues and PullRequests here, but I have the feeling that no explanation has ever been written down.

I'm not sure if a change with that overlap will happen anytime soon.

Personally I do not like the overlap at all, and I have the hunch it is there because of rounding issues - that would be fixed with a7f91ac8. At least some of them, I guess there are more lurking. (*)

Well... meanwhile you could self-patch the font of your choice. To fix the behavior you just need to change the 0.01s in the code to 0.02 or the other way around, whatever suits you.

Happy holdidays for you too :-) Just packed all the gifts for my kids :-D

_(*) Just double checked, although I mention only --mono in the commit, it also affects the stretch and overlap parameters. They work with slightly wrong rounded values (sym_dim is off by some 0.x % due to rounding effects in the current code. Mind the rounding is hidden in fontforge and not in font-patcher, so it's hard to realize it's even there))._

Finii commented 2 years ago

Looks as intentional as can be...

From one overlap that can be True or False to a overlap-factor, and that is 0.01 for some and 0.02 for other glyphs.

image

... *grinning'* but then ... vertical align = center and stretch = vertical is kind of ... redundant. If I stretch to fill verticlly completely, what shall the centering do at all? And still it has been added in that commit. ¯\_(ツ)_/¯

LandonSchropp commented 2 years ago

Do you think the result makes sense? For the  and  characters, is the screenshot I posted the desired behavior? I'm just not sure why we wouldn't want the rounded characters to stretch, even if they were intentionally set that way in a7f91ac.

I'm still not sure exactly what overlap means in this context, but maybe I need to spend a little more time digging. I'll play around with the value a bit and see what it looks like after I change them. I'll submit a PR if things are working a bit nicer, and we can see where things go from there.

Just packed all the gifts for my kids :-D

Exciting! I hope you guys have a great Chrstimas! 🎅

LandonSchropp commented 2 years ago

I tried editing the overlap, but I'm not seeing any change in the font. Maybe I'm doing something wrong. I don't feel like I have enough expertise in working with fonts to fix this.

I still feel like this should be changed. The arc separators should work like the arrow separators. I'd definitely appreciate if somebody with more expertise could pick this up and run with it.

Finii commented 2 years ago

I tried this:

diff --git a/font-patcher b/font-patcher
index 49e2ad79..294f427b 100755
--- a/font-patcher
+++ b/font-patcher
@@ -464,10 +464,10 @@ class font_patcher:
             0xe0b3: {'align': 'r', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.02}},

             # Rounded arcs
-            0xe0b4: {'align': 'l', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.01}},
-            0xe0b5: {'align': 'l', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.01}},
-            0xe0b6: {'align': 'r', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.01}},
-            0xe0b7: {'align': 'r', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.01}},
+            0xe0b4: {'align': 'l', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.02}},
+            0xe0b5: {'align': 'l', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.02}},
+            0xe0b6: {'align': 'r', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.02}},
+            0xe0b7: {'align': 'r', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.02}},

             # Bottom Triangles
             0xe0b8: {'align': 'l', 'valign': 'c', 'stretch': 'xy', 'params': {'overlap': 0.02}},

Then I ran this command in this repo's root dir:

fontforge --script font-patcher --powerline --powerlineextra --no-progressbar src/unpatched-fonts/SourceCodePro/Regular/SourceCodePro-Regular.ttf

Examining the resultant fonts shows that U+E0B0 ('>' shape) and U_E0B4 ('D' shape) have the same extrema points: 0/997 and 0/-286

My problem is that I do not have any Mac, Apple does not allow VMs ;), and the Macs that I occasionally use to test stuff (build hosts in fact) I have no idea if iTerm is on them. That means that you need to test the font yourself. I know of no (other) terminal that rescales glyphs with the user chosen line height.

You can find that font, that I patched, here: https://github.com/ryanoasis/nerd-fonts/blob/experimental/Sauce-bigger-curvies/Sauce%20Code%20Pro%20Nerd%20Font.ttf

Please remove all other Sauces, and install this, and retry. I do not know what it takes to replace a font on Mac, maybe you need to reboot or what ;)

Finii commented 2 years ago

Will be fixed with #780 (I believe :grimacing:)

LandonSchropp commented 2 years ago

@Finii Awesome!

Finii commented 1 year ago

This should be fixed now by 95bdc420da and 0db79a0b97 via #748

LandonSchropp commented 1 year ago

Hey @Finii , have the downs been updated on this page? I'm still seeing the same issue with the latest version of Jetbrains Mono.

Screen Shot 2023-03-22 at 4 30 18 PM
Finii commented 1 year ago

@LandonSchropp No. I can prepare you a patched font set if you need it and can not self-patch.

Working hard on v3.0.0 which will update 'everything', still hope to release in March.

Finii commented 1 year ago

To be honest I have lost track of which fix went into which release, so maybe it would be good if I prepare a 3.0.0 version of JetBrains that you can try? To make sure it is indeed fixed.

Finii commented 1 year ago

From looking at the code this should be fixed in master:

image

Will link to JetBrainsNF 3.0.0 here once it is finished: \<void>

Finii commented 1 year ago

Will link to JetBrainsNF 3.0.0 here once it is finished:

https://github.com/ryanoasis/nerd-fonts/tree/feature/reorganize-naming-testpatch/patched-fonts/JetBrainsMono/Ligatures

LandonSchropp commented 11 months ago

Apologies @Finii, I completely missed your responses.

No need to prepare anything special for me. I can wait for the 3.0 release. 😄

LandonSchropp commented 10 months ago

Hey @Finii, I'm just looping back to this now. I've downloaded the latest release of JetBrains Mono from the main site, but I'm still seeing this issue.

Screen Shot 2023-08-30 at 10 39 42 PM

Do you know if the fix would have been released by now?

Thanks!

CarlosMed commented 6 months ago

@LandonSchropp @Finii was this ever figured out or patched? I'm in the same boat as you on this one. Setting line height on iTerm messes up the curved font for Fira Code Mono.

Finii commented 6 months ago

Sorry @LandonSchropp , I missed your comment.

I just checked, the E0B0 and E0B4 are exactly the same height on 3.1.1.

Then I tried the font with iTerm, and I can not make the E0B0 scale with the extended cell height. What do I have to do?

Screenshot 2023-12-09 at 18 47 22
LandonSchropp commented 6 months ago

Not a problem @Finii. I appreciate your continued help with this issue. 😄

It looks like the "Use built-in Powerline glyphs" setting might be needed. Here are my current iTerm text settings settings:

Screen Shot 2023-12-09 at 10 27 50 PM

Here's a test string to print in the terminal that shows the issue:

RED='\033[0;31m'
GREEN='\033[0;32m'
BLACK_ON_RED='\033[41;30m'
BLACK_ON_GREEN='\033[42;30m'
NO_COLOR='\033[0m'

echo -e "${RED}${BLACK_ON_RED}hello${RED} ${GREEN}${BLACK_ON_GREEN}hello${GREEN}${NO_COLOR}"

And this is how that looks for me:

Screen Shot 2023-12-09 at 10 30 08 PM
Finii commented 6 months ago

Ah yes, I see, thanks @LandonSchropp

They just readjust the cell height when their own glyphs are used - it seems. And they do not provide a d-shaped thing.

Quick dive into the source of iTerm2, it seems to be fixed in master, at least in 3.4.19 these files are not there:

image

Trying bleeding edge iTerm2...

Finii commented 6 months ago

They added all the powerline glyphs and more to the "internal" set, and only the internal set is scaled to increased line height.

Screenshot 2023-12-10 at 11 24 55 Screenshot 2023-12-10 at 11 27 37
Finii commented 6 months ago

Here is the iTerm2 issue

https://gitlab.com/gnachman/iterm2/-/issues/10180

Screenshot 2023-12-10 at 11 41 45
CarlosMed commented 6 months ago

Ah was not aware there was an issue already open in regards to glyphs. I'll follow this issue in case something pops up. Appreciate it @Finii!

edit: upgraded to iTerm nightly Build 3.5.0beta17 and activating the setting Use built-in Powerline glyphs fixes the rendering issue for the half circle.

LandonSchropp commented 6 months ago

I didn't realize that either. Thank you so much @Finii—you rock! ⭐

github-actions[bot] commented 3 weeks ago

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.