Fix horizontal glyph spacing on normal-DPI monitors (#9432)
This PR partially addresses #4504 with a one-liner, all feedback is very welcome. ### AI disclaimer I used Claude Code to help navigate the various layers of the rendering stack, to instrument a ton of intermediate now-deleted log statements, and ultimately to identify and fix this bug. I directed it conversationally, gave it experiments to run, audited all of its work, threw most of it out, and finally landed on this extremely small and simple change that fixes the issue _for me_ but I think for a lot of other cases as well. The fix ended up being small, so hopefully it's easy to review and discuss. Details: # Fix horizontal glyph spacing on non-Retina displays On external monitors (1.0x scale, 72 DPI), text had excessive horizontal spacing between glyphs. The issue was less noticeable on Retina displays (2.0x scale, 144 DPI) due to higher pixel density, but the proportional spacing error was identical. ## Before Background is ghostty 1.2.3 from homebrew, foregreound is iTerm2: <img width="862" height="938" alt="Screenshot 2025-10-31 at 3 04 47 PM" src="https://github.com/user-attachments/assets/feff5279-05cc-4008-b2f5-8ea3b4d6d14b" /> ## After Background is ghostty tip + this change, foreground is iTerm2: <img width="659" height="774" alt="Screenshot 2025-10-31 at 3 00 42 PM" src="https://github.com/user-attachments/assets/702dc7f8-bb46-43ec-8156-f69d003e8a37" /> (my iTerm2 has some custom thickening / brightening you can see; that's not a part of this change) ## Root Cause The metrics calculation in Metrics.zig used `@ceil()` to round the cell width from CoreText's glyph measurements, which created a mismatch between cell width (which determines glyph positions) and the actual glyph advances. At 72 DPI with a 12pt font: - CoreText returns glyph advance: 7.224609375 pixels - Cell width was ceiled to: 8 pixels - Gap per character: 0.78 pixels (~10.8% error) At 144 DPI with the same font: - CoreText returns glyph advance: 14.44921875 pixels (exactly 2x) - Cell width was ceiled to: 15 pixels - Gap per character: 0.55 pixels (~3.8% error) The error at high DPI is much better than at low DPI, since the absolute error is always no more than 1px. ## Fix Changed `@ceil(face_width)` to `@round(face_width)`. This makes cell width match the glyph advances better, reducing the error to at most 0.5px: - 72 DPI: round(7.22) = 7 - 144 DPI: round(14.45) = 14 Height continues using `@ceil()` since vertical space can be slightly larger without visual issues, but... should it? I'm not sure; this is a good topic for discussion.pull/9644/head
commit
3b80f72dac
|
|
@ -223,12 +223,34 @@ pub const FaceMetrics = struct {
|
|||
///
|
||||
/// For any nullable options that are not provided, estimates will be used.
|
||||
pub fn calc(face: FaceMetrics) Metrics {
|
||||
// We use the ceiling of the provided cell width and height to ensure
|
||||
// that the cell is large enough for the provided size, since we cast
|
||||
// it to an integer later.
|
||||
// These are the unrounded advance width and line height values,
|
||||
// which are retained separately from the rounded cell width and
|
||||
// height values (below), for calculations that need to know how
|
||||
// much error there is between the design dimensions of the font
|
||||
// and the pixel dimensions of our cells.
|
||||
const face_width = face.cell_width;
|
||||
const face_height = face.lineHeight();
|
||||
const cell_width = @ceil(face_width);
|
||||
|
||||
// The cell width and height values need to be integers since they
|
||||
// represent pixel dimensions of the grid cells in the terminal.
|
||||
//
|
||||
// We use @round for the cell width to limit the difference from
|
||||
// the "true" width value to no more than 0.5px. This is a better
|
||||
// approximation of the authorial intent of the font than ceiling
|
||||
// would be, and makes the apparent spacing match better between
|
||||
// low and high DPI displays.
|
||||
//
|
||||
// This does mean that it's possible for a glyph to overflow the
|
||||
// edge of the cell by a pixel if it has no side bearings, but in
|
||||
// reality such glyphs are generally meant to connect to adjacent
|
||||
// glyphs in some way so it's not really an issue.
|
||||
//
|
||||
// TODO: Reconsider cell height, should it also be rounded?
|
||||
// We use @ceil because that's what we used initially,
|
||||
// with the idea that it makes sure there's enough room
|
||||
// for glyphs that use the entire line height, but it
|
||||
// does create the same high/low DPI disparity issue...
|
||||
const cell_width = @round(face_width);
|
||||
const cell_height = @ceil(face_height);
|
||||
|
||||
// We split our line gap in two parts, and put half of it on the top
|
||||
|
|
|
|||
Loading…
Reference in New Issue