Fix label clipping by switching to layer-by-layer rendering

[lucaswoj]

cc @mourner @jfirebaugh @ansis @kkaefer

fixes #757

Remaining Tasks

• remove reverse iteration over features (how does mapbox-gl-native do this?)

[lucaswoj]

There's still a chunk of work to be done here but rendering is already so much smoother than master

[mourner]

command npm run preinstall && npm update && npm run postinstall && npm run prepublish took more than 120 minutes to run

Oh wow, wtf.

[yhahn]

😮

[mourner]

I spent a long time trying to do the rebase without squashing but it was really difficult due to broken intermediate states.

When I did a similar thing for the earcut branch, it was much easier to manually look through the old PR bit by bit and enter the changes manually on a fresh master-based branch. Rebasing is hell on very outdated branches, and even if you end up making it work, there's still a lot of broken stuff usually.

[lucaswoj]

It looks like symbol render order (namely, overlapping symbols within the same layer) is going different with layer-by-layer rendering. This might be acceptable for our users but is not acceptable for mapbox-gl-test-suite.

[jfirebaugh]

Symbols should be z-ordered by y coordinate. AFAIK, both gl-js and native (which already uses layer-by-layer rendering) agree on this currently; if this PR changes the ordering in gl-js, it's probably a bug.

[lucaswoj]

Ah! Ok. I didn't know there was a deterministic symbol rendering order. I'll keep working on that one.

Some other test-suite failures appear to be headless-gl specific. They are not replicable in the browser. This one occurs whether we pull in earcut or not.

Test Suite

Browser

[lucaswoj]

I'm pleased to report that the fill rendering problems are resolved now (along with a number of other problems). This is shaping up nicely!

UPDATE: nope 👎

[lucaswoj]

Interesting clue on the disappearing fills: rendering all fills during the "translucent" pass fixes almost all of the fill test cases.

[lucaswoj]

I think I'm onto something -- not rendering background layers fixes most of the rendering problems in test-suite.

[lucaswoj]

While I still can't reproduce the exact rendering errors that I'm seeing in headless-gl, I have stumbled across a test case that produces seemingly-related wrong behavior in the browser. These two styles render exactly the same, even though their layer order is reversed.

{
    "version": 8,
    "sources": {
      "mapbox": {
          "type": "vector",
          "url": "mapbox://mapbox.mapbox-streets-v6"
      }
    },
    "layers": [{
        "id": "background",
        "type": "background",
        "paint": {
            "background-color": "red"
        }
    },
    {
        "id": "water",
        "type": "fill",
        "source": "mapbox",
        "source-layer": "water",
        "paint": {
            "fill-color": "blue",
            "fill-antialias": false
        }
    }]
}

{
    "version": 8,
    "sources": {
      "mapbox": {
          "type": "vector",
          "url": "mapbox://mapbox.mapbox-streets-v6"
      }
    },
    "layers": [{
        "id": "water",
        "type": "fill",
        "source": "mapbox",
        "source-layer": "water",
        "paint": {
            "fill-color": "blue",
            "fill-antialias": false
        }, {
        "id": "background",
        "type": "background",
        "paint": {
            "background-color": "red"
        }
    }]
}

[lucaswoj]

only 25 failing test-suite tests ❗

[lucaswoj]

Before optimization, FPS perf seems pretty good

duration: 3000
44.3 fps https://api.tiles.mapbox.com/mapbox-gl-js/v0.10.0/mapbox-gl.js
52.2 fps https://api.tiles.mapbox.com/mapbox-gl-js/v0.12.1/mapbox-gl.js
48.5 fps /dist/mapbox-gl.js
43.8 fps https://api.tiles.mapbox.com/mapbox-gl-js/v0.10.0/mapbox-gl.js
50.9 fps https://api.tiles.mapbox.com/mapbox-gl-js/v0.12.1/mapbox-gl.js
49.6 fps /dist/mapbox-gl.js

[lucaswoj]

This is ready for some serious 👀 @mourner @jfirebaugh @ansis @tmcw @kkaefer.

[lucaswoj]

If you want to follow the full commit history, it begins at #1197

[jfirebaugh]

Why is the iteration order reversed here (and in LineBucket)?

[lucaswoj]

Because we are using the gl.ONE_MINUS_DST_ALPHA blending function, features are effectively rendered in reverse order.

[jfirebaugh]

Hmm. Native does not work this way. It iterates forward.

If this is affecting rendering there must be some other difference between native and JS that is reversed.

[lucaswoj]

I will spend some more time trying to understand this, I don't have an immediate explanation. This is what I had to do to make test-suite pass 😬

[lucaswoj]

This reversal might lead to some rendering bugs. While rendering order is reversed during translucent passes, it is in the expected order during opaque passes.

[ansis]

The reverse iteration here makes the rendered order of features more similar to the previous rendered order, but that previous order was arbitrary. I think it should be safe to remove this and update the render tests.

[ansis]

squashed, rebased and merged in #1886