-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix(font): Rewrite constraint code for improved icon scaling/alignment #8563
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
Conversation
d1f52fd to
62b36d4
Compare
62b36d4 to
c686c00
Compare
|
Realized due to discord thread that this PR's revised horizontal alignment has an additional benefit: If people specify a EDIT: Encountered another concern about icon alignment in the wild, https://www.reddit.com/r/Ghostty/comments/1nnuyci/icons_not_centered_properly/, which would be addressed by this PR.
Footnotes
|
mitchellh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this is really good work. Sorry for the delay. I went through all the changes and I don't see any issues with this on its face.
The only minor issue is we're again comparing floats directly, but hopefully this doesn't cause issues in the future. If it does, its easy to know what's going on (and the values should be CLOSE so we can fix it then).
c686c00 to
e3ebdc7
Compare
Seems like there needs to be a general, easy-to-use solution for approximate equality testing of containers holding floats (see, e.g., #8563 (review)). How's this?
…ent (#8990) Follow-up to #8563, which broke scaling without alignment. This change recovers the behavior from before #8563, such that a scaled group is clamped to the constraint width and height if necessary, and otherwise, scaling does not shift the center of the group bounding box. As a part of this change, horizontal alignment was rewritten to assume the face is flush with the left edge of the cell. The cell-to-face offset in the rendering code is then applied regardless of the value of `align_horizontal`. This both simplifies the code and improves consistency, as it ensures that the offset is the same for all non-bitmap glyphs (rounded in FreeType, not rounded in CoreText). It's the right thing to do following the align-to-face changes in #8563.
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.
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.
…ent (#8990) Follow-up to #8563, which broke scaling without alignment. This change recovers the behavior from before #8563, such that a scaled group is clamped to the constraint width and height if necessary, and otherwise, scaling does not shift the center of the group bounding box. As a part of this change, horizontal alignment was rewritten to assume the face is flush with the left edge of the cell. The cell-to-face offset in the rendering code is then applied regardless of the value of `align_horizontal`. This both simplifies the code and improves consistency, as it ensures that the offset is the same for all non-bitmap glyphs (rounded in FreeType, not rounded in CoreText). It's the right thing to do following the align-to-face changes in #8563.
Seems like there needs to be a general, easy-to-use solution for approximate equality testing of containers holding floats (see, e.g., #8563 (review)). How's this?


This is a rewrite of the code that applies scaling and alignment constraints. The main intention is to further improve how Nerd Font icons are matched to the primary font. This PR aligns the calculations more closely with how the Nerd Font
font-patcherscript works, except in two cases where we can easily do something unambiguously better (one because of what's arguably a bug in the script, and one because we do multi-cell alignment with knowledge of the pixel-rounded cell grid).A goal of the rewrite is to make the scaling and alignment calculations as clear and easy to follow as possible.
I'll lead with some screenshots. First the status quo, then this PR.


Relevant specs: macOS; 1920x1080; Ghostty config:
Points to note
font-patcher. (See more below.)font-patcherdoes alignment, and thus what Nerd Font users and UI designers expect.Implementation details
I won't get too deep in the weeds here; see the code and comments. In brief:
size_horizontalandsize_verticalare combined to a singlesize, which can be.none, .stretch, .fit, .coveror.fit_cover1. The latter implements theparule fromfont-patcher, except it works better for icons that are small before scaling, like the letter Z prompt in the screenshots. In short, it preserves aspect ratio while clamping the size such that the icon.covers at least one cell and.fits within the available space. See code comments and Scale non-mono icons to be at least as large as mono ryanoasis/nerd-fonts#1926 for details..center1is added, implementing the centering rule fromfont-patcherthat I explained/defended above. In short, we center the icon in the first cell, even it's allowed to span multiple cells. For icons wider than a single cell, the lower bound that prevents them from protruding to the left kicks in and turns this into left-alignment. We keep the regular.centerrule around for use with emojis, et cetera.adjust-cell-{width,height}, don't affect scaling or relative alignment; the icons are always scaled and aligned to the face. (The one place we need to use cell metrics in the calculations is when we usecell_widthto obtain the inter-cell padding needed to correctly center or right-align a glyph across two cells.)Guide
The meat of the changes is 100 % in
src/font/face.zigandsrc/font/nerd_font_codegen.py. Changes to other files only amount to a) adding/changing some struct fields to get numbers to where they need to be (seesrc/font/Metrics.zig), and b) collateral updates to make otherwise unchanged code and tests work with/take advantage of the modified structs.Most files should have a clear and friendly diff. The exception is the bottom half of
src/font/face.zig, where the diff is meaningless and the new code should just be reviewed on its own merits. This is the part where theconstrainfunction is rewritten and refactored. Scarred by countless hours perusingfont-patcher, I tried hard to make the math and logic easy to follow here. I hope I have succeeded 🤞