Enable vertical point label placement via 'text-writing-mode' style property #8399

vakila · 2019-06-26T21:45:20Z

This PR completes the introduction of the 'text-writing-mode' style property (#8372) and makes use of this property to place point labels vertically if needed.
This is the counterpoint to mapbox/mapbox-gl-native#14932 and is heavily based on those changes.

Notes on the placement algorithm:

Vertical labels may only appear if 'text-writing-mode' is set and has 'vertical' as one of its values.
Only labels containing at least one character from a range that supports vertical writing (e.g. CJK characters) may be placed vertically.
Vertical placement is possible with both fixed- and variable-anchor placement.
During placement, we will try all possible orientations (and anchors, if using variable anchors) in the orders of preference described in the style, until we find a usable placement or have exhausted all possibilities without success.

See commit messages for more details about changes.

Launch Checklist

briefly describe the changes in this PR
fix broken unit tests
write tests for all new functionality (Additional render tests still needed per [core] Implement text-writing-mode layout property for symbol layer mapbox-gl-native#14932 (comment))
document any changes to public APIs
post benchmark scores
- Screenshot of version benchmarks on Ubuntu Chrome
manually test the debug page

cc @alexshalamov @asheemmamoowala @chloekraw @mapbox/gl-native @mapbox/map-design-team @mapbox/studio

vakila · 2019-07-02T00:01:28Z

Finally managed to run the benchmarks, screenshot linked in the description above.

asheemmamoowala

I'm not familiar with the symbol placement code, but there's a few nitpicky comments here.

💯 Fo all the render tests!

asheemmamoowala · 2019-07-01T22:02:38Z

src/data/bucket/symbol_bucket.js

@@ -319,6 +324,17 @@ class SymbolBucket implements Bucket {
        this.sortFeaturesByY = zOrderByViewportY && (layout.get('text-allow-overlap') || layout.get('icon-allow-overlap') ||
            layout.get('text-ignore-placement') || layout.get('icon-ignore-placement'));

+        if (layout.get('symbol-placement') === 'point') {


Have you considered doing the duplicate removal during style parsing. That way layer grouping can be done on the de-duped value. As it stands, two layers with all other properties identical, but text-writing-mode set to ['horizontal'] vs ['horizontal', 'horizontal'] will not be grouped.

With that change, the de-duping can also be done for the style, and not for each tile

Like most of this PR I had just copied what @alexshalamov had done in mapbox/mapbox-gl-native#14932, that's why this was done here. I hadn't considered doing it during style parsing (I'm not really familiar with that yet) but what you suggest makes sense to me - I'll look into it.

@asheemmamoowala same is applicable for text-variable-anchor, duplicates should be removed before layers are grouped.

Captured in #8445

src/style-spec/reference/v8.json

src/symbol/quads.js

src/symbol/symbol_layout.js

yarn.lock

asheemmamoowala · 2019-07-03T00:08:50Z

src/symbol/shaping.js

@@ -199,7 +200,8 @@ function shapeText(text: Formatted,
        left: translate[0],
        right: translate[0],
        writingMode,
-        lineCount: lines.length
+        lineCount: lines.length,
+        yOffset: -17 // the y offset *should* be part of the font metadata


This is confusing to me. If the comment is suggesting that the yOffset is hard-coded here because it is not currently part of the font metadata, can that be clarified a bit. Is that going to be worked on in a separate ticket?

It's confusing to me too. This comment predates this PR, I just moved it - it looks like it was introduced here, perhaps @ansis can shed some light on the matter?

src/symbol/placement.js

asheemmamoowala · 2019-07-03T00:21:36Z

src/symbol/placement.js

+                        const height = collisionTextBox.y2 - collisionTextBox.y1;
+                        const textBoxScale = symbolInstance.textBoxScale;
+
+                        let placedBox = { box: null, offscreen: null };


The placedBox type is Missing flow type annotation. Probably also good to annotate the return type of attemptAnchorPlacement()

For my own learning as I am still new to Flow:

how would this pass Flow tests if some annotation was missing?

why would we want to annotate placedBox but not width height etc?

If I'm reading correctly, many (most?) of the functions in this module - including attemptAnchorPlacement() - were already missing return value annotations. Should they all be added, and if so does it make sense for that to be part of this PR? Seems like it might make more sense to be addressed separately..

how would this pass Flow tests if some annotation was missing?

Flow does not test for missing annotations and cannot test objects/methods that are not annotated. That allows subtle bugs in those areas of code to be introduced without the ability to

why would we want to annotate placedBox but not width height etc?

collisionTextBox is typed as a SingleCollisionBox, implying that width and height will be treated as number types.

If I'm reading correctly, many (most?) of the functions in this module [...] were already missing return value annotations. Should they all be added, and if so does it make sense for that to be part of this PR? Seems like it might make more sense to be addressed separately..

I prefer to add annotations to methods when I make a bunch of changes to them - it allows for learning the contracts of a method, and how it is used. But this can also be tackled in a separate PR - as a larger fix.

ansis

Overall this looks great to me. Some parts are tricky to understand but I think that's just the area of the code. I don't have any suggestions to make things clearer. I think your decision to stick close to the -native pr is great and will make future adjustments easier to do in both projects.

👍

src/data/bucket/symbol_bucket.js

src/symbol/shaping.js

@@ -199,7 +200,8 @@ function shapeText(text: Formatted,
        left: translate[0],
        right: translate[0],
        writingMode,
-        lineCount: lines.length
+        lineCount: lines.length,
+        yOffset: -17 // the y offset *should* be part of the font metadata


chloekraw · 2019-07-12T23:51:07Z

@vakila flagging here too that we should wait to merge this until we address the behavior of Latin characters in vertical point labels

cc @asheemmamoowala @ansis

This reverts commit 069c6f0.

…placement - Add placedOrientation attribute to PlacedSymbol - Add verticalTextBox attributes to SymbolInstance alongside textBox & iconBox

Port changes from gl-native@alexshalamov_wip_vertical_labels (shaping.cpp, quads.cpp glyphs.hpp)

- Add writingModes and allowVerticalPlacement properties to SymbolBucket - Compute writingModes from `text-writing-mode` style property - Determine glyph dependencies based on whether vertical placement is allowed - Rotate symbols as needed

- Add updateVerticalLabels to hide glyphs for unused orientations - Modify updateVariableAnchors to hide unused orientation glyphs when using vertical labels and variable anchors

- Attempt to place vertical/horizontal boxes for each symbol according to writing mode order of preference - Integrate with variable-anchor placement to try each anchor/orientation combination possible - Prefer previously placed orientations & anchors - Hide unplaced orientations

Add yOffset property to shaping objects

Add tests for vertical placement of point labels with: - CJK text with punctuation - multiline mixed CJK/latin text - CJK with embedded Arabic RTL text

This reverts commit 3ff7480.

asheemmamoowala · 2019-07-31T06:45:40Z

we should wait to merge this until we address the behavior of Latin characters in vertical point labels

Latin characters will also be verticalized in text-writing-mode:"vertical". That is not implemented in this PR, but can be implemented in a follow up PR.

LGTM!

cc @vakila @chloekraw

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 · 2019-08-01T14:02:11Z

Latin characters will also be verticalized in text-writing-mode:"vertical".

@asheemmamoowala could you please take a look at last two commits? Initially I verticalized all characters that do not have upright glyph orientation, but then, shaped Arabic text became unreadable, so I had to exclude it (and whitespace) from verticalization. Not sure if did it correctly and whether I need to exclude other ranges.

vakila · 2019-08-02T05:12:13Z

Thanks @alexshalamov for the updates! I think excluding Arabic from verticalization is indeed the right approach 👍 As you mention, we may ultimately want to also exclude other scripts that don't work well vertically, e.g. Hindi? But I will defer to @asheemmamoowala.

asheemmamoowala

@alexshalamov The changes to keep all non-complex shaped characters upright look good to me.

Just one request - could you add a test that verifies that Arabic remains in it's natural orientation?

asheemmamoowala

@alexshalamov Before this is finally committed - it would be good to get another set of benchmark results. The benchmarks work with the existing styles so they won't be testing all the new code paths, but it will at least show the impact on existing code.

alexshalamov · 2019-08-04T11:53:12Z

@asheemmamoowala

Just one request - could you add a test that verifies that Arabic remains in it's natural orientation?

We have render test for that:

cjk-arabic-vertical-mode

Do you think there is a need to add new test?

Before this is finally committed - it would be good to get another set of benchmark results.

Sure, I will run tests on Monday and post benchmark results.

Benchmarks for vakila-vertical 7d2733f

asheemmamoowala

Do you think there is a need to add new test?

Sorry I missed that test. No additional tests should be needed.

Benchmark results look good. Is the 1+ms change in SymbolLayout expected or an anomaly in this run.

ansis · 2019-08-07T17:28:06Z

The most recent changes look great.

vakila requested review from asheemmamoowala and ansis June 26, 2019 21:45

vakila self-assigned this Jun 26, 2019

vakila force-pushed the vakila-vertical branch from 689e22a to dde0b00 Compare June 26, 2019 22:03

vakila marked this pull request as ready for review June 28, 2019 19:05

asheemmamoowala reviewed Jul 3, 2019

View reviewed changes

alexshalamov mentioned this pull request Jul 8, 2019

Introduce 'text-writing-mode' layout property that allows to control orientation of point labels #8372

Closed

6 tasks

This was referenced Jul 8, 2019

Use eslint rules for padding-line-between-statements #8444

Closed

De-duplicate array enums for symbol layer properties when parsing the style #8445

Open

alexshalamov mentioned this pull request Jul 9, 2019

[core] Implement text-writing-mode layout property for symbol layer mapbox/mapbox-gl-native#14932

Merged

13 tasks

ansis approved these changes Jul 12, 2019

View reviewed changes

This was referenced Jul 15, 2019

Favor previous anchor only when still in the text-variable-anchor options #8473

Merged

Lay out CJK text vertically in point-placed labels #3470

Closed

alexshalamov and others added 15 commits July 29, 2019 10:53

Add text-writing-mode layout property

17d5b2c

Add render tests for text-writing-mode layout property

b757541

Skip render tests for text-writing-mode property

49b5e22

Revert "Skip render tests for text-writing-mode property"

f42fdc0

This reverts commit 069c6f0.

Add 'text-writing-mode' style spec type & symbol layer prop

301b237

Add attributes to SymbolInstance & PlacedSymbol structs for vertical …

5543064

…placement - Add placedOrientation attribute to PlacedSymbol - Add verticalTextBox attributes to SymbolInstance alongside textBox & iconBox

Rotate glyphs for vertical point labels if needed & correct for y-offset

ee80a96

Port changes from gl-native@alexshalamov_wip_vertical_labels (shaping.cpp, quads.cpp glyphs.hpp)

Track vertical collision boxes in SymbolBucket, if needed

1943700

Add vertical text shaping during symbol layout, if needed

82d1add

Add vertical text collision feature during symbol layout, if needed

9efe670

Hide unused orientation glyphs when drawing symbols

40728c9

- Add updateVerticalLabels to hide glyphs for unused orientations - Modify updateVariableAnchors to hide unused orientation glyphs when using vertical labels and variable anchors

Fix symbol unit tests

ec6be6a

Add yOffset property to shaping objects

Organize text-writing-mode render tests to separate line/point tests

4a7bbdd

Anjana Vakil added 7 commits July 29, 2019 10:55

Add more render tests for vertical CJK point labels

993bcf8

Add tests for vertical placement of point labels with: - CJK text with punctuation - multiline mixed CJK/latin text - CJK with embedded Arabic RTL text

Tiny clean up

ff3dda3

Minor fixes per reviewer comments

0e3a85a

Revert "Update yarn.lock" per reviewer comment

a9ff21f

This reverts commit 3ff7480.

Deduplicate values during SymbolStyleLayer creation (not SymbolBucket)

ab22573

Add type annotations for variable anchor box placement

51ba9bb

Use .indexOf instead of .includes for IE compat; lint fix

20b8146

vakila force-pushed the vakila-vertical branch from fa5ce19 to 20b8146 Compare July 29, 2019 18:05

chloekraw added this to the release-queso milestone Jul 30, 2019

alexshalamov added 2 commits August 1, 2019 16:37

Update mixed-multiline-vertical-horizontal-mode expected results

d1f6cb4

chloekraw requested a review from asheemmamoowala August 1, 2019 17:43

asheemmamoowala suggested changes Aug 2, 2019

View reviewed changes

asheemmamoowala reviewed Aug 2, 2019

View reviewed changes

asheemmamoowala approved these changes Aug 5, 2019

View reviewed changes

ansis merged commit f867842 into master Aug 7, 2019

ansis deleted the vakila-vertical branch August 7, 2019 17:28

wipfli mentioned this pull request Dec 16, 2021

Shield rotation maplibre/maplibre-gl-js#716

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable vertical point label placement via 'text-writing-mode' style property #8399

Enable vertical point label placement via 'text-writing-mode' style property #8399

vakila commented Jun 26, 2019 •

edited

Loading

vakila commented Jul 2, 2019

asheemmamoowala left a comment

asheemmamoowala Jul 1, 2019

vakila Jul 3, 2019

alexshalamov Jul 8, 2019

asheemmamoowala Jul 8, 2019

asheemmamoowala Jul 3, 2019

vakila Jul 3, 2019

This comment was marked as resolved.

asheemmamoowala Jul 3, 2019

vakila Jul 3, 2019

asheemmamoowala Jul 8, 2019

ansis left a comment

This comment was marked as resolved.

chloekraw commented Jul 12, 2019

asheemmamoowala commented Jul 31, 2019

alexshalamov commented Aug 1, 2019

vakila commented Aug 2, 2019

asheemmamoowala left a comment

asheemmamoowala left a comment

alexshalamov commented Aug 4, 2019 •

edited

Loading

asheemmamoowala left a comment

ansis commented Aug 7, 2019

Enable vertical point label placement via 'text-writing-mode' style property #8399

Enable vertical point label placement via 'text-writing-mode' style property #8399

Conversation

vakila commented Jun 26, 2019 • edited Loading

Launch Checklist

vakila commented Jul 2, 2019

asheemmamoowala 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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansis left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

chloekraw commented Jul 12, 2019

asheemmamoowala commented Jul 31, 2019

alexshalamov commented Aug 1, 2019

vakila commented Aug 2, 2019

asheemmamoowala left a comment

Choose a reason for hiding this comment

asheemmamoowala left a comment

Choose a reason for hiding this comment

alexshalamov commented Aug 4, 2019 • edited Loading

asheemmamoowala left a comment

Choose a reason for hiding this comment

ansis commented Aug 7, 2019

vakila commented Jun 26, 2019 •

edited

Loading

alexshalamov commented Aug 4, 2019 •

edited

Loading