Add support for property functions in line-color

[lucaswoj]

... and pave the way for all other line properties

This PR

• adds support for line-color property functions
• builds the infrastructure needed for making other line properties support property functions

The naive implementation of line-color property functions was too slow, prompting me to investigate ways to reduce the perf overhead of setting uniforms per tile. We need to set uniforms per tile as if each were a different GL program because different tiles may actually require different GL programs due to a pending asynchronous operation.

My solution was a Painter#setUniforms(uniforms: Object) method, something I've been thinking about writing for a while anyway as part of #145 (needed for #281).

ref #2729

cc @mollymerp @mourner

[mourner]

If we only use feature.properties, should we pass featureProperties in the outer function directly?

[lucaswoj]

Refactored in the next commit.

[mourner]

This looks great overall. How are the benchmarks looking?

[lucaswoj]

The benchmarks are looking pretty good. I was seeing around 5% slowdown in frame-duration, everything else 0%. I think setUniforms is the source of the slowdown and there are opportunities to make it faster, as you pointed out above. #2938 (comment)

[mollymerp]

Daaang awesome job keeping perf 🏃 !

[lucaswoj]

Branch Benchmarks

7.2 ms, 0% > 16ms
7.6 ms, 1% > 16ms
7.2 ms, 0% > 16ms

Master Benchmarks

frame-duration:

8.5 ms, 11% > 16ms
6.7 ms, 0% > 16ms
6.6 ms, 0% > 16ms
6.5 ms, 0% > 16ms

EDIT: The previous master benchmark was a bit of an anomaly. Updated data above.

[lucaswoj]

I got this running as fast or faster than master but there's a lingering test-suite failure that I can't understand. Any ideas @mourner?

[lucaswoj]

I found the problem with line-dasharray long-segment and fixing it regressed all the perf gains. Pperf hasn't improved at all since the initial implementation in cd78412. I recommend we consider merging that as-written.

[mourner]

fixing it regressed all the perf gains

Interesting, what was the fix about? It's hard to tell because of the rebased commit.

[lucaswoj]

Ah! My apologies. I've gotten into the habit of squashing bugfixes. I'll unsquash and push so you can see. In short, there was a mistake leading to early loop termination. The performance gains / losses make a lot of sense.

[lucaswoj]

The fix for the test is in 62564e8. It's obvious how this led to artificial performance gains 😢

I am going to remove all ineffective perf optimizations from this branch, leaving only the initial commit.

[mourner]

Oh wow. It's fascinating that this was caught by just a single obscure test. :) It may be worth adding a "functional" render test that renders a relatively detailed map tile with all stuff at once to catch things like this.

I agree that we can merge this now — the changes look good, and we can follow-up with perf improvements later if they're possible.

[lucaswoj]

It may be worth adding a "functional" render test that renders a relatively detailed map tile with all stuff at once to catch things like this.

Yes! mapbox/mapbox-gl-test-suite#26

[sguignot]

@lucaswoj Thanks for this :)
Is there a way to use directly the color from a 'color' property of the feature?

Note: I made a few tests using categorical stops but it's a bit heavy when rendering railways from OSM since they use hundreds of different colors in some areas.

[lucaswoj]

@sguignot the closest thing is a proposal for identity functions mapbox/mapbox-gl-function#20. PRs welcome!