From dafabe3296c7063b6b85118d6aa288d5661cbe5b Mon Sep 17 00:00:00 2001
From: Qwerasd <qwerasd205@users.noreply.github.com>
Date: Mon, 6 May 2024 22:40:24 -0400
Subject: [PATCH 1/5] renderer/Metal: improve cell contents tracking

Previous version prevented multiple glyphs from belonging to the same coordinate, which broke quite a few things. This implementation fixes that (and may be more efficient too). Needs clean-up.
---
 src/renderer/Metal.zig      |  24 +-
 src/renderer/metal/cell.zig | 631 +++++++++++++++++++-----------------
 2 files changed, 341 insertions(+), 314 deletions(-)

diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig
index 5395554df..efc52beee 100644
--- a/src/renderer/Metal.zig
+++ b/src/renderer/Metal.zig
@@ -2171,7 +2171,7 @@ fn updateCell(
             break :bg_alpha @intFromFloat(bg_alpha);
         };
 
-        try self.cells.set(self.alloc, .bg, .{
+        try self.cells.add(self.alloc, .bg, .{
             .mode = .rgb,
             .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) },
             .cell_width = cell.gridWidth(),
@@ -2186,19 +2186,25 @@ fn updateCell(
         @intFromFloat(@max(0, @min(255, @round(self.config.background_opacity * 255)))),
     };
 
-    // If the cell has a character, draw it
-    if (cell.hasText()) fg: {
+    // 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,
-            shaper_cell.glyph_index orelse break :fg,
+            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,
@@ -2208,7 +2214,7 @@ fn updateCell(
             .constrained => .fg_constrained,
         };
 
-        try self.cells.set(self.alloc, .text, .{
+        try self.cells.add(self.alloc, .text, .{
             .mode = mode,
             .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) },
             .cell_width = cell.gridWidth(),
@@ -2245,7 +2251,7 @@ fn updateCell(
 
         const color = style.underlineColor(palette) orelse colors.fg;
 
-        try self.cells.set(self.alloc, .underline, .{
+        try self.cells.add(self.alloc, .underline, .{
             .mode = .fg,
             .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) },
             .cell_width = cell.gridWidth(),
@@ -2268,7 +2274,7 @@ fn updateCell(
             },
         );
 
-        try self.cells.set(self.alloc, .strikethrough, .{
+        try self.cells.add(self.alloc, .strikethrough, .{
             .mode = .fg,
             .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) },
             .cell_width = cell.gridWidth(),
@@ -2366,7 +2372,7 @@ fn addPreeditCell(
     };
 
     // Add our opaque background cell
-    try self.cells.set(self.alloc, .bg, .{
+    try self.cells.add(self.alloc, .bg, .{
         .mode = .rgb,
         .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) },
         .cell_width = if (cp.wide) 2 else 1,
@@ -2374,7 +2380,7 @@ fn addPreeditCell(
     });
 
     // Add our text
-    try self.cells.set(self.alloc, .text, .{
+    try self.cells.add(self.alloc, .text, .{
         .mode = .fg,
         .grid_pos = .{ @intCast(coord.x), @intCast(coord.y) },
         .cell_width = if (cp.wide) 2 else 1,
diff --git a/src/renderer/metal/cell.zig b/src/renderer/metal/cell.zig
index c0d7f56a3..239ac14d5 100644
--- a/src/renderer/metal/cell.zig
+++ b/src/renderer/metal/cell.zig
@@ -47,17 +47,80 @@ pub const Key = enum {
 ///      every frame and should be as fast as possible.
 ///
 /// To achieve this, the contents are stored in contiguous arrays by
-/// GPU vertex type and we have an array of mappings indexed by coordinate
+/// GPU vertex type and we have an array of mappings indexed per row
 /// that map to the index in the GPU vertex array that the content is at.
 pub const Contents = struct {
-    /// The map contains the mapping of cell content for every cell in the
-    /// terminal to the index in the cells array that the content is at.
-    /// This is ALWAYS sized to exactly (rows * cols) so we want to keep
-    /// this as small as possible.
-    ///
-    /// Before any operation, this must be initialized by calling resize
-    /// on the contents.
-    map: []Map,
+    const Map = struct {
+        /// The rows of index mappings are stored in a single contiguous array
+        /// where the start of each row can be direct indexed by its y coord,
+        /// and the used length of each row's section is stored separately.
+        rows: []u32,
+
+        /// The used length for each row.
+        lens: []u16,
+
+        /// The size of each row in the contiguous rows array.
+        row_size: u16,
+
+        pub fn init(alloc: Allocator, size: renderer.GridSize) !Map {
+            var map: Map = .{
+                .rows = try alloc.alloc(u32, size.columns * size.rows),
+                .lens = try alloc.alloc(u16, size.rows),
+                .row_size = size.columns,
+            };
+
+            map.reset();
+
+            return map;
+        }
+
+        pub fn deinit(self: *Map, alloc: Allocator) void {
+            alloc.free(self.rows);
+            alloc.free(self.lens);
+        }
+
+        /// Clear all rows in this map.
+        pub fn reset(self: *Map) void {
+            @memset(self.lens, 0);
+        }
+
+        /// Add a mapped index to a row.
+        pub fn add(self: *Map, row: u16, idx: u32) void {
+            assert(row < self.lens.len);
+
+            const start = self.row_size * row;
+            assert(start < self.rows.len);
+
+            // TODO: Currently this makes the assumption that a given row
+            // will never contain more cells than it has columns. That
+            // assumption is easily violated due to graphemes and multiple-
+            // substitution opentype operations. Currently I've just capped
+            // the length so that additional cells will overwrite the last
+            // one once the row size is exceeded. A better behavior should
+            // be decided upon, this one could cause issues.
+            const len = @min(self.row_size - 1, self.lens[row]);
+            assert(len < self.row_size);
+
+            self.rows[start + len] = idx;
+            self.lens[row] = len + 1;
+        }
+
+        /// Get a slice containing all the mappings for a given row.
+        pub fn getRow(self: *Map, row: u16) []u32 {
+            assert(row < self.lens.len);
+
+            const start = self.row_size * row;
+            assert(start < self.rows.len);
+
+            return self.rows[start..][0..self.lens[row]];
+        }
+
+        /// Clear a given row by resetting its len.
+        pub fn clearRow(self: *Map, row: u16) void {
+            assert(row < self.lens.len);
+            self.lens[row] = 0;
+        }
+    };
 
     /// The grid size of the terminal. This is used to determine the
     /// map array index from a coordinate.
@@ -71,6 +134,15 @@ pub const Contents = struct {
     bgs: std.ArrayListUnmanaged(mtl_shaders.CellBg),
     text: std.ArrayListUnmanaged(mtl_shaders.CellText),
 
+    /// The map for the bg cells.
+    bg_map: Map,
+    /// The map for the text cells.
+    tx_map: Map,
+    /// The map for the underline cells.
+    ul_map: Map,
+    /// The map for the strikethrough cells.
+    st_map: Map,
+
     /// True when the cursor should be rendered. This is managed by
     /// the setCursor method and should not be set directly.
     cursor: bool,
@@ -80,14 +152,14 @@ pub const Contents = struct {
     const text_reserved_len = 1;
 
     pub fn init(alloc: Allocator) !Contents {
-        const map = try alloc.alloc(Map, 0);
-        errdefer alloc.free(map);
-
         var result: Contents = .{
-            .map = map,
             .size = .{ .rows = 0, .columns = 0 },
             .bgs = .{},
             .text = .{},
+            .bg_map = try Map.init(alloc, .{ .rows = 0, .columns = 0 }),
+            .tx_map = try Map.init(alloc, .{ .rows = 0, .columns = 0 }),
+            .ul_map = try Map.init(alloc, .{ .rows = 0, .columns = 0 }),
+            .st_map = try Map.init(alloc, .{ .rows = 0, .columns = 0 }),
             .cursor = false,
         };
 
@@ -101,9 +173,12 @@ pub const Contents = struct {
     }
 
     pub fn deinit(self: *Contents, alloc: Allocator) void {
-        alloc.free(self.map);
         self.bgs.deinit(alloc);
         self.text.deinit(alloc);
+        self.bg_map.deinit(alloc);
+        self.tx_map.deinit(alloc);
+        self.ul_map.deinit(alloc);
+        self.st_map.deinit(alloc);
     }
 
     /// Resize the cell contents for the given grid size. This will
@@ -113,22 +188,30 @@ pub const Contents = struct {
         alloc: Allocator,
         size: renderer.GridSize,
     ) !void {
-        const map = try alloc.alloc(Map, size.rows * size.columns);
-        errdefer alloc.free(map);
-        @memset(map, .{});
-
-        alloc.free(self.map);
-        self.map = map;
         self.size = size;
         self.bgs.clearAndFree(alloc);
         self.text.shrinkAndFree(alloc, text_reserved_len);
+
+        self.bg_map.deinit(alloc);
+        self.tx_map.deinit(alloc);
+        self.ul_map.deinit(alloc);
+        self.st_map.deinit(alloc);
+
+        self.bg_map = try Map.init(alloc, size);
+        self.tx_map = try Map.init(alloc, size);
+        self.ul_map = try Map.init(alloc, size);
+        self.st_map = try Map.init(alloc, size);
     }
 
     /// Reset the cell contents to an empty state without resizing.
     pub fn reset(self: *Contents) void {
-        @memset(self.map, .{});
         self.bgs.clearRetainingCapacity();
         self.text.shrinkRetainingCapacity(text_reserved_len);
+
+        self.bg_map.reset();
+        self.tx_map.reset();
+        self.ul_map.reset();
+        self.st_map.reset();
     }
 
     /// Returns the slice of fg cell contents to sync with the GPU.
@@ -154,325 +237,263 @@ pub const Contents = struct {
         self.text.items[0] = cell;
     }
 
-    /// Get the cell contents for the given type and coordinate.
-    pub fn get(
-        self: *const Contents,
-        comptime key: Key,
-        coord: terminal.Coordinate,
-    ) ?key.CellType() {
-        const mapping = self.map[self.index(coord)].array.get(key);
-        if (!mapping.set) return null;
-        return switch (key) {
-            .bg => self.bgs.items[mapping.index],
-
-            .text,
-            .underline,
-            .strikethrough,
-            => self.text.items[mapping.index],
-        };
-    }
-
-    /// Set the cell contents for a given type of content at a given
-    /// coordinate (provided by the celll contents).
-    pub fn set(
+    /// Add a cell to the appropriate list. Adding the same cell twice will
+    /// result in duplication in the vertex buffer. The caller should clear
+    /// the corresponding row with Contents.clear to remove old cells first.
+    pub fn add(
         self: *Contents,
         alloc: Allocator,
         comptime key: Key,
         cell: key.CellType(),
     ) !void {
-        const mapping = self.map[
-            self.index(.{
-                .x = cell.grid_pos[0],
-                .y = cell.grid_pos[1],
-            })
-        ].array.getPtr(key);
-
         // Get our list of cells based on the key (comptime).
         const list = &@field(self, switch (key) {
             .bg => "bgs",
             .text, .underline, .strikethrough => "text",
         });
 
-        // If this content type is already set on this cell, we can
-        // simply update the pre-existing index in the list to the new
-        // contents.
-        if (mapping.set) {
-            list.items[mapping.index] = cell;
-            return;
-        }
-
-        // Otherwise we need to append the new cell to the list.
-        const idx: u31 = @intCast(list.items.len);
+        // Add a new cell to the list.
+        const idx: u32 = @intCast(list.items.len);
         try list.append(alloc, cell);
-        mapping.* = .{ .set = true, .index = idx };
+
+        // And to the appropriate mapping.
+        self.getMap(key).add(cell.grid_pos[1], idx);
     }
 
     /// Clear all of the cell contents for a given row.
-    ///
-    /// Due to the way this works internally, it is best to clear rows
-    /// from the bottom up. This is because when we clear a row, we
-    /// swap remove the last element in the list and then update the
-    /// mapping for the swapped element. If we clear from the top down,
-    /// then we would have to update the mapping for every element in
-    /// the list. If we clear from the bottom up, then we only have to
-    /// update the mapping for the last element in the list.
     pub fn clear(self: *Contents, y: terminal.size.CellCountInt) void {
-        const start_idx = self.index(.{ .x = 0, .y = y });
-        const end_idx = start_idx + self.size.columns;
-        const maps = self.map[start_idx..end_idx];
-        for (0..self.size.columns) |x| {
-            // It is better to clear from the right left due to the same
-            // reasons noted for bottom-up clearing in the doc comment.
-            const rev_x = self.size.columns - x - 1;
-            const map = &maps[rev_x];
+        inline for (std.meta.fields(Key)) |field| {
+            const key: Key = @enumFromInt(field.value);
+            // Get our list of cells based on the key (comptime).
+            const list = &@field(self, switch (key) {
+                .bg => "bgs",
+                .text, .underline, .strikethrough => "text",
+            });
 
-            var it = map.array.iterator();
-            while (it.next()) |entry| {
-                if (!entry.value.set) continue;
+            const map = self.getMap(key);
 
-                // This value is no longer set
-                entry.value.set = false;
+            const start = y * map.row_size;
 
-                // Remove the value at index. This does a "swap remove"
-                // which swaps the last element in to this place. This is
-                // important because after this we need to update the mapping
-                // for the swapped element.
-                const original_index = entry.value.index;
-                const coord_: ?terminal.Coordinate = switch (entry.key) {
-                    .bg => bg: {
-                        _ = self.bgs.swapRemove(original_index);
-                        if (self.bgs.items.len == original_index) break :bg null;
-                        const new = self.bgs.items[original_index];
-                        break :bg .{ .x = new.grid_pos[0], .y = new.grid_pos[1] };
-                    },
+            // We iterate from the end of the row because this makes it more
+            // likely that we remove from the end of the list, which results
+            // in not having to re-map anything.
+            while (map.lens[y] > 0) {
+                map.lens[y] -= 1;
+                const i = start + map.lens[y];
+                const idx = map.rows[i];
 
-                    .text,
-                    .underline,
-                    .strikethrough,
-                    => text: {
-                        _ = self.text.swapRemove(original_index);
-                        if (self.text.items.len == original_index) break :text null;
-                        const new = self.text.items[original_index];
-                        break :text .{ .x = new.grid_pos[0], .y = new.grid_pos[1] };
-                    },
-                };
+                _ = list.swapRemove(idx);
 
-                // If we have the coordinate of the swapped element, then
-                // we need to update it to point at its new index, which is
-                // the index of the element we just removed.
-                //
-                // The reason we wouldn't have a coordinate is if we are
-                // removing the last element in the array, then nothing
-                // is swapped in and nothing needs to be updated.
-                if (coord_) |coord| {
-                    const old_index = switch (entry.key) {
-                        .bg => self.bgs.items.len,
-                        .text, .underline, .strikethrough => self.text.items.len,
-                    };
-                    var old_it = self.map[self.index(coord)].array.iterator();
-                    while (old_it.next()) |old_entry| {
-                        if (old_entry.value.set and
-                            old_entry.value.index == old_index and
-                            entry.key.sharedData(old_entry.key))
-                        {
-                            old_entry.value.index = original_index;
-                            break;
-                        }
-                    }
+                // If we took this cell off the end of the arraylist then
+                // we won't need to re-map anything.
+                if (idx == list.items.len) continue;
+
+                const new = list.items[idx];
+                const new_y = new.grid_pos[1];
+
+                // The cell contents that were moved need to be remapped so
+                // we don't lose track of them.
+                switch (key) {
+                    .bg => self.remapBgs(new_y, idx),
+                    .text, .underline, .strikethrough => self.remapText(new_y, idx),
                 }
             }
         }
     }
 
-    fn index(self: *const Contents, coord: terminal.Coordinate) usize {
-        return coord.y * self.size.columns + coord.x;
+    fn remapText(self: *Contents, row: u16, idx: u32) void {
+        for (self.tx_map.getRow(row)) |*new_idx| {
+            if (new_idx.* == self.text.items.len) {
+                new_idx.* = idx;
+                return;
+            }
+        }
+        for (self.ul_map.getRow(row)) |*new_idx| {
+            if (new_idx.* == self.text.items.len) {
+                new_idx.* = idx;
+                return;
+            }
+        }
+        for (self.st_map.getRow(row)) |*new_idx| {
+            if (new_idx.* == self.text.items.len) {
+                new_idx.* = idx;
+                return;
+            }
+        }
     }
 
-    /// The mapping of a cell at a specific coordinate to the index in the
-    /// vertex arrays where the cell content is at, if it is set.
-    const Map = struct {
-        /// The set of cell content mappings for a given cell for every
-        /// possible key. This is used to determine if a cell has a given
-        /// type of content (i.e. an underlyine styling) and if so what index
-        /// in the cells array that content is at.
-        const Array = std.EnumArray(Key, Mapping);
-
-        /// The mapping for a given key consists of a bit indicating if the
-        /// content is set and the index in the cells array that the content
-        /// is at. We pack this into a 32-bit integer so we only use 4 bytes
-        /// per possible cell content type.
-        const Mapping = packed struct(u32) {
-            set: bool = false,
-            index: u31 = 0,
-        };
-
-        /// The backing array of mappings.
-        array: Array = Array.initFill(.{}),
-
-        pub fn empty(self: *Map) bool {
-            var it = self.array.iterator();
-            while (it.next()) |entry| {
-                if (entry.value.set) return false;
+    fn remapBgs(self: *Contents, row: u16, idx: u32) void {
+        for (self.bg_map.getRow(row)) |*new_idx| {
+            if (new_idx.* == self.bgs.items.len) {
+                new_idx.* = idx;
+                return;
             }
-
-            return true;
         }
-    };
+    }
+
+    fn getMap(self: *Contents, key: Key) *Map {
+        return switch (key) {
+            .bg => &self.bg_map,
+            .text => &self.tx_map,
+            .underline => &self.ul_map,
+            .strikethrough => &self.st_map,
+        };
+    }
 };
 
-test Contents {
-    const testing = std.testing;
-    const alloc = testing.allocator;
+// test Contents {
+//     const testing = std.testing;
+//     const alloc = testing.allocator;
+//
+//     const rows = 10;
+//     const cols = 10;
+//
+//     var c = try Contents.init(alloc);
+//     try c.resize(alloc, .{ .rows = rows, .columns = cols });
+//     defer c.deinit(alloc);
+//
+//     // Assert that get returns null for everything.
+//     for (0..rows) |y| {
+//         for (0..cols) |x| {
+//             try testing.expect(c.get(.bg, .{
+//                 .x = @intCast(x),
+//                 .y = @intCast(y),
+//             }) == null);
+//         }
+//     }
+//
+//     // Set some contents
+//     const cell: mtl_shaders.CellBg = .{
+//         .mode = .rgb,
+//         .grid_pos = .{ 4, 1 },
+//         .cell_width = 1,
+//         .color = .{ 0, 0, 0, 1 },
+//     };
+//     try c.set(alloc, .bg, cell);
+//     try testing.expectEqual(cell, c.get(.bg, .{ .x = 4, .y = 1 }).?);
+//
+//     // Can clear it
+//     c.clear(1);
+//     for (0..rows) |y| {
+//         for (0..cols) |x| {
+//             try testing.expect(c.get(.bg, .{
+//                 .x = @intCast(x),
+//                 .y = @intCast(y),
+//             }) == null);
+//         }
+//     }
+// }
 
-    const rows = 10;
-    const cols = 10;
+// test "Contents clear retains other content" {
+//     const testing = std.testing;
+//     const alloc = testing.allocator;
+//
+//     const rows = 10;
+//     const cols = 10;
+//
+//     var c = try Contents.init(alloc);
+//     try c.resize(alloc, .{ .rows = rows, .columns = cols });
+//     defer c.deinit(alloc);
+//
+//     // Set some contents
+//     const cell1: mtl_shaders.CellBg = .{
+//         .mode = .rgb,
+//         .grid_pos = .{ 4, 1 },
+//         .cell_width = 1,
+//         .color = .{ 0, 0, 0, 1 },
+//     };
+//     const cell2: mtl_shaders.CellBg = .{
+//         .mode = .rgb,
+//         .grid_pos = .{ 4, 2 },
+//         .cell_width = 1,
+//         .color = .{ 0, 0, 0, 1 },
+//     };
+//     try c.set(alloc, .bg, cell1);
+//     try c.set(alloc, .bg, cell2);
+//     c.clear(1);
+//
+//     // Row 2 should still be valid.
+//     try testing.expectEqual(cell2, c.get(.bg, .{ .x = 4, .y = 2 }).?);
+// }
 
-    var c = try Contents.init(alloc);
-    try c.resize(alloc, .{ .rows = rows, .columns = cols });
-    defer c.deinit(alloc);
+// test "Contents clear last added content" {
+//     const testing = std.testing;
+//     const alloc = testing.allocator;
+//
+//     const rows = 10;
+//     const cols = 10;
+//
+//     var c = try Contents.init(alloc);
+//     try c.resize(alloc, .{ .rows = rows, .columns = cols });
+//     defer c.deinit(alloc);
+//
+//     // Set some contents
+//     const cell1: mtl_shaders.CellBg = .{
+//         .mode = .rgb,
+//         .grid_pos = .{ 4, 1 },
+//         .cell_width = 1,
+//         .color = .{ 0, 0, 0, 1 },
+//     };
+//     const cell2: mtl_shaders.CellBg = .{
+//         .mode = .rgb,
+//         .grid_pos = .{ 4, 2 },
+//         .cell_width = 1,
+//         .color = .{ 0, 0, 0, 1 },
+//     };
+//     try c.set(alloc, .bg, cell1);
+//     try c.set(alloc, .bg, cell2);
+//     c.clear(2);
+//
+//     // Row 2 should still be valid.
+//     try testing.expectEqual(cell1, c.get(.bg, .{ .x = 4, .y = 1 }).?);
+// }
 
-    // Assert that get returns null for everything.
-    for (0..rows) |y| {
-        for (0..cols) |x| {
-            try testing.expect(c.get(.bg, .{
-                .x = @intCast(x),
-                .y = @intCast(y),
-            }) == null);
-        }
-    }
-
-    // Set some contents
-    const cell: mtl_shaders.CellBg = .{
-        .mode = .rgb,
-        .grid_pos = .{ 4, 1 },
-        .cell_width = 1,
-        .color = .{ 0, 0, 0, 1 },
-    };
-    try c.set(alloc, .bg, cell);
-    try testing.expectEqual(cell, c.get(.bg, .{ .x = 4, .y = 1 }).?);
-
-    // Can clear it
-    c.clear(1);
-    for (0..rows) |y| {
-        for (0..cols) |x| {
-            try testing.expect(c.get(.bg, .{
-                .x = @intCast(x),
-                .y = @intCast(y),
-            }) == null);
-        }
-    }
-}
-
-test "Contents clear retains other content" {
-    const testing = std.testing;
-    const alloc = testing.allocator;
-
-    const rows = 10;
-    const cols = 10;
-
-    var c = try Contents.init(alloc);
-    try c.resize(alloc, .{ .rows = rows, .columns = cols });
-    defer c.deinit(alloc);
-
-    // Set some contents
-    const cell1: mtl_shaders.CellBg = .{
-        .mode = .rgb,
-        .grid_pos = .{ 4, 1 },
-        .cell_width = 1,
-        .color = .{ 0, 0, 0, 1 },
-    };
-    const cell2: mtl_shaders.CellBg = .{
-        .mode = .rgb,
-        .grid_pos = .{ 4, 2 },
-        .cell_width = 1,
-        .color = .{ 0, 0, 0, 1 },
-    };
-    try c.set(alloc, .bg, cell1);
-    try c.set(alloc, .bg, cell2);
-    c.clear(1);
-
-    // Row 2 should still be valid.
-    try testing.expectEqual(cell2, c.get(.bg, .{ .x = 4, .y = 2 }).?);
-}
-
-test "Contents clear last added content" {
-    const testing = std.testing;
-    const alloc = testing.allocator;
-
-    const rows = 10;
-    const cols = 10;
-
-    var c = try Contents.init(alloc);
-    try c.resize(alloc, .{ .rows = rows, .columns = cols });
-    defer c.deinit(alloc);
-
-    // Set some contents
-    const cell1: mtl_shaders.CellBg = .{
-        .mode = .rgb,
-        .grid_pos = .{ 4, 1 },
-        .cell_width = 1,
-        .color = .{ 0, 0, 0, 1 },
-    };
-    const cell2: mtl_shaders.CellBg = .{
-        .mode = .rgb,
-        .grid_pos = .{ 4, 2 },
-        .cell_width = 1,
-        .color = .{ 0, 0, 0, 1 },
-    };
-    try c.set(alloc, .bg, cell1);
-    try c.set(alloc, .bg, cell2);
-    c.clear(2);
-
-    // Row 2 should still be valid.
-    try testing.expectEqual(cell1, c.get(.bg, .{ .x = 4, .y = 1 }).?);
-}
-
-test "Contents clear modifies same data array" {
-    const testing = std.testing;
-    const alloc = testing.allocator;
-
-    const rows = 10;
-    const cols = 10;
-
-    var c = try Contents.init(alloc);
-    try c.resize(alloc, .{ .rows = rows, .columns = cols });
-    defer c.deinit(alloc);
-
-    // Set some contents
-    const cell1: mtl_shaders.CellBg = .{
-        .mode = .rgb,
-        .grid_pos = .{ 4, 1 },
-        .cell_width = 1,
-        .color = .{ 0, 0, 0, 1 },
-    };
-    const cell2: mtl_shaders.CellBg = .{
-        .mode = .rgb,
-        .grid_pos = .{ 4, 2 },
-        .cell_width = 1,
-        .color = .{ 0, 0, 0, 1 },
-    };
-    try c.set(alloc, .bg, cell1);
-    try c.set(alloc, .bg, cell2);
-
-    const fg1: mtl_shaders.CellText = text: {
-        var cell: mtl_shaders.CellText = undefined;
-        cell.grid_pos = .{ 4, 1 };
-        break :text cell;
-    };
-    const fg2: mtl_shaders.CellText = text: {
-        var cell: mtl_shaders.CellText = undefined;
-        cell.grid_pos = .{ 4, 2 };
-        break :text cell;
-    };
-    try c.set(alloc, .text, fg1);
-    try c.set(alloc, .text, fg2);
-
-    c.clear(1);
-
-    // Should have all of row 2
-    try testing.expectEqual(cell2, c.get(.bg, .{ .x = 4, .y = 2 }).?);
-    try testing.expectEqual(fg2, c.get(.text, .{ .x = 4, .y = 2 }).?);
-}
+// test "Contents clear modifies same data array" {
+//     const testing = std.testing;
+//     const alloc = testing.allocator;
+//
+//     const rows = 10;
+//     const cols = 10;
+//
+//     var c = try Contents.init(alloc);
+//     try c.resize(alloc, .{ .rows = rows, .columns = cols });
+//     defer c.deinit(alloc);
+//
+//     // Set some contents
+//     const cell1: mtl_shaders.CellBg = .{
+//         .mode = .rgb,
+//         .grid_pos = .{ 4, 1 },
+//         .cell_width = 1,
+//         .color = .{ 0, 0, 0, 1 },
+//     };
+//     const cell2: mtl_shaders.CellBg = .{
+//         .mode = .rgb,
+//         .grid_pos = .{ 4, 2 },
+//         .cell_width = 1,
+//         .color = .{ 0, 0, 0, 1 },
+//     };
+//     try c.set(alloc, .bg, cell1);
+//     try c.set(alloc, .bg, cell2);
+//
+//     const fg1: mtl_shaders.CellText = text: {
+//         var cell: mtl_shaders.CellText = undefined;
+//         cell.grid_pos = .{ 4, 1 };
+//         break :text cell;
+//     };
+//     const fg2: mtl_shaders.CellText = text: {
+//         var cell: mtl_shaders.CellText = undefined;
+//         cell.grid_pos = .{ 4, 2 };
+//         break :text cell;
+//     };
+//     try c.set(alloc, .text, fg1);
+//     try c.set(alloc, .text, fg2);
+//
+//     c.clear(1);
+//
+//     // Should have all of row 2
+//     try testing.expectEqual(cell2, c.get(.bg, .{ .x = 4, .y = 2 }).?);
+//     try testing.expectEqual(fg2, c.get(.text, .{ .x = 4, .y = 2 }).?);
+// }
 
 test "Contents.Map size" {
     // We want to be mindful of when this increases because it affects

From adf211f5d54c65a049b49dcf02dd939fadf7ddaf Mon Sep 17 00:00:00 2001
From: Qwerasd <qwerasd205@users.noreply.github.com>
Date: Tue, 7 May 2024 16:45:24 -0400
Subject: [PATCH 2/5] renderer/Metal: ArrayList cell Contents rows

This will allow for unlimited glyphs per row, eliminating the issue run in to with multi-substitution glyphs and combining characters which can result in more glyphs in a row than there are columns.
---
 src/renderer/Metal.zig        |  10 +-
 src/renderer/metal/buffer.zig |  48 +++++
 src/renderer/metal/cell.zig   | 348 +++++++++-------------------------
 3 files changed, 140 insertions(+), 266 deletions(-)

diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig
index efc52beee..e57b2add6 100644
--- a/src/renderer/Metal.zig
+++ b/src/renderer/Metal.zig
@@ -1036,11 +1036,9 @@ pub fn drawFrame(self: *Metal, surface: *apprt.Surface) !void {
     // log.debug("drawing frame index={}", .{self.gpu_state.frame_index});
 
     // Setup our frame data
-    const cells_bg = self.cells.bgCells();
-    const cells_fg = self.cells.fgCells();
     try frame.uniforms.sync(self.gpu_state.device, &.{self.uniforms});
-    try frame.cells_bg.sync(self.gpu_state.device, cells_bg);
-    try frame.cells.sync(self.gpu_state.device, cells_fg);
+    const bg_count = try frame.cells_bg.syncFromArrayLists(self.gpu_state.device, self.cells.bgs.pools);
+    const fg_count = try frame.cells.syncFromArrayLists(self.gpu_state.device, self.cells.text.pools);
 
     // If we have custom shaders, update the animation time.
     if (self.custom_shader_state) |*state| {
@@ -1139,13 +1137,13 @@ pub fn drawFrame(self: *Metal, surface: *apprt.Surface) !void {
         try self.drawImagePlacements(encoder, self.image_placements.items[0..self.image_bg_end]);
 
         // Then draw background cells
-        try self.drawCellBgs(encoder, frame, cells_bg.len);
+        try self.drawCellBgs(encoder, frame, bg_count);
 
         // Then draw images under text
         try self.drawImagePlacements(encoder, self.image_placements.items[self.image_bg_end..self.image_text_end]);
 
         // Then draw fg cells
-        try self.drawCellFgs(encoder, frame, cells_fg.len);
+        try self.drawCellFgs(encoder, frame, fg_count);
 
         // Then draw remaining images
         try self.drawImagePlacements(encoder, self.image_placements.items[self.image_text_end..]);
diff --git a/src/renderer/metal/buffer.zig b/src/renderer/metal/buffer.zig
index 20590afc2..85287a10d 100644
--- a/src/renderer/metal/buffer.zig
+++ b/src/renderer/metal/buffer.zig
@@ -107,5 +107,53 @@ pub fn Buffer(comptime T: type) type {
 
             @memcpy(dst, src);
         }
+
+        /// Like Buffer.sync but takes data from an array of ArrayLists,
+        /// rather than a single array. Returns the number of items synced.
+        pub fn syncFromArrayLists(self: *Self, device: objc.Object, lists: []std.ArrayListUnmanaged(T)) !usize {
+            var total_len: usize = 0;
+            for (lists) |list| {
+                total_len += list.items.len;
+            }
+
+            // If we need more bytes than our buffer has, we need to reallocate.
+            const req_bytes = total_len * @sizeOf(T);
+            const avail_bytes = self.buffer.getProperty(c_ulong, "length");
+            if (req_bytes > avail_bytes) {
+                // Deallocate previous buffer
+                self.buffer.msgSend(void, objc.sel("release"), .{});
+
+                // Allocate a new buffer with enough to hold double what we require.
+                const size = req_bytes * 2;
+                self.buffer = device.msgSend(
+                    objc.Object,
+                    objc.sel("newBufferWithLength:options:"),
+                    .{
+                        @as(c_ulong, @intCast(size * @sizeOf(T))),
+                        mtl.MTLResourceStorageModeShared,
+                    },
+                );
+            }
+
+            // We can fit within the buffer so we can just replace bytes.
+            const dst = dst: {
+                const ptr = self.buffer.msgSend(?[*]u8, objc.sel("contents"), .{}) orelse {
+                    log.warn("buffer contents ptr is null", .{});
+                    return error.MetalFailed;
+                };
+
+                break :dst ptr[0..req_bytes];
+            };
+
+            var i: usize = 0;
+
+            for (lists) |list| {
+                const ptr = @as([*]const u8, @ptrCast(list.items.ptr));
+                @memcpy(dst[i..][0..list.items.len*@sizeOf(T)], ptr);
+                i += list.items.len*@sizeOf(T);
+            }
+
+            return total_len;
+        }
     };
 }
diff --git a/src/renderer/metal/cell.zig b/src/renderer/metal/cell.zig
index 239ac14d5..42a693208 100644
--- a/src/renderer/metal/cell.zig
+++ b/src/renderer/metal/cell.zig
@@ -24,161 +24,84 @@ pub const Key = enum {
             => mtl_shaders.CellText,
         };
     }
-
-    /// Returns true if the two keys share the same data array.
-    fn sharedData(self: Key, other: Key) bool {
-        return switch (self) {
-            inline else => |self_tag| switch (other) {
-                inline else => |other_tag| self_tag.CellType() == other_tag.CellType(),
-            },
-        };
-    }
 };
 
+/// A collection of ArrayLists with methods for bulk operations.
+fn PooledArrayList(comptime T: type) type {
+    return struct {
+        pools: []std.ArrayListUnmanaged(T),
+
+        pub fn init(alloc: Allocator, pool_count: usize) !PooledArrayList(T) {
+            var self: PooledArrayList(T) = .{
+                .pools = try alloc.alloc(std.ArrayListUnmanaged(T), pool_count),
+            };
+
+            for (self.pools) |*list| {
+                list.* = .{};
+            }
+
+            self.reset();
+
+            return self;
+        }
+
+        pub fn deinit(self: *PooledArrayList(T), alloc: Allocator) void {
+            for (self.pools) |*list| {
+                list.deinit(alloc);
+            }
+            alloc.free(self.pools);
+        }
+
+        /// Reset all pools to an empty state without freeing or resizing.
+        pub fn reset(self: *PooledArrayList(T)) void {
+            for (self.pools) |*list| {
+                list.clearRetainingCapacity();
+            }
+        }
+
+        /// Change the pool count and clear the contents of all pools.
+        pub fn resize(self: *PooledArrayList(T), alloc: Allocator, pool_count: u16) !void {
+            const pools  = try alloc.alloc(std.ArrayListUnmanaged(T), pool_count);
+            errdefer alloc.free(pools);
+
+            alloc.free(self.pools);
+
+            self.pools = pools;
+
+            for (self.pools) |*list| {
+                list.* = .{};
+            }
+
+            self.reset();
+        }
+    };
+}
+
 /// The contents of all the cells in the terminal.
 ///
-/// The goal of this data structure is to make it efficient for two operations:
-///
-///   1. Setting the contents of a cell by coordinate. More specifically,
-///      we want to be efficient setting cell contents by row since we
-///      will be doing row dirty tracking.
-///
-///   2. Syncing the contents of the CPU buffers to GPU buffers. This happens
-///      every frame and should be as fast as possible.
-///
-/// To achieve this, the contents are stored in contiguous arrays by
-/// GPU vertex type and we have an array of mappings indexed per row
-/// that map to the index in the GPU vertex array that the content is at.
+/// The goal of this data structure is to allow for efficient row-wise
+/// clearing of data from the GPU buffers, to allow for row-wise dirty
+/// tracking to eliminate the overhead of rebuilding the GPU buffers
+/// each frame.
 pub const Contents = struct {
-    const Map = struct {
-        /// The rows of index mappings are stored in a single contiguous array
-        /// where the start of each row can be direct indexed by its y coord,
-        /// and the used length of each row's section is stored separately.
-        rows: []u32,
-
-        /// The used length for each row.
-        lens: []u16,
-
-        /// The size of each row in the contiguous rows array.
-        row_size: u16,
-
-        pub fn init(alloc: Allocator, size: renderer.GridSize) !Map {
-            var map: Map = .{
-                .rows = try alloc.alloc(u32, size.columns * size.rows),
-                .lens = try alloc.alloc(u16, size.rows),
-                .row_size = size.columns,
-            };
-
-            map.reset();
-
-            return map;
-        }
-
-        pub fn deinit(self: *Map, alloc: Allocator) void {
-            alloc.free(self.rows);
-            alloc.free(self.lens);
-        }
-
-        /// Clear all rows in this map.
-        pub fn reset(self: *Map) void {
-            @memset(self.lens, 0);
-        }
-
-        /// Add a mapped index to a row.
-        pub fn add(self: *Map, row: u16, idx: u32) void {
-            assert(row < self.lens.len);
-
-            const start = self.row_size * row;
-            assert(start < self.rows.len);
-
-            // TODO: Currently this makes the assumption that a given row
-            // will never contain more cells than it has columns. That
-            // assumption is easily violated due to graphemes and multiple-
-            // substitution opentype operations. Currently I've just capped
-            // the length so that additional cells will overwrite the last
-            // one once the row size is exceeded. A better behavior should
-            // be decided upon, this one could cause issues.
-            const len = @min(self.row_size - 1, self.lens[row]);
-            assert(len < self.row_size);
-
-            self.rows[start + len] = idx;
-            self.lens[row] = len + 1;
-        }
-
-        /// Get a slice containing all the mappings for a given row.
-        pub fn getRow(self: *Map, row: u16) []u32 {
-            assert(row < self.lens.len);
-
-            const start = self.row_size * row;
-            assert(start < self.rows.len);
-
-            return self.rows[start..][0..self.lens[row]];
-        }
-
-        /// Clear a given row by resetting its len.
-        pub fn clearRow(self: *Map, row: u16) void {
-            assert(row < self.lens.len);
-            self.lens[row] = 0;
-        }
-    };
-
-    /// The grid size of the terminal. This is used to determine the
-    /// map array index from a coordinate.
     size: renderer.GridSize,
 
-    /// The actual GPU data (on the CPU) for all the cells in the terminal.
-    /// This only contains the cells that have content set. To determine
-    /// if a cell has content set, we check the map.
-    ///
-    /// This data is synced to a buffer on every frame.
-    bgs: std.ArrayListUnmanaged(mtl_shaders.CellBg),
-    text: std.ArrayListUnmanaged(mtl_shaders.CellText),
-
-    /// The map for the bg cells.
-    bg_map: Map,
-    /// The map for the text cells.
-    tx_map: Map,
-    /// The map for the underline cells.
-    ul_map: Map,
-    /// The map for the strikethrough cells.
-    st_map: Map,
-
-    /// True when the cursor should be rendered. This is managed by
-    /// the setCursor method and should not be set directly.
-    cursor: bool,
-
-    /// The amount of text elements we reserve at the beginning for
-    /// special elements like the cursor.
-    const text_reserved_len = 1;
+    bgs: PooledArrayList(mtl_shaders.CellBg),
+    text: PooledArrayList(mtl_shaders.CellText),
 
     pub fn init(alloc: Allocator) !Contents {
-        var result: Contents = .{
+        const result: Contents = .{
             .size = .{ .rows = 0, .columns = 0 },
-            .bgs = .{},
-            .text = .{},
-            .bg_map = try Map.init(alloc, .{ .rows = 0, .columns = 0 }),
-            .tx_map = try Map.init(alloc, .{ .rows = 0, .columns = 0 }),
-            .ul_map = try Map.init(alloc, .{ .rows = 0, .columns = 0 }),
-            .st_map = try Map.init(alloc, .{ .rows = 0, .columns = 0 }),
-            .cursor = false,
+            .bgs = try PooledArrayList(mtl_shaders.CellBg).init(alloc, 0),
+            .text = try PooledArrayList(mtl_shaders.CellText).init(alloc, 0),
         };
 
-        // We preallocate some amount of space for cell contents
-        // we always have as a prefix. For now the current prefix
-        // is length 1: the cursor.
-        try result.text.ensureTotalCapacity(alloc, text_reserved_len);
-        result.text.items.len = text_reserved_len;
-
         return result;
     }
 
     pub fn deinit(self: *Contents, alloc: Allocator) void {
         self.bgs.deinit(alloc);
         self.text.deinit(alloc);
-        self.bg_map.deinit(alloc);
-        self.tx_map.deinit(alloc);
-        self.ul_map.deinit(alloc);
-        self.st_map.deinit(alloc);
     }
 
     /// Resize the cell contents for the given grid size. This will
@@ -189,52 +112,26 @@ pub const Contents = struct {
         size: renderer.GridSize,
     ) !void {
         self.size = size;
-        self.bgs.clearAndFree(alloc);
-        self.text.shrinkAndFree(alloc, text_reserved_len);
+        try self.bgs.resize(alloc, size.rows);
+        try self.text.resize(alloc, size.rows + 1);
 
-        self.bg_map.deinit(alloc);
-        self.tx_map.deinit(alloc);
-        self.ul_map.deinit(alloc);
-        self.st_map.deinit(alloc);
-
-        self.bg_map = try Map.init(alloc, size);
-        self.tx_map = try Map.init(alloc, size);
-        self.ul_map = try Map.init(alloc, size);
-        self.st_map = try Map.init(alloc, size);
+        // Make sure we don't have to allocate for the cursor cell.
+        try self.text.pools[0].ensureTotalCapacity(alloc, 1);
     }
 
     /// Reset the cell contents to an empty state without resizing.
     pub fn reset(self: *Contents) void {
-        self.bgs.clearRetainingCapacity();
-        self.text.shrinkRetainingCapacity(text_reserved_len);
-
-        self.bg_map.reset();
-        self.tx_map.reset();
-        self.ul_map.reset();
-        self.st_map.reset();
+        self.bgs.reset();
+        self.text.reset();
     }
 
-    /// Returns the slice of fg cell contents to sync with the GPU.
-    pub fn fgCells(self: *const Contents) []const mtl_shaders.CellText {
-        const start: usize = if (self.cursor) 0 else 1;
-        return self.text.items[start..];
-    }
-
-    /// Returns the slice of bg cell contents to sync with the GPU.
-    pub fn bgCells(self: *const Contents) []const mtl_shaders.CellBg {
-        return self.bgs.items;
-    }
-
-    /// Set the cursor value. If the value is null then the cursor
-    /// is hidden.
+    /// Set the cursor value. If the value is null then the cursor is hidden.
     pub fn setCursor(self: *Contents, v: ?mtl_shaders.CellText) void {
-        const cell = v orelse {
-            self.cursor = false;
-            return;
-        };
+        self.text.pools[0].clearRetainingCapacity();
 
-        self.cursor = true;
-        self.text.items[0] = cell;
+        if (v) |cell| {
+            self.text.pools[0].appendAssumeCapacity(cell);
+        }
     }
 
     /// Add a cell to the appropriate list. Adding the same cell twice will
@@ -246,98 +143,29 @@ pub const Contents = struct {
         comptime key: Key,
         cell: key.CellType(),
     ) !void {
-        // Get our list of cells based on the key (comptime).
-        const list = &@field(self, switch (key) {
-            .bg => "bgs",
-            .text, .underline, .strikethrough => "text",
-        });
+        const y = cell.grid_pos[1];
 
-        // Add a new cell to the list.
-        const idx: u32 = @intCast(list.items.len);
-        try list.append(alloc, cell);
+        switch (key) {
+            .bg
+            => try self.bgs.pools[y].append(alloc, cell),
 
-        // And to the appropriate mapping.
-        self.getMap(key).add(cell.grid_pos[1], idx);
+            .text,
+            .underline,
+            .strikethrough
+            // We have a special pool containing the cursor cell at the start
+            // of our text pool list, so we need to add 1 to the y to get the
+            // correct index.
+            => try self.text.pools[y + 1].append(alloc, cell),
+        }
     }
 
     /// Clear all of the cell contents for a given row.
     pub fn clear(self: *Contents, y: terminal.size.CellCountInt) void {
-        inline for (std.meta.fields(Key)) |field| {
-            const key: Key = @enumFromInt(field.value);
-            // Get our list of cells based on the key (comptime).
-            const list = &@field(self, switch (key) {
-                .bg => "bgs",
-                .text, .underline, .strikethrough => "text",
-            });
-
-            const map = self.getMap(key);
-
-            const start = y * map.row_size;
-
-            // We iterate from the end of the row because this makes it more
-            // likely that we remove from the end of the list, which results
-            // in not having to re-map anything.
-            while (map.lens[y] > 0) {
-                map.lens[y] -= 1;
-                const i = start + map.lens[y];
-                const idx = map.rows[i];
-
-                _ = list.swapRemove(idx);
-
-                // If we took this cell off the end of the arraylist then
-                // we won't need to re-map anything.
-                if (idx == list.items.len) continue;
-
-                const new = list.items[idx];
-                const new_y = new.grid_pos[1];
-
-                // The cell contents that were moved need to be remapped so
-                // we don't lose track of them.
-                switch (key) {
-                    .bg => self.remapBgs(new_y, idx),
-                    .text, .underline, .strikethrough => self.remapText(new_y, idx),
-                }
-            }
-        }
-    }
-
-    fn remapText(self: *Contents, row: u16, idx: u32) void {
-        for (self.tx_map.getRow(row)) |*new_idx| {
-            if (new_idx.* == self.text.items.len) {
-                new_idx.* = idx;
-                return;
-            }
-        }
-        for (self.ul_map.getRow(row)) |*new_idx| {
-            if (new_idx.* == self.text.items.len) {
-                new_idx.* = idx;
-                return;
-            }
-        }
-        for (self.st_map.getRow(row)) |*new_idx| {
-            if (new_idx.* == self.text.items.len) {
-                new_idx.* = idx;
-                return;
-            }
-        }
-    }
-
-    fn remapBgs(self: *Contents, row: u16, idx: u32) void {
-        for (self.bg_map.getRow(row)) |*new_idx| {
-            if (new_idx.* == self.bgs.items.len) {
-                new_idx.* = idx;
-                return;
-            }
-        }
-    }
-
-    fn getMap(self: *Contents, key: Key) *Map {
-        return switch (key) {
-            .bg => &self.bg_map,
-            .text => &self.tx_map,
-            .underline => &self.ul_map,
-            .strikethrough => &self.st_map,
-        };
+        self.bgs.pools[y].clearRetainingCapacity();
+        // We have a special pool containing the cursor cell at the start
+        // of our text pool list, so we need to add 1 to the y to get the
+        // correct index.
+        self.text.pools[y + 1].clearRetainingCapacity();
     }
 };
 

From 4f26eb203bd2f6d568cbe103f2a690bd711f37c9 Mon Sep 17 00:00:00 2001
From: Qwerasd <qwerasd205@users.noreply.github.com>
Date: Tue, 7 May 2024 20:46:20 -0400
Subject: [PATCH 3/5] renderer/Metal: cell Contents cleanup

Cleaned up naming, some logic changes, added comments.
---
 src/renderer/Metal.zig      |   9 +-
 src/renderer/metal/cell.zig | 173 +++++++++++++++++++++++-------------
 2 files changed, 114 insertions(+), 68 deletions(-)

diff --git a/src/renderer/Metal.zig b/src/renderer/Metal.zig
index e57b2add6..b8a513d6c 100644
--- a/src/renderer/Metal.zig
+++ b/src/renderer/Metal.zig
@@ -566,9 +566,6 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal {
         };
     };
 
-    var cells = try mtl_cell.Contents.init(alloc);
-    errdefer cells.deinit(alloc);
-
     const display_link: ?DisplayLink = switch (builtin.os.tag) {
         .macos => if (options.config.vsync)
             try macos.video.DisplayLink.createWithActiveCGDisplays()
@@ -592,7 +589,7 @@ pub fn init(alloc: Allocator, options: renderer.Options) !Metal {
         .current_background_color = options.config.background,
 
         // Render state
-        .cells = cells,
+        .cells = .{},
         .uniforms = .{
             .projection_matrix = undefined,
             .cell_size = undefined,
@@ -1037,8 +1034,8 @@ pub fn drawFrame(self: *Metal, surface: *apprt.Surface) !void {
 
     // Setup our frame data
     try frame.uniforms.sync(self.gpu_state.device, &.{self.uniforms});
-    const bg_count = try frame.cells_bg.syncFromArrayLists(self.gpu_state.device, self.cells.bgs.pools);
-    const fg_count = try frame.cells.syncFromArrayLists(self.gpu_state.device, self.cells.text.pools);
+    const bg_count = try frame.cells_bg.syncFromArrayLists(self.gpu_state.device, self.cells.bg_rows.lists);
+    const fg_count = try frame.cells.syncFromArrayLists(self.gpu_state.device, self.cells.fg_rows.lists);
 
     // If we have custom shaders, update the animation time.
     if (self.custom_shader_state) |*state| {
diff --git a/src/renderer/metal/cell.zig b/src/renderer/metal/cell.zig
index 42a693208..a6925645f 100644
--- a/src/renderer/metal/cell.zig
+++ b/src/renderer/metal/cell.zig
@@ -26,54 +26,41 @@ pub const Key = enum {
     }
 };
 
-/// A collection of ArrayLists with methods for bulk operations.
-fn PooledArrayList(comptime T: type) type {
+/// A pool of ArrayLists with methods for bulk operations.
+fn ArrayListPool(comptime T: type) type {
     return struct {
-        pools: []std.ArrayListUnmanaged(T),
+        const Self = ArrayListPool(T);
+        const ArrayListT = std.ArrayListUnmanaged(T);
 
-        pub fn init(alloc: Allocator, pool_count: usize) !PooledArrayList(T) {
-            var self: PooledArrayList(T) = .{
-                .pools = try alloc.alloc(std.ArrayListUnmanaged(T), pool_count),
+        // An array containing the lists that belong to this pool.
+        lists: []ArrayListT = &[_]ArrayListT{},
+
+        // The pool will be initialized with empty ArrayLists.
+        pub fn init(alloc: Allocator, list_count: usize, initial_capacity: usize) !Self {
+            const self: Self = .{
+                .lists = try alloc.alloc(ArrayListT, list_count),
             };
 
-            for (self.pools) |*list| {
-                list.* = .{};
+            for (self.lists) |*list| {
+                list.* = try ArrayListT.initCapacity(alloc, initial_capacity);
             }
 
-            self.reset();
-
             return self;
         }
 
-        pub fn deinit(self: *PooledArrayList(T), alloc: Allocator) void {
-            for (self.pools) |*list| {
+        pub fn deinit(self: *Self, alloc: Allocator) void {
+            for (self.lists) |*list| {
                 list.deinit(alloc);
             }
-            alloc.free(self.pools);
+            alloc.free(self.lists);
         }
 
-        /// Reset all pools to an empty state without freeing or resizing.
-        pub fn reset(self: *PooledArrayList(T)) void {
-            for (self.pools) |*list| {
+        /// Clear all lists in the pool.
+        pub fn reset(self: *Self) void {
+            for (self.lists) |*list| {
                 list.clearRetainingCapacity();
             }
         }
-
-        /// Change the pool count and clear the contents of all pools.
-        pub fn resize(self: *PooledArrayList(T), alloc: Allocator, pool_count: u16) !void {
-            const pools  = try alloc.alloc(std.ArrayListUnmanaged(T), pool_count);
-            errdefer alloc.free(pools);
-
-            alloc.free(self.pools);
-
-            self.pools = pools;
-
-            for (self.pools) |*list| {
-                list.* = .{};
-            }
-
-            self.reset();
-        }
     };
 }
 
@@ -83,25 +70,54 @@ fn PooledArrayList(comptime T: type) type {
 /// clearing of data from the GPU buffers, to allow for row-wise dirty
 /// tracking to eliminate the overhead of rebuilding the GPU buffers
 /// each frame.
+///
+/// Must be initialized by resizing before calling any operations.
 pub const Contents = struct {
-    size: renderer.GridSize,
+    size: renderer.GridSize = .{ .rows = 0, .columns = 0 },
 
-    bgs: PooledArrayList(mtl_shaders.CellBg),
-    text: PooledArrayList(mtl_shaders.CellText),
+    /// The ArrayListPool which holds all of the background cells. When sized
+    /// with Contents.resize the individual ArrayLists SHOULD be given enough
+    /// capacity that appendAssumeCapacity may be used, since it should be
+    /// impossible for a row to have more background cells than columns.
+    ///
+    /// HOWEVER, the initial capacity can be exceeded due to multi-glyph
+    /// composites each adding a background cell for the same position.
+    /// This should probably be considered a bug, but for now it means
+    /// that sometimes allocations might happen, so appendAssumeCapacity
+    /// MUST NOT be used.
+    ///
+    /// Rows are indexed as Contents.bg_rows[y].
+    ///
+    /// Must be initialized by calling resize on the Contents struct before
+    /// calling any operations.
+    bg_rows: ArrayListPool(mtl_shaders.CellBg) = .{},
 
-    pub fn init(alloc: Allocator) !Contents {
-        const result: Contents = .{
-            .size = .{ .rows = 0, .columns = 0 },
-            .bgs = try PooledArrayList(mtl_shaders.CellBg).init(alloc, 0),
-            .text = try PooledArrayList(mtl_shaders.CellText).init(alloc, 0),
-        };
-
-        return result;
-    }
+    /// The ArrayListPool which holds all of the foreground cells. When sized
+    /// with Contents.resize the individual ArrayLists are given enough room
+    /// that they can hold a single row with #cols glyphs, underlines, and
+    /// strikethroughs; however, appendAssumeCapacity MUST NOT be used since
+    /// it is possible to exceed this with combining glyphs that add a glyph
+    /// but take up no column since they combine with the previous one, as
+    /// well as with fonts that perform multi-substitutions for glyphs, which
+    /// can result in a similar situation where multiple glyphs reside in the
+    /// same column.
+    ///
+    /// Allocations should nevertheless be exceedingly rare since hitting the
+    /// initial capacity of a list would require a row filled with underlined
+    /// struck through characters, at least one of which is a multi-glyph
+    /// composite.
+    ///
+    /// Rows are indexed as Contents.fg_rows[y + 1], because the first list in
+    /// the pool is reserved for the cursor, which must be the first item in
+    /// the buffer.
+    ///
+    /// Must be initialized by calling resize on the Contents struct before
+    /// calling any operations.
+    fg_rows: ArrayListPool(mtl_shaders.CellText) = .{},
 
     pub fn deinit(self: *Contents, alloc: Allocator) void {
-        self.bgs.deinit(alloc);
-        self.text.deinit(alloc);
+        self.bg_rows.deinit(alloc);
+        self.fg_rows.deinit(alloc);
     }
 
     /// Resize the cell contents for the given grid size. This will
@@ -112,25 +128,58 @@ pub const Contents = struct {
         size: renderer.GridSize,
     ) !void {
         self.size = size;
-        try self.bgs.resize(alloc, size.rows);
-        try self.text.resize(alloc, size.rows + 1);
 
-        // Make sure we don't have to allocate for the cursor cell.
-        try self.text.pools[0].ensureTotalCapacity(alloc, 1);
+        // When we create our bg_rows pool, we give the lists an initial
+        // capacity of size.columns. This is to account for the usual case
+        // where you have a row with normal text and background colors.
+        // This can be exceeded due to multi-glyph composites each adding
+        // a background cell for the same position. This should probably be
+        // considered a bug, but for now it means that sometimes allocations
+        // might happen, and appendAssumeCapacity MUST NOT be used.
+        var bg_rows = try ArrayListPool(mtl_shaders.CellBg).init(alloc, size.rows, size.columns);
+        errdefer bg_rows.deinit(alloc);
+
+        // The foreground lists can hold 3 types of items:
+        // - Glyphs
+        // - Underlines
+        // - Strikethroughs
+        // So we give them an initial capacity of size.columns * 3, which will
+        // avoid any further allocations in the vast majority of cases. Sadly
+        // we can not assume capacity though, since with combining glyphs that
+        // form a single grapheme, and multi-substitutions in fonts, the number
+        // of glyphs in a row is theoretically unlimited.
+        //
+        // We have size.rows + 1 lists because index 0 is used for a special
+        // list containing the cursor cell which needs to be first in the buffer.
+        var fg_rows = try ArrayListPool(mtl_shaders.CellText).init(alloc, size.rows + 1, size.columns * 3);
+        errdefer fg_rows.deinit(alloc);
+
+        self.bg_rows.deinit(alloc);
+        self.fg_rows.deinit(alloc);
+
+        self.bg_rows = bg_rows;
+        self.fg_rows = fg_rows;
+
+        // We don't need 3*cols worth of cells for the cursor list, so we can
+        // replace it with a smaller list. This is technically a tiny bit of
+        // extra work but resize is not a hot function so it's worth it to not
+        // waste the memory.
+        self.fg_rows.lists[0].deinit(alloc);
+        self.fg_rows.lists[0] = try std.ArrayListUnmanaged(mtl_shaders.CellText).initCapacity(alloc, 1);
     }
 
     /// Reset the cell contents to an empty state without resizing.
     pub fn reset(self: *Contents) void {
-        self.bgs.reset();
-        self.text.reset();
+        self.bg_rows.reset();
+        self.fg_rows.reset();
     }
 
     /// Set the cursor value. If the value is null then the cursor is hidden.
     pub fn setCursor(self: *Contents, v: ?mtl_shaders.CellText) void {
-        self.text.pools[0].clearRetainingCapacity();
+        self.fg_rows.lists[0].clearRetainingCapacity();
 
         if (v) |cell| {
-            self.text.pools[0].appendAssumeCapacity(cell);
+            self.fg_rows.lists[0].appendAssumeCapacity(cell);
         }
     }
 
@@ -147,25 +196,25 @@ pub const Contents = struct {
 
         switch (key) {
             .bg
-            => try self.bgs.pools[y].append(alloc, cell),
+            => try self.bg_rows.lists[y].append(alloc, cell),
 
             .text,
             .underline,
             .strikethrough
-            // We have a special pool containing the cursor cell at the start
-            // of our text pool list, so we need to add 1 to the y to get the
+            // We have a special list containing the cursor cell at the start
+            // of our fg row pool, so we need to add 1 to the y to get the
             // correct index.
-            => try self.text.pools[y + 1].append(alloc, cell),
+            => try self.fg_rows.lists[y + 1].append(alloc, cell),
         }
     }
 
     /// Clear all of the cell contents for a given row.
     pub fn clear(self: *Contents, y: terminal.size.CellCountInt) void {
-        self.bgs.pools[y].clearRetainingCapacity();
-        // We have a special pool containing the cursor cell at the start
-        // of our text pool list, so we need to add 1 to the y to get the
+        self.bg_rows.lists[y].clearRetainingCapacity();
+        // We have a special list containing the cursor cell at the start
+        // of our fg row pool, so we need to add 1 to the y to get the
         // correct index.
-        self.text.pools[y + 1].clearRetainingCapacity();
+        self.fg_rows.lists[y + 1].clearRetainingCapacity();
     }
 };
 

From c801e28c399ae10e9714148f5c3418a3dd2671f1 Mon Sep 17 00:00:00 2001
From: Qwerasd <qwerasd205@users.noreply.github.com>
Date: Tue, 7 May 2024 21:13:50 -0400
Subject: [PATCH 4/5] renderer/Metal: cell Contents tests

---
 src/renderer/metal/cell.zig | 280 ++++++++++++++++--------------------
 1 file changed, 123 insertions(+), 157 deletions(-)

diff --git a/src/renderer/metal/cell.zig b/src/renderer/metal/cell.zig
index a6925645f..fcce7e79b 100644
--- a/src/renderer/metal/cell.zig
+++ b/src/renderer/metal/cell.zig
@@ -195,12 +195,11 @@ pub const Contents = struct {
         const y = cell.grid_pos[1];
 
         switch (key) {
-            .bg
-            => try self.bg_rows.lists[y].append(alloc, cell),
+            .bg => try self.bg_rows.lists[y].append(alloc, cell),
 
             .text,
             .underline,
-            .strikethrough
+            .strikethrough,
             // We have a special list containing the cursor cell at the start
             // of our fg row pool, so we need to add 1 to the y to get the
             // correct index.
@@ -218,162 +217,129 @@ pub const Contents = struct {
     }
 };
 
-// test Contents {
-//     const testing = std.testing;
-//     const alloc = testing.allocator;
-//
-//     const rows = 10;
-//     const cols = 10;
-//
-//     var c = try Contents.init(alloc);
-//     try c.resize(alloc, .{ .rows = rows, .columns = cols });
-//     defer c.deinit(alloc);
-//
-//     // Assert that get returns null for everything.
-//     for (0..rows) |y| {
-//         for (0..cols) |x| {
-//             try testing.expect(c.get(.bg, .{
-//                 .x = @intCast(x),
-//                 .y = @intCast(y),
-//             }) == null);
-//         }
-//     }
-//
-//     // Set some contents
-//     const cell: mtl_shaders.CellBg = .{
-//         .mode = .rgb,
-//         .grid_pos = .{ 4, 1 },
-//         .cell_width = 1,
-//         .color = .{ 0, 0, 0, 1 },
-//     };
-//     try c.set(alloc, .bg, cell);
-//     try testing.expectEqual(cell, c.get(.bg, .{ .x = 4, .y = 1 }).?);
-//
-//     // Can clear it
-//     c.clear(1);
-//     for (0..rows) |y| {
-//         for (0..cols) |x| {
-//             try testing.expect(c.get(.bg, .{
-//                 .x = @intCast(x),
-//                 .y = @intCast(y),
-//             }) == null);
-//         }
-//     }
-// }
+test Contents {
+    const testing = std.testing;
+    const alloc = testing.allocator;
 
-// test "Contents clear retains other content" {
-//     const testing = std.testing;
-//     const alloc = testing.allocator;
-//
-//     const rows = 10;
-//     const cols = 10;
-//
-//     var c = try Contents.init(alloc);
-//     try c.resize(alloc, .{ .rows = rows, .columns = cols });
-//     defer c.deinit(alloc);
-//
-//     // Set some contents
-//     const cell1: mtl_shaders.CellBg = .{
-//         .mode = .rgb,
-//         .grid_pos = .{ 4, 1 },
-//         .cell_width = 1,
-//         .color = .{ 0, 0, 0, 1 },
-//     };
-//     const cell2: mtl_shaders.CellBg = .{
-//         .mode = .rgb,
-//         .grid_pos = .{ 4, 2 },
-//         .cell_width = 1,
-//         .color = .{ 0, 0, 0, 1 },
-//     };
-//     try c.set(alloc, .bg, cell1);
-//     try c.set(alloc, .bg, cell2);
-//     c.clear(1);
-//
-//     // Row 2 should still be valid.
-//     try testing.expectEqual(cell2, c.get(.bg, .{ .x = 4, .y = 2 }).?);
-// }
+    const rows = 10;
+    const cols = 10;
 
-// test "Contents clear last added content" {
-//     const testing = std.testing;
-//     const alloc = testing.allocator;
-//
-//     const rows = 10;
-//     const cols = 10;
-//
-//     var c = try Contents.init(alloc);
-//     try c.resize(alloc, .{ .rows = rows, .columns = cols });
-//     defer c.deinit(alloc);
-//
-//     // Set some contents
-//     const cell1: mtl_shaders.CellBg = .{
-//         .mode = .rgb,
-//         .grid_pos = .{ 4, 1 },
-//         .cell_width = 1,
-//         .color = .{ 0, 0, 0, 1 },
-//     };
-//     const cell2: mtl_shaders.CellBg = .{
-//         .mode = .rgb,
-//         .grid_pos = .{ 4, 2 },
-//         .cell_width = 1,
-//         .color = .{ 0, 0, 0, 1 },
-//     };
-//     try c.set(alloc, .bg, cell1);
-//     try c.set(alloc, .bg, cell2);
-//     c.clear(2);
-//
-//     // Row 2 should still be valid.
-//     try testing.expectEqual(cell1, c.get(.bg, .{ .x = 4, .y = 1 }).?);
-// }
+    var c: Contents = .{};
+    try c.resize(alloc, .{ .rows = rows, .columns = cols });
+    defer c.deinit(alloc);
 
-// test "Contents clear modifies same data array" {
-//     const testing = std.testing;
-//     const alloc = testing.allocator;
-//
-//     const rows = 10;
-//     const cols = 10;
-//
-//     var c = try Contents.init(alloc);
-//     try c.resize(alloc, .{ .rows = rows, .columns = cols });
-//     defer c.deinit(alloc);
-//
-//     // Set some contents
-//     const cell1: mtl_shaders.CellBg = .{
-//         .mode = .rgb,
-//         .grid_pos = .{ 4, 1 },
-//         .cell_width = 1,
-//         .color = .{ 0, 0, 0, 1 },
-//     };
-//     const cell2: mtl_shaders.CellBg = .{
-//         .mode = .rgb,
-//         .grid_pos = .{ 4, 2 },
-//         .cell_width = 1,
-//         .color = .{ 0, 0, 0, 1 },
-//     };
-//     try c.set(alloc, .bg, cell1);
-//     try c.set(alloc, .bg, cell2);
-//
-//     const fg1: mtl_shaders.CellText = text: {
-//         var cell: mtl_shaders.CellText = undefined;
-//         cell.grid_pos = .{ 4, 1 };
-//         break :text cell;
-//     };
-//     const fg2: mtl_shaders.CellText = text: {
-//         var cell: mtl_shaders.CellText = undefined;
-//         cell.grid_pos = .{ 4, 2 };
-//         break :text cell;
-//     };
-//     try c.set(alloc, .text, fg1);
-//     try c.set(alloc, .text, fg2);
-//
-//     c.clear(1);
-//
-//     // Should have all of row 2
-//     try testing.expectEqual(cell2, c.get(.bg, .{ .x = 4, .y = 2 }).?);
-//     try testing.expectEqual(fg2, c.get(.text, .{ .x = 4, .y = 2 }).?);
-// }
+    // We should start off empty after resizing.
+    for (0..rows) |y| {
+        try testing.expect(c.bg_rows.lists[y].items.len == 0);
+        try testing.expect(c.fg_rows.lists[y + 1].items.len == 0);
+    }
+    // And the cursor row should have a capacity of 1 and also be empty.
+    try testing.expect(c.fg_rows.lists[0].capacity == 1);
+    try testing.expect(c.fg_rows.lists[0].items.len == 0);
 
-test "Contents.Map size" {
-    // We want to be mindful of when this increases because it affects
-    // renderer memory significantly.
-    try std.testing.expectEqual(@as(usize, 16), @sizeOf(Contents.Map));
+    // Add some contents.
+    const bg_cell: mtl_shaders.CellBg = .{
+        .mode = .rgb,
+        .grid_pos = .{ 4, 1 },
+        .cell_width = 1,
+        .color = .{ 0, 0, 0, 1 },
+    };
+    const fg_cell: mtl_shaders.CellText = .{
+        .mode = .fg,
+        .grid_pos = .{ 4, 1 },
+        .cell_width = 1,
+        .color = .{ 0, 0, 0, 1 },
+        .bg_color = .{ 0, 0, 0, 1 },
+    };
+    try c.add(alloc, .bg, bg_cell);
+    try c.add(alloc, .text, fg_cell);
+    try testing.expectEqual(bg_cell, c.bg_rows.lists[1].items[0]);
+    // The fg row index is offset by 1 because of the cursor list.
+    try testing.expectEqual(fg_cell, c.fg_rows.lists[2].items[0]);
+
+    // And we should be able to clear it.
+    c.clear(1);
+    for (0..rows) |y| {
+        try testing.expect(c.bg_rows.lists[y].items.len == 0);
+        try testing.expect(c.fg_rows.lists[y + 1].items.len == 0);
+    }
+
+    // Add a cursor.
+    const cursor_cell: mtl_shaders.CellText = .{
+        .mode = .cursor,
+        .grid_pos = .{ 2, 3 },
+        .cell_width = 1,
+        .color = .{ 0, 0, 0, 1 },
+        .bg_color = .{ 0, 0, 0, 1 },
+    };
+    c.setCursor(cursor_cell);
+    try testing.expectEqual(cursor_cell, c.fg_rows.lists[0].items[0]);
+
+    // And remove it.
+    c.setCursor(null);
+    try testing.expectEqual(0, c.fg_rows.lists[0].items.len);
+}
+
+test "Contents clear retains other content" {
+    const testing = std.testing;
+    const alloc = testing.allocator;
+
+    const rows = 10;
+    const cols = 10;
+
+    var c: Contents = .{};
+    try c.resize(alloc, .{ .rows = rows, .columns = cols });
+    defer c.deinit(alloc);
+
+    // Set some contents
+    const cell1: mtl_shaders.CellBg = .{
+        .mode = .rgb,
+        .grid_pos = .{ 4, 1 },
+        .cell_width = 1,
+        .color = .{ 0, 0, 0, 1 },
+    };
+    const cell2: mtl_shaders.CellBg = .{
+        .mode = .rgb,
+        .grid_pos = .{ 4, 2 },
+        .cell_width = 1,
+        .color = .{ 0, 0, 0, 1 },
+    };
+    try c.add(alloc, .bg, cell1);
+    try c.add(alloc, .bg, cell2);
+    c.clear(1);
+
+    // Row 2 should still contain its cell.
+    try testing.expectEqual(cell2, c.bg_rows.lists[2].items[0]);
+}
+
+test "Contents clear last added content" {
+    const testing = std.testing;
+    const alloc = testing.allocator;
+
+    const rows = 10;
+    const cols = 10;
+
+    var c: Contents = .{};
+    try c.resize(alloc, .{ .rows = rows, .columns = cols });
+    defer c.deinit(alloc);
+
+    // Set some contents
+    const cell1: mtl_shaders.CellBg = .{
+        .mode = .rgb,
+        .grid_pos = .{ 4, 1 },
+        .cell_width = 1,
+        .color = .{ 0, 0, 0, 1 },
+    };
+    const cell2: mtl_shaders.CellBg = .{
+        .mode = .rgb,
+        .grid_pos = .{ 4, 2 },
+        .cell_width = 1,
+        .color = .{ 0, 0, 0, 1 },
+    };
+    try c.add(alloc, .bg, cell1);
+    try c.add(alloc, .bg, cell2);
+    c.clear(2);
+
+    // Row 1 should still contain its cell.
+    try testing.expectEqual(cell1, c.bg_rows.lists[1].items[0]);
 }

From 7e9bdb84a8815efe28991ea24bf5522854f18439 Mon Sep 17 00:00:00 2001
From: Qwerasd <qwerasd205@users.noreply.github.com>
Date: Tue, 7 May 2024 21:19:22 -0400
Subject: [PATCH 5/5] renderer/Metal: cell Contents asserts

---
 src/renderer/metal/cell.zig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/renderer/metal/cell.zig b/src/renderer/metal/cell.zig
index fcce7e79b..e0a3f797e 100644
--- a/src/renderer/metal/cell.zig
+++ b/src/renderer/metal/cell.zig
@@ -194,6 +194,8 @@ pub const Contents = struct {
     ) !void {
         const y = cell.grid_pos[1];
 
+        assert(y < self.size.rows);
+
         switch (key) {
             .bg => try self.bg_rows.lists[y].append(alloc, cell),
 
@@ -209,6 +211,8 @@ pub const Contents = struct {
 
     /// Clear all of the cell contents for a given row.
     pub fn clear(self: *Contents, y: terminal.size.CellCountInt) void {
+        assert(y < self.size.rows);
+
         self.bg_rows.lists[y].clearRetainingCapacity();
         // We have a special list containing the cursor cell at the start
         // of our fg row pool, so we need to add 1 to the y to get the