Fix issue #7253: port gl-js line breaking behavior #7446

ChrisLoer · 2016-12-15T02:54:58Z

Porting newline-handling changes from gl-js -- biggest change is the "raggedness minimizing" line breaking algorithm from mapbox/mapbox-gl-js#3743.

mention-bot · 2016-12-15T02:54:59Z

@ChrisLoer, thanks for your PR! By analyzing this pull request, we identified @ansis, @1ec5 and @jfirebaugh to be potential reviewers.

jfirebaugh · 2016-12-15T18:45:52Z

src/mbgl/text/glyph_set.cpp

+    // We're iterating from last potential break towards the first
+    for (const PotentialBreak& potentialBreak : potentialBreaks) {
+        const float lineWidth = breakX - potentialBreak.x;
+        edges.push_front(LineBreakEdge{ .priorBreak = &potentialBreak,


Huh. Is this using C99 designated initializers? I was under the impression that these don't work in C++.

Apparently not part of C++ but still supported with a warning by clang and gcc. I'll switch to C++-style struct initialization -- I only picked up this style from Xcode suggesting fixes to the gl-js code I had pasted into the editor.

ChrisLoer · 2016-12-16T18:13:58Z

@brunoabinader this touches the Qt-specific bidi.cpp, although it shouldn't change behavior at all.

jfirebaugh · 2016-12-19T23:13:09Z

platform/default/bidi.cpp

+{
+}
+
+BiDi::~BiDi() {


BiDi::~BiDi() = default;

jfirebaugh · 2016-12-19T23:14:58Z

src/mbgl/text/glyph_set.cpp

+        edges.push_front(LineBreakEdge{ &potentialBreak,
+                                        calculateBadness(lineWidth, targetWidth, penalty, isLastBreak) + potentialBreak.badness });
+    }
+    edges.push_front(LineBreakEdge{ .priorBreak = nullptr, .badness = calculateBadness(breakX, targetWidth, penalty, isLastBreak)});


Replace the designated initializers here.

jfirebaugh · 2016-12-19T23:17:47Z

src/mbgl/text/glyph_set.cpp

+    //  ...and when targetWidth and maxWidth are close, strictly enforcing maxWidth can give
+    //     more lopsided results.
+
+    std::list<LineBreakEdge> edges;


Can this use std::vector instead, with pushes to the back and a reversed sort order?

I switched to using a std::vector, which for better or worse required wrapping the PotentialBreak in a shared_ptr so that I could maintain the singly-linked priorBreak list.

Ah ok. I think it's better to avoid shared_ptr, either by using a container that doesn't invalidate references on insert or by precalculating the number of potential breakpoints and preallocating the vector.

std::list is a container that doesn't invalidate references on insert, right?

jfirebaugh · 2016-12-19T23:20:16Z

src/mbgl/text/glyph_set.cpp

    }
+    potentialBreaks.push_front(evaluateBreak(static_cast<int32_t>(logicalInput.size()), currentX, targetWidth, potentialBreaks, 0, true));


Is it possible to reduce the number of static_casts in this code by using std::size_t in more places?

Pushed the static casts into bidi.cpp implementations.

jfirebaugh · 2016-12-20T22:45:11Z

src/mbgl/text/glyph_set.cpp

+    //     more lopsided results.
+
+    std::vector<LineBreakEdge> edges;
+    // We're iterating from last potential break towards the first


Is this iteration order necessary or could it use for (const auto& potentialBreak : potentialBreaks)?

Oops, the iteration order was vestigial from when I originally implemented this with a sliding window... no need to maintain that now.

jfirebaugh · 2016-12-20T22:45:54Z

src/mbgl/text/glyph_set.cpp

+    }
+    edges.push_back(LineBreakEdge(nullptr, calculateBadness(breakX, targetWidth, penalty, isLastBreak)));
+
+    std::sort(edges.begin(), edges.end(), [](const LineBreakEdge& a, const LineBreakEdge& b) {


Rather than keeping the entire edges vector, wouldn't it suffice to keep a running minimum badness and prior break?

jfirebaugh · 2016-12-20T22:46:31Z

src/mbgl/text/glyph_set.cpp

+        return a.badness < b.badness;
+    });
+
+    return std::make_shared<PotentialBreak>(PotentialBreak(breakIndex, breakX, edges.front().priorBreak, edges.front().badness));


std::make_shared<PotentialBreak>(breakIndex, breakX, ...)

jfirebaugh · 2016-12-20T22:48:23Z

src/mbgl/text/glyph_set.cpp

    }
+    potentialBreaks.push_back(evaluateBreak(logicalInput.size(), currentX, targetWidth, potentialBreaks, 0, true));


Return the result directly rather than doing push_back + back: return leastBadBreaks(*evaluateBreak(...));

ChrisLoer · 2017-01-04T22:06:25Z

@jfirebaugh @nickidlugash @1ec5 I've updated this PR with a commit that addresses some of the small issues that came up testing with Chinese map data (although the issues weren't specific to China).

jfirebaugh

👍

Don't forget to rebase test-suite and update the package.json SHA before merging.

@kkaefer

* Don't include trailing spacing (as opposed to whitespace) in lineLength * Modify BiDi interface to require initial bidi layout and linebreaking to happen in one call. * Code style changes suggested by @kkaefer

- Put "breakable" punctuation (such as a hyphen) on the line that starts the break, not the line after the break. - Process all characters with the line breaking algorithm, even if we don't have glyphs for them. Some fonts have glyph-less breakable characters (we end up treating them similarly to a "zero-width space"). - Don't include trailing white space in raggedness calculations - Make the "favor short final lines" rule more aggressive (unlike the other changes, this one is purely an aesthetic choice)

Updated changelogs to mention #7446, #7356, #7465, #7616, #7445, #7444, #7526, #7586, #7574.

Updated changelogs to mention #7446, #7356, #7465, #7616, #7445, #7444, #7526, #7586, #7574, and #7770. Also corrected the blurb about #7711.

ChrisLoer force-pushed the cloer_linebreak_port branch 2 times, most recently from f87626d to de0b349 Compare December 15, 2016 04:28

ChrisLoer self-assigned this Dec 15, 2016

ChrisLoer added the ✓ ready for review label Dec 15, 2016

jfirebaugh reviewed Dec 15, 2016

View reviewed changes

ChrisLoer force-pushed the cloer_linebreak_port branch from de0b349 to cc49840 Compare December 15, 2016 19:12

jfirebaugh suggested changes Dec 19, 2016

View reviewed changes

ChrisLoer force-pushed the cloer_linebreak_port branch from cc49840 to 3cf7eaa Compare December 20, 2016 18:59

jfirebaugh suggested changes Dec 20, 2016

View reviewed changes

ChrisLoer force-pushed the cloer_linebreak_port branch 3 times, most recently from 6c5a0e4 to a88357d Compare December 21, 2016 21:56

1ec5 mentioned this pull request Dec 29, 2016

Make gl-native handling of newlines conform to gl-js behavior #7253

Closed

ChrisLoer force-pushed the cloer_linebreak_port branch 2 times, most recently from e98a7a7 to be3d57f Compare January 4, 2017 21:26

jfirebaugh approved these changes Jan 5, 2017

View reviewed changes

1ec5 approved these changes Jan 5, 2017

View reviewed changes

ChrisLoer added 4 commits January 5, 2017 08:46

Cleaning up line breaking code.

5ccd5e2

* Don't include trailing spacing (as opposed to whitespace) in lineLength * Modify BiDi interface to require initial bidi layout and linebreaking to happen in one call. * Code style changes suggested by @kkaefer

Render newlines in labels even if there's nothing else on the line.

f910e83

Port raggedness-minimizing line breaking from gl-js.

0be31b5

ChrisLoer force-pushed the cloer_linebreak_port branch from be3d57f to 1015209 Compare January 5, 2017 17:20

ChrisLoer merged commit d5b878a into master Jan 5, 2017

1ec5 added a commit that referenced this pull request Jan 16, 2017

[ios, macos] Updated changelogs

882fd79

Updated changelogs to mention #7446, #7356, #7465, #7616, #7445, #7444, #7526, #7586, #7574.

1ec5 mentioned this pull request Jan 16, 2017

Update iOS, macOS changelogs #7731

Merged

1ec5 removed the ✓ ready for review label Jan 18, 2017

1ec5 added a commit that referenced this pull request Jan 19, 2017

[ios, macos] Updated changelogs

5c144a2

Updated changelogs to mention #7446, #7356, #7465, #7616, #7445, #7444, #7526, #7586, #7574, and #7770. Also corrected the blurb about #7711.

jfirebaugh deleted the cloer_linebreak_port branch May 11, 2017 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #7253: port gl-js line breaking behavior #7446

Fix issue #7253: port gl-js line breaking behavior #7446

ChrisLoer commented Dec 15, 2016

mention-bot commented Dec 15, 2016

jfirebaugh Dec 15, 2016

ChrisLoer Dec 15, 2016

ChrisLoer commented Dec 16, 2016

jfirebaugh Dec 19, 2016

jfirebaugh Dec 19, 2016

jfirebaugh Dec 19, 2016

ChrisLoer Dec 20, 2016

jfirebaugh Dec 20, 2016

ChrisLoer Dec 21, 2016

jfirebaugh Dec 19, 2016

ChrisLoer Dec 20, 2016

jfirebaugh Dec 20, 2016

ChrisLoer Dec 21, 2016

jfirebaugh Dec 20, 2016

jfirebaugh Dec 20, 2016

jfirebaugh Dec 20, 2016

ChrisLoer commented Jan 4, 2017

jfirebaugh left a comment

		}
		potentialBreaks.push_front(evaluateBreak(static_cast<int32_t>(logicalInput.size()), currentX, targetWidth, potentialBreaks, 0, true));

		}
		potentialBreaks.push_back(evaluateBreak(logicalInput.size(), currentX, targetWidth, potentialBreaks, 0, true));

Fix issue #7253: port gl-js line breaking behavior #7446

Fix issue #7253: port gl-js line breaking behavior #7446

Conversation

ChrisLoer commented Dec 15, 2016

mention-bot commented Dec 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisLoer commented Dec 16, 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

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 Jan 4, 2017

jfirebaugh left a comment

Choose a reason for hiding this comment