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

prefer breaking lines at a zero width space. #8255

Merged
merged 3 commits into from
May 23, 2019
Merged

prefer breaking lines at a zero width space. #8255

merged 3 commits into from
May 23, 2019

Conversation

ansis
Copy link
Contributor

@ansis ansis commented May 16, 2019

Mapbox data sources use zero width spaces to suggest better break points for Japanese labels. This somewhat hacky approach avoids adding more complex line breaking logic to -gl-js.

@asheemmamoowala @lily-chai

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • manually test the debug page
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port

Mapbox data sources use zero width spaces to suggest better break points
for Japanese labels. This somewhat hacky approach avoids adding more
complex line breaking logic to -gl-js.
test/unit/symbol/shaping.test.js Outdated Show resolved Hide resolved
src/symbol/shaping.js Outdated Show resolved Hide resolved
src/symbol/shaping.js Outdated Show resolved Hide resolved
@alexshalamov
Copy link
Contributor

@ansis I think I should port it to gl-native, right?

A negative penalty on zero width spaces was not enough to prioritize
them.
@ansis
Copy link
Contributor Author

ansis commented May 20, 2019

@lily-chai found that prioritizing zero width spaces couldn't effectively both break at zws and not break too often. Instead I've implemented penalization for ideographic breaks which seems to work better.

@alexshalamov yes!

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

A render test would be good too. Could this be done with a GeoJSON source?

Can you verify and update the changes to the render tests for text-max-width/ideographic-punctuation-breaking and text-max-width/ideographic-breaking?

calculatePenalty(codePoint, logicalInput.getCharCode(i + 1)),
false));
if ((i < logicalInput.length() - 1)) {
const ideographicBreak = !breakable[codePoint] && charAllowsIdeographicBreaking(codePoint);
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont see any overlap between charAllowsIdeographicBreakingand breakable codepoints. This check could be just charAllowsIdeographicBreaking;

// Penalize breaks between characters that allow ideographic breaking because
// they are less preferable than breaks at spaces (or zero width spaces).
if (penalizableIdeographicBreak) {
penalty += 150;
Copy link
Contributor

Choose a reason for hiding this comment

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

From the rendering test failures, it looks like this change regresses the balanced line breaking in #3743. The penalty is probably too high.

if (penalizableIdeographicBreak) {
penalty += 150;
}

// Penalize open parenthesis at end of line
if (codePoint === 0x28 || codePoint === 0xff08) {
penalty += 50;
Copy link
Contributor

Choose a reason for hiding this comment

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

The text-max-width/ideographic-punctuation-breaking test failure shows that this penalty is too low compared to the new penalty introduced above.

@ansis
Copy link
Contributor Author

ansis commented May 23, 2019

I've fixed the render tests by enabling the penalty only when a zws is present in the string.

Capturing a conversation with @1ec5 from chat: This exception isn't great but shouldn't pose any problems if we change our approach in the future.

@ansis ansis merged commit c7f9941 into master May 23, 2019
@tmpsantos tmpsantos deleted the zws-breaking branch June 7, 2019 11:50
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.

4 participants