Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Implement text-writing-mode layout property for symbol layer #14932

Merged
merged 15 commits into from
Aug 13, 2019

Conversation

alexshalamov
Copy link
Contributor

@alexshalamov alexshalamov commented Jun 17, 2019

Brief description

For languages that support vertical writing mode, map designers might want point labels to be rendered vertically. This PR adds implementation for text-writing-mode layout property that controls preferred writing mode. New property value is an array, whose values are enumeration values from a ( horizontal | vertical ) set.

The new property do not enforce orientation of a rendered label and acts merely as a hint, so that:

  • label consisting of Latin script characters would be still rendered horizontally, even if "text-writing-mode" : ["vertical"] is specified
  • same is applicable for vertical languages, e.g., Mongolian and Manchu, "text-writing-mode" : ["horizontal"] would not affect vertical rendering of a label.

To be done

  • Add render tests (branch)
  • Select property name
  • GL-JS v8 spec documentation (branch)
  • Upstream alexshalamov_fix_diff_imports
  • Upstream GL-JS branch (alexshalamov_wip_vertical_labels) and pin gl-native upstream gl-gs
  • Reduce binary size for the feature (If needed @alexshalamov can follow-up in separate PR)
  • GL-JS port (@vakila is working on it)
  • Implement support for vertical-latin-upright writing mode
  • Implement support for vertical-latin-sideways writing mode
  • Add changelog to SDKs

Future work

  • Experiment with transitions for variable placement / label orientation switch
  • Vertical-only rendering for Mongolian / Manchu
  • De-duplicate text-variable-anchor and text-writing-mode array properties, so that layers can be efficiently grouped by layout id.

@alexshalamov alexshalamov self-assigned this Jun 17, 2019
@alexshalamov alexshalamov force-pushed the alexshalamov_wip_vertical_labels branch 5 times, most recently from 451b773 to 59983ee Compare June 18, 2019 15:06
@alexshalamov alexshalamov marked this pull request as ready for review June 18, 2019 15:55
@alexshalamov alexshalamov requested a review from a team June 18, 2019 15:55
@alexshalamov alexshalamov changed the title [WIP][core] Implement feature that allows vertical point label placement [core] Implement feature that allows vertical point label placement Jun 18, 2019
@chloekraw chloekraw removed the request for review from julianrex June 18, 2019 18:58
@friedbunny friedbunny added Core The cross-platform C++ core, aka mbgl feature text rendering labels Jun 18, 2019
@friedbunny friedbunny added this to the release-p milestone Jun 18, 2019
Copy link
Contributor

@fabian-guerra fabian-guerra left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this. We discussed that changing the return type of textVariableAnchor will require a semver, and for that reason this change will be reverted.

When we add a new enum we also have to provide a round trip conversion to NSValue. See my comment for more info.

platform/darwin/src/MGLSymbolStyleLayer.mm Outdated Show resolved Hide resolved
platform/darwin/src/MGLSymbolStyleLayer.mm Outdated Show resolved Hide resolved
@alexshalamov alexshalamov force-pushed the alexshalamov_wip_vertical_labels branch 2 times, most recently from 6ce364b to 920aeea Compare June 19, 2019 08:31
@alexshalamov alexshalamov force-pushed the alexshalamov_wip_vertical_labels branch 2 times, most recently from 388a80c to bb1bdad Compare June 19, 2019 14:04
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

👍 on the android changes.

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Most of the feedback below is about wordsmithing tweaks, probably in the code generation scripts.

In addition to the feedback below, consider adding rendering tests for the following cases:

  • Vertical CJK containing RTL Arabic text (example)
  • CJK punctuation inside vertical point-placed labels (both left-binding punctuation like periods and right-binding punctuation like opening brackets)
  • Multiline vertical CJK containing LTR Latin text

platform/darwin/scripts/generate-style-code.js Outdated Show resolved Hide resolved
platform/darwin/src/MGLSymbolStyleLayer.h Outdated Show resolved Hide resolved
platform/darwin/src/MGLSymbolStyleLayer.h Outdated Show resolved Hide resolved
platform/darwin/src/MGLSymbolStyleLayer.h Outdated Show resolved Hide resolved
src/mbgl/text/glyph.hpp Show resolved Hide resolved
@alexshalamov alexshalamov force-pushed the alexshalamov_wip_vertical_labels branch 3 times, most recently from 6def5b6 to f8d89ef Compare June 20, 2019 06:48
@alexshalamov
Copy link
Contributor Author

alexshalamov commented Jun 20, 2019

  • Vertical CJK containing RTL Arabic text (example)
  • CJK punctuation inside vertical point-placed labels (both left-binding punctuation like periods and right-binding punctuation like opening brackets)
  • Multiline vertical CJK containing LTR Latin text

@vakila As per yesterday's discussion ^^^ great input from @1ec5 about what kind of tests we might need to add.

@alexshalamov alexshalamov force-pushed the alexshalamov_wip_vertical_labels branch 2 times, most recently from ab12a7d to f915337 Compare August 5, 2019 14:38
@alexshalamov alexshalamov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 6, 2019
@alexshalamov alexshalamov changed the title [core] Implement feature that allows vertical point label placement [core] Implement text-writing-mode layout property for symbol layer Aug 7, 2019
@alexshalamov alexshalamov force-pushed the alexshalamov_wip_vertical_labels branch 2 times, most recently from b275cb6 to 19a49a5 Compare August 9, 2019 15:08
@alexshalamov
Copy link
Contributor Author

@julianrex @fabian-guerra I'm planning to land this PR early next week, please provide your comments, if you have any.

@alexshalamov alexshalamov force-pushed the alexshalamov_wip_vertical_labels branch from 19a49a5 to 6bedb2a Compare August 12, 2019 19:47
This change forces glyphs whose natural orientation in vertical writing
mode is 'sideways' to be rendered in upright orientation (only for non complex
text layouts). This is different compared to W3C / browser behavior that is by
default, renders glyphs in their respective natural orientation.

In the future, there  might need to add a new layout property that would control
glyph orientation separately (e.g., text-glyph-orientation: natural | upright).
@alexshalamov alexshalamov force-pushed the alexshalamov_wip_vertical_labels branch 2 times, most recently from 82a9811 to 955978a Compare August 13, 2019 07:16
@alexshalamov
Copy link
Contributor Author

The gl-js port of this PR has already landed, therefore, I'm landing this PR. Issues related to combination of icon-text-fit text-variable-anchor will be addressed in separate PR.

@alexshalamov alexshalamov merged commit 75874cb into master Aug 13, 2019
@alexshalamov alexshalamov deleted the alexshalamov_wip_vertical_labels branch August 13, 2019 10:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl feature iOS Mapbox Maps SDK for iOS text rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants