Skip to content

Commit dda27d4

Browse files
authored
font/shaper: Fix CoreText position.y for some scripts (#10179)
This PR simplifies and corrects the logic for placing a glyph vertically, by using the `position.y` from `CoreText` directly, instead of using an offset from the cell's starting `y`. The logic was incorrect from the beginning, always treating the first glyph of a cell as being at `y` of zero. We only need to be subtracting the cell's starting `x` to align the glyphs to the cell grid. Enabling the commented out logging, I found no instances of `position.y differs from old offset.y` lines with `JetBrains Mono` with ligatures turned on, but running [ttylang](https://github.com/jacobsandlund/ttylang) (printing the Universal Declaration of Human Rights in various languages) revealed 676 instances of this, with many only slightly off. An example log from some Tai Tham text is the following, and this PR adds a test based on this: ``` ...pos=(0.00,-8.21) run_offset=(69.41,-8.21) cell_offset=(69.41,-8.21) old offset.y=0.00 cps = \u{1a49}\u{1a60} \u{1a3f}▸\u{1a69} \u{1a2f} → ᩉ᩠ᨿᩩᨯ ``` Browsers display this as: ᩉ᩠ᨿᩩ `main` is printing: <img width="852" height="90" alt="CleanShot 2026-01-05 at 10 28 17@2x" src="https://github.com/user-attachments/assets/c97b738c-8fe4-48b5-81f8-e0e79f1a9269" /> this PR prints: <img width="958" height="90" alt="CleanShot 2026-01-05 at 10 29 07@2x" src="https://github.com/user-attachments/assets/88fd26a7-8041-4b33-ab02-56f411204b04" /> Since this is a ligature of two different grapheme clusters, Ghostty ends up subtracting too much of the `x` value with the `cell_offset.x` (starting x), so neither of the screenshots above are correct, but the second is closer and gets the `y` value right. AI disclaimer: I didn't use AI for the code, but did ask it about this Tai Tham text and why it wasn't a single grapheme cluster: https://ampcode.com/threads/T-019b8ea2-1822-75bb-a8eb-55a9ddb9f7ea
2 parents 151dd33 + 15899b7 commit dda27d4

File tree

1 file changed

+131
-61
lines changed

1 file changed

+131
-61
lines changed

src/font/shaper/coretext.zig

Lines changed: 131 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -103,15 +103,9 @@ pub const Shaper = struct {
103103
}
104104
};
105105

106-
const RunOffset = struct {
107-
x: f64 = 0,
108-
y: f64 = 0,
109-
};
110-
111106
const CellOffset = struct {
112107
cluster: u32 = 0,
113108
x: f64 = 0,
114-
y: f64 = 0,
115109
};
116110

117111
/// Create a CoreFoundation Dictionary suitable for
@@ -388,15 +382,15 @@ pub const Shaper = struct {
388382
const line = typesetter.createLine(.{ .location = 0, .length = 0 });
389383
self.cf_release_pool.appendAssumeCapacity(line);
390384

391-
// This keeps track of the current offsets within a run.
392-
var run_offset: RunOffset = .{};
385+
// This keeps track of the current x offset (sum of advance.width)
386+
var run_offset_x: f64 = 0.0;
393387

394-
// This keeps track of the current offsets within a cell.
388+
// This keeps track of the cell starting x and cluster.
395389
var cell_offset: CellOffset = .{};
396390

397391
// For debugging positions, turn this on:
398-
//var start_index: usize = 0;
399-
//var end_index: usize = 0;
392+
//var run_offset_y: f64 = 0.0;
393+
//var cell_offset_y: f64 = 0.0;
400394

401395
// Clear our cell buf and make sure we have enough room for the whole
402396
// line of glyphs, so that we can just assume capacity when appending
@@ -450,39 +444,31 @@ pub const Shaper = struct {
450444

451445
cell_offset = .{
452446
.cluster = cluster,
453-
.x = run_offset.x,
454-
.y = run_offset.y,
447+
.x = run_offset_x,
455448
};
456449

457450
// For debugging positions, turn this on:
458-
// start_index = index;
459-
// end_index = index;
460-
//} else {
461-
// if (index < start_index) {
462-
// start_index = index;
463-
// }
464-
// if (index > end_index) {
465-
// end_index = index;
466-
// }
451+
//cell_offset_y = run_offset_y;
467452
}
468453

469454
// For debugging positions, turn this on:
470-
//try self.debugPositions(alloc, run_offset, cell_offset, position, start_index, end_index, index);
455+
//try self.debugPositions(alloc, run_offset_x, run_offset_y, cell_offset, cell_offset_y, position, index);
471456

472457
const x_offset = position.x - cell_offset.x;
473-
const y_offset = position.y - cell_offset.y;
474458

475459
self.cell_buf.appendAssumeCapacity(.{
476460
.x = @intCast(cluster),
477461
.x_offset = @intFromFloat(@round(x_offset)),
478-
.y_offset = @intFromFloat(@round(y_offset)),
462+
.y_offset = @intFromFloat(@round(position.y)),
479463
.glyph_index = glyph,
480464
});
481465

482466
// Add our advances to keep track of our run offsets.
483467
// Advances apply to the NEXT cell.
484-
run_offset.x += advance.width;
485-
run_offset.y += advance.height;
468+
run_offset_x += advance.width;
469+
470+
// For debugging positions, turn this on:
471+
//run_offset_y += advance.height;
486472
}
487473
}
488474

@@ -655,57 +641,81 @@ pub const Shaper = struct {
655641
fn debugPositions(
656642
self: *Shaper,
657643
alloc: Allocator,
658-
run_offset: RunOffset,
644+
run_offset_x: f64,
645+
run_offset_y: f64,
659646
cell_offset: CellOffset,
647+
cell_offset_y: f64,
660648
position: macos.graphics.Point,
661-
start_index: usize,
662-
end_index: usize,
663649
index: usize,
664650
) !void {
665651
const state = &self.run_state;
666652
const x_offset = position.x - cell_offset.x;
667-
const y_offset = position.y - cell_offset.y;
668-
const advance_x_offset = run_offset.x - cell_offset.x;
669-
const advance_y_offset = run_offset.y - cell_offset.y;
653+
const advance_x_offset = run_offset_x - cell_offset.x;
654+
const advance_y_offset = run_offset_y - cell_offset_y;
670655
const x_offset_diff = x_offset - advance_x_offset;
671-
const y_offset_diff = y_offset - advance_y_offset;
656+
const y_offset_diff = position.y - advance_y_offset;
657+
const positions_differ = @abs(x_offset_diff) > 0.0001 or @abs(y_offset_diff) > 0.0001;
658+
const old_offset_y = position.y - cell_offset_y;
659+
const position_y_differs = @abs(cell_offset_y) > 0.0001;
672660

673-
if (@abs(x_offset_diff) > 0.0001 or @abs(y_offset_diff) > 0.0001) {
661+
if (positions_differ or position_y_differs) {
674662
var allocating = std.Io.Writer.Allocating.init(alloc);
675663
const writer = &allocating.writer;
676-
const codepoints = state.codepoints.items[start_index .. end_index + 1];
664+
const codepoints = state.codepoints.items;
665+
const current_cp = state.codepoints.items[index].codepoint;
666+
var last_cluster: ?u32 = null;
677667
for (codepoints) |cp| {
678-
if (cp.codepoint == 0) continue; // Skip surrogate pair padding
679-
try writer.print("\\u{{{x}}}", .{cp.codepoint});
668+
if ((cp.cluster == cell_offset.cluster or cp.cluster == cell_offset.cluster - 1 or cp.cluster == cell_offset.cluster + 1) and
669+
cp.codepoint != 0 // Skip surrogate pair padding
670+
) {
671+
if (last_cluster) |last| {
672+
if (cp.cluster != last) {
673+
try writer.writeAll(" ");
674+
}
675+
}
676+
if (cp.cluster == cell_offset.cluster and cp.codepoint == current_cp) {
677+
try writer.writeAll("▸");
678+
}
679+
try writer.print("\\u{{{x}}}", .{cp.codepoint});
680+
last_cluster = cp.cluster;
681+
}
680682
}
681683
try writer.writeAll(" → ");
682684
for (codepoints) |cp| {
683-
if (cp.codepoint == 0) continue; // Skip surrogate pair padding
684-
try writer.print("{u}", .{@as(u21, @intCast(cp.codepoint))});
685+
if ((cp.cluster == cell_offset.cluster or cp.cluster == cell_offset.cluster - 1 or cp.cluster == cell_offset.cluster + 1) and
686+
cp.codepoint != 0 // Skip surrogate pair padding
687+
) {
688+
try writer.print("{u}", .{@as(u21, @intCast(cp.codepoint))});
689+
}
685690
}
686691
const formatted_cps = try allocating.toOwnedSlice();
687692

688-
// Note that the codepoints from `start_index .. end_index + 1`
689-
// might not include all the codepoints being shaped. Sometimes a
690-
// codepoint gets represented in a glyph with a later codepoint
691-
// such that the index for the former codepoint is skipped and just
692-
// the index for the latter codepoint is used. Additionally, this
693-
// gets called as we iterate through the glyphs, so it won't
694-
// include the codepoints that come later that might be affecting
695-
// positions for the current glyph. Usually though, for that case
696-
// the positions of the later glyphs will also be affected and show
697-
// up in the logs.
698-
log.warn("position differs from advance: cluster={d} pos=({d:.2},{d:.2}) adv=({d:.2},{d:.2}) diff=({d:.2},{d:.2}) current cp={x}, cps={s}", .{
699-
cell_offset.cluster,
700-
x_offset,
701-
y_offset,
702-
advance_x_offset,
703-
advance_y_offset,
704-
x_offset_diff,
705-
y_offset_diff,
706-
state.codepoints.items[index].codepoint,
707-
formatted_cps,
708-
});
693+
if (positions_differ) {
694+
log.warn("position differs from advance: cluster={d} pos=({d:.2},{d:.2}) adv=({d:.2},{d:.2}) diff=({d:.2},{d:.2}) cps = {s}", .{
695+
cell_offset.cluster,
696+
x_offset,
697+
position.y,
698+
advance_x_offset,
699+
advance_y_offset,
700+
x_offset_diff,
701+
y_offset_diff,
702+
formatted_cps,
703+
});
704+
}
705+
706+
if (position_y_differs) {
707+
log.warn("position.y differs from old offset.y: cluster={d} pos=({d:.2},{d:.2}) run_offset=({d:.2},{d:.2}) cell_offset=({d:.2},{d:.2}) old offset.y={d:.2} cps = {s}", .{
708+
cell_offset.cluster,
709+
x_offset,
710+
position.y,
711+
run_offset_x,
712+
run_offset_y,
713+
cell_offset.x,
714+
cell_offset_y,
715+
old_offset_y,
716+
formatted_cps,
717+
});
718+
}
709719
}
710720
}
711721
};
@@ -1522,6 +1532,66 @@ test "shape Tai Tham vowels (position differs from advance)" {
15221532
try testing.expectEqual(@as(usize, 1), count);
15231533
}
15241534

1535+
test "shape Tai Tham letters (position.y differs from advance)" {
1536+
const testing = std.testing;
1537+
const alloc = testing.allocator;
1538+
1539+
// We need a font that supports Tai Tham for this to work, if we can't find
1540+
// Noto Sans Tai Tham, which is a system font on macOS, we just skip the
1541+
// test.
1542+
var testdata = testShaperWithDiscoveredFont(
1543+
alloc,
1544+
"Noto Sans Tai Tham",
1545+
) catch return error.SkipZigTest;
1546+
defer testdata.deinit();
1547+
1548+
var buf: [32]u8 = undefined;
1549+
var buf_idx: usize = 0;
1550+
1551+
// First grapheme cluster:
1552+
buf_idx += try std.unicode.utf8Encode(0x1a49, buf[buf_idx..]); // HA
1553+
buf_idx += try std.unicode.utf8Encode(0x1a60, buf[buf_idx..]); // SAKOT
1554+
// Second grapheme cluster, combining with the first in a ligature:
1555+
buf_idx += try std.unicode.utf8Encode(0x1a3f, buf[buf_idx..]); // YA
1556+
buf_idx += try std.unicode.utf8Encode(0x1a69, buf[buf_idx..]); // U
1557+
1558+
// Make a screen with some data
1559+
var t = try terminal.Terminal.init(alloc, .{ .cols = 30, .rows = 3 });
1560+
defer t.deinit(alloc);
1561+
1562+
// Enable grapheme clustering
1563+
t.modes.set(.grapheme_cluster, true);
1564+
1565+
var s = t.vtStream();
1566+
defer s.deinit();
1567+
try s.nextSlice(buf[0..buf_idx]);
1568+
1569+
var state: terminal.RenderState = .empty;
1570+
defer state.deinit(alloc);
1571+
try state.update(alloc, &t);
1572+
1573+
// Get our run iterator
1574+
var shaper = &testdata.shaper;
1575+
var it = shaper.runIterator(.{
1576+
.grid = testdata.grid,
1577+
.cells = state.row_data.get(0).cells.slice(),
1578+
});
1579+
var count: usize = 0;
1580+
while (try it.next(alloc)) |run| {
1581+
count += 1;
1582+
1583+
const cells = try shaper.shape(run);
1584+
try testing.expectEqual(@as(usize, 3), cells.len);
1585+
try testing.expectEqual(@as(u16, 0), cells[0].x);
1586+
try testing.expectEqual(@as(u16, 0), cells[1].x);
1587+
try testing.expectEqual(@as(u16, 1), cells[2].x); // U from second grapheme
1588+
1589+
// The U glyph renders at a y below zero
1590+
try testing.expectEqual(@as(i16, -3), cells[2].y_offset);
1591+
}
1592+
try testing.expectEqual(@as(usize, 1), count);
1593+
}
1594+
15251595
test "shape box glyphs" {
15261596
const testing = std.testing;
15271597
const alloc = testing.allocator;

0 commit comments

Comments
 (0)