这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@Finii
Copy link
Collaborator

@Finii Finii commented Feb 15, 2023

Description

This is just a stub and the commits, more will follow

Requirements / Checklist

What does this Pull Request (PR) do?

How should this be manually tested?

Any background context you can provide?

What are the relevant tickets (if any)?

Fixes: #1108
Fixes: #1122
Fixes: #1128
Fixes: #589

Screenshots (if appropriate or helpful)

@Finii
Copy link
Collaborator Author

Finii commented Feb 24, 2023

Test case stuff, taken from Wikipedia:

┌─┬┐  ╔═╦╗  ╓─╥╖  ╒═╤╕
│ ││  ║ ║║  ║ ║║  │ ││
├─┼┤  ╠═╬╣  ╟─╫╢  ╞═╪╡
└─┴┘  ╚═╩╝  ╙─╨╜  ╘═╧╛
┌───────────────────┐
│  ╔═══╗ Some Text  │▒
│  ╚═╦═╝ in the box │▒
╞═╤══╩══╤═══════════╡▒
│ ├──┬──┤           │▒
│ └──┴──┘           │▒
└───────────────────┘▒   
 ▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒▒

as image:

image

@Finii Finii force-pushed the feature/add-box-drawing branch from e72e38d to 733fb55 Compare March 2, 2023 14:10
@Finii
Copy link
Collaborator Author

Finii commented Mar 2, 2023

This looks bad:
image

@Finii
Copy link
Collaborator Author

Finii commented Mar 7, 2023

$ ./gotta-patch-em-all-font-patcher\!.sh -cjv /RobotoMono
# [Nerd Fonts]  Filter given, limiting search and patch to pathname pattern '/RobotoMono'
# [Nerd Fonts]  Total source fonts found: 10
# [Nerd Fonts]  Release timestamp is Tue, 07 Mar 2023 12:29:27 +0100
# [Nerd Fonts]  Processing font 1/10
[...]
Nerd Fonts Patcher v2.3.3-88 (3.6.1) (ff 20220308) executing
  Setting Panose 'Family Kind' to 'Latin Text and Display' (was 'Any')
  Setting Panose 'Proportion' to 'Monospaced' (was 'Any')
Nerd Fonts: WARNING Font vertical metrics inconsistent (HHEA 2400 / TYPO 2150 / WIN 2458), using WIN
[...]
Warning: Extended glyphs wider than basic glyphs
  Advance widths (base/extended): 1229 - 1229 / 1230 - 1449
Warning: Font has negative right side bearing in extended glyphs
[...]
$ cd ../..
$ ls check-fonts/RobotoMono/**/*.ttf
'check-fonts/RobotoMono/Bold/complete/Roboto Mono Nerd Font Complete Bold.ttf'
'check-fonts/RobotoMono/Bold/complete/Roboto Mono Nerd Font Complete Mono Bold.ttf'
'check-fonts/RobotoMono/Bold-Italic/complete/Roboto Mono Nerd Font Complete Bold Italic.ttf'
'check-fonts/RobotoMono/Bold-Italic/complete/Roboto Mono Nerd Font Complete Mono Bold Italic.ttf'
'check-fonts/RobotoMono/Italic/complete/Roboto Mono Nerd Font Complete Italic.ttf'
'check-fonts/RobotoMono/Italic/complete/Roboto Mono Nerd Font Complete Mono Italic.ttf'
'check-fonts/RobotoMono/Light/complete/Roboto Mono Nerd Font Complete Light.ttf'
'check-fonts/RobotoMono/Light/complete/Roboto Mono Nerd Font Complete Mono Light.ttf'
'check-fonts/RobotoMono/Light-Italic/complete/Roboto Mono Nerd Font Complete Light Italic.ttf'
'check-fonts/RobotoMono/Light-Italic/complete/Roboto Mono Nerd Font Complete Mono Light Italic.ttf'
'check-fonts/RobotoMono/Medium/complete/Roboto Mono Nerd Font Complete Medium.ttf'
'check-fonts/RobotoMono/Medium/complete/Roboto Mono Nerd Font Complete Mono Medium.ttf'
'check-fonts/RobotoMono/Medium-Italic/complete/Roboto Mono Nerd Font Complete Medium Italic.ttf'
'check-fonts/RobotoMono/Medium-Italic/complete/Roboto Mono Nerd Font Complete Mono Medium Italic.ttf'
'check-fonts/RobotoMono/Regular/complete/Roboto Mono Nerd Font Complete Mono.ttf'
'check-fonts/RobotoMono/Regular/complete/Roboto Mono Nerd Font Complete.ttf'
'check-fonts/RobotoMono/Thin/complete/Roboto Mono Nerd Font Complete Mono Thin.ttf'
'check-fonts/RobotoMono/Thin/complete/Roboto Mono Nerd Font Complete Thin.ttf'
'check-fonts/RobotoMono/Thin-Italic/complete/Roboto Mono Nerd Font Complete Mono Thin Italic.ttf'
'check-fonts/RobotoMono/Thin-Italic/complete/Roboto Mono Nerd Font Complete Thin Italic.ttf'

@Finii
Copy link
Collaborator Author

Finii commented Mar 7, 2023

Rendering in Tilix (left) and kitty

image

WAT, Tilix is rendering it as solid gray and not checkered black and white?

image

@Finii
Copy link
Collaborator Author

Finii commented Mar 7, 2023

image
image
image

image

@Finii
Copy link
Collaborator Author

Finii commented Mar 7, 2023

Font Book is content

Screenshot 2023-03-07 at 15 14 10

iTerm2:

Screenshot 2023-03-07 at 15 11 24

@Finii
Copy link
Collaborator Author

Finii commented Mar 7, 2023

This is wrong!

Screenshot 2023-03-07 at 17 48 20

introduced by the latest commit 47da025 🤨

@Finii
Copy link
Collaborator Author

Finii commented Mar 7, 2023

Another issue, as seen with iTerm2:

image

It's not high enough. It's not even v centered?

Ah, we inherit this from Hack (hey Hack, that is wrong!!!)

image

and we scale them all together in one group:

        BOX_SCALE_LIST = {'ScaleGroups': [
            [*range(0x2500, 0x2570 + 1), *range(0x2574, 0x257f + 1)], # box drawing
            range(0x2571, 0x2573 + 1), # diagonals
            range(0x2580, 0x259f + 1), # blocks
        ]}

Bounding boxes of the glyphs in that group:

2580: (-12.0, 702.0, 1241.0, 1958.0)
2581: (-12.0, -524.0, 1241.0, -203.0)
2582: (-12.0, -524.0, 1241.0, 99.0)
2583: (-12.0, -524.0, 1241.0, 400.0)
2584: (-12.0, -524.0, 1241.0, 702.0)
2585: (-12.0, -524.0, 1241.0, 1004.0)
2586: (-12.0, -524.0, 1241.0, 1306.0)
2587: (-12.0, -524.0, 1241.0, 1607.0)
2588: (-12.0, -524.0, 1241.0, 1958.0)
2589: (-12.0, -524.0, 1100.0, 1958.0)
258A: (-12.0, -524.0, 937.0, 1958.0)
258B: (-12.0, -524.0, 776.0, 1958.0)
258C: (-12.0, -524.0, 614.0, 1958.0)
258D: (-12.0, -524.0, 452.0, 1958.0)
258E: (-12.0, -524.0, 291.0, 1958.0)
258F: (-12.0, -524.0, 128.0, 1958.0)
2591: (-12.0, -504.0, 1086.0, 1909.0)
2592: (-12.0, -504.0, 1241.0, 1908.0)
2593: (-12.0, -504.0, 1241.0, 1909.0)
2594: (-12.0, 1607.0, 1241.0, 1958.0)
2596: (-12.0, -524.0, 615.0, 702.0)
2598: (-12.0, 702.0, 615.0, 1958.0)
2599: (-12.0, -524.0, 1242.0, 1958.0)
259A: (-12.0, -524.0, 1242.0, 1958.0)
259B: (-12.0, -524.0, 1242.0, 1958.0)
259C: (-12.0, -524.0, 1242.0, 1958.0)
259E: (-12.0, -524.0, 1242.0, 1958.0)
259F: (-12.0, -524.0, 1242.0, 1958.0)

@Finii
Copy link
Collaborator Author

Finii commented Mar 8, 2023

Examining 600 font files

Name # of box glyphs
3270 80
Agave 74
AnonymousPro 40
Arimo 48
AurulentSansMono 0
BitstreamVeraSansMono 0
CascadiaCode 160
CodeNewRoman 48
Cousine 48
DaddyTimeMono 159
DejaVuSansMono 160
DroidSansMono 0
FantasqueSansMono 160
FiraCode 160
FiraMono 148
Gohu 0, 125, 160
Go-Mono 48
Hack 160
Hasklig 160
HeavyData 0
Hermit 17
iA-Writer 0
IBMPlexMono 0
Inconsolata 160
InconsolataGo 0
InconsolataLGC 160
Iosevka 160
JetBrainsMono 160
Lekton 0
LiberationMono 48
Lilex 0
Meslo 32, 160
Monofur 48
Monoid 0, 1
Mononoki 160
MPlus 129
Noto 0, 160
OpenDyslexic 0
Overpass 0, 128
ProFont 0, 48
ProggyClean 0
RobotoMono 0
ShareTechMono 0
SourceCodePro 160
SpaceMono 0
Terminus 150
Tinos 48
Ubuntu 0
UbuntuMono 44
VictorMono 160

@Finii Finii marked this pull request as ready for review March 8, 2023 16:56
Finii added 7 commits March 10, 2023 12:31
Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
For some reason we determine the bounding box xmax value of the 'normal'
extended glyphs. For the cell size we use the advance width of those
glyphs - the xmax values is not utilized at all.

But if we would ever use it, it might be good to see that something
unexpected(?) happened.

This commit is not really necessary. Maybe it is good, maybe it just
adds noise. We can always remove it later.

No functional change.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
All the box drawing glyphs should be scaled and shifted as one (i.e.
equally). For such glyph sets we have the ScaleGroups that handle it
nicely, determining a combined bounding box and calculation scale and
shift from that bounding box instead of the actual glyph's bounding box.

But unfortunately it is hard-wired to do just 'pa' scaling. For the box
drawing glyphs we need 'xy' scaling.

[how]
The preparatory stage calculates the 'pa' factor for ScaleGroups for us.
That is mainly so because the old system worked that way and has no
notion of combinded-bounding-box. The data needed to be stored in one
number, the scale. Later came the correct shifting, which needed the
bounding box. But the scaling still relied on the one scale factor that
is used for x and y.

Instead, if we have a combinded bounding box, we ignore the
precalculated scale factor and calculate a new set of x- and y-scales
based on the requested scaling algorithm. In this way we can get 'xy' or
'pa' or even 'xy2' scaling, or whatever we like, based on the combined
bounding box.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
The whole patching process addresses the glyphs via their unicode
number / codepoint. We ensure the adressability for the to be patched
font, but the symbol fonts can differ.

[how]
Just set the way we want to address the symbol font glyphs.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
When the destination font has box drawing glyphs and we change the
'cell' size, we need to rescale the existing glyphs (so that they fill
the new 'cell'.

[how]
Add a new parameter attribute that skips the copying und just works on
the scaling of glyphs that have this.

For a correct message only the default attribute is checked.

[note]
This just add the possibility, it is not yet used.

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

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
If the to-be-patched font already has all box drawing glyphs we could
use them instead of our extra set from Hack.

But we need to scale them in case the 'cell' size has changed.

[how]
All the mechanics have been already added, we just need to enable it now
in the right cases.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
Finii added 10 commits March 10, 2023 12:32
[why]
Comes from a23e33c, should not have been included in commit.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
Defaults should come first.
Unify empty lines or no empty lines.
Add some docu.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
The unicode 2630 (Trigraph Heaven) is often used in vim powerlines (at
least).

[how]
Draw nice 3 rectangles.
Insert 'pa1', always scaling also in non mono fonts. That needs a new
attribute: '!'.

The scaling is in fact an issue. Using 'pa' is the way of least
resistance.
Without the new attribute the glyph would look different in mono and
nonmono, which is not nice.

Fixes: #589

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
This is more an academic fix. If we calculate widths from with bounding
boxes we always need to take xmin and xmax into account. Usually xmin is
zero and so it does not make any difference.

But maybe one can see better what is calculated, especially as we use
xmin in other cases.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
The box drawing glyphs (center aligned) and some Powerline glyphs (that
are right aligned and have a limited xy-ratio) are positioned wrong.

Affected are all box drawing glyphs (e.g. 2548) and some Powerline
glyphs (i.e. E0B2, E0B6, E0C5, E0C7, and E0D4).

[how]
The box drawing glyphs are center aligned and have overlap. The code
does not correct the overlap (left bearing) but uses the default case of
'make the left bearing zero'. The code does just check left aligned
glyphs and not center aligned ones.
Add the correct overlap for center aligned glyphs (i.e. half the
overlap).

The Powerline glyphs are right aligned. Usually that works, because the
glyphs are created with the right size, so that no additional
manipulation is needed.
But if the glyph has a ratio limit the resulting size will be different.
We could in fact fix the size code, somehow, but that is rather
complicated, formula-wise. Instead we just scale these glyphs (which are
the 5 listed above) and shift them to the right position such that the
correct overlap results.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
Some few glyphs get a wrong left side bearing in `Nerd Font`:
    E0C2: -1230
    E0C5: -857
    E0C7: -667
    E0CA: -1230

These are the powerline glyphs which are right aligned and have overlap
and a target width of 2 cells wide.

[how]
To simplify the code add a new function that decides if a symbol shall
be one or two cells wide.
That function is then used where we had explicit tests already.

Use the function also in the overlap correction code, such that the
overlap is corrected for the right cell occupancy of the concrete glyph.

[note]
I guess that the overlap correction for 'c' alignment for 2 cell wide
glyphs is also broken. But we do not have such glyphs, so we ignore it
for now :-}

This fixes the previous commit 'font-patcher: Fix overlap for align c and r'.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
The greys seem to be too small.

[how]
Separate the blocks into real block, greys and quads. They all have
different scales in Hack which we use to patch in.

If we do not patch in and just scale existing glyphs these three groups
should always be sufficient.

Note that in Hack the quad block 2597 is too small; we could have scaled
it together with the blocks group, but that would raise issues with well
behaved fonts that we just scale and not patch in.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
We have 2 different types of metrics warnings. When one warning has been
issued the other is not displayed (if it would trigger). The reason was
that I thought normally there would be no warnings and if someone would
have to inventigate the sourcefont anyhow.

[how]
But the warnings are quite common, so differentiate a bit more when
generating.
Also improve one warning message to make clear what the warning is
about.

And fix the assignment of advance width to width; which has no
consequence because it is never used (at the moment). But it was
obviously wrong.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
Normally the 'cell' size should not change on patching. If the 'cell'
size does not change we do not need to rescale the box glyphs. They
probably worked before, they work the same afterwards.

Another reason to disable this is Cascadia Code. It has box drawing
glyphs that extend for more up then the normal cell. If we rescale that
to fit a probably new cell size we get a 'midline' that is too low
(because the upper stems are longer).

[how]
Leave the code in, but disable 'just scale do not copy' mode.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
It does probably not make too much sense to add the box drawing glyphs
to proportional fonts. The boxes that are drawn need to be filled with
some text, and if that does not have monospaced property the box will
always look ugly and not fit and change when the font is changed.

[how]
Make the fact if we detect a source font as monospaced or not a property
of the patcher object.

Always determine that property (before we just determined it when the
target font should have monospaced behavior).

Use that new property to enable/disable the box drawing glyphs.

In a way it is now also prepared to add that as command line parameter
should the need for that arise.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii Finii force-pushed the feature/add-box-drawing branch from d32682e to 9834207 Compare March 10, 2023 11:32
@Finii
Copy link
Collaborator Author

Finii commented Mar 10, 2023

Rebase on master (because version changed there), force push.

I believe this is good to go now. It seems to render fine in all terminals (that I checked).
Still a bit hard to decide to replace the box glyphs if the source has SOME included. But alas...
This has other fixes in the code that we definitively want. Maybe we roll back a bit later 😬

@Finii Finii merged commit 4b66c81 into master Mar 10, 2023
@Finii Finii deleted the feature/add-box-drawing branch March 10, 2023 11:35
@igniscyan
Copy link

Rebase on master (because version changed there), force push.

I believe this is good to go now. It seems to render fine in all terminals (that I checked).

Still a bit hard to decide to replace the box glyphs if the source has SOME included. But alas...

This has other fixes in the code that we definitively want. Maybe we roll back a bit later 😬

This has been fun to follow! Thank you for all of the work on this, I love that these glyphs work out of the box now.

LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment