Fix bug causing symbols to be clipped when many are rendered

[AndyMoreland]

mapbox-gl-js version: v0.20.0, v0.20.1, v0.21.0.

Occurs in all browsers I've tested including chrome 51, FF47, FF39 and a few others on Mac OS and windows.

Repro here: http://23.239.25.88/repro.html

To view the behavior, zoom in and out. When zoomed in, the sprites render at the coordinates specified by the geojson point geometry. When you zoom out past zoom level 10, copies of the sprites start being rendered in the "background" (or something) of the labels of the point data and the map is very clearly no longer correct.

I started observing this behavior circa mapbox-gl-js v0.20.0 in maps with more complicated sprite sheets and geometries. I bisected mapbox-gl-js and I believe the behavior was introduced in one of these commits: {e268120, 6537f3e, 92fbfb8}.

This example is probably not minimal, but I trimmed as much of the source map out as I could.

When you cross the zoom-level inflection point you should see the map transition from something like:

which is correct, to:

which is incorrect.

[Que3216]

Would be awesome to have a fix for this!

[mollymerp]

Thanks for reporting this issue @AndyMoreland -- could you please post a link to the style.json for the error reproduction example? I can't reproduce this behavior with other styles I've tried so it will be helpful to see the style to track down the root issue.

[AndyMoreland]

The style I'm using is: http://23.239.25.88/repro.json

The sprite file is: http://23.239.25.88/sprite.json and http://23.239.25.88/sprite.png

[Que3216]

I've made a slightly simpler repro here: http://jsfiddle.net/Lqgnyna3/4/

The problem seems to be caused by large numbers of labels.

[Que3216]

Even simpler repro: http://jsfiddle.net/Lqgnyna3/5/

[lucaswoj]

There are two places that we fork on text-allow-overlap and icon-allow-overlap:

1. when we enable / disable gl.STENCIL_TEST
2. when we populate the grid index

I've ruled out 1. as a source of the bug by hardcoding gl.STENCIL_TEST enabled / disabled and observing the bug.

More investigation is needed to determine if grid index could be the source of the bug. I'm especially interested to see if there are any places where this number of symbols could cause an integer overflow in grid index.

[AndyMoreland]

If it helps, I've updated my initial repro up above to remove the extremely large filter clauses and set text-allow-overlap and icon-allow-overlap to true for all symbol layers, and I still see the issue.

[mollymerp]

@AndyMoreland @Que3216 You're experiencing 2 issues:

1. sprite rendering artifacts: I've isolated this to integer overflow in the SymbolInstancesArray

iconQuadStart(/End)Index = UInt16

iconQuadStart(/End)Index = UInt32

For now, modifying the code I referenced above should solve your rendering artifacts.

1. The second issue is this known bug. Usually I don't see this problem when both icon-overlap and text-overlap are set to true, but apparently with giant geojson point data sources the behavior still exists.

[mollymerp]

We shipped #2966 which adds a warning when there are too many glyphs / icons being rendered in a tile. changing the type of icon(/glyph)QuadStartIndex + icon(/glyph)QuadEndIndex in this code to UInt32 should solve the rendering issues you're experiencing.

[AndyMoreland]

@mollymerp I made the change that you suggested (against v0.22.1), but it caused:

glDrawElements: attempt to access out of range vertices in attribute 0

which seemed to break the rendering.

[AndyMoreland]

Hm. I might have built my fork wrong. It does seem to work now! Thanks.

[bhison]

We shipped #2966 which adds a warning when there are too many glyphs / icons being rendered in a tile. changing the type of icon(/glyph)QuadStartIndex + icon(/glyph)QuadEndIndex in this code to UInt32 should solve the rendering issues you're experiencing.

In case anyone else comes here, I believe that link which is now dead was pointing to this: [removed, see below]

[lucaswoj]

@bhison I think the code you're seeking has moved here: https://github.com/mapbox/mapbox-gl-js/blob/master/src/data/bucket/symbol_attributes.js#L64

[bhison]

@lucaswoj thank you! Do you have an idea how I can change this type? I feel a bit out of my depth here.

[lucaswoj]

@bhison Unfortunately the process is rather involved. You'll need to figure out exactly which attributes are overflowing because of a too-small integer type, fork GL JS, change those types, and create your own build. I don't recommend it.

[bhison]

@lucaswoj oh ok. So the alternative is...have fewer symbols I guess?

[lucaswoj]

@bhison hard to say... it all depends on the situational specifics

[ChrisLoer]

It turns out this situation can also cause an exception to be thrown during rendering (assuming this code hasn't diverged too much from MapLibre) -- in line label projection, the code checks to see if there's enough room for all the glyphs on a line by checking that the first and last glyph fit. The code assume that the glyphs are in increasing-offset order. However, if the offsets get jumbled by the overflow, it's possible for a "middle" glyph to be outside of the min/max offset. When the code tries to place the glyph, it can't find a place to put the glyph on the line and ends up doing a null pointer dereference.