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

Add *-sort-key layout property for circle, fill, line #8467

Merged
merged 17 commits into from
Jul 18, 2019
Merged

Conversation

ahk
Copy link
Contributor

@ahk ahk commented Jul 12, 2019

This pull request adds support for three new style-spec layout properties: circle-sort-key, line-sort-key, and fill-sort-key.

These properties mirror the symbol-sort-key in terms of function and allow for features of circle, line, or fill type to specify their own sort order. Sort keys are expected to be float values in ascending order (back to front). Features with larger sort keys will be drawn in front of features with smaller sort keys.

Addition of the sort key property was motivated by cross-tile draw order issues for symbol and circle layers. Line and fill do not suffer from the same cross-tile sorting issues, but these properties are still useful for designers and are supported for flexibility and consistency.

  • 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
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

This is looking good,.

Mostly had a question about the sortFeaturesByKey flag added to each bucket. Is it really needed?

src/style-spec/reference/v8.json Outdated Show resolved Hide resolved
src/data/bucket/line_bucket.js Outdated Show resolved Hide resolved
@ahk
Copy link
Contributor Author

ahk commented Jul 16, 2019

I think the flow-type errors related to layout.get('circle-sort-key') are true positives for a bug. CircleBucket supports both CircleStyleLayer and HeatmapStyleLayer as layer types (completely didn't notice the heatmap support in the bucket) and HeatmapStyleLayer doesn't have layout properties beyond visibility, and definitely not circle-sort-key. All lookups of layout properties within CircleBucket and drawCircles need to be cased on the layer type being used which seems a bit hacky, but I think is the best path forward. There's still the majority of the bucket and draw logic being shared, and I think appropriately shared.

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.

This looks good to me! Thanks for these latest changes

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.

3 participants