Eliminate fill-extrude- props in favor of fill-extrusion type (#3487)

* Eliminate fill-extrude-{height,base} in favor of separate fill-extrusion type * Resolve Z units to meters (#3509)
mapbox · Nov 2, 2016 · b2de538 · b2de538
1 parent 525b15a
commit b2de538
Show file tree

Hide file tree

Showing 12 changed files with 59 additions and 88 deletions.
diff --git a/js/data/bucket/fill_extrusion_bucket.js b/js/data/bucket/fill_extrusion_bucket.js
@@ -31,30 +31,30 @@ const fillExtrusionInterface = {
         components: 1,
         type: 'Uint16',
         getValue: (layer, globalProperties, featureProperties) => {
-            return [Math.max(layer.getPaintValue("fill-extrude-base", globalProperties, featureProperties), 0)];
+            return [Math.max(layer.getPaintValue("fill-extrusion-base", globalProperties, featureProperties), 0)];
         },
         multiplier: 1,
-        paintProperty: 'fill-extrude-base'
+        paintProperty: 'fill-extrusion-base'
     }, {
         name: 'a_maxH',
         components: 1,
         type: 'Uint16',
         getValue: (layer, globalProperties, featureProperties) => {
-            return [Math.max(layer.getPaintValue("fill-extrude-height", globalProperties, featureProperties), 0)];
+            return [Math.max(layer.getPaintValue("fill-extrusion-height", globalProperties, featureProperties), 0)];
         },
         multiplier: 1,
-        paintProperty: 'fill-extrude-height'
+        paintProperty: 'fill-extrusion-height'
     }, {
         name: 'a_color',
         components: 4,
         type: 'Uint8',
         getValue: (layer, globalProperties, featureProperties) => {
-            const color = layer.getPaintValue("fill-color", globalProperties, featureProperties);
+            const color = layer.getPaintValue("fill-extrusion-color", globalProperties, featureProperties);
             color[3] = 1.0;
             return color;
         },
         multiplier: 255,
-        paintProperty: 'fill-color'
+        paintProperty: 'fill-extrusion-color'
     }]
 };
 

diff --git a/js/data/feature_index.js b/js/data/feature_index.js
@@ -115,6 +115,8 @@ class FeatureIndex {
                 styleLayerDistance = getLineWidth(paint) / 2 + Math.abs(paint['line-offset']) + translateDistance(paint['line-translate']);
             } else if (styleLayer.type === 'fill') {
                 styleLayerDistance = translateDistance(paint['fill-translate']);
+            } else if (styleLayer.type === 'fill-extrusion') {
+                styleLayerDistance = translateDistance(paint['fill-extrusion-translate']);
             } else if (styleLayer.type === 'circle') {
                 styleLayerDistance = paint['circle-radius'] + translateDistance(paint['circle-translate']);
             }
@@ -203,9 +205,10 @@ class FeatureIndex {
                         }
                         if (!multiPolygonIntersectsBufferedMultiLine(translatedPolygon, geometry, halfWidth)) continue;
 
-                    } else if (styleLayer.type === 'fill') {
+                    } else if (styleLayer.type === 'fill' || styleLayer.type === 'fill-extrusion') {
+                        const typePrefix = styleLayer.type;
                         translatedPolygon = translate(queryGeometry,
-                                paint['fill-translate'], paint['fill-translate-anchor'],
+                                paint[`${typePrefix}-translate`], paint[`${typePrefix}-translate-anchor`],
                                 bearing, pixelsToTileUnits);
                         if (!multiPolygonIntersectsMultiPolygon(translatedPolygon, geometry)) continue;
 

diff --git a/js/geo/transform.js b/js/geo/transform.js
@@ -454,6 +454,15 @@ class Transform {
         mat4.rotateZ(m, m, this.angle);
         mat4.translate(m, m, [-this.x, -this.y, 0]);
 
+        const circumferenceOfEarth = 2 * Math.PI * 6378137;
+
+        mat4.scale(m, m, [1, 1,
+            // scale vertically to meters per pixel (inverse of ground resolution):
+            // (2^z * tileSize) / (circumferenceOfEarth * cos(lat * π / 180))
+            (Math.pow(2, this.zoom) * this.tileSize) /
+            (circumferenceOfEarth * Math.abs(Math.cos(this.center.lat * (Math.PI / 180)))),
+            1]);
+
         this.projMatrix = m;
 
         // matrix for conversion from location to screen coordinates

diff --git a/js/render/draw_extrusion.js → js/render/draw_fill_extrusion.js b/js/render/draw_extrusion.js → js/render/draw_fill_extrusion.js
@@ -11,9 +11,10 @@ const pattern = require('./pattern');
 module.exports = draw;
 
 function draw(painter, source, layer, coords) {
-    if (layer.paint['fill-opacity'] === 0) return;
+    if (layer.paint['fill-extrusion-opacity'] === 0) return;
     const gl = painter.gl;
     gl.disable(gl.STENCIL_TEST);
+    gl.enable(gl.DEPTH_TEST);
     painter.depthMask(true);
 
     // Create a new texture to which to render the extrusion layer. This approach
@@ -104,12 +105,12 @@ ExtrusionTexture.prototype.TextureBoundsArray = new StructArrayType({
 ExtrusionTexture.prototype.renderToMap = function() {
     const gl = this.gl;
     const painter = this.painter;
-    const program = painter.useProgram('fillExtrudeTexture');
+    const program = painter.useProgram('extrusionTexture');
 
     gl.activeTexture(gl.TEXTURE0);
     gl.bindTexture(gl.TEXTURE_2D, this.texture);
 
-    gl.uniform1f(program.u_opacity, this.layer.paint['fill-opacity']);
+    gl.uniform1f(program.u_opacity, this.layer.paint['fill-extrusion-opacity']);
     gl.uniform1i(program.u_texture, 1);
 
     gl.uniformMatrix4fv(program.u_matrix, false, mat4.ortho(
@@ -151,11 +152,11 @@ function drawExtrusion(painter, source, layer, coord) {
     const buffers = bucket.buffers;
     const gl = painter.gl;
 
-    const image = layer.paint['fill-pattern'];
+    const image = layer.paint['fill-extrusion-pattern'];
 
     const layerData = buffers.layerData[layer.id];
     const programConfiguration = layerData.programConfiguration;
-    const program = painter.useProgram(image ? 'fillExtrudePattern' : 'fillExtrude', programConfiguration);
+    const program = painter.useProgram(image ? 'fillExtrusionPattern' : 'fillExtrusion', programConfiguration);
     programConfiguration.setUniforms(gl, program, layer, {zoom: painter.transform.zoom});
 
     if (image) {
@@ -164,7 +165,13 @@ function drawExtrusion(painter, source, layer, coord) {
         gl.uniform1f(program.u_height_factor, -Math.pow(2, coord.z) / tile.tileSize / 8);
     }
 
-    setMatrix(program, painter, coord, tile, layer);
+    painter.gl.uniformMatrix4fv(program.u_matrix, false, painter.translatePosMatrix(
+        coord.posMatrix,
+        tile,
+        layer.paint['fill-extrusion-translate'],
+        layer.paint['fill-extrusion-translate-anchor']
+    ));
+
     setLight(program, painter);
 
     for (const segment of buffers.segments) {
@@ -173,21 +180,6 @@ function drawExtrusion(painter, source, layer, coord) {
     }
 }
 
-function setMatrix(program, painter, coord, tile, layer) {
-    const zScale = Math.pow(2, painter.transform.zoom) / 50000;
-
-    painter.gl.uniformMatrix4fv(program.u_matrix, false, mat4.scale(
-        mat4.create(),
-        painter.translatePosMatrix(
-            coord.posMatrix,
-            tile,
-            layer.paint['fill-translate'],
-            layer.paint['fill-translate-anchor']
-        ),
-        [1, 1, zScale, 1])
-    );
-}
-
 function setLight(program, painter) {
     const gl = painter.gl;
     const light = painter.style.light;

diff --git a/js/render/painter.js b/js/render/painter.js
@@ -18,7 +18,7 @@ const draw = {
     circle: require('./draw_circle'),
     line: require('./draw_line'),
     fill: require('./draw_fill'),
-    extrusion: require('./draw_extrusion'),
+    'fill-extrusion': require('./draw_fill_extrusion'),
     raster: require('./draw_raster'),
     background: require('./draw_background'),
     debug: require('./draw_debug')
@@ -274,12 +274,7 @@ class Painter {
         if (layer.type !== 'background' && !coords.length) return;
         this.id = layer.id;
 
-        let type = layer.type;
-        if (type === 'fill' && layer.isExtruded({zoom: this.transform.zoom})) {
-            type = 'extrusion';
-        }
-
-        draw[type](painter, sourceCache, layer, coords);
+        draw[layer.type](painter, sourceCache, layer, coords);
     }
 
     setDepthSublayer(n) {

diff --git a/js/style/style.js b/js/style/style.js
@@ -557,7 +557,6 @@ class Style extends Evented {
         if (util.deepEqual(layer.getPaintProperty(name, klass), value)) return this;
 
         const wasFeatureConstant = layer.isPaintValueFeatureConstant(name);
-        const wasExtruded = layer.type === 'fill' && layer.isExtruded({ zoom: this.zoom });
         layer.setPaintProperty(name, value, klass);
 
         const isFeatureConstant = !(
@@ -567,14 +566,7 @@ class Style extends Evented {
             value.property !== undefined
         );
 
-        const switchBuckets = (
-            layer.type === 'fill' &&
-            name === 'fill-extrude-height' &&
-            (!wasExtruded && !!value) ||
-            (wasExtruded && value === 0)
-        );
-
-        if (!isFeatureConstant || !wasFeatureConstant || switchBuckets) {
+        if (!isFeatureConstant || !wasFeatureConstant) {
             this._updates.layers[layerId] = true;
             if (layer.source) {
                 this._updates.sources[layer.source] = true;

diff --git a/js/style/style_layer.js b/js/style/style_layer.js
@@ -334,6 +334,7 @@ module.exports = StyleLayer;
 const subclasses = {
     'circle': require('./style_layer/circle_style_layer'),
     'fill': require('./style_layer/fill_style_layer'),
+    'fill-extrusion': require('./style_layer/fill_extrusion_style_layer'),
     'line': require('./style_layer/line_style_layer'),
     'symbol': require('./style_layer/symbol_style_layer')
 };

diff --git a/js/style/style_layer/fill_extrusion_style_layer.js b/js/style/style_layer/fill_extrusion_style_layer.js
@@ -0,0 +1,12 @@
+'use strict';
+
+const StyleLayer = require('../style_layer');
+const FillExtrusionBucket = require('../../data/bucket/fill_extrusion_bucket');
+
+class FillExtrusionStyleLayer extends StyleLayer {
+    createBucket(options) {
+        return new FillExtrusionBucket(options);
+    }
+}
+
+module.exports = FillExtrusionStyleLayer;
diff --git a/js/style/style_layer/fill_style_layer.js b/js/style/style_layer/fill_style_layer.js
@@ -2,7 +2,6 @@
 
 const StyleLayer = require('../style_layer');
 const FillBucket = require('../../data/bucket/fill_bucket');
-const FillExtrusionBucket = require('../../data/bucket/fill_extrusion_bucket');
 
 class FillStyleLayer extends StyleLayer {
 
@@ -46,16 +45,7 @@ class FillStyleLayer extends StyleLayer {
         }
     }
 
-    isExtruded(globalProperties) {
-        return !this.isPaintValueFeatureConstant('fill-extrude-height') ||
-            !this.isPaintValueZoomConstant('fill-extrude-height') ||
-            this.getPaintValue('fill-extrude-height', globalProperties) !== 0;
-    }
-
     createBucket(options) {
-        if (this.isExtruded({zoom: options.zoom})) {
-            return new FillExtrusionBucket(options);
-        }
         return new FillBucket(options);
     }
 }

diff --git a/package.json b/package.json
@@ -22,8 +22,8 @@
     "gl-matrix": "^2.3.1",
     "grid-index": "^1.0.0",
     "mapbox-gl-function": "mapbox/mapbox-gl-function#41c6724e2bbd7bd1eb5991451bbf118b7d02b525",
-    "mapbox-gl-shaders": "mapbox/mapbox-gl-shaders#44b65f8090a74cbb0319664d010b8d8a8a1512b0",
-    "mapbox-gl-style-spec": "mapbox/mapbox-gl-style-spec#f5e88adb911b992494169541e17045bf6596a2f7",
+    "mapbox-gl-shaders": "mapbox/mapbox-gl-shaders#830540b26078141f4e38a62dcce1353a2538b1df",
+    "mapbox-gl-style-spec": "mapbox/mapbox-gl-style-spec#7fd0b3107e747745f14162a5b07202a8b74b3a43",
     "mapbox-gl-supported": "^1.2.0",
     "package-json-versionify": "^1.0.2",
     "pbf": "^1.3.2",
@@ -63,7 +63,7 @@
     "in-publish": "^2.0.0",
     "jsdom": "^9.4.2",
     "lodash.template": "^4.4.0",
-    "mapbox-gl-test-suite": "mapbox/mapbox-gl-test-suite#e3c236991ca1c6e3bd07491284b3b216d03cd285",
+    "mapbox-gl-test-suite": "mapbox/mapbox-gl-test-suite#232b9e423a61eec528d21e678d387b5aa133d5bb",
     "minifyify": "^7.0.1",
     "npm-run-all": "^3.0.0",
     "nyc": "^8.3.0",

diff --git a/test/js/style/style.test.js b/test/js/style/style.test.js
@@ -1194,34 +1194,6 @@ test('Style defers expensive methods', (t) => {
     });
 });
 
-test('Style updates source to switch fill[-extrusion] bucket types', (t) => {
-    // Runtime switching between non-extruded and extruded fills requires
-    // switching bucket types, so setPaintProperty in this case should trigger
-    // a worker roundtrip (as also happens when setting property functions)
-
-    const style = new Style(createStyleJSON({
-        "sources": {
-            "streets": createGeoJSONSource(),
-            "terrain": createGeoJSONSource()
-        }
-    }));
-
-    style.on('style.load', () => {
-        style.addLayer({ id: 'fill', type: 'fill', source: 'streets', paint: {}});
-        style.update();
-
-        t.notOk(style._updates.sources.streets);
-
-        style.setPaintProperty('fill', 'fill-color', 'green');
-        t.notOk(style._updates.sources.streets);
-
-        style.setPaintProperty('fill', 'fill-extrude-height', 10);
-        t.ok(style._updates.sources.streets);
-
-        t.end();
-    });
-});
-
 test('Style#query*Features', (t) => {
 
     // These tests only cover filter validation. Most tests for these methods

diff --git a/test/js/ui/map.test.js b/test/js/ui/map.test.js
@@ -382,10 +382,15 @@ test('Map', (t) => {
         function toFixed(bounds) {
             const n = 10;
             return [
-                [bounds[0][0].toFixed(n), bounds[0][1].toFixed(n)],
-                [bounds[1][0].toFixed(n), bounds[1][1].toFixed(n)]
+                [normalizeFixed(bounds[0][0], n), normalizeFixed(bounds[0][1], n)],
+                [normalizeFixed(bounds[1][0], n), normalizeFixed(bounds[1][1], n)]
             ];
         }
+
+        function normalizeFixed(num, n) {
+            // workaround for "-0.0000000000" ≠ "0.0000000000"
+            return parseFloat(num.toFixed(n)).toFixed(n);
+        }
     });
 
     t.test('#setMaxBounds', (t) => {