Relax clearing of tiles on layer removed/readded (#4010)

* Relax clearing of tiles on layer removed/readded Fixes #3895 * Add unit tests
mapbox · Jan 23, 2017 · bf9896c · bf9896c
1 parent 2663093
commit bf9896c
Show file tree

Hide file tree

Showing 3 changed files with 126 additions and 6 deletions.
diff --git a/debug/3895.html b/debug/3895.html
@@ -0,0 +1,52 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <title>Mapbox GL JS debug page</title>
+    <meta charset='utf-8'>
+    <meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no">
+    <link rel='stylesheet' href='/dist/mapbox-gl.css' />
+    <style>
+        body { margin: 0; padding: 0; }
+        html, body, #map { height: 100%; }
+    </style>
+</head>
+
+<body>
+<div id='map'></div>
+
+<script src='/dist/mapbox-gl-dev.js'></script>
+<script src='/debug/access_token_generated.js'></script>
+<script>
+
+var map = window.map = new mapboxgl.Map({
+    container: 'map',
+    center: [-68.13734351262877, 45.137451890638886],
+    zoom: 5,
+    style: 'mapbox://styles/mapbox/streets-v10',
+    hash: true
+});
+
+map.on('load', function () {
+    const layer = {
+        id: 'test',
+        source: 'composite',
+        'source-layer': 'water',
+        type: 'fill',
+        paint: {
+            'fill-color': '#00ff00'
+        }
+    };
+
+    map.addLayer(layer);
+
+    setInterval(function() {
+        const test = map.getLayer('test');
+        if (test) {
+            map.removeLayer('test');
+            map.addLayer(layer);
+        }
+    }, 2000);
+});
+</script>
+</body>
+</html>
diff --git a/js/style/style.js b/js/style/style.js
@@ -455,13 +455,15 @@ class Style extends Evented {
 
         if (this._removedLayers[id] && layer.source) {
             // If, in the current batch, we have already removed this layer
-            // and we are now re-adding it, then we need to clear (rather
-            // than just reload) the underyling source's tiles.
-            // Otherwise, tiles marked 'reloading' will have buffers that are
-            // set up for the _previous_ version of this layer, confusing
+            // and we are now re-adding it with a different `type`, then we
+            // need to clear (rather than just reload) the underyling source's
+            // tiles.  Otherwise, tiles marked 'reloading' will have buckets /
+            // buffers that are set up for the _previous_ version of this
+            // layer, causing, e.g.:
             // https://github.com/mapbox/mapbox-gl-js/issues/3633
+            const removed = this._removedLayers[id];
             delete this._removedLayers[id];
-            this._updatedSources[layer.source] = 'clear';
+            this._updatedSources[layer.source] = removed.type !== layer.type ? 'clear' : 'reload';
         }
         this._updateLayer(layer);
 
@@ -536,7 +538,7 @@ class Style extends Evented {
         }
 
         this._changed = true;
-        this._removedLayers[id] = true;
+        this._removedLayers[id] = layer;
         delete this._layers[id];
         delete this._updatedLayers[id];
         delete this._updatedPaintProps[id];

diff --git a/test/js/style/style.test.js b/test/js/style/style.test.js
@@ -625,6 +625,72 @@ test('Style#addLayer', (t) => {
         });
     });
 
+    t.test('#3895 reloads source (instead of clearing) if adding this layer with the same type, immediately after removing it', (t) => {
+        const style = new Style(util.extend(createStyleJSON(), {
+            "sources": {
+                "mapbox": {
+                    "type": "vector",
+                    "tiles": []
+                }
+            },
+            layers: [{
+                "id": "my-layer",
+                "type": "symbol",
+                "source": "mapbox",
+                "source-layer": "boxmap",
+                "filter": ["==", "id", 0]
+            }]
+        }));
+
+        const layer = {
+            "id": "my-layer",
+            "type": "symbol",
+            "source": "mapbox",
+            "source-layer": "boxmap"
+        };
+
+        style.on('style.load', () => {
+            style.sourceCaches['mapbox'].reload = t.end;
+            style.sourceCaches['mapbox'].clearTiles = t.fail;
+            style.removeLayer('my-layer');
+            style.addLayer(layer);
+            style.update();
+        });
+    });
+
+    t.test('clears source (instead of reloading) if adding this layer with a different type, immediately after removing it', (t) => {
+        const style = new Style(util.extend(createStyleJSON(), {
+            "sources": {
+                "mapbox": {
+                    "type": "vector",
+                    "tiles": []
+                }
+            },
+            layers: [{
+                "id": "my-layer",
+                "type": "symbol",
+                "source": "mapbox",
+                "source-layer": "boxmap",
+                "filter": ["==", "id", 0]
+            }]
+        }));
+
+        const layer = {
+            "id": "my-layer",
+            "type": "circle",
+            "source": "mapbox",
+            "source-layer": "boxmap"
+        };
+
+        style.on('style.load', () => {
+            style.sourceCaches['mapbox'].reload = t.fail;
+            style.sourceCaches['mapbox'].clearTiles = t.end;
+            style.removeLayer('my-layer');
+            style.addLayer(layer);
+            style.update();
+        });
+    });
+
     t.test('fires "data" event', (t) => {
         const style = new Style(createStyleJSON()),
             layer = {id: 'background', type: 'background'};