Unable apply runtime styles for extrusion properties

[scothis]

mapbox-gl-js version: 0.26.0

Steps to Trigger Behavior

1. load a style with a fill layer that has no extrusions properties set
2. map.setPaintProperty(fillLayerId, 'fill-extrude-height', 1);

example http://codepen.io/anon/pen/yaqRpK

Expected Behavior

The style should render the new value and turn the layer from 2d fills into 3d extrusions.

Actual Behavior

The layer disappears, sometimes there is an error on the console right away, sometimes you have to move the map before you see a console error.

[lbud]

There's also a little bug in here that affects setPaintProperty('fill-extrude-*') when a layer is already extruded; I pushed a fix in #3414.

problem

The bigger problem you're seeing @scothis is that in bucket.js, we switch bucket types based on whether we infer that a layer may be extruded — so, if a fill layer's fill-extrude-height property is either a function or is a value greater than 0, we use a fillextrusion bucket, which has 5n vertices (where n = number of vertices of the source polygon); otherwise we use a fill bucket with n vertices. In painter we do such a switch as well.

When you see the polygons disappear, this is because the parsed fill buckets now have a style that sends them down the draw_extrusion render path; when you move the map and it tries to reparse buckets based on this modified style it gets even more confused 😞

solution

We would never want to quintuple the number of vertices for all fill layers, so I think the answer here may be to add a new layout_fill boolean (something like can-extrude) that indicates more explicitly to the worker which bucket type to create for a layer. Fill layers created with can-extrude=true and fill-extrude-height=0 would then be less performant than regular fill layers. We will also likely have to do some trickery to ensure that flat extruded layers are rendered like fills (unaffected by lighting) to keep them consistent with other flat fill layers.

hack

** however **

For those currently blocked by this bug, because of the way we define zoom constant and feature constant in mapbox-gl-function (based on the simple fact of a property being an object, rather than based on the value of the object), a hack that will work is

"fill-extrude-height": {
  "stops": [[15, 0], [16, 0]]
}

Note that this will still suffer from the aforementioned lighting problem (where it won't render the same as it would as a regular flat fill layer because of the lighting).

review

@lucaswoj @scothis @jfirebaugh @mourner what do you think about adding a boolean layout property to indicate which type of bucket to create rather than basing the bucket type on an inference from the fill-extrude-height paint value?

[lucaswoj]

what do you think about adding a boolean layout property to indicate which type of bucket to create rather than basing the bucket type on an inference from the fill-extrude-height paint value?

This is a valid option and it has some drawbacks. Duplicating state creates edge cases and boilerplate.

A design without the "duplicate state" drawback is latching onto the Worker roundtrip and array recalculation that occurs when a data-driven paint property changes to switch bucket types. This has the downside of diluting the sanctity of paint properties and potentially requiring some extra logic; however that is the direction that GL is heading anyway.

[ansis]

Should we reconsider making extruded fills a separate render type? Between these buffer contents differences, lighting differences, and our inability to support fill-opacity for extruded layers are there enough differences to make a separate render type make sense?

[jfirebaugh]

Yes, I think we should have kept them a separate layer type. Another difference is that extruded fills don't support fill-outline-color.

[lbud]

23bad37 addresses this particular bug with @lucaswoj's suggested approach. I've been thinking about @ansis and @jfirebaugh's concerns about having a separate layer type for fill extrusions and am opening a separate spec issue, but I think I should go ahead and merge this fix.

[scothis]

@lbud pulling in master, I'm still seeing issues with adding a fill-extrusion-height property at runtime when a layer is rendered on screen. It now works if the layer is not rendered when the mutation occurs.

[lbud]

This will be solved with mapbox/mapbox-gl-style-spec#554.