From 7686cacde61a6b094eabcfbdd73baee4a225b25a Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Tue, 8 Oct 2024 23:10:43 -0400 Subject: [PATCH 1/8] renderer, shaper: don't use null cells, handle bg and decorations separately Significant rework that also removes a lot of unnecessarily duplicated work while rebuilding cells in both renderers. Fixes multiple issues with decorations and bg colors on wide chars and ligatures, while reducing the amount of special case handling required. --- src/font/shape.zig | 5 +- src/font/shaper/coretext.zig | 28 -- src/font/shaper/harfbuzz.zig | 44 +- src/renderer/Metal.zig | 710 ++++++++++++++++------------ src/renderer/OpenGL.zig | 882 +++++++++++++++++++---------------- 5 files changed, 898 insertions(+), 771 deletions(-) diff --git a/src/font/shape.zig b/src/font/shape.zig index 0423d0ed1..3721c63a6 100644 --- a/src/font/shape.zig +++ b/src/font/shape.zig @@ -44,10 +44,7 @@ pub const Cell = struct { /// this cell is available in the text run. This glyph index is only /// valid for a given GroupCache and FontIndex that was used to create /// the runs. - /// - /// If this is null then this is an empty cell. If there are styles - /// then those should be applied but there is no glyph to render. - glyph_index: ?u32, + glyph_index: u32, }; /// Options for shapers. diff --git a/src/font/shaper/coretext.zig b/src/font/shaper/coretext.zig index 185ff1aca..085f913d3 100644 --- a/src/font/shaper/coretext.zig +++ b/src/font/shaper/coretext.zig @@ -437,15 +437,6 @@ pub const Shaper = struct { // wait for that. if (cell_offset.cluster > cluster) break :pad; - // If we have a gap between clusters then we need to - // add empty cells to the buffer. - for (cell_offset.cluster + 1..cluster) |x| { - try self.cell_buf.append(self.alloc, .{ - .x = @intCast(x), - .glyph_index = null, - }); - } - cell_offset = .{ .cluster = cluster }; } @@ -463,25 +454,6 @@ pub const Shaper = struct { } } - // If our last cell doesn't match our last cluster then we have - // a left-replaced ligature that needs to have spaces appended - // so that cells retain their background colors. - if (self.cell_buf.items.len > 0) pad: { - const last_cell = self.cell_buf.items[self.cell_buf.items.len - 1]; - const last_cp = state.codepoints.items[state.codepoints.items.len - 1]; - if (last_cell.x == last_cp.cluster) break :pad; - assert(last_cell.x < last_cp.cluster); - - // We need to go back to the last matched cluster and add - // padding up to there. - for (last_cell.x + 1..last_cp.cluster + 1) |x| { - try self.cell_buf.append(self.alloc, .{ - .x = @intCast(x), - .glyph_index = null, - }); - } - } - return self.cell_buf.items; } diff --git a/src/font/shaper/harfbuzz.zig b/src/font/shaper/harfbuzz.zig index 6dd520ad4..53efeec54 100644 --- a/src/font/shaper/harfbuzz.zig +++ b/src/font/shaper/harfbuzz.zig @@ -150,7 +150,7 @@ pub const Shaper = struct { // Convert all our info/pos to cells and set it. self.cell_buf.clearRetainingCapacity(); - for (info, pos, 0..) |info_v, pos_v, i| { + for (info, pos) |info_v, pos_v| { // If our cluster changed then we've moved to a new cell. if (info_v.cluster != cell_offset.cluster) cell_offset = .{ .cluster = info_v.cluster, @@ -174,48 +174,6 @@ pub const Shaper = struct { cell_offset.y += pos_v.y_advance; } - // Determine the width of the cell. To do this, we have to - // find the next cluster that has been shaped. This tells us how - // many cells this glyph replaced (i.e. for ligatures). For example - // in some fonts "!=" turns into a single glyph from the component - // parts "!" and "=" so this cell width would be "2" despite - // only having a single glyph. - // - // Many fonts replace ligature cells with space so that this always - // is one (e.g. Fira Code, JetBrains Mono, etc). Some do not - // (e.g. Monaspace). - const cell_width = width: { - if (i + 1 < info.len) { - // We may have to go through multiple glyphs because - // multiple can be replaced. e.g. "===" - for (info[i + 1 ..]) |next_info_v| { - if (next_info_v.cluster != info_v.cluster) { - // We do a saturating sub here because for RTL - // text, the next cluster can be less than the - // current cluster. We don't really support RTL - // currently so we do this to prevent an underflow - // but it isn't correct generally. - break :width next_info_v.cluster -| info_v.cluster; - } - } - } - - // If we reached the end then our width is our max cluster - // minus this one. - const max = run.offset + run.cells; - break :width max - info_v.cluster; - }; - if (cell_width > 1) { - // To make the renderer implementations simpler, we convert - // the extra spaces for width to blank cells. - for (1..cell_width) |j| { - try self.cell_buf.append(self.alloc, .{ - .x = @intCast(info_v.cluster + j), - .glyph_index = null, - }); - } - } - // const i = self.cell_buf.items.len - 1; // log.warn("i={} info={} pos={} cell={}", .{ i, info_v, pos_v, self.cell_buf.items[i] }); } diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig index 20c14cbad..9340ce5d0 100644 --- a/src/renderer/Metal.zig +++ b/src/renderer/Metal.zig @@ -2209,7 +2209,7 @@ fn rebuildCells( var row_it = screen.pages.rowIterator(.left_up, .{ .viewport = .{} }, null); var y: terminal.size.CellCountInt = screen.pages.rows; while (row_it.next()) |row| { - y = y - 1; + y -= 1; if (!rebuild) { // Only rebuild if we are doing a full rebuild or this row is dirty. @@ -2255,82 +2255,317 @@ fn rebuildCells( }, } - // Split our row into runs and shape each one. - var iter = self.font_shaper.runIterator( + // Iterator of of runs for shaping. + var run_iter = self.font_shaper.runIterator( self.font_grid, screen, row, row_selection, if (shape_cursor) screen.cursor.x else null, ); - while (try iter.next(self.alloc)) |run| { - // Try to read the cells from the shaping cache if we can. - const shaper_cells = self.font_shaper_cache.get(run) orelse cache: { - const cells = try self.font_shaper.shape(run); + var shaper_run: ?font.shape.TextRun = try run_iter.next(self.alloc); + var shaper_cells: ?[]const font.shape.Cell = null; + var shaper_cells_i: usize = 0; - // Try to cache them. If caching fails for any reason we continue - // because it is just a performance optimization, not a correctness - // issue. - self.font_shaper_cache.put(self.alloc, run, cells) catch |err| { - log.warn("error caching font shaping results err={}", .{err}); - }; + const row_cells = row.cells(.all); - // The cells we get from direct shaping are always owned by - // the shaper and valid until the next shaping call so we can - // just return them. - break :cache cells; - }; - - for (shaper_cells) |shaper_cell| { - // The shaper can emit null glyphs representing the right half - // of wide characters, we don't need to do anything with them. - if (shaper_cell.glyph_index == null) continue; - - const coord: terminal.Coordinate = .{ - .x = shaper_cell.x, - .y = y, - }; - - // If this cell falls within our preedit range then we - // skip this because preedits are setup separately. - if (preedit_range) |range| { - if (range.y == coord.y and - coord.x >= range.x[0] and - coord.x <= range.x[1]) + for (row_cells, 0..) |*cell, x| { + // If this cell falls within our preedit range then we + // skip this because preedits are setup separately. + if (preedit_range) |range| { + if (range.y == y) { + if (x >= range.x[0] and + x <= range.x[1]) { continue; } - } - // It this cell is within our hint range then we need to - // underline it. - const cell: terminal.Pin = cell: { - var copy = row; - copy.x = coord.x; - break :cell copy; - }; + // After exiting the preedit range we need to catch + // the run position up because of the missed cells. + if (x == range.x[1] + 1) { + while (shaper_run) |run| { + if (run.offset + run.cells > x) break; + shaper_run = try run_iter.next(self.alloc); + shaper_cells = null; + shaper_cells_i = 0; + } + if (shaper_run) |run| { + // Try to read the cells from the shaping cache if we can. + shaper_cells = self.font_shaper_cache.get(run) orelse cache: { + // Otherwise we have to shape them. + const cells = try self.font_shaper.shape(run); - if (self.updateCell( - screen, - cell, - if (link_match_set.contains(screen, cell)) - .single - else - null, - color_palette, - shaper_cell, - run, - coord, - )) |update| { - assert(update); - } else |err| { - log.warn("error building cell, will be invalid x={} y={}, err={}", .{ - coord.x, - coord.y, - err, - }); + // Try to cache them. If caching fails for any reason we + // continue because it is just a performance optimization, + // not a correctness issue. + self.font_shaper_cache.put( + self.alloc, + run, + cells, + ) catch |err| { + log.warn( + "error caching font shaping results err={}", + .{err}, + ); + }; + + // The cells we get from direct shaping are always owned + // by the shaper and valid until the next shaping call so + // we can safely use them. + break :cache cells; + }; + } + if (shaper_cells) |cells| { + while (cells[shaper_cells_i].x < x) { + shaper_cells_i += 1; + } + } + } } } + + const wide = cell.wide; + + const style = row.style(cell); + + const cell_pin: terminal.Pin = cell: { + var copy = row; + copy.x = @intCast(x); + break :cell copy; + }; + + // True if this cell is selected + const selected: bool = if (screen.selection) |sel| + sel.contains(screen, .{ + .page = row.page, + .y = row.y, + .x = @intCast( + // Spacer tails should show the selection + // state of the wide cell they belong to. + if (wide == .spacer_tail) + x -| 1 + else + x, + ), + }) + else + false; + + const bg_style = style.bg(cell, color_palette); + const fg_style = style.fg(color_palette, self.config.bold_is_bright) orelse self.foreground_color; + + // The final background color for the cell. + const bg = bg: { + if (selected) { + break :bg if (self.config.invert_selection_fg_bg) + if (style.flags.inverse) + // Cell is selected with invert selection fg/bg + // enabled, and the cell has the inverse style + // flag, so they cancel out and we get the normal + // bg color. + bg_style + else + // If it doesn't have the inverse style + // flag then we use the fg color instead. + fg_style + else + // If we don't have invert selection fg/bg set then we + // just use the selection background if set, otherwise + // the default fg color. + break :bg self.config.selection_background orelse self.foreground_color; + } + + // Not selected + break :bg if (style.flags.inverse != isCovering(cell.codepoint())) + // Two cases cause us to invert (use the fg color as the bg) + // - The "inverse" style flag. + // - A "covering" glyph; we use fg for bg in that case to + // help make sure that padding extension works correctly. + // If one of these is true (but not the other) + // then we use the fg style color for the bg. + fg_style + else + // Otherwise they cancel out. + bg_style; + }; + + const fg = fg: { + if (selected and !self.config.invert_selection_fg_bg) { + // If we don't have invert selection fg/bg set + // then we just use the selection foreground if + // set, otherwise the default bg color. + break :fg self.config.selection_foreground orelse self.background_color; + } + + // Whether we need to use the bg color as our fg color: + // - Cell is inverted and not selected + // - Cell is selected and not inverted + // Note: if selected then invert sel fg / bg must be + // false since we separately handle it if true above. + break :fg if (style.flags.inverse != selected) + bg_style orelse self.background_color + else + fg_style; + }; + + // Foreground alpha for this cell. + const alpha: u8 = if (style.flags.faint) 175 else 255; + + // If the cell has a background color, set it. + if (bg) |rgb| { + // Determine our background alpha. If we have transparency configured + // then this is dynamic depending on some situations. This is all + // in an attempt to make transparency look the best for various + // situations. See inline comments. + const bg_alpha: u8 = bg_alpha: { + const default: u8 = 255; + + if (self.config.background_opacity >= 1) break :bg_alpha default; + + // If we're selected, we do not apply background opacity + if (selected) break :bg_alpha default; + + // If we're reversed, do not apply background opacity + if (style.flags.inverse) break :bg_alpha default; + + // If we have a background and its not the default background + // then we apply background opacity + if (style.bg(cell, color_palette) != null and !rgb.eql(self.background_color)) { + break :bg_alpha default; + } + + // We apply background opacity. + var bg_alpha: f64 = @floatFromInt(default); + bg_alpha *= self.config.background_opacity; + bg_alpha = @ceil(bg_alpha); + break :bg_alpha @intFromFloat(bg_alpha); + }; + + self.cells.bgCell(y, x).* = .{ + rgb.r, rgb.g, rgb.b, bg_alpha, + }; + } + + // If the invisible flag is set on this cell then we + // don't need to render any foreground elements, so + // we just skip all glyphs with this x coordinate. + // + // NOTE: This behavior matches xterm. Some other terminal + // emulators, e.g. Alacritty, still render text decorations + // and only make the text itself invisible. The decision + // has been made here to match xterm's behavior for this. + if (style.flags.invisible) { + continue; + } + + // Give links a single underline, unless they already have + // an underline, in which case use a double underline to + // distinguish them. + const underline: terminal.Attribute.Underline = if (link_match_set.contains(screen, cell_pin)) + if (style.flags.underline == .single) + .double + else + .single + else + style.flags.underline; + + // We draw underlines first so that they layer underneath text. + // This improves readability when a colored underline is used + // which intersects parts of the text (descenders). + if (underline != .none) self.addUnderline( + @intCast(x), + @intCast(y), + underline, + style.underlineColor(color_palette) orelse fg, + alpha, + ) catch |err| { + log.warn( + "error adding underline to cell, will be invalid x={} y={}, err={}", + .{ x, y, err }, + ); + }; + + // If we're at or past the end of our shaper run then + // we need to get the next run from the run iterator. + if (shaper_cells != null and shaper_cells_i >= shaper_cells.?.len) { + shaper_run = try run_iter.next(self.alloc); + shaper_cells = null; + shaper_cells_i = 0; + } + + // If we haven't shaped this run yet, do so. + if (shaper_cells == null) if (shaper_run) |run| { + // Try to read the cells from the shaping cache if we can. + shaper_cells = self.font_shaper_cache.get(run) orelse cache: { + // Otherwise we have to shape them. + const cells = try self.font_shaper.shape(run); + + // Try to cache them. If caching fails for any reason we + // continue because it is just a performance optimization, + // not a correctness issue. + self.font_shaper_cache.put( + self.alloc, + run, + cells, + ) catch |err| { + log.warn( + "error caching font shaping results err={}", + .{err}, + ); + }; + + // The cells we get from direct shaping are always owned + // by the shaper and valid until the next shaping call so + // we can safely use them. + break :cache cells; + }; + }; + + // NOTE: An assumption is made here that a single cell will never + // be present in more than one shaper run. If that assumption is + // violated, this logic breaks. + if (shaper_cells) |cells| glyphs: { + // If there are no shaper cells for this run, ignore it. + // This can occur for runs of empty cells, and is fine. + if (cells.len == 0) break :glyphs; + + // If we encounter a shaper cell to the left of the current + // cell then we have some problems. This logic relies on x + // position monotonically increasing. + assert(cells[shaper_cells_i].x >= x); + + while (shaper_cells_i < cells.len and cells[shaper_cells_i].x == x) : ({ + shaper_cells_i += 1; + }) { + self.addGlyph( + @intCast(x), + @intCast(y), + cell_pin, + cells[shaper_cells_i], + shaper_run.?, + fg, + alpha, + ) catch |err| { + log.warn( + "error adding glyph to cell, will be invalid x={} y={}, err={}", + .{ x, y, err }, + ); + }; + } + } + + // Finally, draw a strikethrough if necessary. + if (style.flags.strikethrough) self.addStrikethrough( + @intCast(x), + @intCast(y), + fg, + alpha, + ) catch |err| { + log.warn( + "error adding strikethrough to cell, will be invalid x={} y={}, err={}", + .{ x, y, err }, + ); + }; } } @@ -2422,252 +2657,133 @@ fn rebuildCells( // }); } -fn updateCell( +/// Add an underline decoration to the specified cell +fn addUnderline( self: *Metal, - screen: *const terminal.Screen, + x: terminal.size.CellCountInt, + y: terminal.size.CellCountInt, + style: terminal.Attribute.Underline, + color: terminal.color.RGB, + alpha: u8, +) !void { + const sprite: font.Sprite = switch (style) { + .none => unreachable, + .single => .underline, + .double => .underline_double, + .dotted => .underline_dotted, + .dashed => .underline_dashed, + .curly => .underline_curly, + }; + + const render = try self.font_grid.renderGlyph( + self.alloc, + font.sprite_index, + @intFromEnum(sprite), + .{ + .cell_width = 1, + .grid_metrics = self.grid_metrics, + }, + ); + + try self.cells.add(self.alloc, .underline, .{ + .mode = .fg, + .grid_pos = .{ @intCast(x), @intCast(y) }, + .constraint_width = 1, + .color = .{ color.r, color.g, color.b, alpha }, + .glyph_pos = .{ render.glyph.atlas_x, render.glyph.atlas_y }, + .glyph_size = .{ render.glyph.width, render.glyph.height }, + .bearings = .{ + @intCast(render.glyph.offset_x), + @intCast(render.glyph.offset_y), + }, + }); +} + +/// Add a strikethrough decoration to the specified cell +fn addStrikethrough( + self: *Metal, + x: terminal.size.CellCountInt, + y: terminal.size.CellCountInt, + color: terminal.color.RGB, + alpha: u8, +) !void { + const render = try self.font_grid.renderGlyph( + self.alloc, + font.sprite_index, + @intFromEnum(font.Sprite.strikethrough), + .{ + .cell_width = 1, + .grid_metrics = self.grid_metrics, + }, + ); + + try self.cells.add(self.alloc, .strikethrough, .{ + .mode = .fg, + .grid_pos = .{ @intCast(x), @intCast(y) }, + .constraint_width = 1, + .color = .{ color.r, color.g, color.b, alpha }, + .glyph_pos = .{ render.glyph.atlas_x, render.glyph.atlas_y }, + .glyph_size = .{ render.glyph.width, render.glyph.height }, + .bearings = .{ + @intCast(render.glyph.offset_x), + @intCast(render.glyph.offset_y), + }, + }); +} + +// Add a glyph to the specified cell. +fn addGlyph( + self: *Metal, + x: terminal.size.CellCountInt, + y: terminal.size.CellCountInt, cell_pin: terminal.Pin, - cell_underline: ?terminal.Attribute.Underline, - palette: *const terminal.color.Palette, shaper_cell: font.shape.Cell, shaper_run: font.shape.TextRun, - coord: terminal.Coordinate, -) !bool { - const BgFg = struct { - /// Background is optional because in un-inverted mode - /// it may just be equivalent to the default background in - /// which case we do nothing to save on GPU render time. - bg: ?terminal.color.RGB, - - /// Fg is always set to some color, though we may not render - /// any fg if the cell is empty or has no attributes like - /// underline. - fg: terminal.color.RGB, - }; - - // True if this cell is selected - const selected: bool = if (screen.selection) |sel| - sel.contains(screen, cell_pin) - else - false; - + color: terminal.color.RGB, + alpha: u8, +) !void { const rac = cell_pin.rowAndCell(); const cell = rac.cell; - const style = cell_pin.style(cell); - const underline = cell_underline orelse style.flags.underline; - // The colors for the cell. - const colors: BgFg = colors: { - // The normal cell result - const cell_res: BgFg = if (!style.flags.inverse) .{ - // In normal mode, background and fg match the cell. We - // un-optionalize the fg by defaulting to our fg color. - .bg = style.bg(cell, palette), - .fg = style.fg(palette, self.config.bold_is_bright) orelse self.foreground_color, - } else .{ - // In inverted mode, the background MUST be set to something - // (is never null) so it is either the fg or default fg. The - // fg is either the bg or default background. - .bg = style.fg(palette, self.config.bold_is_bright) orelse self.foreground_color, - .fg = style.bg(cell, palette) orelse self.background_color, - }; + // Render + const render = try self.font_grid.renderGlyph( + self.alloc, + shaper_run.font_index, + shaper_cell.glyph_index, + .{ + .grid_metrics = self.grid_metrics, + .thicken = self.config.font_thicken, + }, + ); - // If we are selected, we our colors are just inverted fg/bg - const selection_res: ?BgFg = if (selected) .{ - .bg = if (self.config.invert_selection_fg_bg) - cell_res.fg - else - self.config.selection_background orelse self.foreground_color, - .fg = if (self.config.invert_selection_fg_bg) - cell_res.bg orelse self.background_color - else - self.config.selection_foreground orelse self.background_color, - } else null; + // If the glyph is 0 width or height, it will be invisible + // when drawn, so don't bother adding it to the buffer. + if (render.glyph.width == 0 or render.glyph.height == 0) { + return; + } - // If the cell is "invisible" then we just make fg = bg so that - // the cell is transparent but still copy-able. - const res: BgFg = selection_res orelse cell_res; - if (style.flags.invisible) { - break :colors .{ - .bg = res.bg, - .fg = res.bg orelse self.background_color, - }; - } - - // If our cell has a covering glyph, then our bg is set to our fg - // so that padding extension works correctly. - if (!selected and isCovering(cell.codepoint())) { - break :colors .{ - .bg = res.fg, - .fg = res.fg, - }; - } - - break :colors res; + const mode: mtl_shaders.CellText.Mode = switch (try fgMode( + render.presentation, + cell_pin, + )) { + .normal => .fg, + .color => .fg_color, + .constrained => .fg_constrained, + .powerline => .fg_powerline, }; - // Alpha multiplier - const alpha: u8 = if (style.flags.faint) 175 else 255; - - // If the cell has a background, we always draw it. - if (colors.bg) |rgb| { - // Determine our background alpha. If we have transparency configured - // then this is dynamic depending on some situations. This is all - // in an attempt to make transparency look the best for various - // situations. See inline comments. - const bg_alpha: u8 = bg_alpha: { - const default: u8 = 255; - - if (self.config.background_opacity >= 1) break :bg_alpha default; - - // If we're selected, we do not apply background opacity - if (selected) break :bg_alpha default; - - // If we're reversed, do not apply background opacity - if (style.flags.inverse) break :bg_alpha default; - - // If we have a background and its not the default background - // then we apply background opacity - if (style.bg(cell, palette) != null and !rgb.eql(self.background_color)) { - break :bg_alpha default; - } - - // We apply background opacity. - var bg_alpha: f64 = @floatFromInt(default); - bg_alpha *= self.config.background_opacity; - bg_alpha = @ceil(bg_alpha); - break :bg_alpha @intFromFloat(bg_alpha); - }; - - self.cells.bgCell(coord.y, coord.x).* = .{ - rgb.r, rgb.g, rgb.b, bg_alpha, - }; - if (cell.gridWidth() > 1 and coord.x < self.cells.size.columns - 1) { - self.cells.bgCell(coord.y, coord.x + 1).* = .{ - rgb.r, rgb.g, rgb.b, bg_alpha, - }; - } - } - - // If the cell has an underline, draw it before the character glyph, - // so that it layers underneath instead of overtop, since that can - // make text difficult to read. - if (underline != .none) { - const sprite: font.Sprite = switch (underline) { - .none => unreachable, - .single => .underline, - .double => .underline_double, - .dotted => .underline_dotted, - .dashed => .underline_dashed, - .curly => .underline_curly, - }; - - const render = try self.font_grid.renderGlyph( - self.alloc, - font.sprite_index, - @intFromEnum(sprite), - .{ - .cell_width = 1, - .grid_metrics = self.grid_metrics, - }, - ); - - const color = style.underlineColor(palette) orelse colors.fg; - - var gpu_cell: mtl_cell.Key.underline.CellType() = .{ - .mode = .fg, - .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) }, - .constraint_width = 1, - .color = .{ color.r, color.g, color.b, alpha }, - .glyph_pos = .{ render.glyph.atlas_x, render.glyph.atlas_y }, - .glyph_size = .{ render.glyph.width, render.glyph.height }, - .bearings = .{ - @intCast(render.glyph.offset_x), - @intCast(render.glyph.offset_y), - }, - }; - try self.cells.add(self.alloc, .underline, gpu_cell); - // If it's a wide cell we need to underline the right half as well. - if (cell.gridWidth() > 1 and coord.x < self.cells.size.columns - 1) { - gpu_cell.grid_pos[0] = @intCast(coord.x + 1); - try self.cells.add(self.alloc, .underline, gpu_cell); - } - } - - // If the shaper cell has a glyph, draw it. - if (shaper_cell.glyph_index) |glyph_index| glyph: { - // Render - const render = try self.font_grid.renderGlyph( - self.alloc, - shaper_run.font_index, - glyph_index, - .{ - .grid_metrics = self.grid_metrics, - .thicken = self.config.font_thicken, - }, - ); - - // If the glyph is 0 width or height, it will be invisible - // when drawn, so don't bother adding it to the buffer. - if (render.glyph.width == 0 or render.glyph.height == 0) { - break :glyph; - } - - const mode: mtl_shaders.CellText.Mode = switch (try fgMode( - render.presentation, - cell_pin, - )) { - .normal => .fg, - .color => .fg_color, - .constrained => .fg_constrained, - .powerline => .fg_powerline, - }; - - try self.cells.add(self.alloc, .text, .{ - .mode = mode, - .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) }, - .constraint_width = cell.gridWidth(), - .color = .{ colors.fg.r, colors.fg.g, colors.fg.b, alpha }, - .glyph_pos = .{ render.glyph.atlas_x, render.glyph.atlas_y }, - .glyph_size = .{ render.glyph.width, render.glyph.height }, - .bearings = .{ - @intCast(render.glyph.offset_x + shaper_cell.x_offset), - @intCast(render.glyph.offset_y + shaper_cell.y_offset), - }, - }); - } - - if (style.flags.strikethrough) { - const render = try self.font_grid.renderGlyph( - self.alloc, - font.sprite_index, - @intFromEnum(font.Sprite.strikethrough), - .{ - .cell_width = 1, - .grid_metrics = self.grid_metrics, - }, - ); - - var gpu_cell: mtl_cell.Key.strikethrough.CellType() = .{ - .mode = .fg, - .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) }, - .constraint_width = 1, - .color = .{ colors.fg.r, colors.fg.g, colors.fg.b, alpha }, - .glyph_pos = .{ render.glyph.atlas_x, render.glyph.atlas_y }, - .glyph_size = .{ render.glyph.width, render.glyph.height }, - .bearings = .{ - @intCast(render.glyph.offset_x), - @intCast(render.glyph.offset_y), - }, - }; - try self.cells.add(self.alloc, .strikethrough, gpu_cell); - // If it's a wide cell we need to strike through the right half as well. - if (cell.gridWidth() > 1 and coord.x < self.cells.size.columns - 1) { - gpu_cell.grid_pos[0] = @intCast(coord.x + 1); - try self.cells.add(self.alloc, .strikethrough, gpu_cell); - } - } - - return true; + try self.cells.add(self.alloc, .text, .{ + .mode = mode, + .grid_pos = .{ @intCast(x), @intCast(y) }, + .constraint_width = cell.gridWidth(), + .color = .{ color.r, color.g, color.b, alpha }, + .glyph_pos = .{ render.glyph.atlas_x, render.glyph.atlas_y }, + .glyph_size = .{ render.glyph.width, render.glyph.height }, + .bearings = .{ + @intCast(render.glyph.offset_x + shaper_cell.x_offset), + @intCast(render.glyph.offset_y + shaper_cell.y_offset), + }, + }); } fn addCursor( diff --git a/src/renderer/OpenGL.zig b/src/renderer/OpenGL.zig index 5281a3c7f..54835d097 100644 --- a/src/renderer/OpenGL.zig +++ b/src/renderer/OpenGL.zig @@ -1255,29 +1255,10 @@ pub fn rebuildCells( } // Build each cell - var row_it = screen.pages.rowIterator(.right_down, .{ .viewport = .{} }, null); - var y: terminal.size.CellCountInt = 0; + var row_it = screen.pages.rowIterator(.left_up, .{ .viewport = .{} }, null); + var y: terminal.size.CellCountInt = screen.pages.rows; while (row_it.next()) |row| { - defer y += 1; - - // Our selection value is only non-null if this selection happens - // to contain this row. This selection value will be set to only be - // the selection that contains this row. This way, if the selection - // changes but not for this line, we don't invalidate the cache. - const selection = sel: { - const sel = screen.selection orelse break :sel null; - const pin = screen.pages.pin(.{ .viewport = .{ .y = y } }) orelse - break :sel null; - break :sel sel.containedRow(screen, pin) orelse null; - }; - - // See Metal.zig - const cursor_row = if (cursor_style_) |cursor_style| - cursor_style == .block and - screen.viewportIsBottom() and - y == screen.cursor.y - else - false; + y -= 1; // True if we want to do font shaping around the cursor. We want to // do font shaping as long as the cursor is enabled. @@ -1287,7 +1268,7 @@ pub fn rebuildCells( // If this is the row with our cursor, then we may have to modify // the cell with the cursor. const start_i: usize = self.cells.items.len; - defer if (cursor_row) { + defer if (shape_cursor and cursor_style_ == .block) { const x = screen.cursor.x; const wide = row.cells(.all)[x].wide; const min_x = switch (wide) { @@ -1310,6 +1291,15 @@ pub fn rebuildCells( } }; + // We need to get this row's selection if there is one for proper + // run splitting. + const row_selection = sel: { + const sel = screen.selection orelse break :sel null; + const pin = screen.pages.pin(.{ .viewport = .{ .y = y } }) orelse + break :sel null; + break :sel sel.containedRow(screen, pin) orelse null; + }; + // On primary screen, we still apply vertical padding extension // under certain conditions we feel are safe. This helps make some // scenarios look better while avoiding scenarios we know do NOT look @@ -1332,79 +1322,346 @@ pub fn rebuildCells( }, } - // Split our row into runs and shape each one. - var iter = self.font_shaper.runIterator( + // Iterator of of runs for shaping. + var run_iter = self.font_shaper.runIterator( self.font_grid, screen, row, - selection, + row_selection, if (shape_cursor) screen.cursor.x else null, ); - while (try iter.next(self.alloc)) |run| { - // Try to read the cells from the shaping cache if we can. - const shaper_cells = self.font_shaper_cache.get(run) orelse cache: { - const cells = try self.font_shaper.shape(run); + var shaper_run: ?font.shape.TextRun = try run_iter.next(self.alloc); + var shaper_cells: ?[]const font.shape.Cell = null; + var shaper_cells_i: usize = 0; - // Try to cache them. If caching fails for any reason we continue - // because it is just a performance optimization, not a correctness - // issue. - self.font_shaper_cache.put(self.alloc, run, cells) catch |err| { - log.warn("error caching font shaping results err={}", .{err}); - }; + const row_cells = row.cells(.all); - // The cells we get from direct shaping are always owned by - // the shaper and valid until the next shaping call so we can - // just return them. - break :cache cells; - }; - - for (shaper_cells) |shaper_cell| { - // The shaper can emit null glyphs representing the right half - // of wide characters, we don't need to do anything with them. - if (shaper_cell.glyph_index == null) continue; - - // If this cell falls within our preedit range then we skip it. - // We do this so we don't have conflicting data on the same - // cell. - if (preedit_range) |range| { - if (range.y == y and - shaper_cell.x >= range.x[0] and - shaper_cell.x <= range.x[1]) + for (row_cells, 0..) |*cell, x| { + // If this cell falls within our preedit range then we + // skip this because preedits are setup separately. + if (preedit_range) |range| { + if (range.y == y) { + if (x >= range.x[0] and + x <= range.x[1]) { continue; } - } - // It this cell is within our hint range then we need to - // underline it. - const cell: terminal.Pin = cell: { - var copy = row; - copy.x = shaper_cell.x; - break :cell copy; - }; + // After exiting the preedit range we need to catch + // the run position up because of the missed cells. + if (x == range.x[1] + 1) { + while (shaper_run) |run| { + if (run.offset + run.cells > x) break; + shaper_run = try run_iter.next(self.alloc); + shaper_cells = null; + shaper_cells_i = 0; + } + if (shaper_run) |run| { + // Try to read the cells from the shaping cache if we can. + shaper_cells = self.font_shaper_cache.get(run) orelse cache: { + // Otherwise we have to shape them. + const cells = try self.font_shaper.shape(run); - if (self.updateCell( - screen, - cell, - if (link_match_set.orderedContains(screen, cell)) - .single - else - null, - color_palette, - shaper_cell, - run, - shaper_cell.x, - y, - )) |update| { - assert(update); - } else |err| { - log.warn("error building cell, will be invalid x={} y={}, err={}", .{ - shaper_cell.x, - y, - err, - }); + // Try to cache them. If caching fails for any reason we + // continue because it is just a performance optimization, + // not a correctness issue. + self.font_shaper_cache.put( + self.alloc, + run, + cells, + ) catch |err| { + log.warn( + "error caching font shaping results err={}", + .{err}, + ); + }; + + // The cells we get from direct shaping are always owned + // by the shaper and valid until the next shaping call so + // we can safely use them. + break :cache cells; + }; + } + if (shaper_cells) |cells| { + while (cells[shaper_cells_i].x < x) { + shaper_cells_i += 1; + } + } + } } } + + const wide = cell.wide; + + const style = row.style(cell); + + const cell_pin: terminal.Pin = cell: { + var copy = row; + copy.x = @intCast(x); + break :cell copy; + }; + + // True if this cell is selected + const selected: bool = if (screen.selection) |sel| + sel.contains(screen, .{ + .page = row.page, + .y = row.y, + .x = @intCast( + // Spacer tails should show the selection + // state of the wide cell they belong to. + if (wide == .spacer_tail) + x -| 1 + else + x, + ), + }) + else + false; + + const bg_style = style.bg(cell, color_palette); + const fg_style = style.fg(color_palette, self.config.bold_is_bright) orelse self.foreground_color; + + // The final background color for the cell. + const bg = bg: { + if (selected) { + break :bg if (self.config.invert_selection_fg_bg) + if (style.flags.inverse) + // Cell is selected with invert selection fg/bg + // enabled, and the cell has the inverse style + // flag, so they cancel out and we get the normal + // bg color. + bg_style + else + // If it doesn't have the inverse style + // flag then we use the fg color instead. + fg_style + else + // If we don't have invert selection fg/bg set then we + // just use the selection background if set, otherwise + // the default fg color. + break :bg self.config.selection_background orelse self.foreground_color; + } + + // Not selected + break :bg if (style.flags.inverse != isCovering(cell.codepoint())) + // Two cases cause us to invert (use the fg color as the bg) + // - The "inverse" style flag. + // - A "covering" glyph; we use fg for bg in that case to + // help make sure that padding extension works correctly. + // If one of these is true (but not the other) + // then we use the fg style color for the bg. + fg_style + else + // Otherwise they cancel out. + bg_style; + }; + + const fg = fg: { + if (selected and !self.config.invert_selection_fg_bg) { + // If we don't have invert selection fg/bg set + // then we just use the selection foreground if + // set, otherwise the default bg color. + break :fg self.config.selection_foreground orelse self.background_color; + } + + // Whether we need to use the bg color as our fg color: + // - Cell is inverted and not selected + // - Cell is selected and not inverted + // Note: if selected then invert sel fg / bg must be + // false since we separately handle it if true above. + break :fg if (style.flags.inverse != selected) + bg_style orelse self.background_color + else + fg_style; + }; + + // Foreground alpha for this cell. + const alpha: u8 = if (style.flags.faint) 175 else 255; + + // If the cell has a background color, set it. + const bg_color: [4]u8 = if (bg) |rgb| bg: { + // Determine our background alpha. If we have transparency configured + // then this is dynamic depending on some situations. This is all + // in an attempt to make transparency look the best for various + // situations. See inline comments. + const bg_alpha: u8 = bg_alpha: { + const default: u8 = 255; + + if (self.config.background_opacity >= 1) break :bg_alpha default; + + // If we're selected, we do not apply background opacity + if (selected) break :bg_alpha default; + + // If we're reversed, do not apply background opacity + if (style.flags.inverse) break :bg_alpha default; + + // If we have a background and its not the default background + // then we apply background opacity + if (style.bg(cell, color_palette) != null and !rgb.eql(self.background_color)) { + break :bg_alpha default; + } + + // We apply background opacity. + var bg_alpha: f64 = @floatFromInt(default); + bg_alpha *= self.config.background_opacity; + bg_alpha = @ceil(bg_alpha); + break :bg_alpha @intFromFloat(bg_alpha); + }; + + try self.cells_bg.append(self.alloc, .{ + .mode = .bg, + .grid_col = @intCast(x), + .grid_row = @intCast(y), + .grid_width = cell.gridWidth(), + .glyph_x = 0, + .glyph_y = 0, + .glyph_width = 0, + .glyph_height = 0, + .glyph_offset_x = 0, + .glyph_offset_y = 0, + .r = rgb.r, + .g = rgb.g, + .b = rgb.b, + .a = bg_alpha, + .bg_r = 0, + .bg_g = 0, + .bg_b = 0, + .bg_a = 0, + }); + + break :bg .{ + rgb.r, rgb.g, rgb.b, bg_alpha, + }; + } else .{ + self.draw_background.r, + self.draw_background.g, + self.draw_background.b, + @intFromFloat(@max(0, @min(255, @round(self.config.background_opacity * 255)))), + }; + + // If the invisible flag is set on this cell then we + // don't need to render any foreground elements, so + // we just skip all glyphs with this x coordinate. + // + // NOTE: This behavior matches xterm. Some other terminal + // emulators, e.g. Alacritty, still render text decorations + // and only make the text itself invisible. The decision + // has been made here to match xterm's behavior for this. + if (style.flags.invisible) { + continue; + } + + // Give links a single underline, unless they already have + // an underline, in which case use a double underline to + // distinguish them. + const underline: terminal.Attribute.Underline = if (link_match_set.contains(screen, cell_pin)) + if (style.flags.underline == .single) + .double + else + .single + else + style.flags.underline; + + // We draw underlines first so that they layer underneath text. + // This improves readability when a colored underline is used + // which intersects parts of the text (descenders). + if (underline != .none) self.addUnderline( + @intCast(x), + @intCast(y), + underline, + style.underlineColor(color_palette) orelse fg, + alpha, + bg_color, + ) catch |err| { + log.warn( + "error adding underline to cell, will be invalid x={} y={}, err={}", + .{ x, y, err }, + ); + }; + + // If we're at or past the end of our shaper run then + // we need to get the next run from the run iterator. + if (shaper_cells != null and shaper_cells_i >= shaper_cells.?.len) { + shaper_run = try run_iter.next(self.alloc); + shaper_cells = null; + shaper_cells_i = 0; + } + + // If we haven't shaped this run yet, do so. + if (shaper_cells == null) if (shaper_run) |run| { + // Try to read the cells from the shaping cache if we can. + shaper_cells = self.font_shaper_cache.get(run) orelse cache: { + // Otherwise we have to shape them. + const cells = try self.font_shaper.shape(run); + + // Try to cache them. If caching fails for any reason we + // continue because it is just a performance optimization, + // not a correctness issue. + self.font_shaper_cache.put( + self.alloc, + run, + cells, + ) catch |err| { + log.warn( + "error caching font shaping results err={}", + .{err}, + ); + }; + + // The cells we get from direct shaping are always owned + // by the shaper and valid until the next shaping call so + // we can safely use them. + break :cache cells; + }; + }; + + // NOTE: An assumption is made here that a single cell will never + // be present in more than one shaper run. If that assumption is + // violated, this logic breaks. + if (shaper_cells) |cells| glyphs: { + // If there are no shaper cells for this run, ignore it. + // This can occur for runs of empty cells, and is fine. + if (cells.len == 0) break :glyphs; + + // If we encounter a shaper cell to the left of the current + // cell then we have some problems. This logic relies on x + // position monotonically increasing. + assert(cells[shaper_cells_i].x >= x); + + while (shaper_cells_i < cells.len and cells[shaper_cells_i].x == x) : ({ + shaper_cells_i += 1; + }) { + self.addGlyph( + @intCast(x), + @intCast(y), + cell_pin, + cells[shaper_cells_i], + shaper_run.?, + fg, + alpha, + bg_color, + ) catch |err| { + log.warn( + "error adding glyph to cell, will be invalid x={} y={}, err={}", + .{ x, y, err }, + ); + }; + } + } + + // Finally, draw a strikethrough if necessary. + if (style.flags.strikethrough) self.addStrikethrough( + @intCast(x), + @intCast(y), + fg, + alpha, + bg_color, + ) catch |err| { + log.warn( + "error adding strikethrough to cell, will be invalid x={} y={}, err={}", + .{ x, y, err }, + ); + }; } } @@ -1637,334 +1894,161 @@ fn addCursor( return &self.cells.items[self.cells.items.len - 1]; } -/// Update a single cell. The bool returns whether the cell was updated -/// or not. If the cell wasn't updated, a full refreshCells call is -/// needed. -fn updateCell( +/// Add an underline decoration to the specified cell +fn addUnderline( self: *OpenGL, - screen: *terminal.Screen, + x: terminal.size.CellCountInt, + y: terminal.size.CellCountInt, + style: terminal.Attribute.Underline, + color: terminal.color.RGB, + alpha: u8, + bg: [4]u8, +) !void { + const sprite: font.Sprite = switch (style) { + .none => unreachable, + .single => .underline, + .double => .underline_double, + .dotted => .underline_dotted, + .dashed => .underline_dashed, + .curly => .underline_curly, + }; + + const render = try self.font_grid.renderGlyph( + self.alloc, + font.sprite_index, + @intFromEnum(sprite), + .{ + .cell_width = 1, + .grid_metrics = self.grid_metrics, + }, + ); + + try self.cells.append(self.alloc, .{ + .mode = .fg, + .grid_col = @intCast(x), + .grid_row = @intCast(y), + .grid_width = 1, + .glyph_x = render.glyph.atlas_x, + .glyph_y = render.glyph.atlas_y, + .glyph_width = render.glyph.width, + .glyph_height = render.glyph.height, + .glyph_offset_x = render.glyph.offset_x, + .glyph_offset_y = render.glyph.offset_y, + .r = color.r, + .g = color.g, + .b = color.b, + .a = alpha, + .bg_r = bg[0], + .bg_g = bg[1], + .bg_b = bg[2], + .bg_a = bg[3], + }); +} + +/// Add a strikethrough decoration to the specified cell +fn addStrikethrough( + self: *OpenGL, + x: terminal.size.CellCountInt, + y: terminal.size.CellCountInt, + color: terminal.color.RGB, + alpha: u8, + bg: [4]u8, +) !void { + const render = try self.font_grid.renderGlyph( + self.alloc, + font.sprite_index, + @intFromEnum(font.Sprite.strikethrough), + .{ + .cell_width = 1, + .grid_metrics = self.grid_metrics, + }, + ); + + try self.cells.append(self.alloc, .{ + .mode = .fg, + .grid_col = @intCast(x), + .grid_row = @intCast(y), + .grid_width = 1, + .glyph_x = render.glyph.atlas_x, + .glyph_y = render.glyph.atlas_y, + .glyph_width = render.glyph.width, + .glyph_height = render.glyph.height, + .glyph_offset_x = render.glyph.offset_x, + .glyph_offset_y = render.glyph.offset_y, + .r = color.r, + .g = color.g, + .b = color.b, + .a = alpha, + .bg_r = bg[0], + .bg_g = bg[1], + .bg_b = bg[2], + .bg_a = bg[3], + }); +} + +// Add a glyph to the specified cell. +fn addGlyph( + self: *OpenGL, + x: terminal.size.CellCountInt, + y: terminal.size.CellCountInt, cell_pin: terminal.Pin, - cell_underline: ?terminal.Attribute.Underline, - palette: *const terminal.color.Palette, shaper_cell: font.shape.Cell, shaper_run: font.shape.TextRun, - x: usize, - y: usize, -) !bool { - const BgFg = struct { - /// Background is optional because in un-inverted mode - /// it may just be equivalent to the default background in - /// which case we do nothing to save on GPU render time. - bg: ?terminal.color.RGB, - - /// Fg is always set to some color, though we may not render - /// any fg if the cell is empty or has no attributes like - /// underline. - fg: terminal.color.RGB, - }; - - // True if this cell is selected - const selected: bool = if (screen.selection) |sel| - sel.contains(screen, cell_pin) - else - false; - + color: terminal.color.RGB, + alpha: u8, + bg: [4]u8, +) !void { const rac = cell_pin.rowAndCell(); const cell = rac.cell; - const style = cell_pin.style(cell); - const underline = cell_underline orelse style.flags.underline; - // The colors for the cell. - const colors: BgFg = colors: { - // The normal cell result - const cell_res: BgFg = if (!style.flags.inverse) .{ - // In normal mode, background and fg match the cell. We - // un-optionalize the fg by defaulting to our fg color. - .bg = style.bg(cell, palette), - .fg = style.fg(palette, self.config.bold_is_bright) orelse self.foreground_color, - } else .{ - // In inverted mode, the background MUST be set to something - // (is never null) so it is either the fg or default fg. The - // fg is either the bg or default background. - .bg = style.fg(palette, self.config.bold_is_bright) orelse self.foreground_color, - .fg = style.bg(cell, palette) orelse self.background_color, - }; + // Render + const render = try self.font_grid.renderGlyph( + self.alloc, + shaper_run.font_index, + shaper_cell.glyph_index, + .{ + .grid_metrics = self.grid_metrics, + .thicken = self.config.font_thicken, + }, + ); - // If we are selected, we our colors are just inverted fg/bg - const selection_res: ?BgFg = if (selected) .{ - .bg = if (self.config.invert_selection_fg_bg) - cell_res.fg - else - self.config.selection_background orelse self.foreground_color, - .fg = if (self.config.invert_selection_fg_bg) - cell_res.bg orelse self.background_color - else - self.config.selection_foreground orelse self.background_color, - } else null; + // If the glyph is 0 width or height, it will be invisible + // when drawn, so don't bother adding it to the buffer. + if (render.glyph.width == 0 or render.glyph.height == 0) { + return; + } - // If the cell is "invisible" then we just make fg = bg so that - // the cell is transparent but still copy-able. - const res: BgFg = selection_res orelse cell_res; - if (style.flags.invisible) { - break :colors BgFg{ - .bg = res.bg, - .fg = res.bg orelse self.background_color, - }; - } - - // If our cell has a covering glyph, then our bg is set to our fg - // so that padding extension works correctly. - if (!selected and isCovering(cell.codepoint())) { - break :colors .{ - .bg = res.fg, - .fg = res.fg, - }; - } - - break :colors res; + // If we're rendering a color font, we use the color atlas + const mode: CellProgram.CellMode = switch (try fgMode( + render.presentation, + cell_pin, + )) { + .normal => .fg, + .color => .fg_color, + .constrained => .fg_constrained, + .powerline => .fg_powerline, }; - // Alpha multiplier - const alpha: u8 = if (style.flags.faint) 175 else 255; - - // If the cell has a background, we always draw it. - const bg: [4]u8 = if (colors.bg) |rgb| bg: { - // Determine our background alpha. If we have transparency configured - // then this is dynamic depending on some situations. This is all - // in an attempt to make transparency look the best for various - // situations. See inline comments. - const bg_alpha: u8 = bg_alpha: { - const default: u8 = 255; - - if (self.config.background_opacity >= 1) break :bg_alpha default; - - // If we're selected, we do not apply background opacity - if (selected) break :bg_alpha default; - - // If we're reversed, do not apply background opacity - if (style.flags.inverse) break :bg_alpha default; - - // If we have a background and its not the default background - // then we apply background opacity - if (style.bg(cell, palette) != null and !rgb.eql(self.background_color)) { - break :bg_alpha default; - } - - // We apply background opacity. - var bg_alpha: f64 = @floatFromInt(default); - bg_alpha *= self.config.background_opacity; - bg_alpha = @ceil(bg_alpha); - break :bg_alpha @intFromFloat(bg_alpha); - }; - - try self.cells_bg.append(self.alloc, .{ - .mode = .bg, - .grid_col = @intCast(x), - .grid_row = @intCast(y), - .grid_width = cell.gridWidth(), - .glyph_x = 0, - .glyph_y = 0, - .glyph_width = 0, - .glyph_height = 0, - .glyph_offset_x = 0, - .glyph_offset_y = 0, - .r = rgb.r, - .g = rgb.g, - .b = rgb.b, - .a = bg_alpha, - .bg_r = 0, - .bg_g = 0, - .bg_b = 0, - .bg_a = 0, - }); - - break :bg .{ rgb.r, rgb.g, rgb.b, bg_alpha }; - } else .{ - self.draw_background.r, - self.draw_background.g, - self.draw_background.b, - @intFromFloat(@max(0, @min(255, @round(self.config.background_opacity * 255)))), - }; - - // If the cell has an underline, draw it before the character glyph, - // so that it layers underneath instead of overtop, since that can - // make text difficult to read. - if (underline != .none) { - const sprite: font.Sprite = switch (underline) { - .none => unreachable, - .single => .underline, - .double => .underline_double, - .dotted => .underline_dotted, - .dashed => .underline_dashed, - .curly => .underline_curly, - }; - - const render = try self.font_grid.renderGlyph( - self.alloc, - font.sprite_index, - @intFromEnum(sprite), - .{ - .cell_width = 1, - .grid_metrics = self.grid_metrics, - }, - ); - - const color = style.underlineColor(palette) orelse colors.fg; - - try self.cells.append(self.alloc, .{ - .mode = .fg, - .grid_col = @intCast(x), - .grid_row = @intCast(y), - .grid_width = 1, - .glyph_x = render.glyph.atlas_x, - .glyph_y = render.glyph.atlas_y, - .glyph_width = render.glyph.width, - .glyph_height = render.glyph.height, - .glyph_offset_x = render.glyph.offset_x, - .glyph_offset_y = render.glyph.offset_y, - .r = color.r, - .g = color.g, - .b = color.b, - .a = alpha, - .bg_r = bg[0], - .bg_g = bg[1], - .bg_b = bg[2], - .bg_a = bg[3], - }); - // If it's a wide cell we need to underline the right half as well. - if (cell.gridWidth() > 1 and x < self.grid_size.columns - 1) { - try self.cells.append(self.alloc, .{ - .mode = .fg, - .grid_col = @intCast(x + 1), - .grid_row = @intCast(y), - .grid_width = 1, - .glyph_x = render.glyph.atlas_x, - .glyph_y = render.glyph.atlas_y, - .glyph_width = render.glyph.width, - .glyph_height = render.glyph.height, - .glyph_offset_x = render.glyph.offset_x, - .glyph_offset_y = render.glyph.offset_y, - .r = color.r, - .g = color.g, - .b = color.b, - .a = alpha, - .bg_r = bg[0], - .bg_g = bg[1], - .bg_b = bg[2], - .bg_a = bg[3], - }); - } - } - - // If the shaper cell has a glyph, draw it. - if (shaper_cell.glyph_index) |glyph_index| glyph: { - // Render - const render = try self.font_grid.renderGlyph( - self.alloc, - shaper_run.font_index, - glyph_index, - .{ - .grid_metrics = self.grid_metrics, - .thicken = self.config.font_thicken, - }, - ); - - // If the glyph is 0 width or height, it will be invisible - // when drawn, so don't bother adding it to the buffer. - if (render.glyph.width == 0 or render.glyph.height == 0) { - break :glyph; - } - - // If we're rendering a color font, we use the color atlas - const mode: CellProgram.CellMode = switch (try fgMode( - render.presentation, - cell_pin, - )) { - .normal => .fg, - .color => .fg_color, - .constrained => .fg_constrained, - .powerline => .fg_powerline, - }; - - try self.cells.append(self.alloc, .{ - .mode = mode, - .grid_col = @intCast(x), - .grid_row = @intCast(y), - .grid_width = cell.gridWidth(), - .glyph_x = render.glyph.atlas_x, - .glyph_y = render.glyph.atlas_y, - .glyph_width = render.glyph.width, - .glyph_height = render.glyph.height, - .glyph_offset_x = render.glyph.offset_x + shaper_cell.x_offset, - .glyph_offset_y = render.glyph.offset_y + shaper_cell.y_offset, - .r = colors.fg.r, - .g = colors.fg.g, - .b = colors.fg.b, - .a = alpha, - .bg_r = bg[0], - .bg_g = bg[1], - .bg_b = bg[2], - .bg_a = bg[3], - }); - } - - if (style.flags.strikethrough) { - const render = try self.font_grid.renderGlyph( - self.alloc, - font.sprite_index, - @intFromEnum(font.Sprite.strikethrough), - .{ - .cell_width = 1, - .grid_metrics = self.grid_metrics, - }, - ); - - try self.cells.append(self.alloc, .{ - .mode = .fg, - .grid_col = @intCast(x), - .grid_row = @intCast(y), - .grid_width = 1, - .glyph_x = render.glyph.atlas_x, - .glyph_y = render.glyph.atlas_y, - .glyph_width = render.glyph.width, - .glyph_height = render.glyph.height, - .glyph_offset_x = render.glyph.offset_x, - .glyph_offset_y = render.glyph.offset_y, - .r = colors.fg.r, - .g = colors.fg.g, - .b = colors.fg.b, - .a = alpha, - .bg_r = bg[0], - .bg_g = bg[1], - .bg_b = bg[2], - .bg_a = bg[3], - }); - // If it's a wide cell we need to strike through the right half as well. - if (cell.gridWidth() > 1 and x < self.grid_size.columns - 1) { - try self.cells.append(self.alloc, .{ - .mode = .fg, - .grid_col = @intCast(x + 1), - .grid_row = @intCast(y), - .grid_width = 1, - .glyph_x = render.glyph.atlas_x, - .glyph_y = render.glyph.atlas_y, - .glyph_width = render.glyph.width, - .glyph_height = render.glyph.height, - .glyph_offset_x = render.glyph.offset_x, - .glyph_offset_y = render.glyph.offset_y, - .r = colors.fg.r, - .g = colors.fg.g, - .b = colors.fg.b, - .a = alpha, - .bg_r = bg[0], - .bg_g = bg[1], - .bg_b = bg[2], - .bg_a = bg[3], - }); - } - } - - return true; + try self.cells.append(self.alloc, .{ + .mode = mode, + .grid_col = @intCast(x), + .grid_row = @intCast(y), + .grid_width = cell.gridWidth(), + .glyph_x = render.glyph.atlas_x, + .glyph_y = render.glyph.atlas_y, + .glyph_width = render.glyph.width, + .glyph_height = render.glyph.height, + .glyph_offset_x = render.glyph.offset_x + shaper_cell.x_offset, + .glyph_offset_y = render.glyph.offset_y + shaper_cell.y_offset, + .r = color.r, + .g = color.g, + .b = color.b, + .a = alpha, + .bg_r = bg[0], + .bg_g = bg[1], + .bg_b = bg[2], + .bg_a = bg[3], + }); } /// Returns the grid size for a given screen size. This is safe to call From 57302425966fcf9d79b08433f82a9d15e60cb1e4 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Tue, 8 Oct 2024 23:47:35 -0400 Subject: [PATCH 2/8] coretext: don't emit 0 codepoints for special fonts --- src/font/shaper/coretext.zig | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/font/shaper/coretext.zig b/src/font/shaper/coretext.zig index 085f913d3..6cf494c1e 100644 --- a/src/font/shaper/coretext.zig +++ b/src/font/shaper/coretext.zig @@ -361,6 +361,12 @@ pub const Shaper = struct { self.cell_buf.clearRetainingCapacity(); try self.cell_buf.ensureTotalCapacity(self.alloc, state.codepoints.items.len); for (state.codepoints.items) |entry| { + // We use null codepoints to pad out our list so indices match + // the UTF-16 string we constructed for CoreText. We don't want + // to emit these if this is a special font, since they're not + // part of the original run. + if (entry.codepoint == 0) continue; + self.cell_buf.appendAssumeCapacity(.{ .x = @intCast(entry.cluster), .glyph_index = @intCast(entry.codepoint), From b65ccd45987a37c29710e877d2aec7bb6e0b3787 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Wed, 9 Oct 2024 15:28:01 -0400 Subject: [PATCH 3/8] test(coretext): add test for high plane padding sprite font behavior --- src/font/shaper/coretext.zig | 67 ++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/src/font/shaper/coretext.zig b/src/font/shaper/coretext.zig index 6cf494c1e..d08a93d89 100644 --- a/src/font/shaper/coretext.zig +++ b/src/font/shaper/coretext.zig @@ -1681,6 +1681,73 @@ test "shape cell attribute change" { } } +test "shape high plane sprite font codepoint" { + // While creating runs, the CoreText shaper uses `0` codepoints to + // pad its codepoint list to account for high plane characters which + // use two UTF-16 code units. This is so that, after shaping, the string + // indices can be used to find the originating codepoint / cluster. + // + // This is a problem for special (sprite) fonts, which need to be "shaped" + // by simply returning the input codepoints verbatim. We include logic to + // skip `0` codepoints when constructing the shaped run for sprite fonts, + // this test verifies that it works correctly. + + const testing = std.testing; + const alloc = testing.allocator; + + var testdata = try testShaper(alloc); + defer testdata.deinit(); + + var screen = try terminal.Screen.init(alloc, 10, 3, 0); + defer screen.deinit(); + + // U+1FB70: Vertical One Eighth Block-2 + try screen.testWriteString("\u{1FB70}"); + + var shaper = &testdata.shaper; + var it = shaper.runIterator( + testdata.grid, + &screen, + screen.pages.pin(.{ .screen = .{ .y = 0 } }).?, + null, + null, + ); + // We should get one run + const run = (try it.next(alloc)).?; + // The run state should have the UTF-16 encoding of the character. + try testing.expectEqualSlices( + u16, + &.{ 0xD83E, 0xDF70 }, + shaper.run_state.unichars.items, + ); + // The codepoint list should be padded. + try testing.expectEqualSlices( + Shaper.Codepoint, + &.{ + .{ .codepoint = 0x1FB70, .cluster = 0 }, + .{ .codepoint = 0, .cluster = 0 }, + }, + shaper.run_state.codepoints.items, + ); + // And when shape it + const cells = try shaper.shape(run); + // we should have + // - 1 cell + try testing.expectEqual(1, run.cells); + // - at position 0 + try testing.expectEqual(0, run.offset); + // - with 1 glyph in it + try testing.expectEqual(1, cells.len); + // - at position 0 + try testing.expectEqual(0, cells[0].x); + // - the glyph index should be equal to the codepoint + try testing.expectEqual(0x1FB70, cells[0].glyph_index); + // - it should be a sprite font + try testing.expect(run.font_index.special() != null); + // And we should get a null run after that + try testing.expectEqual(null, try it.next(alloc)); +} + const TestShaper = struct { alloc: Allocator, shaper: Shaper, From 0bb176d22cd18389d0524c636753efe00770a021 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Wed, 9 Oct 2024 15:53:02 -0400 Subject: [PATCH 4/8] renderer: cleanup, reduce nesting, more comments --- src/renderer/Metal.zig | 122 ++++++++++++++++++++++------------------ src/renderer/OpenGL.zig | 122 ++++++++++++++++++++++------------------ 2 files changed, 132 insertions(+), 112 deletions(-) diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig index 9340ce5d0..0665c40bc 100644 --- a/src/renderer/Metal.zig +++ b/src/renderer/Metal.zig @@ -2255,7 +2255,7 @@ fn rebuildCells( }, } - // Iterator of of runs for shaping. + // Iterator of runs for shaping. var run_iter = self.font_shaper.runIterator( self.font_grid, screen, @@ -2272,55 +2272,62 @@ fn rebuildCells( for (row_cells, 0..) |*cell, x| { // If this cell falls within our preedit range then we // skip this because preedits are setup separately. - if (preedit_range) |range| { - if (range.y == y) { - if (x >= range.x[0] and - x <= range.x[1]) - { - continue; - } + if (preedit_range) |range| preedit: { + // We're not on the preedit line, no actions necessary. + if (range.y != y) break :preedit; + // We're before the preedit range, no actions necessary. + if (x < range.x[0]) break :preedit; + // We're in the preedit range, skip this cell. + if (x <= range.x[1]) continue; + // After exiting the preedit range we need to catch + // the run position up because of the missed cells. + // In all other cases, no action is necessary. + if (x != range.x[1] + 1) break :preedit; - // After exiting the preedit range we need to catch - // the run position up because of the missed cells. - if (x == range.x[1] + 1) { - while (shaper_run) |run| { - if (run.offset + run.cells > x) break; - shaper_run = try run_iter.next(self.alloc); - shaper_cells = null; - shaper_cells_i = 0; - } - if (shaper_run) |run| { - // Try to read the cells from the shaping cache if we can. - shaper_cells = self.font_shaper_cache.get(run) orelse cache: { - // Otherwise we have to shape them. - const cells = try self.font_shaper.shape(run); + // Step the run iterator until we find a run that ends + // after the current cell, which will be the soonest run + // that might contain glyphs for our cell. + while (shaper_run) |run| { + if (run.offset + run.cells > x) break; + shaper_run = try run_iter.next(self.alloc); + shaper_cells = null; + shaper_cells_i = 0; + } - // Try to cache them. If caching fails for any reason we - // continue because it is just a performance optimization, - // not a correctness issue. - self.font_shaper_cache.put( - self.alloc, - run, - cells, - ) catch |err| { - log.warn( - "error caching font shaping results err={}", - .{err}, - ); - }; + const run = shaper_run orelse break :preedit; - // The cells we get from direct shaping are always owned - // by the shaper and valid until the next shaping call so - // we can safely use them. - break :cache cells; - }; - } - if (shaper_cells) |cells| { - while (cells[shaper_cells_i].x < x) { - shaper_cells_i += 1; - } - } - } + // If we haven't shaped this run, do so now. + shaper_cells = shaper_cells orelse + // Try to read the cells from the shaping cache if we can. + self.font_shaper_cache.get(run) orelse + cache: { + // Otherwise we have to shape them. + const cells = try self.font_shaper.shape(run); + + // Try to cache them. If caching fails for any reason we + // continue because it is just a performance optimization, + // not a correctness issue. + self.font_shaper_cache.put( + self.alloc, + run, + cells, + ) catch |err| { + log.warn( + "error caching font shaping results err={}", + .{err}, + ); + }; + + // The cells we get from direct shaping are always owned + // by the shaper and valid until the next shaping call so + // we can safely use them. + break :cache cells; + }; + + // Advance our index until we reach or pass + // our current x position in the shaper cells. + while (shaper_cells.?[shaper_cells_i].x < x) { + shaper_cells_i += 1; } } @@ -2493,10 +2500,12 @@ fn rebuildCells( shaper_cells_i = 0; } - // If we haven't shaped this run yet, do so. - if (shaper_cells == null) if (shaper_run) |run| { - // Try to read the cells from the shaping cache if we can. - shaper_cells = self.font_shaper_cache.get(run) orelse cache: { + if (shaper_run) |run| glyphs: { + // If we haven't shaped this run yet, do so. + shaper_cells = shaper_cells orelse + // Try to read the cells from the shaping cache if we can. + self.font_shaper_cache.get(run) orelse + cache: { // Otherwise we have to shape them. const cells = try self.font_shaper.shape(run); @@ -2519,12 +2528,9 @@ fn rebuildCells( // we can safely use them. break :cache cells; }; - }; - // NOTE: An assumption is made here that a single cell will never - // be present in more than one shaper run. If that assumption is - // violated, this logic breaks. - if (shaper_cells) |cells| glyphs: { + const cells = shaper_cells orelse break :glyphs; + // If there are no shaper cells for this run, ignore it. // This can occur for runs of empty cells, and is fine. if (cells.len == 0) break :glyphs; @@ -2534,6 +2540,10 @@ fn rebuildCells( // position monotonically increasing. assert(cells[shaper_cells_i].x >= x); + // NOTE: An assumption is made here that a single cell will never + // be present in more than one shaper run. If that assumption is + // violated, this logic breaks. + while (shaper_cells_i < cells.len and cells[shaper_cells_i].x == x) : ({ shaper_cells_i += 1; }) { diff --git a/src/renderer/OpenGL.zig b/src/renderer/OpenGL.zig index 54835d097..4e3184c28 100644 --- a/src/renderer/OpenGL.zig +++ b/src/renderer/OpenGL.zig @@ -1322,7 +1322,7 @@ pub fn rebuildCells( }, } - // Iterator of of runs for shaping. + // Iterator of runs for shaping. var run_iter = self.font_shaper.runIterator( self.font_grid, screen, @@ -1339,55 +1339,62 @@ pub fn rebuildCells( for (row_cells, 0..) |*cell, x| { // If this cell falls within our preedit range then we // skip this because preedits are setup separately. - if (preedit_range) |range| { - if (range.y == y) { - if (x >= range.x[0] and - x <= range.x[1]) - { - continue; - } + if (preedit_range) |range| preedit: { + // We're not on the preedit line, no actions necessary. + if (range.y != y) break :preedit; + // We're before the preedit range, no actions necessary. + if (x < range.x[0]) break :preedit; + // We're in the preedit range, skip this cell. + if (x <= range.x[1]) continue; + // After exiting the preedit range we need to catch + // the run position up because of the missed cells. + // In all other cases, no action is necessary. + if (x != range.x[1] + 1) break :preedit; - // After exiting the preedit range we need to catch - // the run position up because of the missed cells. - if (x == range.x[1] + 1) { - while (shaper_run) |run| { - if (run.offset + run.cells > x) break; - shaper_run = try run_iter.next(self.alloc); - shaper_cells = null; - shaper_cells_i = 0; - } - if (shaper_run) |run| { - // Try to read the cells from the shaping cache if we can. - shaper_cells = self.font_shaper_cache.get(run) orelse cache: { - // Otherwise we have to shape them. - const cells = try self.font_shaper.shape(run); + // Step the run iterator until we find a run that ends + // after the current cell, which will be the soonest run + // that might contain glyphs for our cell. + while (shaper_run) |run| { + if (run.offset + run.cells > x) break; + shaper_run = try run_iter.next(self.alloc); + shaper_cells = null; + shaper_cells_i = 0; + } - // Try to cache them. If caching fails for any reason we - // continue because it is just a performance optimization, - // not a correctness issue. - self.font_shaper_cache.put( - self.alloc, - run, - cells, - ) catch |err| { - log.warn( - "error caching font shaping results err={}", - .{err}, - ); - }; + const run = shaper_run orelse break :preedit; - // The cells we get from direct shaping are always owned - // by the shaper and valid until the next shaping call so - // we can safely use them. - break :cache cells; - }; - } - if (shaper_cells) |cells| { - while (cells[shaper_cells_i].x < x) { - shaper_cells_i += 1; - } - } - } + // If we haven't shaped this run, do so now. + shaper_cells = shaper_cells orelse + // Try to read the cells from the shaping cache if we can. + self.font_shaper_cache.get(run) orelse + cache: { + // Otherwise we have to shape them. + const cells = try self.font_shaper.shape(run); + + // Try to cache them. If caching fails for any reason we + // continue because it is just a performance optimization, + // not a correctness issue. + self.font_shaper_cache.put( + self.alloc, + run, + cells, + ) catch |err| { + log.warn( + "error caching font shaping results err={}", + .{err}, + ); + }; + + // The cells we get from direct shaping are always owned + // by the shaper and valid until the next shaping call so + // we can safely use them. + break :cache cells; + }; + + // Advance our index until we reach or pass + // our current x position in the shaper cells. + while (shaper_cells.?[shaper_cells_i].x < x) { + shaper_cells_i += 1; } } @@ -1587,10 +1594,12 @@ pub fn rebuildCells( shaper_cells_i = 0; } - // If we haven't shaped this run yet, do so. - if (shaper_cells == null) if (shaper_run) |run| { - // Try to read the cells from the shaping cache if we can. - shaper_cells = self.font_shaper_cache.get(run) orelse cache: { + if (shaper_run) |run| glyphs: { + // If we haven't shaped this run yet, do so. + shaper_cells = shaper_cells orelse + // Try to read the cells from the shaping cache if we can. + self.font_shaper_cache.get(run) orelse + cache: { // Otherwise we have to shape them. const cells = try self.font_shaper.shape(run); @@ -1613,12 +1622,9 @@ pub fn rebuildCells( // we can safely use them. break :cache cells; }; - }; - // NOTE: An assumption is made here that a single cell will never - // be present in more than one shaper run. If that assumption is - // violated, this logic breaks. - if (shaper_cells) |cells| glyphs: { + const cells = shaper_cells orelse break :glyphs; + // If there are no shaper cells for this run, ignore it. // This can occur for runs of empty cells, and is fine. if (cells.len == 0) break :glyphs; @@ -1628,6 +1634,10 @@ pub fn rebuildCells( // position monotonically increasing. assert(cells[shaper_cells_i].x >= x); + // NOTE: An assumption is made here that a single cell will never + // be present in more than one shaper run. If that assumption is + // violated, this logic breaks. + while (shaper_cells_i < cells.len and cells[shaper_cells_i].x == x) : ({ shaper_cells_i += 1; }) { From 7de7bfa20eb5e73628de0af4b8e696693cd77674 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Wed, 9 Oct 2024 16:00:57 -0400 Subject: [PATCH 5/8] coretext: fix tests to account for removal of null cells --- src/font/shaper/coretext.zig | 43 ++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/src/font/shaper/coretext.zig b/src/font/shaper/coretext.zig index d08a93d89..dbc9809e3 100644 --- a/src/font/shaper/coretext.zig +++ b/src/font/shaper/coretext.zig @@ -843,10 +843,10 @@ test "shape inconsolata ligs" { while (try it.next(alloc)) |run| { count += 1; + try testing.expectEqual(@as(usize, 2), run.cells); + const cells = try shaper.shape(run); - try testing.expectEqual(@as(usize, 2), cells.len); - try testing.expect(cells[0].glyph_index != null); - try testing.expect(cells[1].glyph_index == null); + try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); } @@ -868,11 +868,10 @@ test "shape inconsolata ligs" { while (try it.next(alloc)) |run| { count += 1; + try testing.expectEqual(@as(usize, 3), run.cells); + const cells = try shaper.shape(run); - try testing.expectEqual(@as(usize, 3), cells.len); - try testing.expect(cells[0].glyph_index != null); - try testing.expect(cells[1].glyph_index == null); - try testing.expect(cells[2].glyph_index == null); + try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); } @@ -902,11 +901,10 @@ test "shape monaspace ligs" { while (try it.next(alloc)) |run| { count += 1; + try testing.expectEqual(@as(usize, 3), run.cells); + const cells = try shaper.shape(run); - try testing.expectEqual(@as(usize, 3), cells.len); - try testing.expect(cells[0].glyph_index != null); - try testing.expect(cells[1].glyph_index == null); - try testing.expect(cells[2].glyph_index == null); + try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); } @@ -937,11 +935,10 @@ test "shape left-replaced lig in last run" { while (try it.next(alloc)) |run| { count += 1; + try testing.expectEqual(@as(usize, 3), run.cells); + const cells = try shaper.shape(run); - try testing.expectEqual(@as(usize, 3), cells.len); - try testing.expect(cells[0].glyph_index != null); - try testing.expect(cells[1].glyph_index == null); - try testing.expect(cells[2].glyph_index == null); + try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); } @@ -970,12 +967,11 @@ test "shape left-replaced lig in early run" { ); const run = (try it.next(alloc)).?; + + try testing.expectEqual(@as(usize, 4), run.cells); + const cells = try shaper.shape(run); - try testing.expectEqual(@as(usize, 4), cells.len); - try testing.expect(cells[0].glyph_index != null); - try testing.expect(cells[1].glyph_index == null); - try testing.expect(cells[2].glyph_index == null); - try testing.expect(cells[3].glyph_index != null); + try testing.expectEqual(@as(usize, 2), cells.len); } } @@ -1078,8 +1074,7 @@ test "shape emoji width long" { count += 1; const cells = try shaper.shape(run); - // screen.testWriteString isn't grapheme aware, otherwise this is one - try testing.expectEqual(@as(usize, 5), cells.len); + try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); } @@ -1260,9 +1255,9 @@ test "shape box glyphs" { count += 1; const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 2), cells.len); - try testing.expectEqual(@as(u32, 0x2500), cells[0].glyph_index.?); + try testing.expectEqual(@as(u32, 0x2500), cells[0].glyph_index); try testing.expectEqual(@as(u16, 0), cells[0].x); - try testing.expectEqual(@as(u32, 0x2501), cells[1].glyph_index.?); + try testing.expectEqual(@as(u32, 0x2501), cells[1].glyph_index); try testing.expectEqual(@as(u16, 1), cells[1].x); } try testing.expectEqual(@as(usize, 1), count); From ca59367164d330575b8c4a83346c7ec98f1aa340 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Wed, 9 Oct 2024 16:05:09 -0400 Subject: [PATCH 6/8] harfbuzz: fix tests to account for removal of null cells --- src/font/shaper/harfbuzz.zig | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/src/font/shaper/harfbuzz.zig b/src/font/shaper/harfbuzz.zig index 53efeec54..b3c8400b3 100644 --- a/src/font/shaper/harfbuzz.zig +++ b/src/font/shaper/harfbuzz.zig @@ -386,10 +386,10 @@ test "shape inconsolata ligs" { while (try it.next(alloc)) |run| { count += 1; + try testing.expectEqual(@as(usize, 2), run.cells); + const cells = try shaper.shape(run); - try testing.expectEqual(@as(usize, 2), cells.len); - try testing.expect(cells[0].glyph_index != null); - try testing.expect(cells[1].glyph_index == null); + try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); } @@ -411,11 +411,10 @@ test "shape inconsolata ligs" { while (try it.next(alloc)) |run| { count += 1; + try testing.expectEqual(@as(usize, 3), run.cells); + const cells = try shaper.shape(run); - try testing.expectEqual(@as(usize, 3), cells.len); - try testing.expect(cells[0].glyph_index != null); - try testing.expect(cells[1].glyph_index == null); - try testing.expect(cells[2].glyph_index == null); + try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); } @@ -445,11 +444,10 @@ test "shape monaspace ligs" { while (try it.next(alloc)) |run| { count += 1; + try testing.expectEqual(@as(usize, 3), run.cells); + const cells = try shaper.shape(run); - try testing.expectEqual(@as(usize, 3), cells.len); - try testing.expect(cells[0].glyph_index != null); - try testing.expect(cells[1].glyph_index == null); - try testing.expect(cells[2].glyph_index == null); + try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); } @@ -479,8 +477,10 @@ test "shape emoji width" { while (try it.next(alloc)) |run| { count += 1; + try testing.expectEqual(@as(usize, 2), run.cells); + const cells = try shaper.shape(run); - try testing.expectEqual(@as(usize, 2), cells.len); + try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); } @@ -522,8 +522,7 @@ test "shape emoji width long" { const cells = try shaper.shape(run); - // screen.testWriteString isn't grapheme aware, otherwise this is two - try testing.expectEqual(@as(usize, 5), cells.len); + try testing.expectEqual(@as(usize, 1), cells.len); } try testing.expectEqual(@as(usize, 1), count); } @@ -709,9 +708,9 @@ test "shape box glyphs" { try testing.expectEqual(@as(u32, 2), shaper.hb_buf.getLength()); const cells = try shaper.shape(run); try testing.expectEqual(@as(usize, 2), cells.len); - try testing.expectEqual(@as(u32, 0x2500), cells[0].glyph_index.?); + try testing.expectEqual(@as(u32, 0x2500), cells[0].glyph_index); try testing.expectEqual(@as(u16, 0), cells[0].x); - try testing.expectEqual(@as(u32, 0x2501), cells[1].glyph_index.?); + try testing.expectEqual(@as(u32, 0x2501), cells[1].glyph_index); try testing.expectEqual(@as(u16, 1), cells[1].x); } try testing.expectEqual(@as(usize, 1), count); From 97abf3c37055144aa96218896bea21154d3645ec Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 10 Oct 2024 17:20:03 -0700 Subject: [PATCH 7/8] font/noop: conform to new run struct type --- src/font/shaper/noop.zig | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/font/shaper/noop.zig b/src/font/shaper/noop.zig index 25b9a055f..f8988f4ee 100644 --- a/src/font/shaper/noop.zig +++ b/src/font/shaper/noop.zig @@ -111,7 +111,13 @@ pub const Shaper = struct { // expose a public API for this. const face = try run.grid.resolver.collection.getFace(run.font_index); for (state.codepoints.items) |entry| { - const glyph_index = face.glyphIndex(entry.codepoint); + const glyph_index = face.glyphIndex(entry.codepoint) orelse { + // The run iterator shared logic should guarantee that + // there is a glyph index for all codepoints in the run. + // This is not well tested because we don't use the noop + // shaper in any release builds. + unreachable; + }; try self.cell_buf.append(self.alloc, .{ .x = @intCast(entry.cluster), .glyph_index = glyph_index, From e4f4b708c989e68a96ed340f5ce99b0678c6f556 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 11 Oct 2024 13:44:49 -0400 Subject: [PATCH 8/8] font/shaper: explicitly skip invisible cells while shaping Fixes a bug caused by the renderer logic assuming this behavior and not advancing the run iterator when skipping foreground elements in cells with the invisible flag set. --- src/font/shaper/run.zig | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/font/shaper/run.zig b/src/font/shaper/run.zig index 8ef0f790c..22d19979e 100644 --- a/src/font/shaper/run.zig +++ b/src/font/shaper/run.zig @@ -55,6 +55,14 @@ pub const RunIterator = struct { break :max 0; }; + // Invisible cells don't have any glyphs rendered, + // so we explicitly skip them in the shaping process. + while (self.i < max and + self.row.style(&cells[self.i]).flags.invisible) + { + self.i += 1; + } + // We're over at the max if (self.i >= max) return null;