Enable property functions for "fill-color" and "fill-outline-color"

[lucaswoj]

ref mapbox/mapbox-gl-shaders#3
ref mapbox/mapbox-gl-test-suite#99

cc @jakepruitt @jfirebaugh @mourner @tmcw

[lucaswoj]

This is not an ideal long term architecture. I'm thinking on how to improve this...

[mourner]

You could DRY up the code by something like this:

FillStyleLayer.prototype.fallbackMapping = {'fill-outline-color': 'fill-color'};

And then using this object to map properties in StyleLayer, avoiding code duplication.

[lucaswoj]

I'm not sold on that design

• Similar branching logic would still need to be written, but it'd be in StyleLayer rather than FillLayer
• I can't think of many (any?) other style properties that have fallback behaviour like this

---

My preferred solution would be to factor out all the per-paint-property methods in StyleLayer into a StyleValue class (that might replace StyleDeclaration and/or StyleTransition)

type StyleValue<T> {
  isZoomConstant: boolean;
  isFeatureConstant: boolean;
  calculate(zoom:number, featureProperties: {}) => T;
  getInterpolationT(zoom: number) =>;
  ...
}

Then, FillStyleLayer's implementation would only have one overriden method:

getStyleValue: function(name) {
  if (name === 'fill-outline-color' && this.getPaintProperty('fill-outline-color') === undefined) {
    return StyleLayer.prototype.getStyleValue.call(this, 'fill-color');
  } else {
    return StyleLayer.prototype.getStyleValue.call(this, name);
  }
}

---

All that said, I'd be happy to 🚢 with the code in this PR for now. It may be verbose and non-DRY but it is clear, robust, and well-contained.

[mourner]

The proposed refactor sounds great! Yeah, we can follow-up on that later. I'd make sure to run benchmarks before merging though because things like this make things slower if they're in hot path.

[mourner]

var attributes = bucket.paintAttributes.fill[layer.id];

[lucaswoj]

Ready to 🚢 @jfirebaugh @mourner?

[jfirebaugh]

I'm not caught up on gl-js buffer architecture so I don't think I can meaningfully review here.

[lucaswoj]

Main thread perf seems to have taken a hit:

Benchmarks

Buffer

this branch: 854 ms
master: 827 ms

FPS

this branch: 48fps
master: 59 fps

Frame Duration

this branch: 19.7 ms per frame. 72% of frames took longer than 16ms.
master: 7.8 ms per frame. 0% of frames took longer than 16ms.

[mourner]

@lucaswoj whoa, 2.5x slower frame is a blocker for merging. I guess we'll have to refactor this.

[lucaswoj]

@mourner the perf regression was due to a bug in createProgram. New numbers:

Benchmarks

Buffer

this branch: 809 ms
master: 827 ms

FPS

this branch: 60fps
master: 59 fps

Frame Duration

this branch: 9.7 ms per frame. 1% of frames took longer than 16ms.
master: 7.8 ms per frame. 0% of frames took longer than 16ms.

[mourner]

That looks much better. If you can't find a way to fight back that 2ms within a reasonable timeframe (say half an hour), let's 🚢 and figure out later.

[lucaswoj]

@mourner I suspect that 2ms is just measurement error. I'll try to create a cleaner testing environment and see if the discrepancy goes away.

[lucaswoj]

I was wrong -- the difference is definitely significant.

this branch

9.7 ms per frame. 1% of frames took longer than 16ms.
9.8 ms per frame. 3% of frames took longer than 16ms.
10.5 ms per frame. 3% of frames took longer than 16ms.

master

8.3 ms per frame. 0% of frames took longer than 16ms.
8.2 ms per frame. 2% of frames took longer than 16ms.
7.9 ms per frame. 0% of frames took longer than 16ms.

[lucaswoj]

I got a smidgen of perf back by optimizing useProgram further. I suspect that the rest of the perf loss is due to moving this uniform setup inside a hotter loop.

This is something we can and should optimize later but will require some bigger architectural work.

[mourner]

In context of frame performance, even 2ms is 20% difference. Let's make sure we don't loose track of this, maybe ticket?

[lucaswoj]

➡️ #2640