From b27b4c6be7401880ed59ba3165f9059e63a8c544 Mon Sep 17 00:00:00 2001 From: Keavon Chambers Date: Sat, 2 May 2026 21:53:33 -0700 Subject: [PATCH] Remove gaps between text glyph bounding box click targets so selection is more reliable (#4098) * Remove gaps between text glyph bounding box click targets so selection is more reliable * Code review --- node-graph/nodes/text/src/path_builder.rs | 94 +++++++++++++++++++---- 1 file changed, 79 insertions(+), 15 deletions(-) diff --git a/node-graph/nodes/text/src/path_builder.rs b/node-graph/nodes/text/src/path_builder.rs index f8ea84c5..9e7684e7 100644 --- a/node-graph/nodes/text/src/path_builder.rs +++ b/node-graph/nodes/text/src/path_builder.rs @@ -15,8 +15,12 @@ pub struct PathBuilder { origin: DVec2, glyph_subpaths: Vec>, pub vector_table: Table, - /// Per-glyph bbox rectangles collected in single-item mode, published as `ATTR_EDITOR_CLICK_TARGET` in `finalize()`. - merged_click_target_subpaths: Vec>, + /// Per-glyph AABBs collected in single-item mode, published as `ATTR_EDITOR_CLICK_TARGET` in `finalize()`. + merged_click_target_bboxes: Vec<[DVec2; 2]>, + /// Per-glyph baselines, parallel to `merged_click_target_bboxes`. Groups glyphs by line for the widening pass. + merged_click_target_baselines: Vec, + /// Per-glyph AABBs in glyph-local space (multi-item mode), widened in `finalize()` to fill gaps. + per_glyph_bboxes: Vec>, /// Text frame size, stamped per item as `ATTR_EDITOR_TEXT_FRAME` relative to each item's origin. text_frame_size: DVec2, /// First glyph's baseline offset (pre-height-filter). Used for the empty placeholder item so @@ -32,7 +36,9 @@ impl PathBuilder { current_subpath: Subpath::new(Vec::new(), false), glyph_subpaths: Vec::new(), vector_table: if per_glyph_items { Table::new() } else { Table::new_from_element(Vector::default()) }, - merged_click_target_subpaths: Vec::new(), + merged_click_target_bboxes: Vec::new(), + merged_click_target_baselines: Vec::new(), + per_glyph_bboxes: Vec::new(), text_frame_size, first_glyph_offset, scale, @@ -61,8 +67,7 @@ impl PathBuilder { glyph_subpath.apply_transform(skew); } - // Bounding-box rectangle for click-targeting the glyph's full bounds (not just the letterform) - let glyph_bbox_rectangle = subpaths_bounding_box(&self.glyph_subpaths).map(|[min, max]| Subpath::new_rectangle(min, max)); + let glyph_bbox = subpaths_bounding_box(&self.glyph_subpaths); if per_glyph_items { // Frame in item-local space: top-left at `-glyph_offset` so the item transform cancels it @@ -72,18 +77,18 @@ impl PathBuilder { let item = TableRow::new_from_element(Vector::from_subpaths(core::mem::take(&mut self.glyph_subpaths), false)) .with_attribute(ATTR_TRANSFORM, DAffine2::from_translation(glyph_offset)) .with_attribute(ATTR_EDITOR_TEXT_FRAME, frame_in_item_local); - let item = match glyph_bbox_rectangle { - Some(rect) => item.with_attribute(ATTR_EDITOR_CLICK_TARGET, Vector::from_subpaths([rect], false)), - None => item, - }; self.vector_table.push(item); + + // Defer click target creation to `finalize()` where adjacent AABBs get widened + self.per_glyph_bboxes.push(glyph_bbox); } else { for subpath in self.glyph_subpaths.drain(..) { // Unwrapping here is ok because `self.vector_table` is initialized with a single `Table` item self.vector_table.element_mut(0).unwrap().append_subpath(subpath, false); } - if let Some(rect) = glyph_bbox_rectangle { - self.merged_click_target_subpaths.push(rect); + if let Some(bbox) = glyph_bbox { + self.merged_click_target_bboxes.push(bbox); + self.merged_click_target_baselines.push(glyph_offset.y); } } } @@ -154,10 +159,39 @@ impl PathBuilder { self.vector_table.push(item); } - // With "Separate Glyphs" inactive, combine the accumulated per-glyph AABBs as one override `Vector` - if !self.merged_click_target_subpaths.is_empty() { - self.vector_table - .set_attribute(ATTR_EDITOR_CLICK_TARGET, 0, Vector::from_subpaths(self.merged_click_target_subpaths, false)); + // Widen per-glyph AABBs to close horizontal gaps, then publish as click targets + if !self.per_glyph_bboxes.is_empty() { + // Project glyph-local AABBs into layer-local for the widening pass + let entries: Vec<(usize, DVec2, [DVec2; 2])> = self + .per_glyph_bboxes + .iter() + .enumerate() + .filter_map(|(index, bbox)| { + let bbox = (*bbox)?; + let offset = self.vector_table.attribute_cloned_or_default::(ATTR_TRANSFORM, index).translation; + Some((index, offset, [bbox[0] + offset, bbox[1] + offset])) + }) + .collect(); + + let mut layer_bboxes: Vec<[DVec2; 2]> = entries.iter().map(|entry| entry.2).collect(); + let baselines: Vec = entries.iter().map(|entry| entry.1.y).collect(); + widen_horizontal_gaps(&mut layer_bboxes, &baselines); + + // Project back to glyph-local and stamp as click targets + for (entry, widened) in entries.iter().zip(layer_bboxes.iter()) { + let glyph_local = [widened[0] - entry.1, widened[1] - entry.1]; + let rect = Subpath::new_rectangle(glyph_local[0], glyph_local[1]); + self.vector_table.set_attribute(ATTR_EDITOR_CLICK_TARGET, entry.0, Vector::from_subpaths([rect], false)); + } + } + + // "Separate Glyphs" off: widen the accumulated AABBs and bundle as one override `Vector` + if !self.merged_click_target_bboxes.is_empty() { + let mut bboxes = self.merged_click_target_bboxes; + widen_horizontal_gaps(&mut bboxes, &self.merged_click_target_baselines); + + let widened_subpaths: Vec<_> = bboxes.iter().map(|[min, max]| Subpath::new_rectangle(*min, *max)).collect(); + self.vector_table.set_attribute(ATTR_EDITOR_CLICK_TARGET, 0, Vector::from_subpaths(widened_subpaths, false)); } // Fill in text frame for items that don't have one yet (single-item mode, where item 0 = identity) @@ -172,6 +206,36 @@ impl PathBuilder { } } +/// Widen AABBs horizontally so same-line neighbors fill inter-glyph gaps. +/// The shorter glyph (higher min.y) widens toward its taller neighbor; equal heights split the gap. +/// Assumes input is in reading order. Linear runtime. +fn widen_horizontal_gaps(bboxes: &mut [[DVec2; 2]], baselines: &[f64]) { + for i in 0..bboxes.len().saturating_sub(1) { + // Skip cross-line pairs (loose epsilon since baselines come from layout floats) + if (baselines[i] - baselines[i + 1]).abs() > 1e-4 { + continue; + } + + let gap = bboxes[i + 1][0].x - bboxes[i][1].x; + if gap <= 0. { + continue; + } + + let left_top = bboxes[i][0].y; + let right_top = bboxes[i + 1][0].y; + + if left_top > right_top { + bboxes[i][1].x += gap; + } else if right_top > left_top { + bboxes[i + 1][0].x -= gap; + } else { + let half = gap / 2.; + bboxes[i][1].x += half; + bboxes[i + 1][0].x -= half; + } + } +} + fn subpaths_bounding_box(subpaths: &[Subpath]) -> Option<[DVec2; 2]> { subpaths .iter()