Skip to content

fix(font): Final font patcher fixes - #8847

Merged
mitchellh merged 4 commits into
ghostty-org:mainfrom
danielwe:font_patcher_rule_fixes
Oct 3, 2025
Merged

fix(font): Final font patcher fixes#8847
mitchellh merged 4 commits into
ghostty-org:mainfrom
danielwe:font_patcher_rule_fixes

Conversation

@danielwe

@danielwe danielwe commented Sep 22, 2025

Copy link
Copy Markdown
Contributor

This is my final set of fixes to the font patcher/icon scaling code. It builds on #8563 and there's not much reason to pay attention here until that one has been reviewed (the unique changes in this PR only touch the two nerd_font_* files; the other 8 files in the diff are just #8563). However, I wanted to make sure the full set of changes/fixes I propose are out in the open, such that any substantial edits by maintainers (like in #7953) can take into account the full context.

I think this and the related patches should be considered fixes, not features, so I hope they can be considered for a 1.2.x release.

This PR fixes some bugs in the extraction of scale and alignment rules from the font_patcher script. Roughly in order of importance:

  • Nerd fonts apply an offset to some codepoint ranges when extracting glyphs from their original font (e.g., Font Awesome) and placing them in a Nerd Font. Rules are specified in terms of the former codepoints, but must be applied to the latter. This offset was previously not taken into account, so rules were applied to the wrong glyphs, and some glyphs that should have rules didn't get any.
  • Previously, the rules from every single patch set was included, but the embedded Symbols Only font doesn't contain all of them. Most importantly, there's a legacy patch set that only exists for historical reasons and is never used anymore, which was overwriting some other rules because of overlapping codepoint ranges. Also, the Symbols Only font contains no box drawing characters, so those rules should not be included. With this PR, irrelevant patch sets are filtered out.
  • Some patch sets specify overlapping codepoint ranges, though in reality the original fonts don't actually cover the full ranges and the overlaps just imply that they're filling each other's gaps. During font patching, the presence/absence of a glyph at each codepoint in the original font takes care of the ambiguity. Since we don't have that information, we need to hardcode which patch set "wins" for each case (it's not always the latest set in the list). Luckily, there are only two cases.
  • Many glyphs belong to scale groups that should be scaled and aligned as a unit. However, in font_patcher, the scale group is not used for horizontal alignment, unless the entire scale group has a single advance width (remember, the original symbol fonts are not monospace). This PR implements this rule by only setting relative_width and relative_x if the group is monospace.

There are some additional tweaks to ensure that each codepoint actually gets the rule it's supposed to when it belongs to multiple scale groups or patch sets, and to avoid setting rules for codepoints that don't exist in the embedded font.

@danielwe
danielwe requested review from a team as code owners September 22, 2025 22:40
@danielwe

danielwe commented Sep 23, 2025

Copy link
Copy Markdown
Contributor Author

Fixes #8842

Specifically, the second bullet point above is the ticket. The scaling rules for box drawing characters needs to be excluded, since these codepoints are not present in the symbols only embedded font. The rules end up applying to glyphs that don't need them and that they weren't designed for.

Also fixes #8335

@00-kat 00-kat added the font Issue within the font stack (typically src/font) label Sep 23, 2025
@danielwe danielwe changed the title Final font patcher fixes fix(font): Final font patcher fixes Sep 24, 2025
@mitchellh

Copy link
Copy Markdown
Contributor

@danielwe Since I merged your base branch, can you update to remove the commit and fix the conflicts? Thank you!

@danielwe
danielwe force-pushed the font_patcher_rule_fixes branch 2 times, most recently from 291ca2e to 979336f Compare October 1, 2025 16:47
@danielwe

danielwe commented Oct 1, 2025

Copy link
Copy Markdown
Contributor Author

EDIT: This part was moved to #8990.

I added one more small change: Stretched glyphs work best if they're scaled and aligned to the grid, since they're typically meant to align across cells (this is the exception to the argument in #8563 that icons should be aligned to the face). This also lets us eliminate the band-aid of horizontal negative padding (overlap), which is another improvement over font-patcher that's only possible because we're applying constraints with knowledge of the pixel grid.

Here are screenshots showing the difference this makes for the progress bar elements from Nerd Fonts. Notice the overlap artifacts (which would be even more pronounced on a lower-DPI display) no longer appearing after this change.

Screenshot 2025-10-01 at 09 34 36 Screenshot 2025-10-01 at 09 36 10

@danielwe

danielwe commented Oct 2, 2025

Copy link
Copy Markdown
Contributor Author

Now that we have #8990, the change mentioned in the previous comment is a better fit there, so I moved it over, see #8990 (comment).

I also realized the implementation of ungrouped horizontal alignment could be vastly simplified---all that's needed is to not set relative_width and relative_x, with no further changes needed to either the codegen or face.zig. As a result, this PR now only touches nerd_font_{codegen,attributes} and nothing else.

@danielwe
danielwe force-pushed the font_patcher_rule_fixes branch 2 times, most recently from efb6cf7 to 578735c Compare October 2, 2025 03:17
@mitchellh
mitchellh force-pushed the font_patcher_rule_fixes branch from 578735c to 78f1bf1 Compare October 3, 2025 20:50
@mitchellh
mitchellh merged commit f052eb4 into ghostty-org:main Oct 3, 2025
10 checks passed
@github-actions github-actions Bot added this to the 1.2.1 milestone Oct 3, 2025
@danielwe
danielwe deleted the font_patcher_rule_fixes branch October 4, 2025 05:35
mitchellh added a commit that referenced this pull request Oct 6, 2025
This is my final set of fixes to the font patcher/icon scaling code. It
builds on #8563 and there's not much reason to pay attention here until
that one has been reviewed (the unique changes in this PR only touch the
two `nerd_font_*` files; the other 8 files in the diff are just #8563).
However, I wanted to make sure the full set of changes/fixes I propose
are out in the open, such that any substantial edits by maintainers
(like in #7953) can take into account the full context.

I think this and the related patches should be considered fixes, not
features, so I hope they can be considered for a 1.2.x release.

This PR fixes some bugs in the extraction of scale and alignment rules
from the `font_patcher` script. Roughly in order of importance:

* Nerd fonts apply an offset to some codepoint ranges when extracting
glyphs from their original font (e.g., Font Awesome) and placing them in
a Nerd Font. Rules are specified in terms of the former codepoints, but
must be applied to the latter. This offset was previously not taken into
account, so rules were applied to the wrong glyphs, and some glyphs that
should have rules didn't get any.
* Previously, the rules from every single patch set was included, but
the embedded Symbols Only font doesn't contain all of them. Most
importantly, there's a legacy patch set that only exists for historical
reasons and is never used anymore, which was overwriting some other
rules because of overlapping codepoint ranges. Also, the Symbols Only
font contains no box drawing characters, so those rules should not be
included. With this PR, irrelevant patch sets are filtered out.
* Some patch sets specify overlapping codepoint ranges, though in
reality the original fonts don't actually cover the full ranges and the
overlaps just imply that they're filling each other's gaps. During font
patching, the presence/absence of a glyph at each codepoint in the
original font takes care of the ambiguity. Since we don't have that
information, we need to hardcode which patch set "wins" for each case
(it's not always the latest set in the list). Luckily, there are only
two cases.
* Many glyphs belong to scale groups that should be scaled and aligned
as a unit. However, in `font_patcher`, the scale group is _not_ used for
_horizontal_ alignment, _unless_ the entire scale group has a single
advance width (remember, the original symbol fonts are not monospace).
This PR implements this rule by only setting `relative_width` and
`relative_x` if the group is monospace.

There are some additional tweaks to ensure that each codepoint actually
gets the rule it's supposed to when it belongs to multiple scale groups
or patch sets, and to avoid setting rules for codepoints that don't
exist in the embedded font.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

font Issue within the font stack (typically src/font)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants