symbol sorting for viewport labelling #5298
Conversation
src/gl/index_buffer.js
Outdated
| } | ||
|
|
||
| bind() { | ||
| this.gl.bindBuffer(this.gl.ELEMENT_ARRAY_BUFFER, this.buffer); | ||
| } | ||
|
|
||
| updateData(array: TriangleIndexArray | LineIndexArray) { | ||
| this.bind(); |
There was a problem hiding this comment.
Does it make sense to assert here that it's in dynamicDraw mode?
src/data/bucket/symbol_bucket.js
Outdated
| const symbolInstance = this.symbolInstances[i]; | ||
|
|
||
| for (const placedTextSymbolIndex of symbolInstance.placedTextSymbolIndices) { | ||
| const placedSymbol = this.placedGlyphArray.get(placedTextSymbolIndex); |
There was a problem hiding this comment.
Am I right in reading this as "we sort symbols within a segment, but symbols can't reorder between segments"? And if that's the case, aren't we basically giving up on sorting when there are multiple segments? That seems OK since this is an edge case, but if we're going to give up, why not just early-exit the entire sorting process?
There was a problem hiding this comment.
Yep, it's kind of giving up. I hadn't thought about an early-exit but that's probably cleaner. This approach sorts them so that they look mostly right, but is more complicated and probably error-prone. I'll switch to an early exit!
|
Oh also as part of these changes we should un-ignore the tests tagged 5150 in |
|
Err, another concern: doesn't this break the debug collision boxes/circles for sorted symbols? Their index arrays will need the same re-sorting... |
Nope, this leaves collision box rendering unchanged. Their boxes are rendered un an unsorted order, but that seems ok to consider it's just a debug view and these symbols are all allowed to overlap. |
🤦♂️ I didn't think that through all the way -- forgot you're not changing the sort of the |
ansis
force-pushed
the
viewport-collision
branch
from
41a2181
to
019fb57
Compare
ansis
force-pushed
the
viewport-collision-sort
branch
from
cc18560
to
07acec3
Compare
ChrisLoer
force-pushed
the
viewport-collision
branch
from
019fb57
to
41a2181
Compare
which was previously used to sort symbols before buffer creation.
ansis
force-pushed
the
viewport-collision-sort
branch
from
07acec3
to
57d6963
Compare
Previously the symbols were sorted in the y direction before running them through the collision code. Now only the rendering is sorted and the collision order depends on the original data order. I think this is the right approach because: - we should respect data order so that it can be used for label importance - data order is consistent, while y order changes with rotation
|
Looks great to me, let's merge it into We can do it on either branch, but we should be able to remove these three ignores now: We'll have to update the expected results, but the new results should be unambiguously better. 😃 |
This sorts symbols that may overlap based on their vertical position. The sorting happens at the end of symbol placement within
commitPlacement.The sorting happens in each tile independently. This means there might be incorrect sorting near tile edges but I think that fixing that minor drawback would be not worth the added complexity. Unless we switch back to the clipping approach we used before.
@ChrisLoer