-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Improve rendering performance of symbols with symbol-sort-key #9751
Conversation
src/render/draw_symbol.js
Outdated
@@ -236,7 +236,12 @@ function drawLayerSymbols(painter, sourceCache, layer, coords, isText, translate | |||
// Unpitched point labels need to have their rotation applied after projection | |||
const rotateInShader = rotateWithMap && !pitchWithMap && !alongLine; | |||
|
|||
const sortFeaturesByKey = layer.layout.get('symbol-sort-key').constantOr(1) !== undefined; | |||
const canOverlap = layer.layout.get('icon-ignore-placement') || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be refactored into a separate function, but I'm not sure about the best place for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should try add this to SymbolBucket's constructor to avoid the overhead of retrieving this value for every feature while it's guaranteed to be the same, and because there's already similar set up happening there (this.sortFeaturesByKey
). In fact a very similar logic is already present there for sortFeaturesByY
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I made canOverlap
a field in SymbolBucket
, and used it to calculate sortFeaturesByY
as well.
cc @pozdnyakov |
@mourner can you review this since Mikko is out of office for a bit? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for such a long delay getting to review this! This overall looks great — left a comment on a potentially cleaner way to add a check for this.
src/render/draw_symbol.js
Outdated
@@ -236,7 +236,12 @@ function drawLayerSymbols(painter, sourceCache, layer, coords, isText, translate | |||
// Unpitched point labels need to have their rotation applied after projection | |||
const rotateInShader = rotateWithMap && !pitchWithMap && !alongLine; | |||
|
|||
const sortFeaturesByKey = layer.layout.get('symbol-sort-key').constantOr(1) !== undefined; | |||
const canOverlap = layer.layout.get('icon-ignore-placement') || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should try add this to SymbolBucket's constructor to avoid the overhead of retrieving this value for every feature while it's guaranteed to be the same, and because there's already similar set up happening there (this.sortFeaturesByKey
). In fact a very similar logic is already present there for sortFeaturesByY
.
@@ -333,7 +334,8 @@ function drawLayerSymbols(painter, sourceCache, layer, coords, isText, translate | |||
hasHalo | |||
}; | |||
|
|||
if (sortFeaturesByKey) { | |||
if (hasSortKey && bucket.canOverlap) { | |||
sortFeaturesByKey = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just not sure about this particular bit — we need sortFeaturesByKey
after this loop, so I had to assign it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this looks good.
src/data/bucket/symbol_bucket.js
Outdated
@@ -609,6 +614,7 @@ class SymbolBucket implements Bucket { | |||
lineOffset: [number, number], | |||
alongLine: boolean, | |||
feature: SymbolFeature, | |||
layer: SymbolStyleLayer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
layer
is no longer used, so this is a leftover right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, didn't catch that! Fixed 🙂
@@ -333,7 +334,8 @@ function drawLayerSymbols(painter, sourceCache, layer, coords, isText, translate | |||
hasHalo | |||
}; | |||
|
|||
if (sortFeaturesByKey) { | |||
if (hasSortKey && bucket.canOverlap) { | |||
sortFeaturesByKey = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this looks good.
src/symbol/symbol_layout.js
Outdated
@@ -665,6 +667,7 @@ function addSymbol(bucket: SymbolBucket, | |||
iconOffset, | |||
iconAlongLine, | |||
feature, | |||
layer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 layer
leftovers here too,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Thank you! |
Pull request #7678 introduced sorting of symbol features across tiles. Unfortunately, this kind of sorting requires that symbols with distinct values of sort key are rendered in separate draw calls.
This can be a huge performance hit for a large number of labels, especially if
symbol-sort-key
is data driven (e. g. if we want to prioritize city labels by population). In such a case pretty much every label will end up in a separate draw call, which can result in hundreds of extra draw calls.However, this sorting is only required if symbols of the current layer can overlap (i. e. when one of the
*-allow-overlap
or*-ignore-placement
props is true). Otherwise, the rendering order is not important. This pull requests adds a check for this. If it detects that labels of the current layer cannot overlap, it puts all of them in one segment as before (and they get rendered in one draw call).This only affects rendering order and should not affect prioritization during placement at all.
Launch Checklist