Enable `text-offset` with variable label placement #8642

pozdnyakov · 2019-08-15T16:17:15Z

This PR allows using of text-offset together with text-variable-anchor, if text-radial-offset is not provided. Assuming text-offset values are [x, y] the offset is calculated based on the text-variable-anchor values as following:

"left": [-x, 0]
"right": [x, 0]
"top": [0, -y]
"bottom": [0, y]
"top-left": [-x, -y]
"top-right": [x, -y]
"bottom-left": [-x, y]
"bottom-right": [x, y]

We allow only positive text-offset values for simplicity.

This is an alternate approach to #8609

Fixes #8598

asheemmamoowala · 2019-08-19T17:18:58Z

cc @mapbox/map-design-team @mapbox/studio

asheemmamoowala

This will need an update to the style-spec and their docs as well before merge.

In addition to the docs change, this is also likely to break styles where both text-variable-anchor has been specified without text-radial-offset. In those cases the text-offset is being ignored at present, but this change would consider it when computing the radial offset. This behavior change will need to be called out clearly.

pozdnyakov · 2019-08-20T08:27:21Z

In addition to the docs change, this is also likely to break styles where both text-variable-anchor has been specified without text-radial-offset.

The advantage of this approach is that it won't break the existing styles, as at the moment using of text-offset together with text-variable-anchor is not allowed by the style specification 😄

pozdnyakov · 2019-08-20T09:00:15Z

This will need an update to the style-spec and their docs as well before merge.

Done in the latest commit, please take a look

src/symbol/symbol_layout.js

ansis · 2019-08-20T16:21:47Z

I'd be in favour of remove the two-component radial offset if we implement this for text-offset. I'll take another closer look at the implementation after a studio/map design review but it looks good!

dereklieu · 2019-08-20T17:41:24Z

at the moment using of text-offset together with text-variable-anchor is not allowed by the style specification

I don't believe it's not allowed, instead having one means the other is ignored. In Studio, if you set text-offset, then set text-variable-anchor, the text-offset value is not removed from your stylesheet.

To @asheemmamoowala's point, this does have the potential to break some maps and we would need to call it out. The impact may be somewhat contained as radial offset is a relatively new feature in Studio.

The changes to the style spec look good to me.

asheemmamoowala · 2019-08-22T00:16:48Z

@ansis: I'll take another closer look at the implementation after a studio/map design review but it looks good!

@dereklieu : The changes to the style spec look good to me.

@ansis could you take a look at the implementation?

alexshalamov

Thanks for fixing comment, from my side, lgtm
As for the final approval, would be great get ✔️ from @ansis @asheemmamoowala

chloekraw · 2019-08-27T21:25:59Z

We allow only positive text-offset values for simplicity.

One thing I want to make sure is that this change still works if users enter in negative values into text-offset for backwards compatibility and user-friendliness reasons. Imagine you have text-offset set based on the documentation and then you decide to add text-variable-anchor to your style. You might assume it will just work.

One way to make this happen is to take the absolute value of the user input, and then manipulate the signs of the x,y values based on the anchor position, according to the logic from #8642 (comment). Are we doing this in the current implementation?

The documentation for text-offset could then read something like:

Layout property. Optional array of numbers. Units in ems. Defaults to [0,0]. Requires text-field. Disabled by text-radial-offset. ~~Disabled by text-variable-anchor.~~

Offset distance of text from its anchor. Positive values indicate right and down, while negative values indicate left and up.

If used with text-variable-anchor, the signs of values are automatically adjusted based on the current anchor position.

(Technically sometimes the value adjusted to 0, which arguably isn't captured in "the signs of values." I couldn't think of a concise, elegant way to explicitly capture that and I think it's probably alright to leave that out.)

cc @mapbox/map-design-team

asheemmamoowala

Just have a couple comments:

Instead of ignoring negative values in text-offset, can absolute value be used ?
Can the size of the render test images be reduced?

src/style-spec/reference/v8.json

src/symbol/symbol_layout.js

asheemmamoowala · 2019-08-28T05:01:55Z

test/integration/render-tests/text-variable-anchor/all-anchors-offset-zero/style.json

+  "version": 8,
+  "metadata": {
+    "test": {
+      "height": 256,


We try to keep the image sizes as small as possible. Would 64x64 work? Curious if you made it large for variable-anchor placements to be applicable.

Thanks for noticing! 64x64 is too small, but 128x128 fits very well

pozdnyakov · 2019-08-28T08:29:51Z

@asheemmamoowala @chloekraw thanks for your comments! Applied in the latest commits.

chloekraw · 2019-08-29T02:16:01Z

src/style-spec/reference/v8.json

@@ -2214,7 +2214,7 @@
    },
    "text-offset": {
      "type": "array",
-      "doc": "Offset distance of text from its anchor. Positive values indicate right and down, while negative values indicate left and up.",
+      "doc": "Offset distance of text from its anchor. Positive values indicate right and down, while negative values indicate left and up. Note that in combination with `text-variable-anchor` absolute values are used and they indicate radial offsets along x- and y-axis accordingly.",


@pozdnyakov few nits with this documentation entry:

Note that in combination with text-variable-anchor absolute values are used and they indicate radial offsets along x- and y-axis accordingly."

Can "radial offset" be accurately applied here if the offset methodology is rectangular?

Remove "note that"

Start sentence with "If used with text-variable-anchor," (seems more consistent with other docs entries)

It's a bit ambiguous what "they" refers to and what's taken as absolute values

My preferred entry:

If used with text-variable-anchor, input values will be taken as absolute values. Offsets along the x- and y-axis will be applied automatically based on the anchor position.

Thanks for your proposal!

ansis

I left notes in two spots where I think things could be a bit clearer but overall this looks great! thanks

ansis · 2019-08-29T05:28:18Z

src/data/bucket/symbol_attributes.js

@@ -96,7 +96,7 @@ export const symbolInstance = createLayout([
    { type: 'Uint16', name: 'numIconVertices' },
    { type: 'Uint32', name: 'crossTileID' },
    { type: 'Float32', name: 'textBoxScale'},
-    { type: 'Float32', name: 'radialTextOffset'}


Would renaming this to something else like variableTextOffset make it clearer that it isn't just the radial offset property that gets stored here?

Maybe textOffset is even better, once assigned, it is not variable any more here 🤔

ansis · 2019-08-29T05:46:05Z

src/symbol/symbol_layout.js

    }

-    return [x, y];


Number.POSITIVE_INFINITY is useful because it can be stored in the Float32Array. I think it's a bit unintuitive and could maybe be made clearer. Do you have any ideas how this could be done?

Maybe using an object here ( { isRadial: false, x: 1, y: 2 })?
And maybe moving the line radialTextOffset1 = Number.POSITIVE_INFINITY; even closer to the line where symbol instance is created so that it is clear that it is being encoded?

I don't have super clear ideas on how this could be better but I know it wasn't clear what was happening without looking elsewhere.

I've introduced const INVALID_TEXT_OFFSET = Number.POSITIVE_INFINITY;, hope it will improve readability

This PR allows using of `text-offset` together with `text-variable-anchor`, if `text-radial-offset` is not provided. Assuming we got `text-offset` [x, y] the offset is calculated based on the `text-variable-anchor` values as following: "left": [-x, 0] "right": [x, 0] "top": [0, -y] "bottom": [0, y] "top-left": [-x, -y] "top-right": [x, -y] "bottom-left": [-x, y] "bottom-right": [x, y] We allow only positive `text-offset` values for simplicity.

Co-Authored-By: Asheem Mamoowala <[email protected]>

Also, render tests image size was decreased.

chloekraw

Looks great, thank you @pozdnyakov!

Port of mapbox/mapbox-gl-js#8642

pozdnyakov mentioned this pull request Aug 15, 2019

text-radial-offset does not account for icon size or bounding boxes #8598

Closed

pozdnyakov force-pushed the mikhail_text_offset_variable_anchor branch from 5edb0b0 to cfc3284 Compare August 19, 2019 15:28

pozdnyakov self-assigned this Aug 19, 2019

pozdnyakov requested review from alexshalamov and asheemmamoowala August 19, 2019 15:32

pozdnyakov marked this pull request as ready for review August 19, 2019 15:32

pozdnyakov requested a review from ansis August 19, 2019 15:33

pozdnyakov mentioned this pull request Aug 19, 2019

Introduce two component text radial offset #8609

Closed

asheemmamoowala reviewed Aug 20, 2019

View reviewed changes

alexshalamov reviewed Aug 20, 2019

View reviewed changes

src/symbol/symbol_layout.js Outdated Show resolved Hide resolved

asheemmamoowala added this to the release-ristretto milestone Aug 22, 2019

alexshalamov approved these changes Aug 23, 2019

View reviewed changes

asheemmamoowala reviewed Aug 28, 2019

View reviewed changes

ansis requested review from tristen and removed request for tristen August 28, 2019 15:03

asheemmamoowala approved these changes Aug 28, 2019

View reviewed changes

chloekraw reviewed Aug 29, 2019

View reviewed changes

ansis approved these changes Aug 29, 2019

View reviewed changes

pozdnyakov and others added 4 commits August 29, 2019 18:27

Update specification

59afffb

Enable zero text-radial-offset

d803520

Update text-radial-offset description

8772e1f

Co-Authored-By: Asheem Mamoowala <[email protected]>

pozdnyakov added 4 commits August 29, 2019 18:27

text-offset absolute values are used with variable label placement

3aa5a22

Also, render tests image size was decreased.

Update text-offset description

36f7a13

radialText... rename

833ccda

Introduce INVALID_TEXT_OFFSET

36dbda2

pozdnyakov force-pushed the mikhail_text_offset_variable_anchor branch from 62d00bf to 36dbda2 Compare August 29, 2019 15:57

chloekraw approved these changes Aug 29, 2019

View reviewed changes

chloekraw mentioned this pull request Aug 30, 2019

[core] Enable two-component text-radial-offset to account for wide/tall icons mapbox/mapbox-gl-native#15365

Closed

pozdnyakov merged commit 7f939cb into master Aug 30, 2019

pozdnyakov deleted the mikhail_text_offset_variable_anchor branch August 30, 2019 08:35

pozdnyakov mentioned this pull request Sep 2, 2019

[core] Enable text-offset with variable label placement mapbox/mapbox-gl-native#15542

Merged

pozdnyakov added a commit to mapbox/mapbox-gl-native that referenced this pull request Sep 2, 2019

[core] Enable text-offset with variable label placement

5e90b16

Port of mapbox/mapbox-gl-js#8642

pozdnyakov added a commit to mapbox/mapbox-gl-native that referenced this pull request Sep 3, 2019

[core] Enable text-offset with variable label placement

acfbdda

Port of mapbox/mapbox-gl-js#8642

pozdnyakov added a commit to mapbox/mapbox-gl-native that referenced this pull request Sep 3, 2019

[core] Enable text-offset with variable label placement

3b5a43c

Port of mapbox/mapbox-gl-js#8642

chloekraw mentioned this pull request Sep 9, 2019

Master ticket: Variable label placement enhancements #8599

Open

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

Enable `text-offset` with variable label placement #8642

Enable `text-offset` with variable label placement #8642

pozdnyakov commented Aug 15, 2019 •

edited

Loading

asheemmamoowala commented Aug 19, 2019

asheemmamoowala left a comment

pozdnyakov commented Aug 20, 2019

pozdnyakov commented Aug 20, 2019

ansis commented Aug 20, 2019

dereklieu commented Aug 20, 2019

asheemmamoowala commented Aug 22, 2019

alexshalamov left a comment

chloekraw commented Aug 27, 2019

asheemmamoowala left a comment

asheemmamoowala Aug 28, 2019

pozdnyakov Aug 28, 2019

pozdnyakov commented Aug 28, 2019

chloekraw Aug 29, 2019

pozdnyakov Aug 29, 2019

ansis left a comment

ansis Aug 29, 2019

pozdnyakov Aug 29, 2019

ansis Aug 29, 2019

pozdnyakov Aug 29, 2019

chloekraw left a comment

Enable text-offset with variable label placement #8642

Enable text-offset with variable label placement #8642

Conversation

pozdnyakov commented Aug 15, 2019 • edited Loading

asheemmamoowala commented Aug 19, 2019

asheemmamoowala left a comment

Choose a reason for hiding this comment

pozdnyakov commented Aug 20, 2019

pozdnyakov commented Aug 20, 2019

ansis commented Aug 20, 2019

dereklieu commented Aug 20, 2019

asheemmamoowala commented Aug 22, 2019

alexshalamov left a comment

Choose a reason for hiding this comment

chloekraw commented Aug 27, 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

pozdnyakov commented Aug 28, 2019

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

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

chloekraw left a comment

Choose a reason for hiding this comment

Enable `text-offset` with variable label placement #8642

Enable `text-offset` with variable label placement #8642

pozdnyakov commented Aug 15, 2019 •

edited

Loading