Fix building flat roofs with user provided data when terrain is enabled

[karimnaaji]

This is a small change that removes the dependency on the feature data having the property named 'height' in its data fields and adds render tests and release testing coverage for this. This enables flat roofs always, to allow custom data source to use the code path for flat roofs, as explained in #10170 . There are valid use cases to conditionally enable flat roofs using a style spec property for explicit usage, we will evaluate independently of this fix (refer #10170 (comment)). The render tests and demo are using a rather large example (taken from the user demo provided in the ticket), let me know if this needs to be trimmed down a bit.

Using the custom data source from #10170 (comment), added from demo/fill-extrusion-querying:

Before

After

Launch Checklist

• briefly describe the changes in this PR
• include before/after visuals or gifs if this PR includes visual changes
• write tests for all new functionality
• manually test the debug page
• tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Improve terrain support of fill-extrusions flat roofs when used with custom data sources</changelog>

[mourner]

The render tests and demo are using a rather large example (taken from the user demo provided in the ticket), let me know if this needs to be trimmed down a bit.

Yeah, +27k lines of code just for a small fix is too much to add. I would try to make a much simpler test / debug page.

[astojilj]

Current code is a conservative hack (best effort) to enable flat roofs for some of the buildings.
Don't think it is a good idea to enable it for any feature that is larger than the building (countries, routes) - those kind of features would span across multiple tiles and we wouldn't be able to control when flat roof is used and where it is not.

There are valid use cases to conditionally enable flat roofs using a style spec property for explicit usage, we will evaluate independently of this fix (refer #10170 (comment))

IMHO, that style spec property for explicit usage would be a great fix for this, too - I'd be concerned about regressions if merging this PR without style spec property for explicit usage.

[karimnaaji]

Addressed comments and updates to further control the behavior in 79361bc:

• Added style spec property fill-extrusion-flat-roofs to allow controlling flat roofs as suggested in Fix building flat roofs with user provided data when terrain is enabled #10347 (comment) and Building roofs with terrain enabled behave inconsistently #10170 (comment), the spec is the following, defaulting to true to allow for backward compatibility:

    "fill-extrusion-flat-roofs": {
      "type": "boolean",
      "default": true,
      "doc": "Whether or not the extruded fill should use flat roofs when used along with terrain.",
      "sdk-support": {
        "basic functionality": {
          "js": "2.2.0"
        }
      },
      "expression": {
        "interpolated": false,
        "parameters": [
          "zoom"
        ]
      },
      "property-type": "data-constant"
    },

cc @mapbox/map-design-team for review of the spec.

• Updated fill-extrusion querying to account for this spec:

Screen.Recording.2021-02-02.at.2.45.32.PM.mov

• Added tests for the above and trimmed down other regression and example sizes

@mourner @astojilj PTAL.

[asheemmamoowala]

The introduction of this new style spec property changes the default behavior of fill-extrusion layers and breaks compatibility with existing styles with terrain enabled. Would you consider flattening extrusions on terrain by default as the pre-dominant use might be with smaller polygons? For polygons where the use is not expected to auto adjust the extrusion height against terrain, would the default behavior be to simply slope along side terrain or to remove the terrain adjustment and set height based on mean sea level? Is there enough of a use case to directly support extrusions that slope?

The fill-extrusion-flat-roofs name seems very specific to buildings, ie: roofs. Is there a more generic name that can be used here?

[karimnaaji]

@asheemmamoowala

The introduction of this new style spec property changes the default behavior of fill-extrusion layers and breaks compatibility with existing styles with terrain enabled.

The default behavior is the current behavior (flattening) to explicitly not break compatibility, do you have any specific example that could help clarify this concern?

For polygons where the use is not expected to auto adjust the extrusion height against terrain, would the default behavior be to simply slope along side terrain or to remove the terrain adjustment and set height based on mean sea level? Is there enough of a use case to directly support extrusions that slope?

Currently it is set to slope along side the terrain. The main use case is described in #10170 (comment) for vegetation coverage, but this is mainly to allow control over turning this off:

Since we don't always honor flat roofs due to the small spans area restriction (flat roofs are usually only supported on a subset of fill-extrusions which are polygons that do not span across multiple tiles), the introduction of the spec would allow to turn this off and not automatically have flattening enabled when terrain is on. Having flat roofs always, with custom user data sets that may not be fill-extrusions representing buildings, may become inconvenient if we don't allow such control, especially considering that these polygons may or may not be spanning over a few tiles.

That being said, I'm happy to revert the commits for the initially stated scope of this PR for further evaluation.

[asheemmamoowala]

The default behavior is the current behavior (flattening) to explicitly not break compatibility, do you have any specific example that could help clarify this concern?

Apologies for blowing past that part of the default value. 🤦

Since we don't always honor flat roofs due to the small spans area restriction (flat roofs are usually only supported on a subset of fill-extrusions which are polygons that do not span across multiple tiles), the introduction of the spec would allow to turn this off and not automatically have flattening enabled when terrain is on. Having flat roofs always, with custom user data sets that may not be fill-extrusions representing buildings, may become inconvenient if we don't allow such control, especially considering that these polygons may or may not be spanning over a few tiles.

So when polygons spanning multiple tiles need to be rendered, how does using this property provide a result that a map designer would want?

[karimnaaji]

So when polygons spanning multiple tiles need to be rendered, how does using this property provide a result that a map designer would want?

I don't think the the spec toggle has any side effect preventing further improvements over the multiple-tile spanning polygon flat roofs case, which is our current behavior and default. But in doubt, I reverted the spec addition to only the initial scope of this PR.

Addressed changes requested