Fix icon-fit with variable label placement #8755

arindam1993 · 2019-09-14T01:46:06Z

This is an attempt to isolate and cherry pick just the fix required for #8583 from #8737 .
The issue seems to be caused by the fact that draw_symbol.js:#updateVariableAnchors applies an offset to the text glyphs , but never touches the icon buffer at all.

This PR adds an attribute to the placedSymbol buffers, called associatedSymbolIndex, this lets the text placedSymbol instances know which icon they're associated with.
So at render time the offsets can be applied in both buffers.
It also moves the offset calculation out of the paint loop, since icons and text need to share the same data.

This still leaves out making icon-text-fit update the collision box (#8722) and making it work with vertical labels.

UPDATE:
demo, 1 frame desynch fixed

Added a render test
Before (on master):

After:

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
~~document any changes to public APIs~~
post benchmark scores
manually test the debug page
~~tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes~~
~~tagged @mapbox/gl-native if this PR includes shader changes or needs a native port~~

src/data/bucket/symbol_bucket.js

asheemmamoowala · 2019-09-17T21:34:37Z

debug/8583.html

@@ -0,0 +1,133 @@
+<!DOCTYPE html>


Are you planning to commit this file? It would be more useful for the next developer if it was named more appropriately.

vakila

LGTM! Thanks for adding the render test @arindam1993 , both that & the debug page (on Linux Firefox/Chrome) look good 💚

One question below, and relatedly: do we need tests that this works also when combined with multi-line and/or vertical labels?

Some of those (currently ignored) tests are failing, for example text-writing-mode/point_label/mixed-multiline-vertical-horizontal-mode-icon-text-fit, and the failure doesn't look to me like it has to do with the 1px issue addressed in #8741. Thoughts?

vakila · 2019-09-17T23:01:33Z

src/render/draw_symbol.js

-            updateVariableAnchors(bucket, rotateWithMap, pitchWithMap, variableOffsets, symbolSize,
-                                  tr, labelPlaneMatrix, coord.posMatrix, tileScale, size);
-        } else if (isText && size && bucket.allowVerticalPlacement) {
+        } else if (isText && size && bucket.allowVerticalPlacement && !variablePlacement) {


Perhaps I'm missing something but why the && !variablePlacement here? IIRC we can have both vertical & variable placement enabled at the same time, are we accounting for that case?

No, we're not. In the interest of time time this PR only handles icon-text-fit rendering for horizontal variable anchor labels.

👌 Maybe we should ticket that out

vakila · 2019-09-17T23:02:01Z

src/render/draw_symbol.js

+    const variablePlacement = layer.layout.get('text-variable-anchor');
+
+    //Compute variable-offsets before painting since icons and text data positioning
+    //depend on each other in this case.


arindam1993 · 2019-09-17T23:53:44Z

LGTM! Thanks for adding the render test @arindam1993 , both that & the debug page (on Linux Firefox/Chrome) look good 💚

One question below, and relatedly: do we need tests that this works also when combined with multi-line and/or vertical labels?

Some of those (currently ignored) tests are failing, for example text-writing-mode/point_label/mixed-multiline-vertical-horizontal-mode-icon-text-fit, and the failure doesn't look to me like it has to do with the 1px issue addressed in #8741. Thoughts?

Yes, will address that and shifting of icon collision boxes in a separate PR. This was intended as a fix to the biggest visual glitch.

vakila

👍 since we're not attempting to resolve the combo-vertical-variable-placement issue in this PR

asheemmamoowala

Just a few additional notes:

This PR does not change the behavior for multi-line labels - that case remains broken.
Works well with CJK characters
Needs follow up issues for fixing icon-text-fit with vertical symbols and fixing collision boxes

src/data/array_types.js

asheemmamoowala · 2019-09-18T02:58:52Z

src/symbol/symbol_layout.js

-    const verticalTextBoxStartIndex = verticalTextCollisionFeature ? verticalTextCollisionFeature.boxStartIndex : bucket.collisionBoxArray.length;
-    const verticalTextBoxEndIndex = verticalTextCollisionFeature ? verticalTextCollisionFeature.boxEndIndex : bucket.collisionBoxArray.length;
-
+    //Place icon first, so text can have a reference it its index in the placed symbol array.


nit: typo in reference it its index

asheemmamoowala · 2019-09-18T04:37:45Z

src/render/draw_symbol.js

+        const size = symbolSize.evaluateSizeForZoom(sizeData, tr.zoom);
+
+        const s = pixelsToTileUnits(tile, 1, painter.transform.zoom);
+        const labelPlaneMatrix = symbolProjection.getLabelPlaneMatrix(coord.posMatrix, pitchWithMap, rotateWithMap, painter.transform, s);


nit: Avoid temporary variable buffers and s

kkaefer

Is the intention to fix text-writing-mode/point_label/cjk-variable-anchors-vertical-horizontal-mode-icon-text-fit as well? Regardless of the icon-text-fit aspect, it seems like the

Here's the native version with collision boxes activated (which arguably seems wrong too):

And this is the JS version in this branch:

for reference, this is the JS version before this patch:

It looks like there are no tests that are unignored, which means there are render test cases missing that were broken in the old behavior, but work with this patch applied. Could you please add some?

arindam1993 · 2019-09-18T20:47:53Z

Is the intention to fix text-writing-mode/point_label/cjk-variable-anchors-vertical-horizontal-mode-icon-text-fit as well? Regardless of the icon-text-fit aspect, it seems like the

Here's the native version with collision boxes activated (which arguably seems wrong too):

And this is the JS version in this branch:

for reference, this is the JS version before this patch:

No I didn't intend to fix that, this change only ensures that the offset thats calculated and applied to text-variable-anchor at render time updateVariableAnchors gets applied to the icon quad as well, it doesnt touch placement or layout.

It looks like there are no tests that are unignored, which means there are render test cases missing that were broken in the old behavior, but work with this patch applied. Could you please add some?

I have added one(see original PR body), but its not fully representative of this fix since its doing collision. Updating so icon/text-overlap is true so collision is ignored but the probelm with un-offset icons is still surfaced.

Master:

This branch:

arindam1993 · 2019-09-18T21:56:31Z

@asheemmamoowala
Cleaner render tests added:
Master:
icon-text-fit/text-variable-anchor:

icon-text-fit/text-variable-anchor-overlap:

This branch:
icon-text-fit/text-variable-anchor:

icon-text-fit/text-variable-anchor-overlap:

arindam1993 · 2019-09-20T02:08:26Z

I've fixed the vertical label getting hidden issue. this is what the text-writing-mode/point_label/cjk-variable-anchors-vertical-horizontal-mode-icon-text-fit looks like now:

The box sizing is still incorrect, but this PR isn't intended to fix that.

stale

kkaefer

Thanks for looking at the vertical icons!

Arindam Bose added 5 commits September 13, 2019 18:16

initial icon offset syncing implementation

e24bc4b

ensure u_label_plane_matrix is passed to icon shader

e479fc5

fix broken render tests

cb04ddf

compute variable placement text-offsets before painting

868ff57

Add render test

3a13ebc

arindam1993 changed the title ~~[dnm] Attempting to fix icon-fit with variable label placement~~ Attempting to fix icon-fit with variable label placement Sep 17, 2019

arindam1993 marked this pull request as ready for review September 17, 2019 20:42

arindam1993 changed the title ~~Attempting to fix icon-fit with variable label placement~~ Fix icon-fit with variable label placement Sep 17, 2019

mourner reviewed Sep 17, 2019

View reviewed changes

src/data/bucket/symbol_bucket.js Show resolved Hide resolved

asheemmamoowala reviewed Sep 17, 2019

View reviewed changes

Rename debug page based on CR

93ad8e7

arindam1993 added this to the release-ristretto milestone Sep 17, 2019

vakila reviewed Sep 17, 2019

View reviewed changes

vakila self-requested a review September 18, 2019 00:18

vakila approved these changes Sep 18, 2019

View reviewed changes

asheemmamoowala previously requested changes Sep 18, 2019

View reviewed changes

kkaefer requested changes Sep 18, 2019

View reviewed changes

Arindam Bose added 2 commits September 18, 2019 12:50

Remove private accessToken and spritesheet

b943aa5

Address more CR comments

5d1184e

Arindam Bose added 2 commits September 18, 2019 13:49

Update render test to ignore collision.

c5fcaa0

make render tests less noisy and add two separate cases

ad28ddc

Make vertical labels slightly more robust

e0186a3

kkaefer approved these changes Sep 20, 2019

View reviewed changes

kkaefer merged commit 1dac2c8 into master Sep 20, 2019

kkaefer deleted the fix-8583 branch September 20, 2019 06:17

kkaefer mentioned this pull request Sep 25, 2019

port fix for icon-text-fit combined with variable text placement #8737

Closed

snyk-bot mentioned this pull request Dec 5, 2019

[Snyk] Upgrade mapbox-gl from 1.3.1 to 1.5.0 dyna-dot/iClient-JavaScript-s#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix icon-fit with variable label placement #8755

Fix icon-fit with variable label placement #8755

arindam1993 commented Sep 14, 2019 •

edited

Loading

asheemmamoowala Sep 17, 2019

vakila left a comment

vakila Sep 17, 2019

arindam1993 Sep 17, 2019

vakila Sep 18, 2019

vakila Sep 17, 2019

arindam1993 commented Sep 17, 2019

vakila left a comment

asheemmamoowala left a comment

asheemmamoowala Sep 18, 2019

asheemmamoowala Sep 18, 2019

kkaefer left a comment

arindam1993 commented Sep 18, 2019

arindam1993 commented Sep 18, 2019

arindam1993 commented Sep 20, 2019

kkaefer left a comment

Fix icon-fit with variable label placement #8755

Fix icon-fit with variable label placement #8755

Conversation

arindam1993 commented Sep 14, 2019 • edited Loading

Launch Checklist

asheemmamoowala Sep 17, 2019

Choose a reason for hiding this comment

vakila left a comment

Choose a reason for hiding this comment

vakila Sep 17, 2019

Choose a reason for hiding this comment

arindam1993 Sep 17, 2019

Choose a reason for hiding this comment

vakila Sep 18, 2019

Choose a reason for hiding this comment

vakila Sep 17, 2019

Choose a reason for hiding this comment

arindam1993 commented Sep 17, 2019

vakila left a comment

Choose a reason for hiding this comment

asheemmamoowala left a comment

Choose a reason for hiding this comment

asheemmamoowala Sep 18, 2019

Choose a reason for hiding this comment

asheemmamoowala Sep 18, 2019

Choose a reason for hiding this comment

kkaefer left a comment

Choose a reason for hiding this comment

arindam1993 commented Sep 18, 2019

arindam1993 commented Sep 18, 2019

arindam1993 commented Sep 20, 2019

kkaefer left a comment

Choose a reason for hiding this comment

arindam1993 commented Sep 14, 2019 •

edited

Loading