From ea551990eb1877617b329e1f9cf235e9ff0e08f2 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 3 Aug 2024 21:35:56 -0700 Subject: [PATCH] renderer: disable window-padding-color=extend in certain scenarios There are scenarios where this configuration looks bad. This commit introduces some heuristics to prevent it. Here are the heuristics: * Extension is always enabled on alt screen. * Extension is disabled if a row contains any default bg color. The thinking is that in this scenario, using the default bg color looks just fine. * Extension is disabled if a row is marked as a prompt (using semantic prompt sequences). The thinking here is that prompts often contain perfect fit glyphs such as Powerline glyphs and those look bad when extended. This introduces some CPU cost to the extension feature but it should be minimal and respects dirty tracking. This is unfortunate but the feature makes many terminal scenarios look much better and the performance cost is minimal so I believe it is worth it. Further heuristics are likely warranted but this should be a good starting set. --- pkg/opengl/Program.zig | 1 + src/config/Config.zig | 11 ++++ src/renderer/Metal.zig | 28 +++++++- src/renderer/OpenGL.zig | 110 +++++++++++++++++++++++++++++-- src/renderer/metal/shaders.zig | 4 ++ src/renderer/shaders/cell.metal | 11 +++- src/renderer/shaders/cell.v.glsl | 6 +- src/terminal/PageList.zig | 29 ++++++++ 8 files changed, 189 insertions(+), 11 deletions(-) diff --git a/pkg/opengl/Program.zig b/pkg/opengl/Program.zig index 3a2f2036a..824687e83 100644 --- a/pkg/opengl/Program.zig +++ b/pkg/opengl/Program.zig @@ -104,6 +104,7 @@ pub fn setUniform( // Perform the correct call depending on the type of the value. switch (@TypeOf(value)) { + bool => glad.context.Uniform1i.?(loc, if (value) 1 else 0), comptime_int => glad.context.Uniform1i.?(loc, value), f32 => glad.context.Uniform1f.?(loc, value), @Vector(2, f32) => glad.context.Uniform2f.?(loc, value[0], value[1]), diff --git a/src/config/Config.zig b/src/config/Config.zig index b3da3c279..3f9b7890d 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -674,6 +674,17 @@ keybind: Keybinds = .{}, /// * `background` - The background color specified in `background`. /// * `extend` - Extend the background color of the nearest grid cell. /// +/// The "extend" value will be disabled in certain scenarios. On primary +/// screen applications (i.e. not something like Neovim), the color will not +/// be extended vertically if any of the following are true: +/// +/// * The nearest row has any cells that have the default backgroudn color. +/// The thinking is that in this case, the default background color looks +/// fine as a padding color. +/// * The nearest row is a prompt row (requires shell integration). The +/// thinking here is that prompts often contain powerline glyphs that +/// do not look good extended. +/// /// The default value is "extend". This allows for smooth resizing of a /// terminal grid without having visible empty areas around the edge. The edge /// cells may appear slightly larger due to the extension. diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig index 400eba995..7df88afb9 100644 --- a/src/renderer/Metal.zig +++ b/src/renderer/Metal.zig @@ -625,6 +625,8 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal { .cell_size = undefined, .grid_size = undefined, .grid_padding = undefined, + .padding_extend_top = true, + .padding_extend_bottom = true, .min_contrast = options.config.min_contrast, .cursor_pos = .{ std.math.maxInt(u16), std.math.maxInt(u16) }, .cursor_color = undefined, @@ -872,6 +874,7 @@ pub fn updateFrame( const Critical = struct { bg: terminal.color.RGB, screen: terminal.Screen, + screen_type: terminal.ScreenType, mouse: renderer.State.Mouse, preedit: ?renderer.State.Preedit, cursor_style: ?renderer.CursorStyle, @@ -1001,6 +1004,7 @@ pub fn updateFrame( break :critical .{ .bg = self.background_color, .screen = screen_copy, + .screen_type = state.terminal.active_screen, .mouse = state.mouse, .preedit = preedit, .cursor_style = cursor_style, @@ -1018,6 +1022,7 @@ pub fn updateFrame( try self.rebuildCells( critical.full_rebuild, &critical.screen, + critical.screen_type, critical.mouse, critical.preedit, critical.cursor_style, @@ -1988,6 +1993,8 @@ pub fn setScreenSize( @floatFromInt(blank.bottom), @floatFromInt(blank.left), }, + .padding_extend_top = old.padding_extend_top, + .padding_extend_bottom = old.padding_extend_bottom, .min_contrast = old.min_contrast, .cursor_pos = old.cursor_pos, .cursor_color = old.cursor_color, @@ -2085,6 +2092,7 @@ fn rebuildCells( self: *Metal, rebuild: bool, screen: *terminal.Screen, + screen_type: terminal.ScreenType, mouse: renderer.State.Mouse, preedit: ?renderer.State.Preedit, cursor_style_: ?renderer.CursorStyle, @@ -2126,8 +2134,14 @@ fn rebuildCells( }; } else null; - // If we are doing a full rebuild, then we clear the entire cell buffer. - if (rebuild) self.cells.reset(); + if (rebuild) { + // If we are doing a full rebuild, then we clear the entire cell buffer. + self.cells.reset(); + + // We also reset our padding extension depending on the screen type + self.uniforms.padding_extend_top = screen_type == .alternate; + self.uniforms.padding_extend_bottom = screen_type == .alternate; + } // Go row-by-row to build the cells. We go row by row because we do // font shaping by row. In the future, we will also do dirty tracking @@ -2159,6 +2173,16 @@ fn rebuildCells( 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 + // good. + if (y == 0 and screen_type == .primary) { + self.uniforms.padding_extend_top = !row.neverExtendBg(); + } else if (y == self.cells.size.rows - 1 and screen_type == .primary) { + self.uniforms.padding_extend_bottom = !row.neverExtendBg(); + } + // Split our row into runs and shape each one. var iter = self.font_shaper.runIterator( self.font_grid, diff --git a/src/renderer/OpenGL.zig b/src/renderer/OpenGL.zig index e37dff83c..fc0e18390 100644 --- a/src/renderer/OpenGL.zig +++ b/src/renderer/OpenGL.zig @@ -53,12 +53,19 @@ grid_metrics: font.face.Metrics, /// Current screen size dimensions for this grid. This is set on the first /// resize event, and is not immediately available. screen_size: ?renderer.ScreenSize, +grid_size: renderer.GridSize, /// The current set of cells to render. Each set of cells goes into /// a separate shader call. cells_bg: std.ArrayListUnmanaged(CellProgram.Cell), cells: std.ArrayListUnmanaged(CellProgram.Cell), +/// The last viewport that we based our rebuild off of. If this changes, +/// then we do a full rebuild of the cells. The pointer values in this pin +/// are NOT SAFE to read because they may be modified, freed, etc from the +/// termio thread. We treat the pointers as integers for comparison only. +cells_viewport: ?terminal.Pin = null, + /// The size of the cells list that was sent to the GPU. This is used /// to detect when the cells array was reallocated/resized and handle that /// accordingly. @@ -120,6 +127,10 @@ draw_mutex: DrawMutex = drawMutexZero, /// terminal is in reversed mode. draw_background: terminal.color.RGB, +/// Whether we're doing padding extension for vertical sides. +padding_extend_top: bool = true, +padding_extend_bottom: bool = true, + /// The images that we may render. images: ImageMap = .{}, image_placements: ImagePlacementList = .{}, @@ -394,6 +405,7 @@ pub fn init(alloc: Allocator, options: renderer.Options) !OpenGL { .cells = .{}, .grid_metrics = grid.metrics, .screen_size = null, + .grid_size = .{}, .gl_state = gl_state, .font_grid = grid, .font_shaper = shaper, @@ -669,6 +681,12 @@ pub fn setFontGrid(self: *OpenGL, grid: *font.SharedGrid) void { self.font_shaper_cache.deinit(self.alloc); self.font_shaper_cache = font_shaper_cache; + // Update our grid size if we have a screen size. If we don't, its okay + // because this will get set when we get the screen size set. + if (self.screen_size) |size| { + self.grid_size = self.gridSize(size); + } + // Defer our GPU updates self.deferred_font_size = .{ .metrics = grid.metrics }; } @@ -684,8 +702,10 @@ pub fn updateFrame( // Data we extract out of the critical area. const Critical = struct { + full_rebuild: bool, gl_bg: terminal.color.RGB, screen: terminal.Screen, + screen_type: terminal.ScreenType, mouse: renderer.State.Mouse, preedit: ?renderer.State.Preedit, cursor_style: ?renderer.CursorStyle, @@ -715,6 +735,9 @@ pub fn updateFrame( self.foreground_color = bg; } + // Get the viewport pin so that we can compare it to the current. + const viewport_pin = state.terminal.screen.pages.pin(.{ .viewport = .{} }).?; + // We used to share terminal state, but we've since learned through // analysis that it is faster to copy the terminal state than to // hold the lock wile rebuilding GPU cells. @@ -758,9 +781,55 @@ pub fn updateFrame( try self.prepKittyGraphics(state.terminal); } + // If we have any terminal dirty flags set then we need to rebuild + // the entire screen. This can be optimized in the future. + const full_rebuild: bool = rebuild: { + { + const Int = @typeInfo(terminal.Terminal.Dirty).Struct.backing_integer.?; + const v: Int = @bitCast(state.terminal.flags.dirty); + if (v > 0) break :rebuild true; + } + { + const Int = @typeInfo(terminal.Screen.Dirty).Struct.backing_integer.?; + const v: Int = @bitCast(state.terminal.screen.dirty); + if (v > 0) break :rebuild true; + } + + // If our viewport changed then we need to rebuild the entire + // screen because it means we scrolled. If we have no previous + // viewport then we must rebuild. + const prev_viewport = self.cells_viewport orelse break :rebuild true; + if (!prev_viewport.eql(viewport_pin)) break :rebuild true; + + break :rebuild false; + }; + + // Reset the dirty flags in the terminal and screen. We assume + // that our rebuild will be successful since so we optimize for + // success and reset while we hold the lock. This is much easier + // than coordinating row by row or as changes are persisted. + state.terminal.flags.dirty = .{}; + state.terminal.screen.dirty = .{}; + { + var it = state.terminal.screen.pages.pageIterator( + .right_down, + .{ .screen = .{} }, + null, + ); + while (it.next()) |chunk| { + var dirty_set = chunk.page.data.dirtyBitSet(); + dirty_set.unsetAll(); + } + } + + // Update our viewport pin for dirty tracking + self.cells_viewport = viewport_pin; + break :critical .{ + .full_rebuild = full_rebuild, .gl_bg = self.background_color, .screen = screen_copy, + .screen_type = state.terminal.active_screen, .mouse = state.mouse, .preedit = preedit, .cursor_style = cursor_style, @@ -782,7 +851,9 @@ pub fn updateFrame( // Build our GPU cells try self.rebuildCells( + critical.full_rebuild, &critical.screen, + critical.screen_type, critical.mouse, critical.preedit, critical.cursor_style, @@ -1089,7 +1160,9 @@ fn prepKittyImage( /// the renderer will do this when it needs more memory space. pub fn rebuildCells( self: *OpenGL, + rebuild: bool, screen: *terminal.Screen, + screen_type: terminal.ScreenType, mouse: renderer.State.Mouse, preedit: ?renderer.State.Preedit, cursor_style_: ?renderer.CursorStyle, @@ -1135,6 +1208,12 @@ pub fn rebuildCells( // remains visible. var cursor_cell: ?CellProgram.Cell = null; + if (rebuild) { + // We also reset our padding extension depending on the screen type + self.padding_extend_top = screen_type == .alternate; + self.padding_extend_bottom = screen_type == .alternate; + } + // Build each cell var row_it = screen.pages.rowIterator(.right_down, .{ .viewport = .{} }, null); var y: terminal.size.CellCountInt = 0; @@ -1185,6 +1264,16 @@ pub fn rebuildCells( } }; + // 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 + // good. + if (y == 0 and screen_type == .primary) { + self.padding_extend_top = !row.neverExtendBg(); + } else if (y == self.grid_size.rows - 1 and screen_type == .primary) { + self.padding_extend_bottom = !row.neverExtendBg(); + } + // Split our row into runs and shape each one. var iter = self.font_shaper.runIterator( self.font_grid, @@ -1787,13 +1876,11 @@ pub fn setScreenSize( // Store our screen size self.screen_size = dim; self.padding.explicit = pad; - - // Recalculate the rows/columns. - const grid_size = self.gridSize(dim); + self.grid_size = self.gridSize(dim); log.debug("screen size screen={} grid={} cell={} padding={}", .{ dim, - grid_size, + self.grid_size, renderer.CellSize{ .width = self.grid_metrics.cell_width, .height = self.grid_metrics.cell_height, @@ -1996,6 +2083,21 @@ fn drawCellProgram( self.deferred_config = null; } + // Apply our padding extension fields + { + const program = gl_state.cell_program; + const bind = try program.program.use(); + defer bind.unbind(); + try program.program.setUniform( + "padding_vertical_top", + self.padding_extend_top, + ); + try program.program.setUniform( + "padding_vertical_bottom", + self.padding_extend_bottom, + ); + } + // Draw background images first try self.drawImages( gl_state, diff --git a/src/renderer/metal/shaders.zig b/src/renderer/metal/shaders.zig index 5a9be3b73..082c5ba5b 100644 --- a/src/renderer/metal/shaders.zig +++ b/src/renderer/metal/shaders.zig @@ -124,6 +124,10 @@ pub const Uniforms = extern struct { /// top, right, bottom, left. grid_padding: [4]f32 align(16), + /// True if vertical padding gets the extended color of the nearest row. + padding_extend_top: bool align(1), + padding_extend_bottom: bool align(1), + /// The minimum contrast ratio for text. The contrast ratio is calculated /// according to the WCAG 2.0 spec. min_contrast: f32 align(4), diff --git a/src/renderer/shaders/cell.metal b/src/renderer/shaders/cell.metal index bfe55aade..df2f79de4 100644 --- a/src/renderer/shaders/cell.metal +++ b/src/renderer/shaders/cell.metal @@ -5,6 +5,8 @@ struct Uniforms { float2 cell_size; ushort2 grid_size; float4 grid_padding; + bool padding_extend_top; + bool padding_extend_bottom; float min_contrast; ushort2 cursor_pos; uchar4 cursor_color; @@ -101,11 +103,14 @@ vertex CellBgVertexOut cell_bg_vertex(unsigned int vid [[vertex_id]], cell_size_scaled.x = cell_size_scaled.x * input.cell_width; // If we're at the edge of the grid, we add our padding to the background - // to extend it. Note: grid_padding is top/right/bottom/left. - if (input.grid_pos.y == 0) { + // to extend it. Note: grid_padding is top/right/bottom/left. We always + // extend horiziontally because there is no downside but there are various + // heuristics to disable vertical extension. + if (input.grid_pos.y == 0 && uniforms.padding_extend_top) { cell_pos.y -= uniforms.grid_padding.r; cell_size_scaled.y += uniforms.grid_padding.r; - } else if (input.grid_pos.y == uniforms.grid_size.y - 1) { + } else if (input.grid_pos.y == uniforms.grid_size.y - 1 && + uniforms.padding_extend_bottom) { cell_size_scaled.y += uniforms.grid_padding.b; } if (input.grid_pos.x == 0) { diff --git a/src/renderer/shaders/cell.v.glsl b/src/renderer/shaders/cell.v.glsl index 9b2b90026..3c533f2ad 100644 --- a/src/renderer/shaders/cell.v.glsl +++ b/src/renderer/shaders/cell.v.glsl @@ -57,6 +57,8 @@ uniform sampler2D text_color; uniform vec2 cell_size; uniform vec2 grid_size; uniform vec4 grid_padding; +uniform bool padding_vertical_top; +uniform bool padding_vertical_bottom; uniform mat4 projection; uniform float min_contrast; @@ -171,10 +173,10 @@ void main() { case MODE_BG: // If we're at the edge of the grid, we add our padding to the background // to extend it. Note: grid_padding is top/right/bottom/left. - if (grid_coord.y == 0) { + if (grid_coord.y == 0 && padding_vertical_top) { cell_pos.y -= grid_padding.r; cell_size_scaled.y += grid_padding.r; - } else if (grid_coord.y == grid_size.y - 1) { + } else if (grid_coord.y == grid_size.y - 1 && padding_vertical_bottom) { cell_size_scaled.y += grid_padding.b; } if (grid_coord.x == 0) { diff --git a/src/terminal/PageList.zig b/src/terminal/PageList.zig index 64b741f2c..442cee7e8 100644 --- a/src/terminal/PageList.zig +++ b/src/terminal/PageList.zig @@ -3254,6 +3254,35 @@ pub const Pin = struct { set.set(self.y); } + /// Returns true if the row of this pin should never have its background + /// color extended for filling padding space in the renderer. This is + /// a set of heuristics that help making our padding look better. + pub fn neverExtendBg(self: Pin) bool { + // Any semantic prompts should not have their background extended + // because prompts often contain special formatting (such as + // powerline) that looks bad when extended. + const rac = self.rowAndCell(); + switch (rac.row.semantic_prompt) { + .prompt, .prompt_continuation, .input => return true, + .unknown, .command => {}, + } + + for (self.cells(.all)) |*cell| { + // If any cell has a default background color then we don't + // extend because the default background color probably looks + // good enough as an extension. + switch (cell.content_tag) { + .bg_color_palette, .bg_color_rgb => {}, + .codepoint, .codepoint_grapheme => { + const s = self.style(cell); + if (s.bg_color == .none) return true; + }, + } + } + + return false; + } + /// Iterators. These are the same as PageList iterator funcs but operate /// on pins rather than points. This is MUCH more efficient than calling /// pointFromPin and building up the iterator from points.