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

Mark allow-overlap symbols visible even outside of collision grid. #7244

Merged
merged 1 commit into from
Sep 6, 2018

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented Sep 6, 2018

Fixes issue #7172, port of mapbox/mapbox-gl-native#12698.

The problem was that if allow-overlap: true symbols were placed while they were completely outside the collision grid (more than 100px past the edge of the viewport), their default opacity would get overwritten with a "not placed", and then they'd fade in once they came on-screen and got placed.

screenshot 2018-09-06 10 21 39

The solution is to force them to be visible even if they're not included in the collision grid. It seems like we should be able to just force them to be marked as "placed", but we don't want them to take up space in the collision grid.

This is unfortunately an extra cost in the inner placement loop, but it shouldn't be too much. FWIW, nothing show up in the Paint benchmark, but that would only catch a very large regression:

screenshot 2018-09-06 10 31 16

  • 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

/cc @ansis

@ChrisLoer ChrisLoer requested a review from ansis September 6, 2018 17:39
Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

👍

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.

2 participants