Repository navigation
Conversation
|
Actually, maybe a better approach is to only do this if the face's line gap is less than a pixel. Otherwise, there's no issue. |
632546d to
da3d0d9
Compare
|
Pushed an update such that the correction is only applied when actually needed to keep ascenders and descenders inside the cell. |
da3d0d9 to
96be954
Compare
#8563) > This PR will probably need a rebase on the final outcome of #8550 and ~#8552~ #8580, but I'm putting it out here so folks can begin taking a look if they want. 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-patcher` script 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. <img width="505" height="357" alt="Screenshot 2025-09-07 at 17 23 51" src="https://github.com/user-attachments/assets/8e3ff9fd-3b66-4d54-be38-d54cf3b6cc5b" /><img width="505" height="357" alt="Screenshot 2025-09-07 at 17 20 39" src="https://github.com/user-attachments/assets/84fbe076-2e3f-4879-b9b2-91ce86b9ef5f" /> Relevant specs: macOS; 1920x1080; Ghostty config: ```ini font-family = "CommitMono" font-size = "15" adjust-cell-height = "+20%" ``` **Points to note** * Icons are generally larger, making better use of the available space. * Icons are aligned nearly a pixel lower, better matching the text. This is because alignment is now calculated from face metrics/bearings, not the pixel-rounded cell. (See more below.) * Relative sizes are better matched. Note especially that tall and narrow icons, like the git branch symbol and icons depicting sheets of paper, look conspicuously small in the status quo. With this PR, they're better matched to other icons. * Look at the letter Z icon I use as prompt character for zsh. It's _tiny_ in the status quo, but properly sized with this PR. This demonstrates the most important and clear-cut improvement we make over `font-patcher`. (See more below.) * Icons wider than a single cell are now left-aligned rather than centered across two cells. I think this is preferable and makes better use of space in most relevant contexts. - Consider a Neovim bufferline showing the buffer title as a filetype icon followed by the file name. Padding on the left would be a waste of space, but having that extra space on the right can improve legibility. - In listings, such as in the screenshots, columns look tidier when their left edges are straight rather than ragged. - This is how `font-patcher` does 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_horizontal` and `size_vertical` are combined to a single `size`, which can be `.none, .stretch, .fit, .cover` or `.fit_cover1`. The latter implements the `pa` rule from `font-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 `.cover`s at least one cell and `.fit`s within the available space. See code comments and ryanoasis/nerd-fonts/pull/1926 for details. * An alignment mode `.center1` is added, implementing the centering rule from `font-patcher` that 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 `.center` rule around for use with emojis, et cetera. * Scaling and alignment calculations only use the unrounded face metrics and bearings. This ensures that pixel rounding of the cell and baseline, and `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 use `cell_width` to obtain the inter-cell padding needed to correctly center or right-align a glyph across two cells.) - We can do this with impunity because we're blessed with sprite glyphs in place of the "icons" that are actually box drawing and block graphics characters 馃檶 **Guide** The meat of the changes is 100 % in `src/font/face.zig` and `src/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 (see `src/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 the `constrain` function is rewritten and refactored. Scarred by countless hours perusing `font-patcher`, I tried hard to make the math and logic easy to follow here. I hope I have succeeded 馃
|
Other PRs were merged first, can you rebase this please. 馃槃 Thank you! |
96be954 to
5bd79ea
Compare
| .face_width = 16.0, | ||
| .face_height = 33.568, | ||
| .face_y = -0.08, | ||
| .face_y = 0.92, |
There was a problem hiding this comment.
Sorry @danielwe can you review this change from a rebase? This seems suspicious that this changed so wildly.
There was a problem hiding this comment.
Nope, that's correct---it's exactly the change this PR implements: adding an extra pixel row on the bottom if needed to ensure that cell_baseline >= -face.descent, or equivalently, face_y + half_line_gap >= 0. This font has half_line_gap == 0, so this simplifies to face_y >= 0. Since it was negative before, we should expect it to increase by 1 with this PR, which is what we see here.
That said, as explained in the OP, I'm kind of 50/50 on whether this is the correct thing to do. What it does is ensure that well-behaved glyphs stay within the terminal's grid cell, so they never draw on the same pixel row as well-behaved glyphs on the next line, and never protrude below selection/cursor highlighting. (By well-behaved here I mean that the outlines don't overshoot the font's ascent and descent metrics.) The price is that some fonts at some sizes end up with a cell height of @ceil(line_height) + 1 instead of just @ceil(line_height).
There was a problem hiding this comment.
And in case it was unclear earlier: The reason @ceil(line_height) + 1 is sometimes needed to avoid overshoot is that we round the baseline to a pixel boundary. If instead we simply centered the face vertically within the cell, @ceil(line_height) would always be sufficient.
|
Debating closing vs. rebasing this, but I'll add a screenshot showing the problem(?) it solves first. This PR would add another row of pixels at the bottom of the cell for this particular font, font size, and DPI, avoiding the protruding descenders. |
3 participants
The alignment of the baseline with a pixel boundary would, half of the time, make the face protrude slightly below the cell. This fixes that such that the face is always fully contained within the cell, at the cost of sometimes making the cell a pixel taller.
I'm actually not sure whether this or the existing behavior is preferable, but figured a small PR implementing the change is the easiest and most precise way of asking.
Pros of the existing implementation:
Cons of the existing implementation, addressed by this PR:
Curious to hear what people think.