Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom spacing #90

Closed
wants to merge 121 commits into from
Closed

Custom spacing #90

wants to merge 121 commits into from

Conversation

benoitkugler
Copy link
Contributor

This is a draft PR to support custom spacing between words or letters.

The core implementation is complete, but I'm not sure yet what the exported API should be.

My use case will be for an 'all-in-one' segmenting-shaping-spacing-wrapping-justifying function, so that the addWordSpacing and addLetterSpacing could stay hidden, but perhaps you would have other use cases in mind ?

@whereswaldon
Copy link
Member

This is a draft PR to support custom spacing between words or letters.

The core implementation is complete, but I'm not sure yet what the exported API should be.

My use case will be for an 'all-in-one' segmenting-shaping-spacing-wrapping-justifying function, so that the addWordSpacing and addLetterSpacing could stay hidden, but perhaps you would have other use cases in mind ?

This is also my use-case, but Gio implements its own all-in-one operation for this right now. Maybe we will eventually be able to use one provided by go-text, but for now I'd prefer to have the flexibility of keeping it in Gio. So I would want these operations exported.

I don't think I understand how to use addLetterSpacing realistically though. I guess you can apply it before line wrapping in order to space out glyphs, but then you didn't know where the start/end of lines were, so you end up with extra space at the beginning/end of line wrap locations. If you were going to use this for justifying text post-wrapping, it would seem nicer to offer an API that you could supply the advance that you want to "spread" among the glyph clusters and let that do all of the work.

I'm also not sure it makes sense to define these as operations on single runs. Both word spacing and letter spacing make sense at run boundaries in multi-run text like bidi or just rich text with varying styles. Maybe these should accept []Output to act on?

@benoitkugler
Copy link
Contributor Author

benoitkugler commented Jul 10, 2023

Thank you very much for your comments, which enlighten many shortcomings.

I don't think I understand how to use addLetterSpacing realistically though. I guess you can apply it before line wrapping in order to space out glyphs, but then you didn't know where the start/end of lines were, so you end up with extra space at the beginning/end of line wrap locations. If you were going to use this for justifying text post-wrapping, it would seem nicer to offer an API that you could supply the advance that you want to "spread" among the glyph clusters and let that do all of the work.

Fair point.. but the additional spacing will also influence line wrapping, so we would need something like

  • apply additional spacing
  • line break
  • adjust the boundaries of lines to remove unwanted space at start and end of a line
  • apply alignment/justification (probably a separate PR)

Does it seems correct ?

I'm also not sure it makes sense to define these as operations on single runs. Both word spacing and letter spacing make sense at run boundaries in multi-run text like bidi or just rich text with varying styles. Maybe these should accept []Output to act on?

Hum yes, you right. We need a finer handling of run boundaries..

@andydotxyz
Copy link
Contributor

My use case will be for an 'all-in-one' segmenting-shaping-spacing-wrapping-justifying function, so that the addWordSpacing and addLetterSpacing could stay hidden, but perhaps you would have other use cases in mind ?

If we are looking all in one then it also needs to encompass tab alignment as well (where both initial and also mid-text tab characters need to align with something outside the current shaping info).
I would expect that is outside the scope, and so like Gio I expect Fyne might need to keep it internally.

It does sound like this is the right place for inserting the extra space as you say, but perhaps stopping short of an all-in-one in terms of its usage?

@benoitkugler
Copy link
Contributor Author

The last commit tries to address your comments.

There is now 3 exported methods, to be used like this :

  • for each run, add word spacing and letter spacing (that could be done by an implementation of RunIterator)
  • wrap
  • for each line, call TrimLetterSpacing to cleanup the line

Do you think such an approach is usable ?

We will probably need a similar mechanism for word spacing. By the way, how do you handle spaces at the line boundaries in the toolkits ? Does the line wrapper make sure no line begin with a space ?

@whereswaldon
Copy link
Member

The last commit tries to address your comments.

There is now 3 exported methods, to be used like this :

* for each run, add word spacing and letter spacing (that could be done by an implementation of `RunIterator`)

* wrap

* for each line, call `TrimLetterSpacing` to cleanup the line

Do you think such an approach is usable ?

Maybe. I'd like to talk through your next question and circle back to this.

We will probably need a similar mechanism for word spacing. By the way, how do you handle spaces at the line boundaries in the toolkits ? Does the line wrapper make sure no line begin with a space ?

This is actually a problem that I've been grappling with in Gio. In the default case it isn't an issue, but if we have LTR text right-aligned or RTL text left-aligned, the current line wrapper will make the glyph at the edge of the line a space. This can look terrible. Consider wrapping "the quick brown fox" in LTR but right-aligned. I'll mock out what happens in monospace below (using an underscore to indicate actual space glyphs):

  The_
quick_
brown_
   fox

One way to solve this problem would be to make the line wrapper alignment-aware so that it could instead choose:

   The
_quick
_brown
  _fox

However, this both makes the results harder to cache and the algorithm more complex.

It's tempting to say that we can just trim the space characters at the end of lines, but that doesn't work well with text editors. If you trim the space, there's no visible difference between the cursor being before and after that space glyph. It makes for a somewhat confusing editing experience.

The line wrapper also currently always respects the width of whitespace glyphs. The above examples wrap to a width of 6 monospace glyphs, but would actually behave unexpectedly if wrapping to width 5 (with WhenNecessary as a break policy):

 The_
quick
_brow
n_fox

Our current algorithm isn't smart enough to collapse the space after "quick", and thus we break "brown" across lines.

I've been meaning to research how other text toolkits and browsers handle these cases to see what the reasonable thing to do is, but I haven't gotten to it yet.

@benoitkugler Do you happen to already know how this stuff is usually handled?

whereswaldon and others added 16 commits July 12, 2023 08:56
This commit adds a mechanism to list the approximate set of covered scripts for
a rune set. I've opted to only test one rune per page in the set in the interest
of performance, so it's possible for this to miss certain covered scripts. It seems
better than nothing though.

Signed-off-by: Chris Waldon <[email protected]>
This commit adds a map to the fontmap tracking the fonts providing
some coverage on a per-script basis.

Signed-off-by: Chris Waldon <[email protected]>
This commit extends our fallback face resolution logic to check for fonts covering
the same script that the provided rune belongs. We first try only fonts with matching
aspects, and then we try all fonts with non-matching aspects. The non-matching case is
critical for making things like emoji work, as emoji rarely match the aspect of the text
containing them.

Signed-off-by: Chris Waldon <[email protected]>
Signed-off-by: Chris Waldon <[email protected]>
A future commit will actually populate the field
This commit enforces a naming convention within versions of the cache
file such that applications supporting different cache index versions
will simply maintain separate indices. typesetting will always use the
latest cache format that it knows about, creating it from scratch if that
cache version is not locally available.

Signed-off-by: Chris Waldon <[email protected]>
@benoitkugler
Copy link
Contributor Author

I've quickly look at Pango, and what they do is actually zeroing the advance of a space who ends up at the end of a line.

If you trim the space, there's no visible difference between the cursor being before and after that space glyph.

Are you sure this a problem across line boundaries ? It seems that the line break is a sufficient visual indicator, isn't it ?

The Pango behavior seems similar to what browsers are doing. See for instance the textearea this comment is written into..

Comment on lines 19 to 42
if g.RuneCount != 1 {
continue
}
r := text[g.ClusterIndex]
switch r {
case '\u0020', // space
'\u00A0', // no-break space
'\u1361', // Ethiopic word space
'\U00010100', '\U00010101', // Aegean word separators
'\U0001039F', // Ugaritic word divider
'\U0001091F': // Phoenician word separator
default:
continue
}
// we have a word separator: add space
// we do it by enlarging the separator glyph advance
// and distributing space around the glyph content
if isVertical {
run.Glyphs[i].YAdvance += additionalSpacing
run.Glyphs[i].YOffset += additionalSpacing / 2
} else {
run.Glyphs[i].XAdvance += additionalSpacing
run.Glyphs[i].XOffset += additionalSpacing / 2 // distribute space around the char
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code makes the assumption that the cluster contains only one glyph mapping to the space rune. While I hope this is true for all reasonable fonts, I suppose it isn't necessarily true. We should do one of:

  • ignore multi-glyph clusters in this loop
  • distribute the space we want to add across all glyphs in the cluster

The first option is much simpler. :D

Jacalz and others added 28 commits December 31, 2023 23:21
[segmenter] Add word segmentation
This commit ensures that strings of only whitespace runes (and other ignorable
runes for the purposes of face selection) are actually assigned a face at all.
The prior code could accidentally leave the face nil for inputs of all whitespace,
resulting in unexpected line metrics for the whitespace.

Signed-off-by: Chris Waldon <[email protected]>
* [language] use a better convention for scripts constants

* [fontscan] bump format version since the Script binary encoding has changed
* [unicodedata] add vertical orientation

* expose vertical script data for more efficient query

* [shaping] initial support for vertical glyph rotation

* [shaping] rename file for consistency with wrapping.go

* [shaping] fix vertical bounds, properly taking into account negative XBearing

* add sideways helper for outlines

* [shaping] add sideways mode to the shaper

* [shaping] add sample for RTL vertical text

* [shaping] add vertical orientation to the input segmenter

* [shaping] organize test rasterizer and add vertical script examples

* [shaping] more idiomatic samples in test

* [harfbuzz] properly scale glyph X and Y offsets

* properly apply YOffset

* fix rotation; add converter helpers

* fix dot position

* [shaping] adjust baseline for vertical sideways text

* [all] reorganize font packages

* fix doc typo

* [font] cleanup comments

* move Sideways

* use new API in merged files

* fix staticcheck warning (double import)

* merge from v0.1.1

* Update font/cmap.go

typo

Co-authored-by: Chris Waldon <[email protected]>

* Update font/renderer.go

typo

Co-authored-by: Chris Waldon <[email protected]>

* [font] reorder fields to better group variable tables

---------

Co-authored-by: Chris Waldon <[email protected]>
* properly collapse trailing white spaces

* do not reuse the same Output twice in tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants