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

[tile mode] Fix variable symbols placement #16141

Merged
merged 1 commit into from
Jan 27, 2020

Conversation

pozdnyakov
Copy link
Contributor

This commit allows the variable symbols to cross the tile border only if their anchor is the first anchor from the text-variable-anchor list.

This commit allows the variable symbols to cross the tile border only
if their anchor is the first anchor from the `text-variable-anchor` list.
@pozdnyakov pozdnyakov added the bug label Jan 24, 2020
@pozdnyakov pozdnyakov requested a review from riastrad January 24, 2020 08:40
@pozdnyakov pozdnyakov self-assigned this Jan 24, 2020
@riastrad
Copy link
Contributor

riastrad commented Jan 24, 2020

I've tested this locally and it seems to be correcting the behavior we observed. I'm going to QA a bit more, and if I don't catch anything then I think this should be good to go.

@pozdnyakov are we planning to add a regression test for the edge case that triggered this? (If so, do you mind linking to the PR that's tracking that?)

@pozdnyakov
Copy link
Contributor Author

@pozdnyakov are we planning to add a regression test for the edge case that triggered this?

@riastrad the existing all-anchors-tile-map-mode test already does it.. I'm now adding a bunch of new render tests for the tile mode (the new tests will have anchor visible to make the result more clear).

@pozdnyakov
Copy link
Contributor Author

mapbox/mapbox-gl-js#9235 <- another test that would catch the issue in a more clear way

Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

lgtm, new render test was tested locally and would be included in a next gl-js bump

@pozdnyakov pozdnyakov merged commit 951b90c into master Jan 27, 2020
@pozdnyakov pozdnyakov deleted the mikhail_tile_mode_variable_placement_fix branch January 27, 2020 15:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants