parley_engine: Always shape full text, ensuring ShapedText covers the full text - #727
Conversation
| /// A span of text inside an [`Item`] with constant script and bidirectional embedding level. | ||
| #[derive(Clone, Debug)] | ||
| pub struct Item { | ||
| pub struct Segment { |
There was a problem hiding this comment.
I struggled a bit with the new names in this file.
The flow today is roughly:
- callers pass
Items toshape_text shape_textuses anItemizerto split each item range intoSegments- each
Segmentproduces one or moreShapedRuns
I found it surprising that an Itemizer produces Segments rather than Items. At the same time, Segmenter (as a rename) or similar doesn't quite feel right. Both Item and Segment also feel fairly generic 🤔
I'm wondering whether we should rename Item to ShapingContextSpan and Segment to ItemizedRun.
Then the flow would read:
- callers pass
ShapingContextSpans toshape_text - the
Itemizersplits them intoItemizedRuns - each
ItemizedRunproduces one or moreShapedRuns
I don't feel too strongly here - especially considering the TODO comments which suggest the naming and semantics may change significantly as this migration evolves. Please defer to your own best judgement here.
There was a problem hiding this comment.
Yeah, very fair. I've decided not to fix it here yet. It's likely the exact semantics are going to change a bit more.
Two bits of additional context perhaps help.
First, the distinction between the "item" and "segment" is not really a technical necessity, but it follows browser behavior: browsers reset grapheme segmentation at their "item" boundaries (style changes), but they don't reset on script changes (and Blink also doesn't reset on bidi changes). This moves us in that direction. But, if my understanding is correct, style changes don't technically need to reset grapheme segmentation, so it's just a choice.
Second, an item is user-provided, whereas segments are derived from the text.
At the same time,
Segmenter(as a rename) or similar doesn't quite feel right.
I agree, especially because it may be confused with the icu4x segmenters. One option is to perhaps name everything "items" again, where users can provide additional item boundaries and perhaps can specify whether they want those to reset grapheme segmentation or not. ShapeOptions could be provided by some callback called by parley_engine with the item ranges (i.e., the user's boundaries, plus constant bidi and script).
| // TODO: probably move this out of `ShapeOptions`, and supply it as a parameter on | ||
| // `Shaper::shape_text`. |
| // Abort on error. This happens iff `FontSelector::select_font` failed to return a | ||
| // font. By aborting we ensure `ShapedText` covers the source text contiguously (as | ||
| // we need a font to construct `ShapedRun`). | ||
| return; |
There was a problem hiding this comment.
Do we plan to return an Error from shape_text in the future?
…ghtly unfortunate form)
The style indices are text-wide, whereas the options are now per-item. Fixes the TODO from linebender#727 (comment).
…der#741) LLM Contributions: Review. The style indices are text-wide, whereas the options are per-item since linebender#727. Fixes the TODO from linebender#727 (comment).
To shape, you now call
Shaper::shape_text, which owns the itemization loop. Together with font selection being infallible (#722),ShapedTextis then guaranteed to represent the full source text contiguously. That's useful to simplify some bookkeeping. See #715 (comment) for some prior discussion on this. Because users previously looped items themselves and would callShaper::shape_item, that meant items could be skipped or submitted out-of-order.Because control flow moved into
parley_engine, some structures are introduced.Shaper::shape_texttakes an iterator of items encoding the span and holding the item'sShapeOptions(previously,parleyconstructed the options inside the loop and passed them as a parameter toShaper::shape_item). Font selection is now a trait instead of a callback, with one method for selection and one method called at the start of each segment.Note, "segment" is a new name, and encodes maximal spans within the user's items that have constant bidi level and script. The distinction between items and segments becomes more important later on, because browsers reset grapheme segmentation at item boundaries (where shaping options change), but not at segment boundaries.
Performance on the Japanese benchmarks is improved quite a bit, mostly because we're smarter about reusing the
FontSelector. Performance on Arabic and Latin is about +1.5%, but addressing the TODO I left on the the item iterator inparleyappears to make this a net small performance win on those scripts (i.e., iterating style runs and inline boxes to figure out where item boundaries are, instead of anO(chars)walk). That's for a separate PR: I just had an LLM address that TODO for an initial measurement, and have not reviewed that code at all.Benchmarks with the state as in this PR
Experimental: benchmarks after addressing the item iterator TODO