From 0faf6097d069e097680fe4e5a74181d120431297 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 3 Jul 2023 11:08:12 -0700 Subject: [PATCH] Change font metrics to all be integers, not floats. Font metrics realistically should be integral. Cell widths, cell heights, etc. do not make sense to be floats, since our grid is integral. There is no such thing as a "half cell" (or any point). The reason we historically had these all as f32 is simplicity mixed with history. OpenGL APIs and shaders all use f32 for their values, we originally only supported OpenGL, and all the font rendering used to be directly in the renderer code (like... a year+ ago). When we refactored the font metrics calculation to its own system and also added additional renderers like Metal (which use f64, not f32), we never updated anything. We just kept metrics as f32 and casted everywhere. With CoreText and #177 this finally reared its ugly head. By forgetting a simple rounding on cell metric calculation, our integral renderers (sprite fonts) were off by 1 pixel compared to the GPU renderers. Insidious. Let's represent font metrics with the types that actually make sense: a cell width/height, etc. is _integral_. When we get to the GPU, we now cast to floats. We also cast to floats whenever we're doing more precise math (i.e. mouse offset calculation). In this case, we're only converting to floats from a integral type which is going to be much safer and less prone to uncertain rounding than converting to an int from a float type. Fixes #177 --- src/Surface.zig | 29 ++++++++++++++----------- src/font/Group.zig | 6 +++--- src/font/face.zig | 14 ++++++------ src/font/face/coretext.zig | 38 +++++++++++++++------------------ src/font/face/freetype.zig | 33 ++++++++++++---------------- src/font/face/web_canvas.zig | 17 ++++++++------- src/renderer/Metal.zig | 32 ++++++++++++++++----------- src/renderer/OpenGL.zig | 23 ++++++++++++++------ src/renderer/opengl/Program.zig | 5 ++++- src/renderer/size.zig | 22 +++++++++++++------ 10 files changed, 121 insertions(+), 98 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index d309747f3..85e462c87 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -429,8 +429,8 @@ pub fn init( // Set a minimum size that is cols=10 h=4. This matches Mac's Terminal.app // but is otherwise somewhat arbitrary. try rt_surface.setSizeLimits(.{ - .width = @intFromFloat(cell_size.width * 10), - .height = @intFromFloat(cell_size.height * 4), + .width = cell_size.width * 10, + .height = cell_size.height * 4, }, null); // Call our size callback which handles all our retina setup @@ -662,10 +662,10 @@ pub fn imePoint(self: *const Surface) apprt.IMEPos { const x: f64 = x: { // Simple x * cell width gives the top-left corner - var x: f64 = @floatCast(@as(f32, @floatFromInt(cursor.x)) * self.cell_size.width); + var x: f64 = @floatFromInt(cursor.x * self.cell_size.width); // We want the midpoint - x += self.cell_size.width / 2; + x += @as(f64, @floatFromInt(self.cell_size.width)) / 2; // And scale it x /= content_scale.x; @@ -675,10 +675,10 @@ pub fn imePoint(self: *const Surface) apprt.IMEPos { const y: f64 = y: { // Simple x * cell width gives the top-left corner - var y: f64 = @floatCast(@as(f32, @floatFromInt(cursor.y)) * self.cell_size.height); + var y: f64 = @floatFromInt(cursor.y * self.cell_size.height); // We want the bottom - y += self.cell_size.height; + y += @floatFromInt(self.cell_size.height); // And scale it y /= content_scale.y; @@ -1332,7 +1332,7 @@ pub fn scrollCallback( // If the new offset is less than a single unit of scroll, we save // the new pending value and do not scroll yet. - const cell_size = self.cell_size.height; + const cell_size: f64 = @floatFromInt(self.cell_size.height); if (@fabs(poff) < cell_size) { self.mouse.pending_scroll_y = poff; break :y .{}; @@ -1359,7 +1359,7 @@ pub fn scrollCallback( } const poff = self.mouse.pending_scroll_x + (xoff * -1); - const cell_size = self.cell_size.width; + const cell_size: f64 = @floatFromInt(self.cell_size.width); if (@fabs(poff) < cell_size) { self.mouse.pending_scroll_x = poff; break :x .{}; @@ -1724,7 +1724,7 @@ pub fn mouseButtonCallback( // If we move our cursor too much between clicks then we reset // the multi-click state. if (self.mouse.left_click_count > 0) { - const max_distance = self.cell_size.width; + const max_distance: f64 = @floatFromInt(self.cell_size.width); const distance = @sqrt( std.math.pow(f64, pos.x - self.mouse.left_click_xpos, 2) + std.math.pow(f64, pos.y - self.mouse.left_click_ypos, 2), @@ -1951,10 +1951,13 @@ fn dragLeftClickSingle( // // the boundary point at which we consider selection or non-selection - const cell_xboundary = self.cell_size.width * 0.6; + const cell_xboundary = @as(f32, @floatFromInt(self.cell_size.width)) * 0.6; // first xpos of the clicked cell - const cell_xstart = @as(f32, @floatFromInt(self.mouse.left_click_point.x)) * self.cell_size.width; + const cell_xstart = @as( + f32, + @floatFromInt(self.mouse.left_click_point.x), + ) * @as(f32, @floatFromInt(self.cell_size.width)); const cell_start_xpos = self.mouse.left_click_xpos - cell_xstart; // If this is the same cell, then we only start the selection if weve @@ -2038,7 +2041,7 @@ fn posToViewport(self: Surface, xpos: f64, ypos: f64) terminal.point.Viewport { return .{ .x = if (xpos_adjusted < 0) 0 else x: { // Our cell is the mouse divided by cell width - const cell_width: f64 = @floatCast(self.cell_size.width); + const cell_width: f64 = @floatFromInt(self.cell_size.width); const x: usize = @intFromFloat(xpos_adjusted / cell_width); // Can be off the screen if the user drags it out, so max @@ -2047,7 +2050,7 @@ fn posToViewport(self: Surface, xpos: f64, ypos: f64) terminal.point.Viewport { }, .y = if (ypos_adjusted < 0) 0 else y: { - const cell_height: f64 = @floatCast(self.cell_size.height); + const cell_height: f64 = @floatFromInt(self.cell_size.height); const y: usize = @intFromFloat(ypos_adjusted / cell_height); break :y @min(y, self.grid_size.rows - 1); }, diff --git a/src/font/Group.zig b/src/font/Group.zig index 02e9ef4a6..11f4f94be 100644 --- a/src/font/Group.zig +++ b/src/font/Group.zig @@ -329,10 +329,10 @@ pub const Wasm = struct { // Set details for our sprite font self.sprite = font.sprite.Face{ - .width = @intFromFloat(metrics.cell_width), - .height = @intFromFloat(metrics.cell_height), + .width = metrics.cell_width, + .height = metrics.cell_height, .thickness = 2, - .underline_position = @intFromFloat(metrics.underline_position), + .underline_position = metrics.underline_position, }; } diff --git a/src/font/face.zig b/src/font/face.zig index 24f0f5368..458a49f62 100644 --- a/src/font/face.zig +++ b/src/font/face.zig @@ -39,23 +39,23 @@ pub const DesiredSize = struct { /// Metrics associated with the font that are useful for renderers to know. pub const Metrics = struct { /// Recommended cell width and height for a monospace grid using this font. - cell_width: f32, - cell_height: f32, + cell_width: u32, + cell_height: u32, /// For monospace grids, the recommended y-value from the bottom to set /// the baseline for font rendering. This is chosen so that things such /// as the bottom of a "g" or "y" do not drop below the cell. - cell_baseline: f32, + cell_baseline: u32, /// The position of the underline from the top of the cell and the /// thickness in pixels. - underline_position: f32, - underline_thickness: f32, + underline_position: u32, + underline_thickness: u32, /// The position and thickness of a strikethrough. Same units/style /// as the underline fields. - strikethrough_position: f32, - strikethrough_thickness: f32, + strikethrough_position: u32, + strikethrough_thickness: u32, }; /// Additional options for rendering glyphs. diff --git a/src/font/face/coretext.zig b/src/font/face/coretext.zig index 3ed30cc2a..339782c36 100644 --- a/src/font/face/coretext.zig +++ b/src/font/face/coretext.zig @@ -261,7 +261,7 @@ pub const Face = struct { const offset_y: i32 = offset_y: { // Our Y coordinate in 3D is (0, 0) bottom left, +y is UP. // We need to calculate our baseline from the bottom of a cell. - const baseline_from_bottom = self.metrics.cell_height - self.metrics.cell_baseline; + const baseline_from_bottom: f64 = @floatFromInt(self.metrics.cell_height - self.metrics.cell_baseline); // Next we offset our baseline by the bearing in the font. We // ADD here because CoreText y is UP. @@ -328,7 +328,7 @@ pub const Face = struct { max = @max(advances[i].width, max); } - break :cell_width @floatCast(max); + break :cell_width @floatCast(@ceil(max)); }; // Calculate the cell height by using CoreText's layout engine @@ -424,8 +424,8 @@ pub const Face = struct { }; // All of these metrics are based on our layout above. - const cell_height = layout_metrics.height; - const cell_baseline = layout_metrics.ascent; + const cell_height = @ceil(layout_metrics.height); + const cell_baseline = @ceil(layout_metrics.ascent); const underline_thickness = @ceil(@as(f32, @floatCast(ct_font.getUnderlineThickness()))); const strikethrough_position = cell_baseline * 0.6; const strikethrough_thickness = underline_thickness; @@ -441,24 +441,20 @@ pub const Face = struct { // const units_per_em = ct_font.getUnitsPerEm(); // const units_per_point = @intToFloat(f64, units_per_em) / ct_font.getSize(); - // std.log.warn("font size size={d}", .{ct_font.getSize()}); - // std.log.warn("font metrics width={d}, height={d} baseline={d} underline_pos={d} underline_thickness={d}", .{ - // cell_width, - // cell_height, - // cell_baseline, - // underline_position, - // underline_thickness, - // }); - - return font.face.Metrics{ - .cell_width = cell_width, - .cell_height = cell_height, - .cell_baseline = cell_baseline, - .underline_position = underline_position, - .underline_thickness = underline_thickness, - .strikethrough_position = strikethrough_position, - .strikethrough_thickness = strikethrough_thickness, + const result = font.face.Metrics{ + .cell_width = @intFromFloat(cell_width), + .cell_height = @intFromFloat(cell_height), + .cell_baseline = @intFromFloat(cell_baseline), + .underline_position = @intFromFloat(underline_position), + .underline_thickness = @intFromFloat(underline_thickness), + .strikethrough_position = @intFromFloat(strikethrough_position), + .strikethrough_thickness = @intFromFloat(strikethrough_thickness), }; + + // std.log.warn("font size size={d}", .{ct_font.getSize()}); + // std.log.warn("font metrics={}", .{result}); + + return result; } }; diff --git a/src/font/face/freetype.zig b/src/font/face/freetype.zig index a09a523ef..c5dc85d44 100644 --- a/src/font/face/freetype.zig +++ b/src/font/face/freetype.zig @@ -325,7 +325,7 @@ pub const Face = struct { // baseline calculation. The baseline calculation is so that everything // is properly centered when we render it out into a monospace grid. // Note: we add here because our X/Y is actually reversed, adding goes UP. - break :offset_y glyph_metrics.bitmap_top + @as(c_int, @intFromFloat(self.metrics.cell_baseline)); + break :offset_y glyph_metrics.bitmap_top + @as(c_int, @intCast(self.metrics.cell_baseline)); }; // log.warn("renderGlyph width={} height={} offset_x={} offset_y={} glyph_metrics={}", .{ @@ -475,25 +475,20 @@ pub const Face = struct { .thickness = underline_thickness, }; - // log.warn("METRICS={} width={d} height={d} baseline={d} underline_pos={d} underline_thickness={d} strikethrough={}", .{ - // size_metrics, - // cell_width, - // cell_height, - // cell_height - cell_baseline, - // underline_position, - // underline_thickness, - // strikethrough, - // }); - - return .{ - .cell_width = cell_width, - .cell_height = cell_height, - .cell_baseline = cell_baseline, - .underline_position = underline_position, - .underline_thickness = underline_thickness, - .strikethrough_position = strikethrough.pos, - .strikethrough_thickness = strikethrough.thickness, + const result = font.face.Metrics{ + .cell_width = @intFromFloat(cell_width), + .cell_height = @intFromFloat(cell_height), + .cell_baseline = @intFromFloat(cell_baseline), + .underline_position = @intFromFloat(underline_position), + .underline_thickness = @intFromFloat(underline_thickness), + .strikethrough_position = @intFromFloat(strikethrough.pos), + .strikethrough_thickness = @intFromFloat(strikethrough.thickness), }; + + // std.log.warn("font size size={d}", .{ct_font.getSize()}); + // std.log.warn("font metrics={}", .{result}); + + return result; } /// Convert freetype "font units" to pixels using the Y scale. diff --git a/src/font/face/web_canvas.zig b/src/font/face/web_canvas.zig index 458e018c1..84fa6a887 100644 --- a/src/font/face/web_canvas.zig +++ b/src/font/face/web_canvas.zig @@ -273,16 +273,17 @@ pub const Face = struct { const underline_position = cell_height - 1; const underline_thickness: f32 = 1; - self.metrics = .{ - .cell_width = cell_width, - .cell_height = cell_height, - .cell_baseline = cell_baseline, - .underline_position = underline_position, - .underline_thickness = underline_thickness, - .strikethrough_position = underline_position, - .strikethrough_thickness = underline_thickness, + const result = font.face.Metrics{ + .cell_width = @intFromFloat(cell_width), + .cell_height = @intFromFloat(cell_height), + .cell_baseline = @intFromFloat(cell_baseline), + .underline_position = @intFromFloat(underline_position), + .underline_thickness = @intFromFloat(underline_thickness), + .strikethrough_position = @intFromFloat(underline_position), + .strikethrough_thickness = @intFromFloat(underline_thickness), }; + self.metrics = result; log.debug("metrics font={s} value={}", .{ self.font_str, self.metrics }); } diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig index 943b9a6ea..7d75219bf 100644 --- a/src/renderer/Metal.zig +++ b/src/renderer/Metal.zig @@ -216,10 +216,10 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal { // Set the sprite font up options.font_group.group.sprite = font.sprite.Face{ - .width = @intFromFloat(metrics.cell_width), - .height = @intFromFloat(metrics.cell_height), + .width = metrics.cell_width, + .height = metrics.cell_height, .thickness = 2, - .underline_position = @intFromFloat(metrics.underline_position), + .underline_position = metrics.underline_position, }; // Create the font shaper. We initially create a shaper that can support @@ -301,8 +301,8 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal { .uniforms = .{ .projection_matrix = undefined, .cell_size = undefined, - .strikethrough_position = metrics.strikethrough_position, - .strikethrough_thickness = metrics.strikethrough_thickness, + .strikethrough_position = @floatFromInt(metrics.strikethrough_position), + .strikethrough_thickness = @floatFromInt(metrics.strikethrough_thickness), }, // Fonts @@ -460,9 +460,12 @@ pub fn setFontSize(self: *Metal, size: font.face.DesiredSize) !void { // Update our uniforms self.uniforms = .{ .projection_matrix = self.uniforms.projection_matrix, - .cell_size = .{ new_cell_size.width, new_cell_size.height }, - .strikethrough_position = metrics.strikethrough_position, - .strikethrough_thickness = metrics.strikethrough_thickness, + .cell_size = .{ + @floatFromInt(new_cell_size.width), + @floatFromInt(new_cell_size.height), + }, + .strikethrough_position = @floatFromInt(metrics.strikethrough_position), + .strikethrough_thickness = @floatFromInt(metrics.strikethrough_thickness), }; // Recalculate our cell size. If it is the same as before, then we do @@ -480,10 +483,10 @@ pub fn setFontSize(self: *Metal, size: font.face.DesiredSize) !void { // Set the sprite font up self.font_group.group.sprite = font.sprite.Face{ - .width = @intFromFloat(self.cell_size.width), - .height = @intFromFloat(self.cell_size.height), + .width = self.cell_size.width, + .height = self.cell_size.height, .thickness = 2, - .underline_position = @intFromFloat(metrics.underline_position), + .underline_position = metrics.underline_position, }; // Notify the window that the cell size changed. @@ -792,7 +795,10 @@ pub fn setScreenSize(self: *Metal, dim: renderer.ScreenSize) !void { @as(f32, @floatFromInt(padded_dim.height)) + padding.bottom, -1 * padding.top, ), - .cell_size = .{ self.cell_size.width, self.cell_size.height }, + .cell_size = .{ + @floatFromInt(self.cell_size.width), + @floatFromInt(self.cell_size.height), + }, .strikethrough_position = old.strikethrough_position, .strikethrough_thickness = old.strikethrough_thickness, }; @@ -1008,7 +1014,7 @@ pub fn updateCell( shaper_run.font_index, shaper_cell.glyph_index, .{ - .max_height = @intFromFloat(@ceil(self.cell_size.height)), + .max_height = @intCast(self.cell_size.height), .thicken = self.config.font_thicken, }, ); diff --git a/src/renderer/OpenGL.zig b/src/renderer/OpenGL.zig index e48c9d9bf..84c7ad76e 100644 --- a/src/renderer/OpenGL.zig +++ b/src/renderer/OpenGL.zig @@ -161,10 +161,19 @@ const SetFontSize = struct { fn apply(self: SetFontSize, r: *const OpenGL) !void { try r.program.setUniform( "cell_size", - @Vector(2, f32){ self.metrics.cell_width, self.metrics.cell_height }, + @Vector(2, f32){ + @floatFromInt(self.metrics.cell_width), + @floatFromInt(self.metrics.cell_height), + }, + ); + try r.program.setUniform( + "strikethrough_position", + @as(f32, @floatFromInt(self.metrics.strikethrough_position)), + ); + try r.program.setUniform( + "strikethrough_thickness", + @as(f32, @floatFromInt(self.metrics.strikethrough_thickness)), ); - try r.program.setUniform("strikethrough_position", self.metrics.strikethrough_position); - try r.program.setUniform("strikethrough_thickness", self.metrics.strikethrough_thickness); } }; @@ -661,10 +670,10 @@ fn resetFontMetrics( // Set details for our sprite font font_group.group.sprite = font.sprite.Face{ - .width = @intFromFloat(metrics.cell_width), - .height = @intFromFloat(metrics.cell_height), + .width = metrics.cell_width, + .height = metrics.cell_height, .thickness = 2, - .underline_position = @intFromFloat(metrics.underline_position), + .underline_position = metrics.underline_position, }; return metrics; @@ -1148,7 +1157,7 @@ pub fn updateCell( shaper_run.font_index, shaper_cell.glyph_index, .{ - .max_height = @intFromFloat(@ceil(self.cell_size.height)), + .max_height = @intCast(self.cell_size.height), .thicken = self.config.font_thicken, }, ); diff --git a/src/renderer/opengl/Program.zig b/src/renderer/opengl/Program.zig index fa5eee8dc..d266bd226 100644 --- a/src/renderer/opengl/Program.zig +++ b/src/renderer/opengl/Program.zig @@ -101,7 +101,10 @@ pub inline fn setUniform( c.GL_FALSE, @ptrCast(&value), ), - else => unreachable, + else => { + log.warn("unsupported uniform type {}", .{@TypeOf(value)}); + unreachable; + }, } try errors.getError(); } diff --git a/src/renderer/size.zig b/src/renderer/size.zig index b84298fa8..24aedeb4c 100644 --- a/src/renderer/size.zig +++ b/src/renderer/size.zig @@ -14,8 +14,8 @@ const log = std.log.scoped(.renderer_size); /// The units for the width and height are in world space. They have to /// be normalized for any renderer implementation. pub const CellSize = struct { - width: f32, - height: f32, + width: u32, + height: u32, /// Initialize the cell size information from a font group. This ensures /// that all renderers use the same cell sizing information for the same @@ -71,8 +71,14 @@ pub const GridSize = struct { /// Update the columns/rows for the grid based on the given screen and /// cell size. pub fn update(self: *GridSize, screen: ScreenSize, cell: CellSize) void { - self.columns = @max(1, @as(Unit, @intFromFloat(@as(f32, @floatFromInt(screen.width)) / cell.width))); - self.rows = @max(1, @as(Unit, @intFromFloat(@as(f32, @floatFromInt(screen.height)) / cell.height))); + const cell_width: f32 = @floatFromInt(cell.width); + const cell_height: f32 = @floatFromInt(cell.height); + const screen_width: f32 = @floatFromInt(screen.width); + const screen_height: f32 = @floatFromInt(screen.height); + const calc_cols: Unit = @intFromFloat(screen_width / cell_width); + const calc_rows: Unit = @intFromFloat(screen_height / cell_height); + self.columns = @max(1, calc_cols); + self.rows = @max(1, calc_rows); } }; @@ -86,9 +92,13 @@ pub const Padding = struct { /// Returns padding that balances the whitespace around the screen /// for the given grid and cell sizes. pub fn balanced(screen: ScreenSize, grid: GridSize, cell: CellSize) Padding { + // Turn our cell sizes into floats for the math + const cell_width: f32 = @floatFromInt(cell.width); + const cell_height: f32 = @floatFromInt(cell.height); + // The size of our full grid - const grid_width = @as(f32, @floatFromInt(grid.columns)) * cell.width; - const grid_height = @as(f32, @floatFromInt(grid.rows)) * cell.height; + const grid_width = @as(f32, @floatFromInt(grid.columns)) * cell_width; + const grid_height = @as(f32, @floatFromInt(grid.rows)) * cell_height; // The empty space to the right of a line and bottom of the last row const space_right = @as(f32, @floatFromInt(screen.width)) - grid_width;