Full support for bidirectional line breaking #7123

ChrisLoer · 2016-11-19T01:14:18Z

Fix for #7112

Also makes small changes to handling of leading/trailing whitespace in labels:

Trim whitespace before calculating line max-width for alignment/justification
Don't crash if line-breaking algorithm creates a line with just a single whitespace character on it

mention-bot · 2016-11-19T01:14:19Z

@ChrisLoer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kkaefer, @1ec5 and @ansis to be potential reviewers.

1ec5 · 2016-11-20T11:39:03Z

src/mbgl/text/bidi.cpp

+    UErrorCode errorCode = U_ZERO_ERROR;
+    ubidi_setLine(bidiParagraph, start, end, bidiLine, &errorCode);
+
+    // TODO: At this point we're committed to the line breaking working. As a fallback we could try to just return the equivalent substring of the original reordered paragraph, but maybe we should just throw an exception?


What kinds of errors could we expect to occur here? If it’s possible for line breaking to fail due to something in the vector tile data, then we might need a graceful fallback. Otherwise, if this is more about allocation or other catastrophic failures, throwing an exception would make more sense.

I believe the assumptions are:

Input text is UTF-16

Line breaks are correctly determined (as I look more closely, I think my logic is broken right now for input that already has line breaks in it)

applyBidiToParagraph is called before applyBidiLineBreaking (I think I'll try to change the interface to make this a requirement)

No allocation errors, etc.

In other words, only programmer mistakes should lead to an error. So I'll go with an exception.

1ec5 · 2016-11-20T11:46:18Z

src/mbgl/text/glyph_set.cpp

+
+        // Collapse invisible characters.
+        uint32_t lineEndIndex = shaping.positionedGlyphs.size() - 1;
+        // TODO refactor: Seems like isVisible should be called isInvisible? Also there's an edge case here with one character invisible lines


Oh, you’re totally right, the logic should be inverted so it actually returns whether the character is visible.

Even inverted, isVisible still seems a little confusing to me -- there are lots of code points that won't put pixels on the screen other than space, newline, and zero-width space. On the other hand, we don't really care about invisible code points as long as we don't have glyphs for them (e.g. U+200E LEFT-TO-RIGHT MARK) or they have a glyph but the glyph has zero-width advance. Is what we're trying to achieve here something like trimWhitespace, applied to both the beginning and end of the line?

Essentially: when the line is centered, we don’t want the trailing newline, space, or zero-width space to shift the centered line off to one side. (Here’s the GL JS equivalent.)

I had to torture the map a little to get it to happen, but there is a crash bug here: if you have labels that include trailing line breaks (you can copy and paste them into Studio even though you can't enter them directly), you can end up with "lines" that just have a single line break on them. In that case lineEnd-- wraps around to 2^32.

Good catch! Trailing line breaks can come from other sources, such as runtime styling and hand-crafted GeoJSON too.

ChrisLoer · 2016-11-23T21:25:15Z

@1ec5, would you have a chance to review again? I tweaked the line breaking algorithm to bring it into conformance with the current ideographic breaking behavior, but there are still some subtle differences in the handling of whitespace (we can trim more than one character of whitespace from a line, we trim from the front as well as the back, and we calculate alignment after the whitespace trimming instead of before). I also added support for multi-paragaph text runs, although in practice we're unlikely to see those in labels. Most ICU errors now raise exceptions.

I see that one of the bitrise checks is failing, but I don't have a login with bitrise to see what the check is or why it's failing, so I'm not sure how to address that.

I'm also not sure how to merge the tests along with this PR. The tests are passing now pointing against the cloer_icu_multiline branch of mapbox-gl-test-suite, but the master branch contains tests that don't seem to pass on the master branch of mapbox-gl-native yet.

1ec5 · 2016-11-23T22:19:22Z

I see that one of the bitrise checks is failing, but I don't have a login with bitrise to see what the check is or why it's failing, so I'm not sure how to address that.

I’ve added you to all the gl-native apps on Bitrise.

The tests are passing now pointing against the cloer_icu_multiline branch of mapbox-gl-test-suite, but the master branch contains tests that don't seem to pass on the master branch of mapbox-gl-native yet.

If test-suite has gained new tests since gl-native’s master branch last pinned to it, you can mark those tests as ignored with links to issues in this repository that track those bugs or features, then commit those changes directly to test-suite’s master branch. Then you can rebase test-suite’s cloer_icu_multiline branch atop that change and point this gl-native branch’s package.json to that commit.

When you’re ready to land this PR, rebase test-suite’s cloer_icu_multiline branch again and update the version pin here. Quickly merge your test-suite branch to master before any intervening changes make their way in, then merge this PR.

1ec5 · 2016-11-23T22:49:28Z

src/mbgl/text/glyph_set.cpp

+    // totalWidth doesn't include the last character for magical tuning reasons. This makes the algorithm a little
+    //  more agressive about trying to fit the text into fewer lines, taking advantage of the tolerance for going a little
+    //  over maxWidth
+    for (uint32_t i = 0; i < logicalInput.size() - 1; i++) {


Can this be an iterator-style for loop?

Could be, but then we have to add extra logic to remove the last character, and I don't think the code gets much simpler. Or am I missing something?

Ah, I missed that. 👍

1ec5 · 2016-11-23T22:59:25Z

src/mbgl/text/glyph_set.cpp

+    for (std::u16string line : lines)
+    {
+        // Collapse whitespace so it doesn't throw off justification
+        boost::algorithm::trim_if(line, boost::algorithm::is_any_of(u" \t\n\v\f\r"));


This doesn’t include the ideographic space (U+3000) which is used in CJK, or some of the more exotic Western spaces such as the thin space and mutton. Since we’re bringing in ICU, can we take advantage of any character class functionality in there? Or are we concerned about it being too slow?

I experimented with that idea, but it pulled in a significant chunk of new code with the ICU UnicodeString class (which isn't used by the BiDi code), and also required an extra string copy just to do the trimming. Both of those could be non-issues performance-wise, but this didn't feel like an important enough use case to pull in the dependency. If our STL specialized ctype<char16_t>, boost::algorithm::trim would work without us having to define whitespace, but alas it doesn't seem to. Maybe for now I'll just add U+3000 to our whitespace list? I can also move the list back into 18n.cpp and just pass the predicate to trim_if.

Sure, managing the invisible character class manually sounds fine to me.

1ec5

Other than needing to handle more invisible characters – something that can happen in a separate PR if necessary – this PR looks fine to me. Someone more familiar with the mbgl codebase should take a look, though, in case I missed anything. I’d also encourage you to run the files you touched through clang-format at some point. There are some whitespace-only lines that we try to avoid in mbgl.

jfirebaugh

Minor nits only -- feel free to merge when addressed.

jfirebaugh · 2016-11-30T00:39:29Z

src/mbgl/text/bidi.cpp

+}
+
+void ProcessedBiDiText::mergeParagraphLineBreaks(std::set<int32_t>& lineBreakPoints) {
+    for (int32_t i = 0; i < ubidi_countParagraphs(bidi.bidiText); i++) {


Can/should ubidi_countParagraphs(bidi.bidiText) be extracted from the loop condition, so it's executed only once?

jfirebaugh · 2016-11-30T00:41:16Z

src/mbgl/text/bidi.cpp

+    ubidi_setLine(bidiText, start, end, bidiLine, &errorCode);
+
+    if (U_FAILURE(errorCode))
+        throw std::runtime_error("msg");


"msg" looks like a placeholder string.

…di functionality. - Trim whitespace from labels before determining their max-width for alignment. - Fix crash on labels that contain lines with only a single character of whitespace.

kkaefer · 2016-11-29T10:33:45Z

src/mbgl/util/i18n.hpp

@@ -6,9 +6,6 @@ namespace mbgl {
 namespace util {
 namespace i18n {

-/** Returns whether a character is a visible character. */
-bool isVisible(uint16_t chr);


Does this mean we're now also rendering invisible characters?

This was only used to remove trailing characters from a line, and only newline/space/zero-width space. I replaced it with trim because I think that's the semantics we're going for -- we don't want whitespace trailing/leading whitespace to affect layout. Zero-width space will get "rendered" now, although it won't affect layout unless the font we're using assigns it non-zero width.

"Invisible" sounds to me like a font property (if there's no glyph, the code point is effectively invisible, also the font may have a glyph that doesn't draw any pixels). As you note below Unicode also has lots of extra "whitespace" characters. This change doesn't affect our handling of them.

kkaefer · 2016-11-29T10:34:41Z

src/mbgl/text/glyph_set.cpp

-    const uint32_t height = (line + 1) * lineHeight;
+    for (std::u16string line : lines) {
+        // Collapse whitespace so it doesn't throw off justification
+        boost::algorithm::trim_if(line, boost::algorithm::is_any_of(u" \t\n\v\f\r"));


How does this with other whitespace characters that are in other Unicode blocks? (e.g. non-breaking spaces, thin spaces, em/en spaces, ideographic spaces etc.)

They won't be trimmed, so they may affect layout slightly. Whether they allow word breaking is controlled by i18n::allowsWordBreaking. It looks like we don't currently break on thin/em/en spaces, either.

kkaefer · 2016-11-29T10:37:33Z

src/mbgl/text/glyph_set.cpp

+    // algorithm a little
+    //  more agressive about trying to fit the text into fewer lines, taking advantage of the
+    //  tolerance for going a little
+    //  over maxWidth


comment formatting

Ack, I should have noticed this after running clang-format.

kkaefer · 2016-11-29T10:38:31Z

src/mbgl/text/bidi.cpp

+    ubidi_setLine(bidiText, start, end, bidiLine, &errorCode);
+
+    if (U_FAILURE(errorCode))
+        throw std::runtime_error("msg");


Can we please use braces around all control statement blocks?

OK, is that our standard style? I'll start doing that.

kkaefer · 2016-11-29T10:40:05Z

src/mbgl/text/bidi.cpp

+
+BiDi::BiDi() {
+    bidiText = ubidi_open();
+    bidiLine = ubidi_open();


Why two handles?

It's just the way the ICU interface works. ubidi_setLine wants two UBiDi* objects: one "input" representing the whole run of text, and one "output" representing bidirectional information for a single line.

kkaefer · 2016-11-29T10:40:33Z

src/mbgl/text/bidi.cpp

+    // Because we set UBIDI_REMOVE_BIDI_CONTROLS, the output may be smaller than what we reserve
+    //  Setting UBIDI_INSERT_LRM_FOR_NUMERIC would require
+    //  ubidi_getLength(pBiDi)+2*ubidi_countRuns(pBiDi)
+    int32_t outputLength = ubidi_getProcessedLength(bidiLine);


kkaefer · 2016-11-29T10:40:45Z

src/mbgl/text/bidi.cpp

+    //  Setting UBIDI_INSERT_LRM_FOR_NUMERIC would require
+    //  ubidi_getLength(pBiDi)+2*ubidi_countRuns(pBiDi)
+    int32_t outputLength = ubidi_getProcessedLength(bidiLine);
+    std::unique_ptr<UChar[]> outputText = std::make_unique<UChar[]>(outputLength);


We can just use auto here

Oh, right! I left C++ world at C++98, still getting used to all the great additions...

kkaefer · 2016-12-01T10:04:43Z

argh sorry; I had a few review comments, but apparently I never clicked on on submit.

ChrisLoer · 2016-12-01T17:19:52Z

@kkaefer Porting to gl-js is exposing some further changes I'll want to make (like #7253). When I make a PR for those changes I'll incorporate your suggestions from here. I don't see anything you flagged that's broken and needs immediate fixing -- let me know if you think otherwise.

kkaefer · 2016-12-01T17:33:43Z

Yeah, just minor nitpicks and suggestions.

1ec5 · 2016-12-02T11:56:21Z

package.json

@@ -24,7 +24,7 @@
    "lodash": "^4.16.4",
    "mapbox-gl-shaders": "mapbox/mapbox-gl-shaders#597115a1e1bd982944b068f8accde34eada74fc2",
    "mapbox-gl-style-spec": "mapbox/mapbox-gl-style-spec#7f62a4fc9f21e619824d68abbc4b03cbc1685572",
-    "mapbox-gl-test-suite": "mapbox/mapbox-gl-test-suite#c32d0c5ac80e3b7393bc17b8944e64fa5cffd90a",
+    "mapbox-gl-test-suite": "mapbox/mapbox-gl-test-suite#d85534f77e1b06fbfe7aa610c98a363be86fceb4",


mapbox/mapbox-gl-test-suite#186 was rebased onto that repository’s master branch, so mapbox/mapbox-gl-test-suite@d85534f has been orphaned. This line should have been changed to point to mapbox/mapbox-gl-test-suite@0c6f3e0 before this PR was merged.

Oops, sorry about that, and thanks for fixing!

Followup to #7123.

1ec5 added Core The cross-platform C++ core, aka mbgl text rendering labels Nov 19, 2016

1ec5 changed the title ~~[core] Full support for bidirectional line breaking~~ Full support for bidirectional line breaking Nov 19, 2016

1ec5 reviewed Nov 20, 2016

View reviewed changes

ChrisLoer force-pushed the cloer_icu_multiline branch 2 times, most recently from 2aa65d0 to c1ee2b8 Compare November 23, 2016 06:22

1ec5 suggested changes Nov 23, 2016

View reviewed changes

1ec5 approved these changes Nov 23, 2016

View reviewed changes

ChrisLoer force-pushed the cloer_icu_multiline branch from fad248b to 24b9538 Compare November 28, 2016 21:39

jfirebaugh approved these changes Nov 30, 2016

View reviewed changes

ChrisLoer force-pushed the cloer_icu_multiline branch 2 times, most recently from d03e46f to 2335c93 Compare November 30, 2016 18:23

[core] Full support for line breaking bidirectional text using ICU bi…

d2ba42b

…di functionality. - Trim whitespace from labels before determining their max-width for alignment. - Fix crash on labels that contain lines with only a single character of whitespace.

ChrisLoer force-pushed the cloer_icu_multiline branch from 2335c93 to d2ba42b Compare November 30, 2016 19:03

ChrisLoer merged commit 5f29c3b into master Nov 30, 2016

ChrisLoer mentioned this pull request Nov 30, 2016

Better line-breaking support for bidirectional text #7112

Closed

1ec5 mentioned this pull request Dec 1, 2016

[POC] Naïve right-to-left labeling #6057

Closed

4 tasks

kkaefer reviewed Dec 1, 2016

View reviewed changes

1ec5 reviewed Dec 2, 2016

View reviewed changes

1ec5 added a commit that referenced this pull request Dec 2, 2016

[test] Updated mapbox-gl-test-suite version

0b17de6

Followup to #7123.

1ec5 mentioned this pull request Dec 2, 2016

Update mapbox-gl-test-suite version #7276

Merged

jfirebaugh pushed a commit that referenced this pull request Dec 2, 2016

[test] Updated mapbox-gl-test-suite version

8403883

Followup to #7123.

jfirebaugh deleted the cloer_icu_multiline branch December 12, 2016 18:47

Full support for bidirectional line breaking #7123

Full support for bidirectional line breaking #7123

Conversation

ChrisLoer commented Nov 19, 2016 • edited Loading

mention-bot commented Nov 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisLoer commented Nov 23, 2016

1ec5 commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

jfirebaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkaefer commented Dec 1, 2016

ChrisLoer commented Dec 1, 2016

kkaefer commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisLoer commented Nov 19, 2016 •

edited

Loading