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

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

Closed
LandonSchropp opened this issue Sep 6, 2021 · 32 comments
Closed

Comments

@LandonSchropp
Copy link

LandonSchropp commented Sep 6, 2021

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

  • Are you using OS X, Linux or Windows? And which specific version or distribution?

I'm running macOS 11.5.2.

★ Optional

Thanks for the awesome fonts!

@LandonSchropp LandonSchropp changed the title Curved separators do not extend to full height of character box [Bug] Curved separators do not extend to full height of character box Sep 6, 2021
@Bahnschrift
Copy link

Bahnschrift commented Sep 18, 2021

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

@Bahnschrift
Copy link

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
Copy link
Author

@Bahnschrift Awesome! Thank you. 🙂

@Finii
Copy link
Collaborator

Finii commented Nov 24, 2021

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
Copy link
Collaborator

Finii commented Nov 24, 2021

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
Copy link
Collaborator

Finii commented Nov 24, 2021

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
1edd4ca 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
Copy link
Author

@Finii That's quite the explanation. 🙂

@LandonSchropp
Copy link
Author

@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
Copy link
Collaborator

Finii commented Dec 23, 2021

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 a7f91ac. 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
Copy link
Collaborator

Finii commented Dec 23, 2021

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
Copy link
Author

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
Copy link
Author

LandonSchropp commented Dec 24, 2021

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 added a commit that referenced this issue Dec 27, 2021
See Issue #661

Not to be merged.

Signed-off-by: Fini Jastrow <[email protected]>
@Finii
Copy link
Collaborator

Finii commented Dec 27, 2021

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
Copy link
Collaborator

Finii commented Feb 8, 2022

Will be fixed with #780 (I believe 😬)

@LandonSchropp
Copy link
Author

@Finii Awesome!

@Finii
Copy link
Collaborator

Finii commented Jan 12, 2023

This should be fixed now by 95bdc42 and 0db79a0 via #748

@Finii Finii closed this as completed Jan 12, 2023
@LandonSchropp
Copy link
Author

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
Copy link
Collaborator

Finii commented Mar 23, 2023

@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
Copy link
Collaborator

Finii commented Mar 23, 2023

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
Copy link
Collaborator

Finii commented Mar 23, 2023

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
Copy link
Collaborator

Finii commented Mar 23, 2023

@LandonSchropp
Copy link
Author

LandonSchropp commented Jul 11, 2023

Apologies @Finii, I completely missed your responses.

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

@LandonSchropp
Copy link
Author

LandonSchropp commented Aug 31, 2023

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
Copy link

@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
Copy link
Collaborator

Finii commented Dec 9, 2023

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
Copy link
Author

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
Copy link
Collaborator

Finii commented Dec 10, 2023

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
Copy link
Collaborator

Finii commented Dec 10, 2023

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
Copy link
Collaborator

Finii commented Dec 10, 2023

Here is the iTerm2 issue

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

Screenshot 2023-12-10 at 11 41 45

@CarlosMed
Copy link

CarlosMed commented Dec 10, 2023

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
Copy link
Author

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

Copy link
Contributor

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 Jun 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants