Don't regenerate crossTileIDs when a bucket is updated.

Fixes issue #6002, which caused symbols to fade in and out during setData animations.
mapbox · Jan 16, 2018 · b11ddc9 · b11ddc9
1 parent f232d74
commit b11ddc9
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 8 deletions.
diff --git a/src/symbol/cross_tile_symbol_index.js b/src/symbol/cross_tile_symbol_index.js
@@ -127,9 +127,19 @@ class CrossTileSymbolLayerIndex {
 
     addBucket(tileID: OverscaledTileID, bucket: SymbolBucket, crossTileIDs: CrossTileIDs) {
         if (this.indexes[tileID.overscaledZ] &&
-            this.indexes[tileID.overscaledZ][tileID.key] &&
-            this.indexes[tileID.overscaledZ][tileID.key].bucketInstanceId === bucket.bucketInstanceId) {
-            return false;
+            this.indexes[tileID.overscaledZ][tileID.key]) {
+            if (this.indexes[tileID.overscaledZ][tileID.key].bucketInstanceId ===
+                bucket.bucketInstanceId) {
+                return false;
+            } else {
+                // We're replacing this bucket with an updated version
+                // Remove the old bucket's "used crossTileIDs" now so that
+                // the new bucket can claim them.
+                // The old index entries themselves stick around until
+                // 'removeStaleBuckets' is called.
+                this.removeBucketCrossTileIDs(tileID.overscaledZ,
+                    this.indexes[tileID.overscaledZ][tileID.key]);
+            }
         }
 
         for (const symbolInstance of bucket.symbolInstances) {
@@ -175,17 +185,21 @@ class CrossTileSymbolLayerIndex {
         return true;
     }
 
+    removeBucketCrossTileIDs(zoom: string | number, removedBucket: TileLayerIndex) {
+        for (const key in removedBucket.indexedSymbolInstances) {
+            for (const symbolInstance of removedBucket.indexedSymbolInstances[key]) {
+                delete this.usedCrossTileIDs[zoom][symbolInstance.crossTileID];
+            }
+        }
+    }
+
     removeStaleBuckets(currentIDs: { [string | number]: boolean }) {
         let tilesChanged = false;
         for (const z in this.indexes) {
             const zoomIndexes = this.indexes[z];
             for (const tileKey in zoomIndexes) {
                 if (!currentIDs[zoomIndexes[tileKey].bucketInstanceId]) {
-                    for (const key in zoomIndexes[tileKey].indexedSymbolInstances) {
-                        for (const symbolInstance of zoomIndexes[tileKey].indexedSymbolInstances[key]) {
-                            delete this.usedCrossTileIDs[z][symbolInstance.crossTileID];
-                        }
-                    }
+                    this.removeBucketCrossTileIDs(z, zoomIndexes[tileKey]);
                     delete zoomIndexes[tileKey];
                     tilesChanged = true;
                 }

diff --git a/test/unit/symbol/cross_tile_symbol_index.js b/test/unit/symbol/cross_tile_symbol_index.js
@@ -142,12 +142,55 @@ test('CrossTileSymbolIndex.addLayer', (t) => {
         t.equal(mainInstances[0].crossTileID, 1);
         t.equal(mainInstances[1].crossTileID, 2);
 
+        const layerIndex = index.layerIndexes[styleLayer.id];
+        t.deepEqual(Object.keys(layerIndex.usedCrossTileIDs[6]), [1, 2]);
+
         // copies parent ids without duplicate ids in this tile
         index.addLayer(styleLayer, [childTile]);
         t.equal(childInstances[0].crossTileID, 1); // A' copies from A
         t.equal(childInstances[1].crossTileID, 2); // B' copies from B
         t.equal(childInstances[2].crossTileID, 3); // C' gets new ID
 
+        // Updates per-zoom usedCrossTileIDs
+        t.deepEqual(Object.keys(layerIndex.usedCrossTileIDs[6]), []);
+        t.deepEqual(Object.keys(layerIndex.usedCrossTileIDs[7]), [1, 2, 3]);
+
+        t.end();
+    });
+
+    t.test('does not regenerate ids for same zoom', (t) => {
+        const index = new CrossTileSymbolIndex();
+
+        const tileID = new OverscaledTileID(6, 0, 6, 8, 8);
+        const firstInstances = [
+            makeSymbolInstance(1000, 1000, ""), // A
+            makeSymbolInstance(1000, 1000, "")  // B
+        ];
+        const firstTile = makeTile(tileID, firstInstances);
+
+        const secondInstances = [
+            makeSymbolInstance(1000, 1000, ""), // A'
+            makeSymbolInstance(1000, 1000, ""), // B'
+            makeSymbolInstance(1000, 1000, ""), // C'
+        ];
+        const secondTile = makeTile(tileID, secondInstances);
+
+        // assigns new ids
+        index.addLayer(styleLayer, [firstTile]);
+        t.equal(firstInstances[0].crossTileID, 1);
+        t.equal(firstInstances[1].crossTileID, 2);
+
+        const layerIndex = index.layerIndexes[styleLayer.id];
+        t.deepEqual(Object.keys(layerIndex.usedCrossTileIDs[6]), [1, 2]);
+
+        // uses same ids when tile gets updated
+        index.addLayer(styleLayer, [secondTile]);
+        t.equal(secondInstances[0].crossTileID, 1); // A' copies from A
+        t.equal(secondInstances[1].crossTileID, 2); // B' copies from B
+        t.equal(secondInstances[2].crossTileID, 3); // C' gets new ID
+
+        t.deepEqual(Object.keys(layerIndex.usedCrossTileIDs[6]), [1, 2, 3]);
+
         t.end();
     });