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

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

Merged
merged 1 commit into from
Jul 16, 2019

Conversation

asheemmamoowala
Copy link
Contributor

Addresses #8397

The text-variable-anchor implementation favored the previous anchor placement for symbols, even if the previous anchor was no longer an option in the text-variable-anchor value (when changed by setStlye or setlayoutProperty)

The benchmarks are unaffected because they don't exercise the text-variable-anchor property codepath. They use streets-v10 style which predates this property.

This PR also needs to be ported to gl-native:

https://github.com/mapbox/mapbox-gl-native/blob/a0e2064f75a229a0bc369ce023e2eaaccb6af074/src/mbgl/text/placement.cpp#L198-L202

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • 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

cc @mapbox/studio @mapbox/gl-native @vakila

Copy link
Contributor

@samanpwbb samanpwbb left a comment

Choose a reason for hiding this comment

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

This change will make text-variable-anchor updates deterministic in many cases, but doesn't fully resolve the problem.


Here's an example sequence in Studio where this change will result in a highly deterministic label placement outcome:

  1. ["top", "bottom"] // starting value
  2. ["top"] // user deletes bottom
  3. ["bottom"] // user adjusts top value to bottom
  4. ["bottom", "center"] // user adds a new element (starting value is always center)
  5. ["bottom", "top"] // users adjusts center value to top.

And their resulting style would pretty accurately reflect how the style will be rendered after a hard refresh.

Here's another sequence that would result in a less deterministic label placement outcome:

  1. ["top", "bottom"]
  2. ["top", "bottom", "center"] // user adds another element
  3. ["top", "bottom", "top"] // user changes 3rd element's value to top
  4. ["bottom", "top"] // user deletes the first stop

The property value is the same as in the first example, but the preference for "top" then "bottom" is never cleared.


I see two possible follow-up tasks:

That said, I think this PR gets us pretty far and unblocks Studio well enough.

@asheemmamoowala
Copy link
Contributor Author

Prevent duplicate values: #8370, which would make step (2) from the second example impossible.

The de-duplication is also ticketed out in #8445

Always clear all preferences whenever text-variable-anchor changes via setPaintProperty / setStyle.

With #8399 this will require clearing out the preferred placement for multiple properties.

@peterqliu peterqliu self-requested a review July 15, 2019 23:38
Copy link
Contributor

@peterqliu peterqliu left a comment

Choose a reason for hiding this comment

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

🚢

Copy link
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

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

LGTM!

@pozdnyakov
Copy link
Contributor

Port on gl-native: mapbox/mapbox-gl-native#15129

@asheemmamoowala asheemmamoowala merged commit 8e77fc8 into master Jul 16, 2019
@asheemmamoowala asheemmamoowala deleted the fix-8397 branch July 16, 2019 16:25
pozdnyakov added a commit to mapbox/mapbox-gl-native that referenced this pull request Jul 17, 2019
pozdnyakov added a commit to mapbox/mapbox-gl-native that referenced this pull request Jul 17, 2019
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