add `symbol-sort-key` style property #7678

ansis · 2018-12-06T16:38:37Z

This property allows users to specify a sorting order for symbols. Symbols are sorted in ascending order based on the key. Symbols with lower keys will appear below symbols with higher keys when they are rendered with overlap. Symbols with lower keys will be placed before symbols with higher keys.

This fixes #7111 by defining a sort order across tile boundaries.

This also adds an auto option for symbol-z-order which will be the new default. It does not change the behavior of the default value though.

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
post benchmark scores (everything passed)
manually test the debug page
tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes
think about adding a benchmark for symbols

features

[
    { id: 1, properties: { key: 1 }, geometry: {}},
    { id: 0, properties: { key: 0 }, geometry: {}},
    { id: 2, properties: { key: 2 }, geometry: {}},
];

With the following style you would render feature 2 above feature 1 above feature 0:

"symbol-sort-key": ["get", "key"],
"icon-allow-overlap": true

With the following style it would give feature 0 priority over other features if they collide:

"symbol-sort-key": ["get", "key"],
"icon-allow-overlap": false

@mapbox/studio @mapbox/maps-design

does this accomplish what you want out of sorting feature?
Would another name be clearer?
Is ascending sort order the right choice?

Sorting between layers is currently out of scope.

This property allows users to specify a sorting order for symbols. Symbols are sorted in ascending order based on the key. Symbols with lower keys will appear below symbols with higher keys when they are rendered with overlap. Symbols with lower keys will be placed before symbols with higher keys. This also fixes #7111 by defining a sort order across tile boundaries.

ansis · 2018-12-07T08:53:37Z

Tagging @kkaefer because he opened the initial proposal in #4361. This implementation adds sorting as a layout property instead of a top level property because adding layout properties seems easier. This also uses a key instead of a comparator because while it is less powerful it is a bit simpler with expressions and a bit more optimizable.

ChrisLoer · 2018-12-07T10:12:49Z

src/render/draw_symbol.js

        painter.transform.zoom, buffers.programConfigurations.get(layer.id),
        buffers.dynamicLayoutVertexBuffer, buffers.opacityVertexBuffer);
 }
+
+function drawSymbolsSorted(painter: Painter, renderData: Array<SymbolTileRenderState>, layer, colorMode, stencilMode) {


Can we refactor this so it's "prepare draw state per tile, then draw once per segment" whether we're sorting or not? I think it would be easier to follow that way -- we wouldn't have to deal with the ambiguous naming that drawLayerSymbols is sometimes a preparation step and sometimes an actual draw step.

I think my thinking here was to avoid the extra overhead for symbol layers that don't need this but I should probably benchmark to see if this is valid. It looks like it should be pretty cheap to do what you are suggesting.

@ChrisLoer I've pushed a commit to always use the preparation step. Can you review?

kkaefer · 2018-12-07T10:36:12Z

Is ascending sort order the right choice?

We could add a "symbol-sort-order": "desc" | "asc" to specify the order. Without that key, you'd have to resort to a hack like "symbol-sort-key": ["-", 100000000, ["get", "key"]] for numeric keys (and it's unclear to me what you'd use for textual keys).

uses a key instead of a comparator because while it is less powerful it is a bit simpler with expressions

We could use an expression like ["<", ["get", "key", "lhs"], ["get", "key", "rhs"]] to allow comparators. The special names lhs/rhs in get and has expressions would be the two features being compared during sorting. However, you're right in pointing out that this is more complex (and probably slower) than using a sort key.

ansis · 2018-12-10T23:53:22Z

We could add a "symbol-sort-order": "desc" | "asc"

We decided that we should leave this out and add it later if it seems useful.

We could use an expression like ["<", ["get", "key", "lhs"], ["get", "key", "rhs"]] to allow comparators.

We decided to use keys for simplicity. If we need to support tie breaking later one we can try to add arrays of keys.

kkaefer · 2018-12-11T15:54:50Z

/cc @mapbox/studio

tristen

Changes LGTM. I can see this being worthwhile to account for in Studio. Especially for custom data where features may not be sorted correctly.

The original issue brought up supporting this for circle types. Curious what the timing is for that along with other layer types like line, fill?

ChrisLoer · 2018-12-11T19:38:23Z

LGTM! But let's add at least one render test that exercises sorting at a tile boundary?

ansis · 2018-12-11T19:46:45Z

@ChrisLoer thanks for catching that, I forgot to commit them. The render tests test sorting at boundaries for text, icons and for text, and with placement enabled.

ChrisLoer

🎉

nickidlugash · 2018-12-11T21:37:23Z

Is ascending sort order the right choice?

This answer is kinda hard to answer though since the hierarchy is reverse for the two styling options (overlapping or not). I could see use cases for both ascending and descending, but this seems reasonable if we have to pick one.

Would another name be clearer?

symbol-sort-key sounds fine. I guess symbol-sort-property would be the other idea that comes to mind, since we refer to data fields as property in multiple other areas of the spec.

mapcarta · 2019-03-06T23:00:17Z

Could symbol-sort-key also work for text overlap? It currently only seems to work for icon overlap ('icon-allow-overlap':false). The idea could be extended to be text overlap for the following style:

[ 'icon-allow-overlap':true, 'text-allow-overlap':false ]

aMoniker · 2019-04-05T15:34:50Z

@ansis

We decided that we should leave ["symbol-sort-order": "desc" | "asc"] out and add it later if it seems useful.

This was an unfortunate decision for us. We have a symbol layer rendering location names, and a circle layer rendering their points. The source is the same for both, and is sorted in descending order by our own concept of location "importance".

The top labels are rendered first, and preclude others that follow due to collision. However, the circle layer renders the least important points last, which means they occlude the more important points. This matters for us since our circles are color-coded (low importance locations are usually grey).

Our case would be solved by introducing symbol-sort-order, though it would also mean we'd have to ship an extra property for every single point which indicates its order.

More ideal would be adding another option to symbol-z-order called something like source-reversed, so we could return our source points in reverse order so circles render properly, while also using source-reversed on the symbol layer so those render properly.

It seems like either suggestion above shouldn't be too bad implement, though I can't say with certainty without knowing the codebase. Would it be possible to introduce one of the features above, or any other option that would solve this use case?

ansis · 2019-05-20T19:52:21Z

@aMoniker sorry, I missed this comment earlier. Would it be possible for you to add a property in the data which you could use with symbol-sort-key to reverse? I understand that a reversal property might be simpler for your use case but we need to balance supporting specific use cases with the complexity that adding new properties introduces.

aMoniker · 2019-05-21T01:59:21Z

@ansis No worries, I appreciate the reply. I suppose we could add a ranking property to the features shipped by our tile server for use with symbol-sort-key, though from the PR description it sounds like we'd have to ship an inverse of that property since lower values take precedence when colliding. We'd also have to reverse the order in which the features were returned, so that the circle layer renders properly.

ansis · 2019-05-21T15:24:17Z

You could also use expression to invert the value ["-", ["get", "my-sort-key-property"]]

ansis added 2 commits December 6, 2018 17:43

fixup

c4431e4

ansis requested a review from ChrisLoer December 6, 2018 16:38

ChrisLoer reviewed Dec 7, 2018

View reviewed changes

have one render path instead of two

2ce2118

samanpwbb requested a review from tristen December 11, 2018 18:54

tristen approved these changes Dec 11, 2018

View reviewed changes

add render tests

71fd325

ChrisLoer approved these changes Dec 11, 2018

View reviewed changes

ansis merged commit 64ebdda into master Dec 12, 2018

ansis deleted the symbol-sort branch December 12, 2018 17:58

pozdnyakov mentioned this pull request Mar 4, 2019

Add symbol-sort-key style property mapbox/mapbox-gl-native#14028

Closed

alexshalamov mentioned this pull request Apr 10, 2019

[core] Port symbol-sort-key symbol layout property mapbox/mapbox-gl-native#14386

Merged

osvodef mentioned this pull request Jun 3, 2020

Improve rendering performance of symbols with symbol-sort-key #9751

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `symbol-sort-key` style property #7678

add `symbol-sort-key` style property #7678

ansis commented Dec 6, 2018 •

edited

Loading

ansis commented Dec 7, 2018

ChrisLoer Dec 7, 2018

ansis Dec 7, 2018

ansis Dec 10, 2018

kkaefer commented Dec 7, 2018

ansis commented Dec 10, 2018

kkaefer commented Dec 11, 2018

tristen left a comment

ChrisLoer commented Dec 11, 2018

ansis commented Dec 11, 2018

ChrisLoer left a comment

nickidlugash commented Dec 11, 2018 •

edited by chloekraw

Loading

mapcarta commented Mar 6, 2019

aMoniker commented Apr 5, 2019

ansis commented May 20, 2019

aMoniker commented May 21, 2019

ansis commented May 21, 2019

add symbol-sort-key style property #7678

add symbol-sort-key style property #7678

Conversation

ansis commented Dec 6, 2018 • edited Loading

Launch Checklist

ansis commented Dec 7, 2018

ChrisLoer Dec 7, 2018

Choose a reason for hiding this comment

ansis Dec 7, 2018

Choose a reason for hiding this comment

ansis Dec 10, 2018

Choose a reason for hiding this comment

kkaefer commented Dec 7, 2018

ansis commented Dec 10, 2018

kkaefer commented Dec 11, 2018

tristen left a comment

Choose a reason for hiding this comment

ChrisLoer commented Dec 11, 2018

ansis commented Dec 11, 2018

ChrisLoer left a comment

Choose a reason for hiding this comment

nickidlugash commented Dec 11, 2018 • edited by chloekraw Loading

mapcarta commented Mar 6, 2019

aMoniker commented Apr 5, 2019

ansis commented May 20, 2019

aMoniker commented May 21, 2019

ansis commented May 21, 2019

add `symbol-sort-key` style property #7678

add `symbol-sort-key` style property #7678

ansis commented Dec 6, 2018 •

edited

Loading

nickidlugash commented Dec 11, 2018 •

edited by chloekraw

Loading