From 87e4fb9e25c6de3eb78ea22d65d822f3a6247d5d Mon Sep 17 00:00:00 2001 From: Atul Sharma Date: Thu, 30 Jul 2026 00:09:17 +0100 Subject: [PATCH] Slice shaped text by cluster instead of guessing a range Rendered text duplicated letters. Suite came out as SSuite, financial as fifinancial, allocated as allocatedd and 78701 as 778701. The text itself was always correct, so this only ever showed up once glyphs were positioned. Line breaking cuts a shaped segment into chunks at break opportunities, and each chunk needs its share of the already-shaped glyphs. It picked them by slicing the glyph array with character indices, which is only right when shaping happens to be one glyph per character. When it was not, it fell back to estimating a proportional range with rounding, and rounding at both ends let neighbouring chunks claim the same glyph. The glyph was then drawn twice and a letter appeared doubled. One ligature was enough to poison a whole run. The character count and the glyph count only have to disagree once for every chunk in that run to take the estimating path, which is why letters doubled in words with no ligature anywhere near them. Shaping already knows the answer. Each glyph carries a cluster, the byte it came from, so ShapedText and TextSegment now keep it and chunks select the glyphs whose cluster falls in their byte range. Nothing is estimated. A cluster belongs to the chunk holding its first byte, so a ligature spanning a break lands wholly on one side and is drawn once. Sub-segment clusters are rebased so a chunk can be split again. Page-one PNGs change for letter, contract, invoice, quote and report. Verified by rendering letter and invoice before and after: the doubled letters are gone and nothing else moved. Baseline updated with a reason. Closes #23. --- crates/rdocx-layout/src/engine.rs | 4 + crates/rdocx-layout/src/font.rs | 11 ++ crates/rdocx-layout/src/line.rs | 173 +++++++++++++++++++++++---- crates/rdocx-layout/src/paginator.rs | 4 + scripts/hash_baseline.json | 12 +- 5 files changed, 172 insertions(+), 32 deletions(-) diff --git a/crates/rdocx-layout/src/engine.rs b/crates/rdocx-layout/src/engine.rs index e61e8dc..0a4270a 100644 --- a/crates/rdocx-layout/src/engine.rs +++ b/crates/rdocx-layout/src/engine.rs @@ -586,6 +586,7 @@ pub fn layout_paragraph( font_id, font_size: marker_font_size, glyph_ids: shaped.glyph_ids, + clusters: shaped.clusters, advances: shaped.advances, width: shaped.width, ascent: metrics.ascent, @@ -718,6 +719,7 @@ pub fn layout_paragraph( font_id, font_size, glyph_ids: shaped.glyph_ids, + clusters: shaped.clusters, advances: shaped.advances, width: shaped.width, ascent: metrics.ascent, @@ -768,6 +770,7 @@ pub fn layout_paragraph( font_id, font_size, glyph_ids: shaped.glyph_ids, + clusters: shaped.clusters, advances: shaped.advances, width: shaped.width, ascent: metrics.ascent, @@ -797,6 +800,7 @@ pub fn layout_paragraph( font_id, font_size: sup_size, glyph_ids: shaped.glyph_ids, + clusters: shaped.clusters, advances: shaped.advances, width: shaped.width, ascent: sup_metrics.ascent, diff --git a/crates/rdocx-layout/src/font.rs b/crates/rdocx-layout/src/font.rs index 1c7467b..f625f5f 100644 --- a/crates/rdocx-layout/src/font.rs +++ b/crates/rdocx-layout/src/font.rs @@ -37,6 +37,13 @@ pub struct ShapedText { pub glyph_ids: Vec, /// Per-glyph advances in points. pub advances: Vec, + /// Byte offset into the shaped text that each glyph came from. + /// + /// Shaping is not one glyph per character. A ligature turns several + /// characters into one glyph, so glyph index and character index drift + /// apart. This is the shaper's own mapping back to the source, and it is + /// the only reliable way to slice a shaped run. + pub clusters: Vec, /// Total width in points. pub width: f64, } @@ -545,6 +552,7 @@ impl FontManager { return Ok(ShapedText { glyph_ids: Vec::new(), advances: Vec::new(), + clusters: Vec::new(), width: 0.0, }); } @@ -571,10 +579,12 @@ impl FontManager { let mut glyph_ids = Vec::with_capacity(infos.len()); let mut advances = Vec::with_capacity(positions.len()); + let mut clusters = Vec::with_capacity(infos.len()); let mut total_width = 0.0; for (info, pos) in infos.iter().zip(positions.iter()) { glyph_ids.push(info.glyph_id as u16); + clusters.push(info.cluster); let advance = pos.x_advance as f64 * scale; advances.push(advance); total_width += advance; @@ -583,6 +593,7 @@ impl FontManager { Ok(ShapedText { glyph_ids, advances, + clusters, width: total_width, }) } diff --git a/crates/rdocx-layout/src/line.rs b/crates/rdocx-layout/src/line.rs index 4e9c6ff..5df3f3d 100644 --- a/crates/rdocx-layout/src/line.rs +++ b/crates/rdocx-layout/src/line.rs @@ -41,6 +41,10 @@ pub struct TextSegment { pub font_size: f64, pub glyph_ids: Vec, pub advances: Vec, + /// Byte offset in `text` that each glyph came from, straight from the + /// shaper. Required to slice this segment correctly, because shaping is + /// not one glyph per character. + pub clusters: Vec, pub width: f64, pub ascent: f64, pub descent: f64, @@ -399,32 +403,51 @@ fn split_text_subsegment(seg: &TextSegment, byte_start: usize, byte_end: usize) } let sub_text = seg.text[byte_start..byte_end].to_string(); - let total_chars = seg.text.chars().count(); - let char_start = seg.text[..byte_start].chars().count(); - let char_count = sub_text.chars().count(); - - let (sub_glyphs, sub_advances, sub_width) = if seg.glyph_ids.len() == total_chars { - // 1:1 char-to-glyph mapping (common for Latin text) - let end = (char_start + char_count).min(seg.glyph_ids.len()); - let glyphs = seg.glyph_ids[char_start..end].to_vec(); - let advances = seg.advances[char_start..end].to_vec(); - let width: f64 = advances.iter().sum(); - (glyphs, advances, width) - } else if seg.glyph_ids.is_empty() || seg.text.is_empty() { - // No glyphs, or no text to apportion them across - (vec![], vec![], 0.0) - } else { - // Non-1:1 mapping (ligatures, complex scripts) — proportional estimate - let byte_frac = (byte_end - byte_start) as f64 / seg.text.len() as f64; - let est_glyphs = (seg.glyph_ids.len() as f64 * byte_frac).round() as usize; - let glyph_start = (seg.glyph_ids.len() as f64 * byte_start as f64 / seg.text.len() as f64) - .round() as usize; - let glyph_end = (glyph_start + est_glyphs).min(seg.glyph_ids.len()); - let glyphs = seg.glyph_ids[glyph_start..glyph_end].to_vec(); - let advances = seg.advances[glyph_start..glyph_end].to_vec(); - let width: f64 = advances.iter().sum(); - (glyphs, advances, width) - }; + + // Select glyphs by the shaper's own cluster values, which say the byte + // each glyph came from. Slicing by character index instead only works + // when shaping happens to be one glyph per character, and estimating a + // proportional range when it is not made neighbouring chunks overlap, so + // a glyph was drawn twice and a letter appeared doubled. + let (sub_glyphs, sub_advances, sub_width) = + if seg.clusters.len() == seg.glyph_ids.len() && !seg.clusters.is_empty() { + let mut glyphs = Vec::new(); + let mut advances = Vec::new(); + for (i, &cluster) in seg.clusters.iter().enumerate() { + let c = cluster as usize; + if c >= byte_start && c < byte_end { + glyphs.push(seg.glyph_ids[i]); + advances.push(seg.advances[i]); + } + } + let width: f64 = advances.iter().sum(); + (glyphs, advances, width) + } else if seg.glyph_ids.is_empty() || seg.text.is_empty() { + // No glyphs, or no text to apportion them across + (vec![], vec![], 0.0) + } else { + // No cluster data, which should not happen for anything the shaper + // produced. Fall back to a 1:1 slice rather than guessing a range. + let char_start = seg.text[..byte_start].chars().count(); + let char_count = sub_text.chars().count(); + let end = (char_start + char_count).min(seg.glyph_ids.len()); + let start = char_start.min(end); + let glyphs = seg.glyph_ids[start..end].to_vec(); + let advances = seg.advances[start..end].to_vec(); + let width: f64 = advances.iter().sum(); + (glyphs, advances, width) + }; + + // A cluster is attributed to the chunk containing its starting byte, so + // a ligature spanning a break lands wholly in the first chunk. Its glyph + // is drawn once, which is the point. + let sub_clusters: Vec = seg + .clusters + .iter() + .copied() + .filter(|&c| (c as usize) >= byte_start && (c as usize) < byte_end) + .map(|c| c - byte_start as u32) + .collect(); InlineItem::Text(TextSegment { text: sub_text, @@ -432,6 +455,7 @@ fn split_text_subsegment(seg: &TextSegment, byte_start: usize, byte_end: usize) font_size: seg.font_size, glyph_ids: sub_glyphs, advances: sub_advances, + clusters: sub_clusters, width: sub_width, ascent: seg.ascent, descent: seg.descent, @@ -608,6 +632,9 @@ fn shape_leader( font_size, glyph_ids, advances, + // Leader glyphs are generated by repetition, not shaped from the + // text, so there is no cluster mapping to carry. + clusters: Vec::new(), width: tab_width, // fill the entire tab gap ascent: metrics.ascent, descent: metrics.descent, @@ -719,6 +746,7 @@ mod tests { font_size: 12.0, glyph_ids: vec![], advances: vec![], + clusters: vec![], width, ascent: 10.0, descent: 3.0, @@ -851,4 +879,97 @@ mod tests { assert!((w - 300.0).abs() < 0.01); assert_eq!(leader, Some('.')); } + + /// Build a segment whose shaping is not one glyph per character, the way + /// a font with an "fi" ligature shapes "financial": 9 characters, 8 + /// glyphs, and the first glyph covering two characters. + fn ligature_segment() -> TextSegment { + let text = "financial"; + // cluster = byte the glyph came from. The fi ligature covers bytes 0 + // and 1, so no glyph reports cluster 1. + let clusters = vec![0, 2, 3, 4, 5, 6, 7, 8]; + let mut seg = make_text_segment(text, 90.0); + seg.glyph_ids = (0..clusters.len() as u16).map(|i| i + 100).collect(); + seg.advances = vec![10.0; clusters.len()]; + seg.clusters = clusters; + seg + } + + /// Splitting a shaped segment must never emit the same glyph twice. + /// + /// Glyphs used to be selected by slicing on character index, with a + /// proportional guess when shaping was not one glyph per character. + /// Neighbouring chunks then overlapped and a glyph was drawn in both, so + /// a letter appeared doubled on the page. + #[test] + fn splitting_a_shaped_segment_never_repeats_a_glyph() { + let seg = ligature_segment(); + let total = seg.glyph_ids.len(); + + // Split at every byte boundary and collect what each half emits. + for cut in 1..seg.text.len() { + if !seg.text.is_char_boundary(cut) { + continue; + } + let mut seen = Vec::new(); + for (start, end) in [(0, cut), (cut, seg.text.len())] { + if let InlineItem::Text(part) = split_text_subsegment(&seg, start, end) { + seen.extend(part.glyph_ids); + } + } + let mut sorted = seen.clone(); + sorted.sort_unstable(); + sorted.dedup(); + assert_eq!( + sorted.len(), + seen.len(), + "cut at {cut} drew a glyph twice: {seen:?}" + ); + assert_eq!( + seen.len(), + total, + "cut at {cut} lost or gained a glyph: {seen:?}" + ); + } + } + + /// A ligature spanning a break belongs to one side of it, not both. + #[test] + fn a_ligature_is_not_split_across_chunks() { + let seg = ligature_segment(); + // Cut between the two characters the ligature covers. + let InlineItem::Text(first) = split_text_subsegment(&seg, 0, 1) else { + panic!("expected text"); + }; + let InlineItem::Text(second) = split_text_subsegment(&seg, 1, seg.text.len()) else { + panic!("expected text"); + }; + assert_eq!( + first.glyph_ids, + vec![100], + "the ligature glyph belongs to the chunk holding its first byte" + ); + assert!( + !second.glyph_ids.contains(&100), + "and must not be repeated in the next chunk" + ); + } + + /// Clusters are rebased onto the sub-segment, so a chunk can be split + /// again without the offsets pointing outside its own text. + #[test] + fn sub_segment_clusters_are_relative_to_the_sub_segment() { + let seg = ligature_segment(); + let InlineItem::Text(part) = split_text_subsegment(&seg, 3, 7) else { + panic!("expected text"); + }; + assert!( + part.clusters + .iter() + .all(|&c| (c as usize) < part.text.len()), + "clusters {:?} must index into {:?}", + part.clusters, + part.text + ); + } } diff --git a/crates/rdocx-layout/src/paginator.rs b/crates/rdocx-layout/src/paginator.rs index 2b8581d..11e773d 100644 --- a/crates/rdocx-layout/src/paginator.rs +++ b/crates/rdocx-layout/src/paginator.rs @@ -1412,6 +1412,7 @@ mod tests { font_size: 12.0, glyph_ids: vec![1, 2, 3], advances: vec![6.0, 6.0, 6.0], + clusters: vec![0, 1, 2], width: 40.0, ascent: height * 0.77, descent: height * 0.23, @@ -1510,6 +1511,7 @@ mod tests { font_size: 12.0, glyph_ids: vec![1], advances: vec![10.0], + clusters: vec![], width: 20.0, ascent: 10.0, descent: 3.0, @@ -1685,6 +1687,7 @@ mod tests { font_size: 12.0, glyph_ids: vec![1; text.len()], advances: vec![seg_width / text.len() as f64; text.len()], + clusters: vec![], width: seg_width, ascent: 10.0, descent: 3.0, @@ -1722,6 +1725,7 @@ mod tests { font_size: 12.0, glyph_ids: vec![1, 2, 3], advances: vec![8.0, 8.0, 8.0], + clusters: vec![], width: 60.0, ascent: 10.0, descent: 3.0, diff --git a/scripts/hash_baseline.json b/scripts/hash_baseline.json index 5ce7248..7cc0533 100644 --- a/scripts/hash_baseline.json +++ b/scripts/hash_baseline.json @@ -1,6 +1,6 @@ { "entries": { - "contract:page1.png": "504977f2017680736254709429370708554166410856873fd5c38a1d5283b98a", + "contract:page1.png": "5b25f35ca837a7632f201aa91e2441968b763587d4c66923830111b6764e77c9", "contract:word/document.xml": "1ecd9138e09122a5980ca80451ddd8f93d4e6d3af8bb80e582dc8cb697277dfe", "contract:word/numbering.xml": "01d6cb0aa0ecc30b3d2a77c4df062b1c727d70ff9cbc0824910f049daf878a26", "contract:word/styles.xml": "0dc0b047b6019b798b83bfe4d66eb91d14b03caea3f6bdb0934d48fbb863fba4", @@ -8,11 +8,11 @@ "feature_showcase:word/document.xml": "b5c4efbb49263060558242bad0cca79d4b03a43b62d72feed66789b4926b44f5", "feature_showcase:word/numbering.xml": "2aa0599486d98573be0b7febb04ec63c08d3dfc5341b0f722b93cae113803bbc", "feature_showcase:word/styles.xml": "b815acd04cc89189d5b2b0caf822435d941c8cc70077f34932e4a2b3ce1b6595", - "invoice:page1.png": "7693aab43aaf6971edc2b0fab6b50e978db1192c18811a8e2649113339348d49", + "invoice:page1.png": "850572f788d246ac8beceec0a68ac5da63d4904327315a712140cea977fe1bcd", "invoice:word/document.xml": "bb2d71711f6a613044dfabc166ae340354345666a98687e02df815efa220c655", "invoice:word/numbering.xml": null, "invoice:word/styles.xml": "0dc0b047b6019b798b83bfe4d66eb91d14b03caea3f6bdb0934d48fbb863fba4", - "letter:page1.png": "d3a00cf6c37a1eb6c1897e36b062c10e8b0fdc8c2a6f26c1034026f83578d1c0", + "letter:page1.png": "79b23d965cd43bbdbb3cf2128df8f51b5d2d53d4a8934aaa34ed00b2aef8821e", "letter:word/document.xml": "54f74a133887b1b93da1070517f9d7f687dbdc27b1307f1696c4b41f66d5fc05", "letter:word/numbering.xml": "c6511604704117eb00ad2faffb9173e4f48d22f62557ca72a51d60b7907c8058", "letter:word/styles.xml": "0dc0b047b6019b798b83bfe4d66eb91d14b03caea3f6bdb0934d48fbb863fba4", @@ -20,14 +20,14 @@ "proposal:word/document.xml": "1f8222a2d0785f6cc33b868274aaf53796ccbcea254f62b102dc51cc075dee6b", "proposal:word/numbering.xml": "061e20beb3409ce0f3feda1a99793956124a2967dd23afe1b528dd8fdeb82182", "proposal:word/styles.xml": "dae1d6c1083bba6bb4b29ba2ab748a713f5da60b8c2cd4d52224c6f0a8f21bef", - "quote:page1.png": "6f69877633a4d09e80cb9172ba8f2d4d220cf7776260fcaa21c5be84362be103", + "quote:page1.png": "e969347efaf9973367195736ed1eeeec2f61e600bd5c55363ea6ef398569e01b", "quote:word/document.xml": "dd3b7c8bae85d8e22a8c08f12fc4a1ec4829b00a84fbe8cf13f9204213b4cf68", "quote:word/numbering.xml": "c6511604704117eb00ad2faffb9173e4f48d22f62557ca72a51d60b7907c8058", "quote:word/styles.xml": "0dc0b047b6019b798b83bfe4d66eb91d14b03caea3f6bdb0934d48fbb863fba4", - "report:page1.png": "9bfaa63b0b14482ad765d9e6528077e96e574a037e701e3d60ea32bdd96d7d32", + "report:page1.png": "792f1fdc032c7ec1b6a3e451aa7f94fc45e8ea810d12b9815ced82d2fa7fbd82", "report:word/document.xml": "5a834b2ebe01156c082f25ea05483c135d3718beae7d80b2224e0a02f9b93365", "report:word/numbering.xml": "2aa0599486d98573be0b7febb04ec63c08d3dfc5341b0f722b93cae113803bbc", "report:word/styles.xml": "0dc0b047b6019b798b83bfe4d66eb91d14b03caea3f6bdb0934d48fbb863fba4" }, - "reason": "List levels now apply their own w:pPr indent (#13), so page-one PNGs for letter and contract shift. Verified by rendering both before and after: the only visual change is list markers moving to their level indent with correct hanging indent. No other content moved." + "reason": "Glyphs are now selected by shaper cluster instead of a proportional guess (#23), so neighbouring chunks no longer overlap and letters are no longer drawn twice. Page-one PNGs change for letter, contract, invoice, quote and report. Verified by rendering letter and invoice before and after: doubled letters such as SSuite, fifinancial and 778701 are gone and nothing else moved." }