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

fix #6548, never group symbol layers by layout #6556

Closed
wants to merge 1 commit into from
Closed

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Apr 23, 2018

@ChrisLoer this is a possible fix for #6548 and alternative to #6550

Instead of storing CrossTileIDs per-layer this just makes it so that symbol buckets never have more than one layer. The layer grouping is an attempted optimization to reduce bucket creation time but this doesn't really matter for symbol layers because there aren't really very many (any?) production uses for having multiple layers with the same layout properties.

EDIT: at first this seemed like a nicer approach than #6550 but I'm starting to lean back towards #6550. The other pr seems like a cleaner more logical exception, but this approach could possibly avoid some future bugs from layer sharing.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • [N/A] document any changes to public APIs
  • [N/A] post benchmark scores
  • manually test the debug page

The layer grouping optimizing doesn't ever really help for symbol layers
and removing it fixes problems related to layers sharing cross tile ids.
@ansis ansis requested a review from ChrisLoer April 23, 2018 14:30
@ChrisLoer
Copy link
Contributor

I was thinking of an approach like this too, but I realized that this approach changes behavior from pre-#5150, while #6550 (I think) restores pre-#5150 behavior.

@jfirebaugh gave an example of something you might want to do that takes advantage of grouping symbol layers by layout: add a text layer, and then add a second with text-translate in order to implement a drop-shadow. Grouping by layout allows the two layers to share collision detection behavior. I don't think that's something we have to support, but I think it's a good argument that, other things being equal, we should avoid changing behavior here.

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