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

font-patcher: Add progress indicators #1733

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Finii
Copy link
Collaborator

@Finii Finii commented Nov 14, 2024

Fixes: #1345

Description

Please explain the changes you made here.

Requirements / Checklist

  • Read the Contributing Guidelines
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan.
    Issue number where discussion took place: #xxx
  • If this contains a font/glyph add its origin as background info below (e.g. URL)
  • Verified the license of any newly added font, glyph, or glyph set. License is: xxx

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)?

Screenshots (if appropriate or helpful)

@Finii Finii added this to the v3.3.0 milestone Nov 14, 2024
@Finii
Copy link
Collaborator Author

Finii commented Nov 14, 2024

Something is wrong with the width when --variable and the glyph is align = c and has overlap 🤔

Compare handling of EE01 and 2630

@Finii
Copy link
Collaborator Author

Finii commented Nov 14, 2024

image

  • 2630
    • 1080
    • 0 ; 0
  • E0B0
    • 1200
    • -72 ; 0
  • EE00
    • 1178
    • 113 ; 59
  • EE01
    • 1297
    • -60 ; -37 (@ 1260)

💡 fixed - The symbol font may not have negative bearings

[note]
Glyphs taken from FiraCode; remove negative bearings and add a helper
glyphs for vertical alignment that is used for the combined boundingbox
calculation but not patched in.

Fixes: #1345

Signed-off-by: Fini Jastrow <[email protected]>
@Finii Finii force-pushed the feature/progress-indicators branch from d89c9a4 to 054dbc0 Compare November 14, 2024 11:40
@Finii
Copy link
Collaborator Author

Finii commented Nov 14, 2024

😬

image

@Finii
Copy link
Collaborator Author

Finii commented Nov 14, 2024

There is obviously a problem with center aligned glyphs... But that is already there before this MR :-(

Two notabke commits:

acd49cc font-patcher: Fix overlap for align c and r
5b36e8e font-patcher: Keep overlap in proportional font

    font-patcher: Fix overlap for align c and r
    
    [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.
    font-patcher: Keep overlap in proportional font
    
    [why]
    The previous commit is somewhat incomplete in some cases and plain wrong
    in others (with proportional fonts). Examples:
    
    The Heard 0x2665 gets a positive right side bearing, which is
    unexpected. The commits prevents negative right side bearings but not
    positive ones.
    
    The glyphs with overlap (which are the Powerline ones) like 0xE0B0 and
    0xE0B2 end up in wrong sizes.
    
    This can especially be seen with the Symbols-Only (non-Mono) font,
    because that is (secretly) a 'Nerd Font Propo' (--variable-width-glyphs)
    font.
    
    [how]
    This is kind of a design choice: As with the other patched font variants
    we ignore existing borders (positive bearings) around the glyph. The
    previous commit tried to keep them, which seems to be impossible and
    is inconsistent). Also negative bearings would be ignored (but there are
    none).
    
    The only place where bearings come into play is now when we have
    overlap. All non-overlap glyphs render without any bearing.
    
    If we have overlap we need to
    a) reduce the width by the overlap
    b) introduce a negative bearing on the appropriate side
    
    First we remove any left side bearing by transforming the glyph to the
    side, such that the bearing becomes zero.
    
    For left-side overlap we additionally transform the glyph by the overlap
    amount to the left (as usual). This creates the neg. left bearing.
    
    For right-side overlap we keep the left bearing to be zero.
    
    After correcting the left-side bearing (by transforming) we set the
    corrected width. That is the width subtracted by the overlap.
    In the left-aligned case this makes the right-side bearing zero.
    In the right-aligned case this results in a negative right-side bearing.
    
    Note how fontforge handles size and bearing changes:
      Fontforge handles the width change like this:
      - Keep existing left_side_bearing
      - Set width
      - Calculate and set new right_side_bearing

Glyphs to check

  • 2548
  • 2630
  • 2665
  • E0B0
  • E0B2
  • E0B6
  • E0C5
  • E0C7
  • E0D4

master branch

image
NFP
image
NF
image
NFM

Commit 81d052b

image
NFP
image
NF
image
NFM

Commit ?

image
NFP
image
NF

Another

image
NFP
image
NF
image
NFM

@Finii Finii force-pushed the feature/progress-indicators branch from 054dbc0 to 943980a Compare November 14, 2024 14:29
@Finii
Copy link
Collaborator Author

Finii commented Nov 14, 2024

image

Not centered in SymbolsOnlyNF

Also here in Propo

image

@Finii
Copy link
Collaborator Author

Finii commented Nov 14, 2024

DEBUG: Advance of 0xF0A6 in BBDIMS set 0xF0A4 - 0xF0A7 is 1417, expecting 1890
DEBUG: Advance of 0xF155 in BBDIMS set 0xF153 - 0xF15A is 1175, expecting 1181
DEBUG: Advance of 0xF177 in BBDIMS set 0xF175 - 0xF178 is 1890, expecting 1417
DEBUG: Advance of 0xF05C in BBDIMS set 0xF059 - 0xF0B1 is 1537, expecting 1533
FONTA_SCALE_LIST         range(0xf0a4, 0xf0a7 + 1), # pointing hands
FONTA_SCALE_LIST         range(0xf153, 0xf15a + 1), # currencies   
FONTA_SCALE_LIST         range(0xf175, 0xf178 + 1), # long arrows   
WEATH_SCALE_LIST         [*range(0xf059, 0xf061 + 1), 0xf0b1], # wind directions 

@Finii Finii force-pushed the feature/progress-indicators branch 2 times, most recently from ac13251 to 6a07537 Compare November 15, 2024 11:47
[why]
It can be confusing what is or is not synced when scaling-shifting.

[how]
Add text from commit:
  90bde73  font-patcher: Use ScaleGlyph BB to align glyph

Also add a new (documentary) property for the tables that can be checked
so that nothing unexpected will happen.

Signed-off-by: Fini Jastrow <[email protected]>
[why]
All the glyphs have overlap (negative bearings). That in itself is ok,
altough it (the glyph width for example) is not consistent for all
subsets.

The overlaps can interact strangely with the overlap given by the
font-patcher script.

[how]
It seems better to remove the negative bearings.
Also some glyph width are corrected (unified).

The font is then stored uncompressed now.

[note]
The script used to modify the font:

import fontforge, psMat

font = fontforge.open('src/glyphs/extraglyphs_orig.sfd')
font.encoding = 'UnicodeFull'

def adjust(f, rang, left, width):
    for c in rang:
        g = f[c]
        if c in range(0x2599, 0x259f + 1):
            plus = 0
            g.transform(psMat.scale(1233/1234, 1))
        if c == 0x259D or c == 0x2595:
            g.width = width
            g.right_side_bearing = 0
        else:
            g.left_side_bearing = int(g.left_side_bearing + left)
            g.width = width
        print('{:X} {} {} {} {}'.format(c, g.boundingBox(), g.left_side_bearing, g.width, g.right_side_bearing))

adjust(font,
    [*range(0x2500, 0x2570 + 1), *range(0x2574, 0x257f + 1)],
    20, 1233 + 40)
adjust(font,
    range(0x2571, 0x2573 + 1),
    87, 1280+127)
adjust(font,
    range(0x2580, 0x259f + 1),
    0, 1233)

font.generate('TEST.sfd')
font.generate('TEST.ttf')

Signed-off-by: Fini Jastrow <[email protected]>
[why]
The shifting in the final patched font is broken for the 'blocks'
subset. The reason is that it contains a glyph that is one unit too
small.

[how]
Manually shift the points to make it as wide as the others.

Signed-off-by: Fini Jastrow <[email protected]>
Signed-off-by: Fini Jastrow <[email protected]>
@Finii Finii force-pushed the feature/progress-indicators branch from 6a07537 to c589ee2 Compare November 15, 2024 15:35
@Finii
Copy link
Collaborator Author

Finii commented Nov 15, 2024

So lets crate a list of fonts that need to be tested

  • CascadiaCode NF NFM NFP
  • SymbolsOnly NF(P) NFM
  • BigBlueTermPlus NF NFM NFP

Preparing fonts prior to this PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Fira Code's progress bars and spinner.
1 participant