Skip to content

fix(font): Ensure face is fully contained within cell #8550

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.

Sign up for GitHub

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

Closed
danielwe wants to merge 1 commit into from
Closed

fix(font): Ensure face is fully contained within cell #8550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 17 additions & 14 deletions src/font/Collection.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1203,55 +1203,58 @@ test "metrics" {

try comparison.expectApproxEqual(font.Metrics{
.cell_width = 8,
// The cell height is 17 px because the calculation is
// The cell height is 18 px because the calculation is
//
// ascender - descender + gap
//
// which, for inconsolata is
//
// 859 - -190 + 0
// 859 - (-190) + 0
//
// font units, at 1000 units per em that works out to 1.049 em,
// and 1em should be the point size * dpi scale, so 12 * (96/72)
// which is 16, and 16 * 1.049 = 16.784, which finally is rounded
// to 17.
// which is 16, and 16 * 1.049 = 16.784.
//
// The icon height is (2 * cap_height + face_height) / 3
// = (2 * 623 + 1049) / 3 = 765, and 16 * 0.765 = 12.24.
.cell_height = 17,
.cell_baseline = 3,
// The unrounded baseline is at
// -descender + (gap / 2) = -(-190) + 0 = 0.19 em. In pixels,
// that's 16 * 0.19 = 3.04, which rounds up to 4, so the
// bottom padding to align the baseline with a pixel boundary
// is 4 - 3.04 = 0.96. Hence, the top of the face is at
// 16.784 + 0.96 = 17.744, which finally is rounded up to 18.
.cell_height = 18,
.cell_baseline = 4,
.underline_position = 17,
.underline_thickness = 1,
.strikethrough_position = 10,
.strikethrough_thickness = 1,
.overline_position = 0,
.overline_thickness = 1,
.box_thickness = 1,
.cursor_height = 17,
.cursor_height = 18,
.icon_height = 12.24,
.face_width = 8.0,
.face_height = 16.784,
.face_y = -0.04,
.face_y = 0.96,
}, c.metrics);

// Resize should change metrics
try c.setSize(.{ .points = 24, .xdpi = 96, .ydpi = 96 });
try comparison.expectApproxEqual(font.Metrics{
.cell_width = 16,
.cell_height = 34,
.cell_baseline = 6,
.cell_height = 35,
.cell_baseline = 7,
.underline_position = 34,
.underline_thickness = 2,
.strikethrough_position = 19,
.strikethrough_thickness = 2,
.overline_position = 0,
.overline_thickness = 2,
.box_thickness = 2,
.cursor_height = 34,
.cursor_height = 35,
.icon_height = 24.48,
.face_width = 16.0,
.face_height = 33.568,
.face_y = -0.08,
.face_y = 0.92,

@mitchellh mitchellh Oct 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @danielwe can you review this change from a rebase? This seems suspicious that this changed so wildly.

@danielwe danielwe Oct 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@danielwe danielwe Oct 3, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}, c.metrics);
}

Expand Down
20 changes: 17 additions & 3 deletions src/font/Metrics.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ pub fn calc(face: FaceMetrics) Metrics {
const face_width = face.cell_width;
const face_height = face.lineHeight();
const cell_width = @ceil(face_width);
const cell_height = @ceil(face_height);
var cell_height = @ceil(face_height);

// We split our line gap in two parts, and put half of it on the top
// of the cell and the other half on the bottom, so that our text never
Expand All @@ -235,13 +235,27 @@ pub fn calc(face: FaceMetrics) Metrics {
// Unlike all our other metrics, `cell_baseline` is relative to the
// BOTTOM of the cell.
const face_baseline = half_line_gap - face.descent;
const cell_baseline = @round(face_baseline);
var cell_baseline = @round(face_baseline);

// For line gaps of less than a pixel, the @round may give
// cell_baseline < -face.descent, meaning descenders would protrude
// below the cell. We need to correct for this.
if (cell_baseline < -face.descent) {
cell_baseline += 1;
cell_height += 1;
}

// We keep track of the vertical bearing of the face in the cell
const face_y = cell_baseline - face_baseline;

// We calculate a top_to_baseline to make following calculations simpler.
const top_to_baseline = cell_height - cell_baseline;
var top_to_baseline = cell_height - cell_baseline;

// Same correction may by needed for ascenders.
if (top_to_baseline < face.ascent) {
top_to_baseline += 1;
cell_height += 1;
}

// Get the other font metrics or their estimates. See doc comments
// in FaceMetrics for explanations of the estimation heuristics.
Expand Down