Better support for vertical text #124

benoitkugler · 2023-12-08T16:20:05Z

This PR aims at fixing #111, by supporting "sideways" glyph orientation for vertical text.

It is roughly divided in two parts :

Adding a "sideways" flag in Direction, and implementing "sideways" mode in Shape. A convenience method is provided : GlyphOutline.Sideways to be used by renderers to rotate glyph.
Resolving the preferred orientation base on Unicode rules : this is done in Segmenter.Split

The only change required for users/renderers is thus to check against the Ouput.Direction.IsSideways() boolean, to properly rotate glyphs when rendering.

Happy to read your thoughts !

(@hajimehoshi Maybe you would like to test this change ? The code in ExampleShaper_Shape and drawVGlyphs may be useful to do so)

…XBearing

hajimehoshi · 2023-12-08T17:28:08Z

Thanks! I tested Monglian and Japanese texts. Fonts are Noto Sans Monglian and M+ 1 Regular.

SetSideways(false):

SetSideways(true):

Mongolian texts are not rendered correctly in both cases.
With SetSideways(true), horizontal glyphs in Japanese like "ー" and "…" are now used. This is not correct as a vertical Japanese text. I am not sure this is intended or not. Also, alphabets are not rotated in both cases.

This is just a quick report. I'll try to make a minimal test case if needed.

benoitkugler · 2023-12-08T19:02:31Z

@hajimehoshi Thank you for the quick tests !

Could you make sure you use Segmenter.Split to properly assign orientation (based on the script) ?
If you manually specify SetSideways, you don't benefit from auto-selection.

Also, in sideways mode, it seems you do not use GlyphOutlines.Sideways when drawing ?

hajimehoshi · 2023-12-08T19:17:02Z

I'll test those tomorrow. Could you tell me what APIs were newly added?

benoitkugler · 2023-12-08T19:31:48Z

I'll test those tomorrow. Could you tell me what APIs were newly added?

So I've updated the PR description with links towards the new API : Segmenter.Split, Direction.IsSideways, GlyphOutline.Sideways. Hope it is clearer now :)

hajimehoshi · 2023-12-09T09:03:07Z

// If [yOffset] is zero, the resulting glyph will typically be displayed
// under the baseline. To position the glyph over the baseline, pass
// the YAdvance as [yOffset] (a positive value to lift the glyph up).

yOffset is float32 while YAdvance is fixed.Int26_6. How should I convert this appropriately?

hajimehoshi · 2023-12-09T09:13:02Z

I utilized Segment.Split and Glyph.Sideways without Direction.SetSideways, and the result seems much better.

The Monglians seems fine! I found that mixing Japanese characters and English characters doesn't work well. In the above figure "あHello, World.あ"'s Y offsets does match. The last あ overlaps the adjacent English sentence. Should I adjust this with HAscent for each outputs?

Thanks!

EDIT: You can test this with

go run github.com/hajimehoshi/ebiten/v2/examples/texti18n@874d48f945b0d0480d2975e2a7ad69120fd949b4

EDIT2: Arabic results was also changed unexpectedly. I'll take a look...

EDIT3: Fixed the Arabic issue at 363a4f853717300361c6c17ac51dc4e513b87aef by reversing the split inputs for RTL texts.

benoitkugler · 2023-12-09T10:13:26Z

// If [yOffset] is zero, the resulting glyph will typically be displayed
// under the baseline. To position the glyph over the baseline, pass
// the YAdvance as [yOffset] (a positive value to lift the glyph up).

yOffset is float32 while YAdvance is fixed.Int26_6. How should I convert this appropriately?

Since YAdvance has been scaled by Input.Size (output = font_units * Size / face.Upem), and Sideways expects a value in font units, you should pass YAdvance / (Size * face.Upem) to convert back to fonts units. More precisely, you could use
float32((-YAdvance / Size).Round()) / float32(face.Upem())
which is a bit inaccurate due to Rounding. A better way would be to use a fixed.Int26_6 to float32 conversion :

func toFloat32(a fixed.Int26_6) float32 {
    return float32(a) / 64
}

hajimehoshi · 2023-12-09T10:33:34Z

A better way would be to use a fixed.Int26_6 to float32 conversion :

OK, I actually did this:

func fixed26_6ToFloat32(x fixed.Int26_6) float32 { 
    return float32(x>>6) + float32(x&((1<<6)-1))/float32(1<<6)
}

EDIT: Your /64 is much better. This worked correctly even with negative numbers.

hajimehoshi · 2023-12-09T10:39:26Z

Since YAdvance has been scaled by Input.Size (output = font_units * Size / face.Upem), and Sideways expect a value in font units,

So, the simple conversion like YAdvance / 64 doesn't sound correct in terms of unit conversion, but is this actually better...?

hajimehoshi · 2023-12-09T10:52:10Z

I think I was misunderstanding. Your intention is like this?

fixed26_6ToFloat32(-YAdvance) / Size / float32(face.Upem())

hajimehoshi · 2023-12-09T11:02:43Z

I'm sorry for spamming, but

Since YAdvance has been scaled by Input.Size (output = font_units * Size / face.Upem), and Sideways expect a value in font units, you should pass YAdvance / (Size * face.Upem) to convert back to fonts units.

YAdvance is a value in the scaled units, while Sideways expects a value in the font units. So

-YAdvance / Size * face.Upem

seems correct, but am I missing something? With multiplying Upem, the rendering result is broken, but I still don't understand why -YAdvance / Size / face.Upem works (though there is still the overlapping issue). The below result is with multiplying Upem.

benoitkugler · 2023-12-09T22:21:56Z

YAdvance is a value in the scaled units, while Sideways expects a value in the font units. So
-YAdvance / Size * face.Upem
seems correct, but am I missing something?

I agree with this formula. Hum... I'll try to render the glyphs from your example and investigate tomorrow.

benoitkugler · 2023-12-11T10:52:58Z

@hajimehoshi I've rendered the Mongolian and (a sample of) Japanese texts, and have noticed none of the visual issues you have found.

See the outputs I get :

Could you try and find where your pipeline differs from this one ?

hajimehoshi · 2023-12-11T11:12:46Z

I'll check this later, but

glyphData.Sideways(float32(-g.YAdvance.Round()) / sizeFactor)

Isn't the value almost 0?

EDIT: NVM, I missed that sizeFactor := float32(out.Size.Round()) / float32(out.Face.Upem())

shaping/render_test.go

benoitkugler · 2023-12-14T14:29:45Z

There is one issue remaining when laying out mixed "sideways" and "upright" : the baseline are not nicely aligned.

So, I've explored a little bit the Opentype BASE table and thought about baseline. I think there are two different concepts :

the general support of mixed baselines in a single line (for both horizontal and vertical directions), which is tricky and requires font knowledge
the adjustment of 'sideways' and 'upright' text

Implementing the first one is probably desirable, but postponed to another PR.

Implementing the second one is visually more important, and may be done quite simply, as in 582c627. Now, calling Line.AdjustBaseline just before rendering will make 'sideways' text follow the same convention than 'upright', that is roughly placing the baseline in the middle of each glyph.

We may want to fine tune the implementation of Line.AdjustBaseline latter, but it seems to be a good enough start.

@hajimehoshi Could you confirm that using Line.AdjustBaseline visually improves the rendering ?

whereswaldon

@benoitkugler Thanks, as always, for the excellent work here. It's exciting to see us grow better vertical text support. The changes make sense, and seem to be implemented at the right level of abstraction. It's unclear to me whether this needs a second review from @andydotxyz. I don't think the public API is breaking, but it is changing (the new exported fields/methods on Direction).

@hajimehoshi Thank you so much for the excellent bug reports and features that drove this change. Thanks to your efforts, Ebitengine, Gio, Fyne, and (hopefully) many more projects will now have more correct text shaping.

opentype/api/font.go

hajimehoshi · 2023-12-14T15:44:02Z

@benoitkugler

Now everything looks fine. Thank you very much!

What I did is to add these lines to expect the side effect of AdjustBaseline. I hope this is correct.

line := shaping.Line{out}
line.AdjustBaseline()

// After that, `out` is directly used and `line` is no longer used.

hajimehoshi · 2023-12-14T15:50:15Z

I found an interesting thing: the baseline varies for an English sentence. In the above figure, the existence of g matters. Is this intended?

hajimehoshi · 2023-12-14T15:55:49Z

I found an interesting thing: the baseline varies for an English sentence. In the above figure, the existence of g matters. Is this intended?

Unfortunately this is an issue as the baseline are not alinged with English texts. In the above figure, I rendered "Hello\nEbitengine\nWorld" with the same line spaces, but the baseline are not alinged as expected.

whereswaldon · 2023-12-14T16:02:41Z

Perhaps we should align the text not by the ascent/descent of the particular glyphs within it, but by the face's advertised ascent/descent? That should be independent of specific glyph contents, and it should look decent.

whereswaldon · 2023-12-14T16:05:26Z

I found an interesting thing: the baseline varies for an English sentence. In the above figure, the existence of g matters. Is this intended?

Unfortunately this is an issue as the baseline are not alinged with English texts. In the above figure, I rendered "Hello\nEbitengine\nWorld" with the same line spaces, but the baseline are not alinged as expected.

This particular issue is more of a line-spacing/line-height problem. The typesetting repo doesn't currently implement this part of the text display process internally. It's up to applications to decide where to start the baseline of each line of text. See the discussion here for some examples of the complexities inherent in this space: gioui/gio#123

hajimehoshi · 2023-12-14T16:26:40Z

Perhaps we should align the text not by the ascent/descent of the particular glyphs within it, but by the face's advertised ascent/descent? That should be independent of specific glyph contents, and it should look decent.

I agree with you. A face's metrics should be used rather than each glyph's metrics.

It's up to applications to decide where to start the baseline of each line of text.

OK, but if an application moves the dot position for each line with a constant interval, each line should be aligned, right?

benoitkugler · 2023-12-14T16:35:33Z

OK, but if an application moves the dot position for each line with a constant interval, each line should be aligned, right?

Well, in theory, the shaper (harfbuzz) could decide to move glyphs up (for instance if the font provides a positioning table), so I'm not sure this promise holds.
Also, consider lines with different text sizes, or even with different fonts.

benoitkugler · 2023-12-14T16:58:53Z

This is what I get using the font line extents (left) vs glyph extents (right).
It seems to me the left version still feels a bit akward..

hajimehoshi · 2023-12-14T17:00:26Z

Also, consider lines with different text sizes, or even with different fonts.

Yeah, but in my example, the same size and face were used. I'm OK even if lines are not aligned when multiple faces or sizes are used.

hajimehoshi · 2023-12-14T17:04:06Z

It seems to me the left version still feels a bit akward..

In Chrome browsers, the baseline seems the same as the right. 'g' is put so that the glyph is extended to the left side. I think this makes sense.

whereswaldon · 2023-12-14T17:05:36Z

OK, but if an application moves the dot position for each line with a constant interval, each line should be aligned, right?

Well, in theory, the shaper (harfbuzz) could decide to move glyphs up (for instance if the font provides a positioning table), so I'm not sure this promise holds. Also, consider lines with different text sizes, or even with different fonts.

I think conceptually it's correct to shift the gap between dots by a fixed value. It's true that situationally the glyphs may look misaligned if the shaper did something fancy, but that would be a feature, not a bug.

whereswaldon · 2023-12-14T17:08:02Z

It seems to me the left version still feels a bit akward..

In Chrome browsers, the baseline seems the same as the right. 'g' is put so that the glyph is extended to the left side. I think this makes sense.

To achieve this, would we shift the glyphs by half the distance from the face's baseline to its ascent? I'd expect that to visually center the above-the-baseline portion of the text. I don't know for sure if this would be correct for all scripts, but it certainly would be for Latin and CJK.

benoitkugler · 2023-12-14T17:24:43Z

To achieve this, would we shift the glyphs by half the distance from the face's baseline to its ascent? I'd expect that to visually center the above-the-baseline portion of the text. I don't know for sure if this would be correct for all scripts, but it certainly would be for Latin and CJK.

Hum, I've just tried that and it seems to move the glyphs a bit too much down. I think the proper way to go would be to align the baseline : ideographic and roman.
So we would detect what baseline to use based on the script and align them.

However, it seems to me this does not solve the "only english" example. Maybe, in this case we could shift by the font advance as @whereswaldon suggested earlier.

benoitkugler · 2023-12-14T17:28:28Z

Well, what do you think of handling the baseline issue in a separate PR ? This one is already large and it seems we need more discussion on how to do it anyway.

whereswaldon · 2023-12-14T17:30:10Z

Well, what do you think of handling the baseline issue in a separate PR ? This one is already large and it seems we need more discussion on how to do it anyway.

Fine by me

hajimehoshi · 2023-12-14T17:30:34Z

Well, what do you think of handling the baseline issue in a separate PR ? This one is already large and it seems we need more discussion on how to do it anyway.

+1. I'm fine with this issue addressed in another PR. I really appreciate your team's hard work!

hajimehoshi · 2023-12-19T04:02:39Z

@andydotxyz friendly ping

andydotxyz

Thanks for this, and my apologies for the delay.
I had hoped to test this by using it in our render package but I am not managing to get the time for such coding sorry.

A couple of comments inline, the only one that is a change request being the typo in docs.
I do wonder though, and may welcome other comments, if adding the rasterising render code here might start duplicating the render repo and whether it would be worth testing this through usage of the rasteriser we maintain as a public API?

andydotxyz · 2023-12-20T08:46:41Z

di/direction.go

 }

+// SwitchAwis switch from horizontal to vertical (and vice versa), preserving


SwitchAxis probably ;) The linter should have caught this, perhaps our CI needs a few improvements.
Possibly "switches" not "switch" as well if this line is being updated.

Updated, thank you.

andydotxyz · 2023-12-20T08:52:12Z

shaping/output.go

+// Note that this method only update cross-axis metrics,
+// so that the advance is preserved. As such, it is valid
+// to call this method after line wrapping, for instance.
+func (l Line) AdjustBaseline() {


Reading the doc implies this should be called AlignBaselines instead.
Just a thought, not a change request.

I've added an "s" to imply each run may be shifted.

andydotxyz · 2023-12-20T08:53:47Z

shaping/render_test.go

@@ -0,0 +1,195 @@
+package shaping


We might want to look at whether this is needed in the typesetting repo as there is more render work in the render repository, it would be a shame to duplicate for test purposes.

I agree this file seems a bit out of place, but, in the meantime, I found this quick and dirty visualisation tool very handy when developing.

As you said, a better solution would be to use render. Perhaps we can keep this file for now, and once render supports vertical text, move the examples there and remove shaping/render_test.go ?

andydotxyz

Thanks

benoitkugler added 12 commits November 28, 2023 11:56

[unicodedata] add vertical orientation

22adb71

Merge branch 'main' into vertical-scripts

e439bd9

expose vertical script data for more efficient query

78a27fe

Merge branch 'vertical-bounds' into vertical-scripts

d06c8e8

[shaping] initial support for vertical glyph rotation

2f88fba

[shaping] rename file for consistency with wrapping.go

93d0246

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

12a97d9

…XBearing

add sideways helper for outlines

cdf3a0c

[shaping] add sideways mode to the shaper

0361ff7

[shaping] add sample for RTL vertical text

a8f0dee

[shaping] add vertical orientation to the input segmenter

cb04a76

Merge branch 'main' into vertical-scripts

8b5e415

benoitkugler requested review from whereswaldon and andydotxyz as code owners December 8, 2023 16:20

benoitkugler linked an issue Dec 8, 2023 that may be closed by this pull request

How to render vertical texts #111

Closed

[shaping] organize test rasterizer and add vertical script examples

9796152

hajimehoshi reviewed Dec 12, 2023

View reviewed changes

shaping/render_test.go Outdated Show resolved Hide resolved

benoitkugler added 2 commits December 13, 2023 10:32

fix dot position

291c26f

[shaping] adjust baseline for vertical sideways text

582c627

whereswaldon approved these changes Dec 14, 2023

View reviewed changes

opentype/api/font.go Outdated Show resolved Hide resolved

fix doc typo

d4aaea9

[shaping] fix image dims computation in visual tests

03cc2f6

andydotxyz requested changes Dec 20, 2023

View reviewed changes

Cleanup names and documentation

efc3936

andydotxyz approved these changes Dec 21, 2023

View reviewed changes

benoitkugler merged commit 48cc05a into main Dec 21, 2023
20 checks passed

benoitkugler deleted the vertical-scripts branch December 21, 2023 12:45

hajimehoshi mentioned this pull request Jan 27, 2024

inconsistent baselines for vertical texts #135

Open

		}

		// SwitchAwis switch from horizontal to vertical (and vice versa), preserving

Better support for vertical text #124

Better support for vertical text #124

Conversation

benoitkugler commented Dec 8, 2023 • edited Loading

hajimehoshi commented Dec 8, 2023

benoitkugler commented Dec 8, 2023

hajimehoshi commented Dec 8, 2023

benoitkugler commented Dec 8, 2023

hajimehoshi commented Dec 9, 2023 • edited Loading

hajimehoshi commented Dec 9, 2023 • edited Loading

benoitkugler commented Dec 9, 2023 • edited Loading

hajimehoshi commented Dec 9, 2023 • edited Loading

hajimehoshi commented Dec 9, 2023

hajimehoshi commented Dec 9, 2023

hajimehoshi commented Dec 9, 2023 • edited Loading

benoitkugler commented Dec 9, 2023 • edited Loading

benoitkugler commented Dec 11, 2023 • edited Loading

hajimehoshi commented Dec 11, 2023 • edited Loading

benoitkugler commented Dec 14, 2023

whereswaldon left a comment

Choose a reason for hiding this comment

hajimehoshi commented Dec 14, 2023

hajimehoshi commented Dec 14, 2023

hajimehoshi commented Dec 14, 2023 • edited Loading

whereswaldon commented Dec 14, 2023

whereswaldon commented Dec 14, 2023

hajimehoshi commented Dec 14, 2023

benoitkugler commented Dec 14, 2023 • edited Loading

benoitkugler commented Dec 14, 2023

hajimehoshi commented Dec 14, 2023

hajimehoshi commented Dec 14, 2023

whereswaldon commented Dec 14, 2023

whereswaldon commented Dec 14, 2023

benoitkugler commented Dec 14, 2023 • edited Loading

benoitkugler commented Dec 14, 2023

whereswaldon commented Dec 14, 2023

hajimehoshi commented Dec 14, 2023

hajimehoshi commented Dec 19, 2023

andydotxyz left a comment

Choose a reason for hiding this comment

andydotxyz Dec 20, 2023

Choose a reason for hiding this comment

benoitkugler Dec 20, 2023

Choose a reason for hiding this comment

andydotxyz Dec 20, 2023

Choose a reason for hiding this comment

benoitkugler Dec 20, 2023

Choose a reason for hiding this comment

andydotxyz Dec 20, 2023

Choose a reason for hiding this comment

benoitkugler Dec 20, 2023

Choose a reason for hiding this comment

andydotxyz left a comment

Choose a reason for hiding this comment

benoitkugler commented Dec 8, 2023 •

edited

Loading

hajimehoshi commented Dec 9, 2023 •

edited

Loading

hajimehoshi commented Dec 9, 2023 •

edited

Loading

benoitkugler commented Dec 9, 2023 •

edited

Loading

hajimehoshi commented Dec 9, 2023 •

edited

Loading

hajimehoshi commented Dec 9, 2023 •

edited

Loading

benoitkugler commented Dec 9, 2023 •

edited

Loading

benoitkugler commented Dec 11, 2023 •

edited

Loading

hajimehoshi commented Dec 11, 2023 •

edited

Loading

hajimehoshi commented Dec 14, 2023 •

edited

Loading

benoitkugler commented Dec 14, 2023 •

edited

Loading

benoitkugler commented Dec 14, 2023 •

edited

Loading