Merge pull request #1733 from mitchellh/dirty-fixes

Track dirty bits in more places

src/renderer/Metal.zig

@@ -98,10 +98,11 @@ current_background_color: terminal.color.RGB,
 /// cells goes into a separate shader.
 cells: mtl_cell.Contents,
 
-/// If this is true, it forces a full cell contents rebuild on the next frame.
+/// The last viewport that we based our rebuild off of. If this changes,
-/// This can be used to force a rebuild whenever internal state changes for
+/// then we do a full rebuild of the cells. The pointer values in this pin
-/// our cells structures (i.e. a resize).
+/// are NOT SAFE to read because they may be modified, freed, etc from the
-cells_rebuild: bool = true,
+/// termio thread. We treat the pointers as integers for comparison only.
+cells_viewport: ?terminal.Pin = null,
 
 /// Set to true after rebuildCells is called. This can be used
 /// to determine if any possible changes have been made to the
@@ -844,6 +845,7 @@ pub fn updateFrame(
         preedit: ?renderer.State.Preedit,
         cursor_style: ?renderer.CursorStyle,
         color_palette: terminal.color.Palette,
+        viewport_pin: terminal.Pin,
 
         /// If true, rebuild the full screen.
         full_rebuild: bool,
@@ -883,6 +885,9 @@ pub fn updateFrame(
             return;
         }
 
+        // 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 while rebuilding GPU cells.
@@ -929,8 +934,11 @@ pub fn updateFrame(
                 if (v > 0) break :rebuild true;
             }
 
-            // Temporary while: https://github.com/mitchellh/ghostty/issues/1731
+            // If our viewport changed then we need to rebuild the entire
-            if (true) break :rebuild true;
+            // 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;
         };
@@ -960,6 +968,7 @@ pub fn updateFrame(
             .preedit = preedit,
             .cursor_style = cursor_style,
             .color_palette = state.terminal.color_palette.colors,
+            .viewport_pin = viewport_pin,
             .full_rebuild = full_rebuild,
         };
     };
@@ -970,7 +979,7 @@ pub fn updateFrame(
 
     // Build our GPU cells
     try self.rebuildCells(
-        critical.full_rebuild or self.cells_rebuild,
+        critical.full_rebuild,
         &critical.screen,
         critical.mouse,
         critical.preedit,
@@ -978,6 +987,9 @@ pub fn updateFrame(
         &critical.color_palette,
     );
 
+    // Update our viewport pin
+    self.cells_viewport = critical.viewport_pin;
+
     // Update our background color
     self.current_background_color = critical.bg;
 
@@ -1784,7 +1796,9 @@ pub fn setScreenSize(
 
     // Reset our cell contents.
     try self.cells.resize(self.alloc, grid_size);
-    self.cells_rebuild = true;
+
+    // Reset our viewport to force a rebuild
+    self.cells_viewport = null;
 
     // If we have custom shaders then we update the state
     if (self.custom_shader_state) |*state| {
@@ -2043,7 +2057,6 @@ fn rebuildCells(
 
     // Update that our cells rebuilt
     self.cells_rebuilt = true;
-    self.cells_rebuild = false;
 
     // Log some things
     // log.debug("rebuildCells complete cached_runs={}", .{

src/terminal/PageList.zig

@@ -473,6 +473,11 @@ pub fn clone(
             continue;
         }
 
+        // We want to maintain the dirty bits from the original page so
+        // instead of setting a range we grab the dirty bit and then
+        // set it on the new page in the new location.
+        var dirty = page.data.dirtyBitSet();
+
         // Kind of slow, we want to shift the rows up in the page up to
         // end and then resize down.
         const rows = page.data.rows.ptr(page.data.memory);
@@ -483,6 +488,7 @@ pub fn clone(
             const old_dst = dst.*;
             dst.* = src.*;
             src.* = old_dst;
+            dirty.setValue(i, dirty.isSet(i + chunk.start));
         }
         page.data.size.rows = @intCast(len);
         total_rows += len;
@@ -2023,6 +2029,12 @@ pub fn eraseRow(
         }
     }
 
+    {
+        // Set all the rows as dirty in this page
+        var dirty = page.data.dirtyBitSet();
+        dirty.setRangeValue(.{ .start = pn.y, .end = page.data.size.rows }, true);
+    }
+
     // We iterate through all of the following pages in order to move their
     // rows up by 1 as well.
     while (page.next) |next| {
@@ -2054,6 +2066,10 @@ pub fn eraseRow(
 
         fastmem.rotateOnce(Row, rows[0..page.data.size.rows]);
 
+        // Set all the rows as dirty
+        var dirty = page.data.dirtyBitSet();
+        dirty.setRangeValue(.{ .start = 0, .end = page.data.size.rows }, true);
+
         // Our tracked pins for this page need to be updated.
         // If the pin is in row 0 that means the corresponding row has
         // been moved to the previous page. Otherwise, move it up by 1.
@@ -2306,6 +2322,10 @@ pub fn eraseRows(
         // Our new size is the amount we scrolled
         chunk.page.data.size.rows = @intCast(scroll_amount);
         erased += chunk.end;
+
+        // Set all the rows as dirty
+        var dirty = chunk.page.data.dirtyBitSet();
+        dirty.setRangeValue(.{ .start = 0, .end = chunk.page.data.size.rows }, true);
     }
 
     // If we deleted active, we need to regrow because one of our invariants

src/terminal/Screen.zig

@@ -565,10 +565,17 @@ pub fn cursorDownScroll(self: *Screen) !void {
                 self.cursor.page_row,
                 self.cursor.page_pin.page.data.getCells(self.cursor.page_row),
             );
+
+            var dirty = self.cursor.page_pin.page.data.dirtyBitSet();
+            dirty.set(0);
         } else {
             // eraseRow will shift everything below it up.
             try self.pages.eraseRow(.{ .active = .{} });
 
+            // Note we don't need to mark anything dirty in this branch
+            // because eraseRow will mark all the rotated rows as dirty
+            // in the entire page.
+
             // We need to move our cursor down one because eraseRows will
             // preserve our pin directly and we're erasing one row.
             const page_pin = self.cursor.page_pin.down(1).?;
@@ -626,6 +633,9 @@ pub fn cursorDownScroll(self: *Screen) !void {
         self.cursor.page_row = page_rac.row;
         self.cursor.page_cell = page_rac.cell;
 
+        // Our new row is always dirty
+        self.cursorMarkDirty();
+
         // Clear the new row so it gets our bg color. We only do this
         // if we have a bg color at all.
         if (self.cursor.style.bg_color != .none) {
@@ -2882,6 +2892,7 @@ test "Screen: cursorAbsolute across pages preserves style" {
         try testing.expect(styleval.flags.bold);
     }
 }
+
 test "Screen: scrolling" {
     const testing = std.testing;
     const alloc = testing.allocator;
@@ -2909,6 +2920,11 @@ test "Screen: scrolling" {
         }, cell.content.color_rgb);
     }
 
+    // Everything is dirty because we have no scrollback
+    try testing.expect(s.pages.isDirty(.{ .active = .{ .x = 0, .y = 0 } }));
+    try testing.expect(s.pages.isDirty(.{ .active = .{ .x = 0, .y = 1 } }));
+    try testing.expect(s.pages.isDirty(.{ .active = .{ .x = 0, .y = 2 } }));
+
     // Scrolling to the bottom does nothing
     s.scroll(.{ .active = {} });
 
@@ -2934,6 +2950,9 @@ test "Screen: scrolling with a single-row screen no scrollback" {
         defer alloc.free(contents);
         try testing.expectEqualStrings("", contents);
     }
+
+    // Screen should be dirty
+    try testing.expect(s.pages.isDirty(.{ .active = .{ .x = 0, .y = 0 } }));
 }
 
 test "Screen: scrolling with a single-row screen with scrollback" {
@@ -2952,6 +2971,12 @@ test "Screen: scrolling with a single-row screen with scrollback" {
         try testing.expectEqualStrings("", contents);
     }
 
+    // Active should be dirty
+    try testing.expect(s.pages.isDirty(.{ .active = .{ .x = 0, .y = 0 } }));
+
+    // Our scrollback should not be dirty
+    try testing.expect(!s.pages.isDirty(.{ .screen = .{ .x = 0, .y = 0 } }));
+
     s.scroll(.{ .delta_row = -1 });
     {
         const contents = try s.dumpStringAlloc(alloc, .{ .viewport = .{} });