font: round cell height from line height instead of ceiling (#9648)

This change should give more consistent results between high and low DPI
displays, and generally approximate the authorial intent of the metrics
a little better.

Also changed the cell height adjustment to prioritize the top or bottom
when adjusting by an odd number depending on whether the face is higher
or lower in the cell than it "should" be. This should make it easier for
users who have an issue with a glyph protruding from the cell to adjust
the height and resolve it.

src/font/Metrics.zig

@@ -1,6 +1,7 @@
 const Metrics = @This();
 
 const std = @import("std");
+const assert = @import("../quirks.zig").inlineAssert;
 
 /// Recommended cell width and height for a monospace grid using this font.
 cell_width: u32,
@@ -47,8 +48,9 @@ face_width: f64,
 /// The unrounded face height, used in scaling calculations.
 face_height: f64,
 
-/// The vertical bearing of face within the pixel-rounded
+/// The offset from the bottom of the cell to the bottom
-/// and possibly height-adjusted cell
+/// of the face's bounding box, based on the rounded and
+/// potentially adjusted cell height.
 face_y: f64,
 
 /// Minimum acceptable values for some fields to prevent modifiers
@@ -245,25 +247,46 @@ pub fn calc(face: FaceMetrics) Metrics {
     // 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?
+    // The same is true for the height. Some fonts are poorly authored
-    //       We use @ceil because that's what we used initially,
+    // and have a descender on a normal glyph that extends right up to
-    //       with the idea that it makes sure there's enough room
+    // the descent value of the face, and this can result in the glyph
-    //       for glyphs that use the entire line height, but it
+    // overflowing the bottom of the cell by a pixel, which isn't good
-    //       does create the same high/low DPI disparity issue...
+    // but if we try to prevent it by increasing the cell height then
+    // we get line heights that are too large for most users and even
+    // more inconsistent across DPIs.
+    //
+    // Users who experience such cell-height overflows should:
+    //
+    // 1. Nag the font author to either redesign the glyph to not go
+    //    so low, or else adjust the descent value in the metadata.
+    //
+    // 2. Add an `adjust-cell-height` entry to their config to give
+    //    the cell enough room for the glyph.
     const cell_width = @round(face_width);
-    const cell_height = @ceil(face_height);
+    const cell_height = @round(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
     // bumps up against either edge of the cell vertically.
     const half_line_gap = face.line_gap / 2;
 
-    // Unlike all our other metrics, `cell_baseline` is relative to the
+    // NOTE: Unlike all our other metrics, `cell_baseline` is
-    // BOTTOM of the cell.
+    // relative to the BOTTOM of the cell rather than the top.
     const face_baseline = half_line_gap - face.descent;
-    const cell_baseline = @round(face_baseline);
+    // We calculate the baseline by trying to center the face vertically
+    // in the pixel-rounded cell height, so that before rounding it will
+    // be an even distance from the top and bottom of the cell, meaning
+    // it either sticks out the same amount or is inset the same amount,
+    // depending on whether the cell height was rounded up or down from
+    // the line height. We do this by adding half the difference between
+    // the cell height and the face height.
+    const cell_baseline = @round(face_baseline - (cell_height - face_height) / 2);
 
-    // We keep track of the vertical bearing of the face in the cell
+    // We keep track of the offset from the bottom of the cell
+    // to the bottom of the face's "true" bounding box, which at
+    // this point, since nothing has been scaled yet, is equivalent
+    // to the offset between the baseline we draw at (cell_baseline)
+    // and the one the font wants (face_baseline).
     const face_y = cell_baseline - face_baseline;
 
     // We calculate a top_to_baseline to make following calculations simpler.
@@ -333,29 +356,48 @@ pub fn apply(self: *Metrics, mods: ModifierSet) void {
                 // here is to center the baseline so that text is vertically
                 // centered in the cell.
                 if (comptime tag == .cell_height) {
-                    // We split the difference in half because we want to
+                    const original_f64: f64 = @floatFromInt(original);
-                    // center the baseline in the cell. If the difference
+                    const new_f64: f64 = @floatFromInt(new);
-                    // is odd, one more pixel is added/removed on top than
+                    const diff = new_f64 - original_f64;
-                    // on the bottom.
+                    const half_diff = diff / 2.0;
-                    if (new > original) {
+
-                        const diff = new - original;
+                    // If the diff is even, the number of pixels we add
-                        const diff_bottom = diff / 2;
+                    // will be the same for the top and the bottom, but
-                        const diff_top = diff - diff_bottom;
+                    // if the diff is odd then we want to add the extra
-                        self.face_y += @floatFromInt(diff_bottom);
+                    // pixel to the edge of the cell that needs it most.
-                        self.cell_baseline +|= diff_bottom;
+                    //
-                        self.underline_position +|= diff_top;
+                    // How much the edge "needs it" depends on whether
-                        self.strikethrough_position +|= diff_top;
+                    // the face is higher or lower than it should be to
-                        self.overline_position +|= @as(i32, @intCast(diff_top));
+                    // be perfectly centered in the cell.
-                    } else {
+                    //
-                        const diff = original - new;
+                    // If the face were perfectly centered then face_y
-                        const diff_bottom = diff / 2;
+                    // would be equal to half of the difference between
-                        const diff_top = diff - diff_bottom;
+                    // the cell height and the face height.
-                        self.face_y -= @floatFromInt(diff_bottom);
+                    const position_with_respect_to_center =
-                        self.cell_baseline -|= diff_bottom;
+                        self.face_y - (original_f64 - self.face_height) / 2;
-                        self.underline_position -|= diff_top;
+
-                        self.strikethrough_position -|= diff_top;
+                    const diff_top, const diff_bottom =
-                        self.overline_position -|= @as(i32, @intCast(diff_top));
+                        if (position_with_respect_to_center > 0)
-                    }
+                            // The baseline is higher than it should be, so we
+                            // add the extra to the top, or if it's a negative
+                            // diff it gets added to the bottom because of how
+                            // floor and ceil work.
+                            .{ @ceil(half_diff), @floor(half_diff) }
+                        else
+                            // The baseline is lower than it should be, so we
+                            // add the extra to the bottom, or vice versa for
+                            // negative diffs.
+                            .{ @floor(half_diff), @ceil(half_diff) };
+
+                    // The cell baseline and face_y values are relative to the
+                    // bottom of the cell so we add the bottom diff to them.
+                    addFloatToInt(&self.cell_baseline, diff_bottom);
+                    self.face_y += diff_bottom;
+
+                    // These are all relative to the top of the cell.
+                    addFloatToInt(&self.underline_position, diff_top);
+                    addFloatToInt(&self.strikethrough_position, diff_top);
+                    self.overline_position +|= @as(i32, @intFromFloat(diff_top));
                 }
             },
             inline .icon_height => {
@@ -373,6 +415,21 @@ pub fn apply(self: *Metrics, mods: ModifierSet) void {
     self.clamp();
 }
 
+/// Helper function for adding an f64 to a u32.
+///
+/// Performs saturating addition or subtraction
+/// depending on the sign of the provided float.
+///
+/// The f64 is asserted to have an integer value.
+inline fn addFloatToInt(int: *u32, float: f64) void {
+    assert(@floor(float) == float);
+    int.* =
+        if (float >= 0.0)
+            int.* +| @as(u32, @intFromFloat(float))
+        else
+            int.* -| @as(u32, @intFromFloat(-float));
+}
+
 /// Clamp all metrics to their allowable range.
 fn clamp(self: *Metrics) void {
     inline for (std.meta.fields(Metrics)) |field| {
@@ -592,7 +649,9 @@ test "Metrics: adjust cell height smaller" {
     defer set.deinit(alloc);
     // We choose numbers such that the subtracted number of pixels is odd,
     // as that's the case that could most easily have off-by-one errors.
-    // Here we're removing 25 pixels: 12 on the bottom, 13 on top.
+    // Here we're removing 25 pixels: 13 on the bottom, 12 on top, split
+    // that way because we're simulating a face that's 0.33px higher than
+    // it "should" be (due to rounding).
     try set.put(alloc, .cell_height, .{ .percent = 0.75 });
 
     var m: Metrics = init();
@@ -602,14 +661,15 @@ test "Metrics: adjust cell height smaller" {
     m.strikethrough_position = 30;
     m.overline_position = 0;
     m.cell_height = 100;
+    m.face_height = 99.67;
     m.cursor_height = 100;
     m.apply(set);
-    try testing.expectEqual(-11.67, m.face_y);
+    try testing.expectEqual(-12.67, m.face_y);
     try testing.expectEqual(@as(u32, 75), m.cell_height);
-    try testing.expectEqual(@as(u32, 38), m.cell_baseline);
+    try testing.expectEqual(@as(u32, 37), m.cell_baseline);
-    try testing.expectEqual(@as(u32, 42), m.underline_position);
+    try testing.expectEqual(@as(u32, 43), m.underline_position);
-    try testing.expectEqual(@as(u32, 17), m.strikethrough_position);
+    try testing.expectEqual(@as(u32, 18), m.strikethrough_position);
-    try testing.expectEqual(@as(i32, -13), m.overline_position);
+    try testing.expectEqual(@as(i32, -12), m.overline_position);
     // Cursor height is separate from cell height and does not follow it.
     try testing.expectEqual(@as(u32, 100), m.cursor_height);
 }
@@ -622,7 +682,9 @@ test "Metrics: adjust cell height larger" {
     defer set.deinit(alloc);
     // We choose numbers such that the added number of pixels is odd,
     // as that's the case that could most easily have off-by-one errors.
-    // Here we're adding 75 pixels: 37 on the bottom, 38 on top.
+    // Here we're adding 75 pixels: 37 on the bottom, 38 on top, split
+    // that way because we're simulating a face that's 0.33px higher
+    // than it "should" be (due to rounding).
     try set.put(alloc, .cell_height, .{ .percent = 1.75 });
 
     var m: Metrics = init();
@@ -632,6 +694,7 @@ test "Metrics: adjust cell height larger" {
     m.strikethrough_position = 30;
     m.overline_position = 0;
     m.cell_height = 100;
+    m.face_height = 99.67;
     m.cursor_height = 100;
     m.apply(set);
     try testing.expectEqual(37.33, m.face_y);