Remove icon/text-collision-group from style spec.

Re-implement basic collision group support based on a global "crossSourceCollisions" map option that replicates pre-#5150 behavior. Render tests maintain the same structure/results, but are now based on grouping-by-source.
mapbox · Jun 8, 2018 · 003d3ce · 003d3ce
1 parent f9ddc66
commit 003d3ce
Show file tree

Hide file tree

Showing 14 changed files with 147 additions and 83 deletions.
diff --git a/flow-typed/style-spec.js b/flow-typed/style-spec.js
@@ -208,7 +208,6 @@ declare type SymbolLayerSpecification = {|
         "symbol-avoid-edges"?: PropertyValueSpecification<boolean>,
         "icon-allow-overlap"?: PropertyValueSpecification<boolean>,
         "icon-ignore-placement"?: PropertyValueSpecification<boolean>,
-        "icon-collision-group"?: string,
         "icon-optional"?: PropertyValueSpecification<boolean>,
         "icon-rotation-alignment"?: PropertyValueSpecification<"map" | "viewport" | "auto">,
         "icon-size"?: DataDrivenPropertyValueSpecification<number>,
@@ -239,7 +238,6 @@ declare type SymbolLayerSpecification = {|
         "text-offset"?: DataDrivenPropertyValueSpecification<[number, number]>,
         "text-allow-overlap"?: PropertyValueSpecification<boolean>,
         "text-ignore-placement"?: PropertyValueSpecification<boolean>,
-        "text-collision-group"?: string,
         "text-optional"?: PropertyValueSpecification<boolean>,
         "visibility"?: "visible" | "none"
     |},

diff --git a/src/data/bucket.js b/src/data/bucket.js
@@ -14,7 +14,8 @@ export type BucketParameters<Layer: TypedStyleLayer> = {
     pixelRatio: number,
     overscaling: number,
     collisionBoxArray: CollisionBoxArray,
-    sourceLayerIndex: number
+    sourceLayerIndex: number,
+    sourceID: string
 }
 
 export type PopulateParameters = {

diff --git a/src/data/bucket/symbol_bucket.js b/src/data/bucket/symbol_bucket.js
@@ -293,6 +293,7 @@ class SymbolBucket implements Bucket {
     collisionCircle: CollisionBuffers;
     uploaded: boolean;
     sourceLayerIndex: number;
+    sourceID: string;
 
     constructor(options: BucketParameters<SymbolStyleLayer>) {
         this.collisionBoxArray = options.collisionBoxArray;
@@ -313,6 +314,8 @@ class SymbolBucket implements Bucket {
         const layout = this.layers[0].layout;
         this.sortFeaturesByY = layout.get('text-allow-overlap') || layout.get('icon-allow-overlap') ||
             layout.get('text-ignore-placement') || layout.get('icon-ignore-placement');
+
+        this.sourceID = options.sourceID;
     }
 
     createArrays() {

diff --git a/src/source/worker_tile.js b/src/source/worker_tile.js
@@ -109,7 +109,8 @@ class WorkerTile {
                     pixelRatio: this.pixelRatio,
                     overscaling: this.overscaling,
                     collisionBoxArray: this.collisionBoxArray,
-                    sourceLayerIndex: sourceLayerIndex
+                    sourceLayerIndex: sourceLayerIndex,
+                    sourceID: this.source
                 });
 
                 bucket.populate(features, options);

diff --git a/src/style-spec/reference/v8.json b/src/style-spec/reference/v8.json
@@ -979,19 +979,6 @@
       },
       "property-type": "data-constant"
     },
-    "icon-collision-group": {
-      "type": "string",
-      "doc": "Restricts collision detection to other symbols in the same group.",
-      "requires": [
-        "icon-image"
-      ],
-      "sdk-support": {
-        "basic functionality": {
-          "js": "0.45.0"
-        },
-        "data-driven styling": {}
-      }
-    },
     "icon-optional": {
       "type": "boolean",
       "default": false,
@@ -2018,19 +2005,6 @@
       },
       "property-type": "data-constant"
     },
-    "text-collision-group": {
-      "type": "string",
-      "doc": "Restricts collision detection to other symbols in the same group.",
-      "requires": [
-        "text-field"
-      ],
-      "sdk-support": {
-        "basic functionality": {
-          "js": "0.45.0"
-        },
-        "data-driven styling": {}
-      }
-    },
     "text-optional": {
       "type": "boolean",
       "default": false,

diff --git a/src/style/pauseable_placement.js b/src/style/pauseable_placement.js
@@ -40,9 +40,12 @@ class PauseablePlacement {
     _inProgressLayer: ?LayerPlacement;
 
     constructor(transform: Transform, order: Array<string>,
-            forceFullPlacement: boolean, showCollisionBoxes: boolean, fadeDuration: number) {
+                forceFullPlacement: boolean,
+                showCollisionBoxes: boolean,
+                fadeDuration: number,
+                crossSourceCollisions: boolean) {
 
-        this.placement = new Placement(transform, fadeDuration);
+        this.placement = new Placement(transform, fadeDuration, crossSourceCollisions);
         this._currentPlacementIndex = order.length - 1;
         this._forceFullPlacement = forceFullPlacement;
         this._showCollisionBoxes = showCollisionBoxes;

diff --git a/src/style/style.js b/src/style/style.js
@@ -1004,7 +1004,7 @@ class Style extends Evented {
         }
     }
 
-    _updatePlacement(transform: Transform, showCollisionBoxes: boolean, fadeDuration: number) {
+    _updatePlacement(transform: Transform, showCollisionBoxes: boolean, fadeDuration: number, crossSourceCollisions: boolean) {
         let symbolBucketsChanged = false;
         let placementCommitted = false;
 
@@ -1033,7 +1033,7 @@ class Style extends Evented {
         const forceFullPlacement = this._layerOrderChanged;
 
         if (forceFullPlacement || !this.pauseablePlacement || (this.pauseablePlacement.isDone() && !this.placement.stillRecent(browser.now()))) {
-            this.pauseablePlacement = new PauseablePlacement(transform, this._order, forceFullPlacement, showCollisionBoxes, fadeDuration);
+            this.pauseablePlacement = new PauseablePlacement(transform, this._order, forceFullPlacement, showCollisionBoxes, fadeDuration, crossSourceCollisions);
             this._layerOrderChanged = false;
         }
 

diff --git a/src/style/style_layer/symbol_style_layer_properties.js b/src/style/style_layer/symbol_style_layer_properties.js
@@ -20,7 +20,6 @@ export type LayoutProps = {|
     "symbol-avoid-edges": DataConstantProperty<boolean>,
     "icon-allow-overlap": DataConstantProperty<boolean>,
     "icon-ignore-placement": DataConstantProperty<boolean>,
-    "icon-collision-group": DataConstantProperty<string>,
     "icon-optional": DataConstantProperty<boolean>,
     "icon-rotation-alignment": DataConstantProperty<"map" | "viewport" | "auto">,
     "icon-size": DataDrivenProperty<number>,
@@ -51,7 +50,6 @@ export type LayoutProps = {|
     "text-offset": DataDrivenProperty<[number, number]>,
     "text-allow-overlap": DataConstantProperty<boolean>,
     "text-ignore-placement": DataConstantProperty<boolean>,
-    "text-collision-group": DataConstantProperty<string>,
     "text-optional": DataConstantProperty<boolean>,
 |};
 
@@ -61,7 +59,6 @@ const layout: Properties<LayoutProps> = new Properties({
     "symbol-avoid-edges": new DataConstantProperty(styleSpec["layout_symbol"]["symbol-avoid-edges"]),
     "icon-allow-overlap": new DataConstantProperty(styleSpec["layout_symbol"]["icon-allow-overlap"]),
     "icon-ignore-placement": new DataConstantProperty(styleSpec["layout_symbol"]["icon-ignore-placement"]),
-    "icon-collision-group": new DataConstantProperty(styleSpec["layout_symbol"]["icon-collision-group"]),
     "icon-optional": new DataConstantProperty(styleSpec["layout_symbol"]["icon-optional"]),
     "icon-rotation-alignment": new DataConstantProperty(styleSpec["layout_symbol"]["icon-rotation-alignment"]),
     "icon-size": new DataDrivenProperty(styleSpec["layout_symbol"]["icon-size"]),
@@ -92,7 +89,6 @@ const layout: Properties<LayoutProps> = new Properties({
     "text-offset": new DataDrivenProperty(styleSpec["layout_symbol"]["text-offset"]),
     "text-allow-overlap": new DataConstantProperty(styleSpec["layout_symbol"]["text-allow-overlap"]),
     "text-ignore-placement": new DataConstantProperty(styleSpec["layout_symbol"]["text-ignore-placement"]),
-    "text-collision-group": new DataConstantProperty(styleSpec["layout_symbol"]["text-collision-group"]),
     "text-optional": new DataConstantProperty(styleSpec["layout_symbol"]["text-optional"]),
 });
 

diff --git a/src/symbol/placement.js b/src/symbol/placement.js
@@ -132,8 +132,9 @@ export class Placement {
     fadeDuration: number;
     retainedQueryData: {[number]: RetainedQueryData};
     collisionGroups: CollisionGroups;
+    crossSourceCollisions: boolean;
 
-    constructor(transform: Transform, fadeDuration: number) {
+    constructor(transform: Transform, fadeDuration: number, crossSourceCollisions: boolean) {
         this.transform = transform.clone();
         this.collisionIndex = new CollisionIndex(this.transform);
         this.placements = {};
@@ -142,6 +143,7 @@ export class Placement {
         this.fadeDuration = fadeDuration;
         this.retainedQueryData = {};
         this.collisionGroups = new CollisionGroups();
+        this.crossSourceCollisions = crossSourceCollisions;
     }
 
     placeLayerTile(styleLayer: StyleLayer, tile: Tile, showCollisionBoxes: boolean, seenCrossTileIDs: { [string | number]: boolean }) {
@@ -195,10 +197,9 @@ export class Placement {
         const iconWithoutText = !bucket.hasTextData() || layout.get('text-optional');
         const textWithoutIcon = !bucket.hasIconData() || layout.get('icon-optional');
 
-        const textCollisionGroup =
-            this.collisionGroups.get(layout.get('text-collision-group'));
-        const iconCollisionGroup =
-            this.collisionGroups.get(layout.get('icon-collision-group'));
+        const collisionGroup = this.crossSourceCollisions ?
+            this.collisionGroups.get() :
+            this.collisionGroups.get(bucket.sourceID);
 
         for (const symbolInstance of bucket.symbolInstances) {
             if (!seenCrossTileIDs[symbolInstance.crossTileID]) {
@@ -225,7 +226,7 @@ export class Placement {
                 }
                 if (symbolInstance.collisionArrays.textBox) {
                     placedGlyphBoxes = this.collisionIndex.placeCollisionBox(symbolInstance.collisionArrays.textBox,
-                            layout.get('text-allow-overlap'), textPixelRatio, posMatrix, textCollisionGroup.predicate);
+                            layout.get('text-allow-overlap'), textPixelRatio, posMatrix, collisionGroup.predicate);
                     placeText = placedGlyphBoxes.box.length > 0;
                     offscreen = offscreen && placedGlyphBoxes.offscreen;
                 }
@@ -246,7 +247,7 @@ export class Placement {
                             textLabelPlaneMatrix,
                             showCollisionBoxes,
                             layout.get('text-pitch-alignment') === 'map',
-                            textCollisionGroup.predicate);
+                            collisionGroup.predicate);
                     // If text-allow-overlap is set, force "placedCircles" to true
                     // In theory there should always be at least one circle placed
                     // in this case, but for now quirks in text-anchor
@@ -260,7 +261,7 @@ export class Placement {
                 }
                 if (symbolInstance.collisionArrays.iconBox) {
                     placedIconBoxes = this.collisionIndex.placeCollisionBox(symbolInstance.collisionArrays.iconBox,
-                            layout.get('icon-allow-overlap'), textPixelRatio, posMatrix, iconCollisionGroup.predicate);
+                            layout.get('icon-allow-overlap'), textPixelRatio, posMatrix, collisionGroup.predicate);
                     placeIcon = placedIconBoxes.box.length > 0;
                     offscreen = offscreen && placedIconBoxes.offscreen;
                 }
@@ -276,15 +277,15 @@ export class Placement {
 
                 if (placeText && placedGlyphBoxes) {
                     this.collisionIndex.insertCollisionBox(placedGlyphBoxes.box, layout.get('text-ignore-placement'),
-                            bucket.bucketInstanceId, textFeatureIndex, textCollisionGroup.ID);
+                            bucket.bucketInstanceId, textFeatureIndex, collisionGroup.ID);
                 }
                 if (placeIcon && placedIconBoxes) {
                     this.collisionIndex.insertCollisionBox(placedIconBoxes.box, layout.get('icon-ignore-placement'),
-                            bucket.bucketInstanceId, iconFeatureIndex, iconCollisionGroup.ID);
+                            bucket.bucketInstanceId, iconFeatureIndex, collisionGroup.ID);
                 }
                 if (placeText && placedGlyphCircles) {
                     this.collisionIndex.insertCollisionCircles(placedGlyphCircles.circles, layout.get('text-ignore-placement'),
-                            bucket.bucketInstanceId, textFeatureIndex, textCollisionGroup.ID);
+                            bucket.bucketInstanceId, textFeatureIndex, collisionGroup.ID);
                 }
 
                 assert(symbolInstance.crossTileID !== 0);

diff --git a/src/ui/map.js b/src/ui/map.js
@@ -123,7 +123,8 @@ const defaultOptions = {
     maxTileCacheSize: null,
 
     transformRequest: null,
-    fadeDuration: 300
+    fadeDuration: 300,
+    crossSourceCollisions: true
 };
 
 /**
@@ -198,6 +199,7 @@ const defaultOptions = {
  *   Expected to return an object with a `url` property and optionally `headers` and `credentials` properties.
  * @param {boolean} [options.collectResourceTiming=false] If `true`, Resource Timing API information will be collected for requests made by GeoJSON and Vector Tile web workers (this information is normally inaccessible from the main Javascript thread). Information will be returned in a `resourceTiming` property of relevant `data` events.
  * @param {number} [options.fadeDuration=300] Controls the duration of the fade-in/fade-out animation for label collisions, in milliseconds. This setting affects all symbol layers. This setting does not affect the duration of runtime styling transitions or raster tile cross-fading.
+ * @param {boolean} [options.crossSourceCollisions=true] If `true`, symbols from multiple sources can collide with each other during collision detection. If `false`, collision detection is run separately for the symbols in each source.
  * @example
  * var map = new mapboxgl.Map({
  *   container: 'map',
@@ -247,6 +249,7 @@ class Map extends Camera {
     _hash: Hash;
     _delegatedListeners: any;
     _fadeDuration: number;
+    _crossSourceCollisions: boolean;
     _crossFadingFactor: number;
     _collectResourceTiming: boolean;
     _renderTaskQueue: TaskQueue;
@@ -306,6 +309,7 @@ class Map extends Camera {
         this._bearingSnap = options.bearingSnap;
         this._refreshExpiredTiles = options.refreshExpiredTiles;
         this._fadeDuration = options.fadeDuration;
+        this._crossSourceCollisions = options.crossSourceCollisions;
         this._crossFadingFactor = 1;
         this._collectResourceTiming = options.collectResourceTiming;
         this._renderTaskQueue = new TaskQueue();
@@ -1654,7 +1658,7 @@ class Map extends Camera {
             this.style._updateSources(this.transform);
         }
 
-        this._placementDirty = this.style && this.style._updatePlacement(this.painter.transform, this.showCollisionBoxes, this._fadeDuration);
+        this._placementDirty = this.style && this.style._updatePlacement(this.painter.transform, this.showCollisionBoxes, this._fadeDuration, this._crossSourceCollisions);
 
         // Actually draw
         this.painter.render(this.style, {