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 line-sort-key and fill-sort-key #8383

Closed
wants to merge 6 commits into from
Closed

Add line-sort-key and fill-sort-key #8383

wants to merge 6 commits into from

Conversation

ahk
Copy link
Contributor

@ahk ahk commented Jun 24, 2019

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

These properties mirror the symbol-sort-key in terms of function and allow for features of either 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.

Launch Checklist

  • 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

cc @mapbox/studio @mapbox/map-design-team @mapbox/gl-native

@ahk ahk force-pushed the fill-sort-key branch from a658c79 to d61792d Compare June 24, 2019 10:37
@ahk ahk marked this pull request as ready for review June 24, 2019 19:01
@chloekraw chloekraw requested a review from ansis June 24, 2019 19:03
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.

Overall this looks great! The only changes that need to be made are updating the js version number that will support this feature to 1.2.0

"type": "number",
"doc": "Sorts features in ascending order based on this value. Features with a higher sort key will appear above features with a lower sort key.",
"sdk-support": {
"js": "1.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be 1.2.0

"type": "number",
"doc": "Sorts features in ascending order based on this value. Features with a higher sort key will appear above features with a lower sort key when they overlap. Features with a lower sort key will have priority over other features when doing placement.",
"sdk-support": {
"js": "1.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be 1.2.0 as well

@tmpsantos
Copy link
Contributor

  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port

Thanks for the heads up

@andrewharvey
Copy link
Collaborator

👍

Though it looks like this won't support sorting based on feature-state:

fill-sort-key: "feature-state" data expressions are not supported with layout properties.

That's okay, but it would open up more use cases of this feature in the future if it was supported.

@chloekraw
Copy link
Contributor

Though it looks like this won't support sorting based on feature-state. That's okay, but it would open up more use cases of this feature in the future if it was supported.

@andrewharvey my understanding is that the limitation you describe is rooted in the fact that feature-state broadly speaking has not been extended to layout properties, as opposed to a specific choice that we made for the implementation in this pr. but let me know if I'm missing something

@ahk ahk closed this Jul 12, 2019
@andrewharvey
Copy link
Collaborator

👉 #8467

@shayke
Copy link

shayke commented Aug 4, 2019

Is there a reason why feature-state is not supported?
In our case, we color line features per state and it's mandatory to sort the features per status (selected ones should be on top). Currently we solve that by separating the layer into hovered, selected, normal lines to support hover -> selected -> normal z-index which kind of defeats the purpose of feature-state anyway (since before all you could do is separate layers and apply filters).

@andrewharvey
Copy link
Collaborator

andrewharvey commented Aug 5, 2019

@shayke I believe the reason is in #8383 (comment), it's just because no one has implemented feature-state for layout properties, as far as I know. Your workaround is exactly what I would suggest.

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.

6 participants