Add "crossSourceCollisions" option to restore per-source collision detection #6566

ChrisLoer · 2018-04-25T20:22:13Z

This is an alternative PR to #6028 that removes text-collision-group and icon-collision-group from the style spec, and instead optionally groups layers based on their source. Setting map.crossSourceCollisions = false will restore the collision detection behavior from before cross-source collision detection (PR #5150).

The only change versus #6028 is the public interface -- the implementation under-the-hood is the same. The reasons for deliberately reducing the functionality are:

Studio team is uncomfortable with supporting the previous "collision group" proposal. The concern is not so much the immediate cost of building UI to support it as the concern that similar functionality will be available in the future as part of a larger "style components" feature, and backwards compatibility would require us to maintain two separate ways of doing the same thing (a serious design smell).
Since the use-cases we have in mind for wanting separate collision detection are generally of the form "put user-supplied data on top of a basemap", we expect that using "source" as the de-facto grouping mechanism for collision detection will be workable, even if it's not how we'd think about the problem to begin with. Also, we know it's backwards-compatible with pre-Viewport collision detection #5150 behavior.
Although we don't escape backwards compatibility concerns by moving to the map, the situation improves:
- Because the scope of the functionality is limited to "cross source placement", it should be relatively easy to implement continuing support for this option using whatever collision system we develop in the future.
- If we have to make a breaking change, it's not as painful for the SDK as it is for the style-spec
- By limiting this to a runtime option, we don't have to worry about dependence on this behavior working its way into widely used styles.

/cc @ansis @anandthakker @ttsirkia @samanpwbb @nickidlugash

anandthakker · 2018-04-27T08:54:22Z

This compromise & rationale mostly sounds good to me. One question: will @mapbox/studio support a way to toggle this runtime option on/off? Without that, a designer building a map style intended for use with crossSourceCollisions = false wouldn't be able to do all of their design work in Studio.

ChrisLoer · 2018-04-27T22:03:33Z

One question: will @mapbox/studio support a way to toggle this runtime option on/off? Without that, a designer building a map style intended for use with crossSourceCollisions = false wouldn't be able to do all of their design work in Studio.

Good question: @samanpwbb?

samanpwbb · 2018-04-28T00:27:46Z

One question: will @mapbox/studio support a way to toggle this runtime option on/off? Without that, a designer building a map style intended for use with crossSourceCollisions = false wouldn't be able to do all of their design work in Studio.

Right now Studio auto-composites all tilesets by default, and doesn't un-composite even if you turn off the autocomposite feature, so I could see this causing more confusion than anything else if we were to add the option. It is possible, and if it's a frequent feature request we could consider doing it.

phiphou · 2018-05-03T17:25:31Z

Does this PR solve my problem in #6290 ?

ChrisLoer · 2018-05-03T18:00:54Z

I believe so: since the markers you want to have their own collision detection are already in the separate "markers" GeoJSON source, you would just have to set the map option crossSourceCollisions: false.

nickidlugash · 2018-05-03T19:33:04Z

This seems like a good, forward-thinking solution 👍

One question: will @mapbox/studio support a way to toggle this runtime option on/off? Without that, a designer building a map style intended for use with crossSourceCollisions = false wouldn't be able to do all of their design work in Studio.

Note that this is already true for other runtime map options and plugins that can significantly affect the display of labels, such as the localIdeographFontFamily map option and mapbox-gl-rtl-text plugin.

anandthakker · 2018-05-03T20:01:23Z

src/symbol/grid_index.js

        } else {
            const queryArgs = {
                hitTest,
                seenUids: { box: {}, circle: {} }
            };
-            this._forEachCell(x1, y1, x2, y2, this._queryCell, result, queryArgs);
+            this._forEachCell(x1, y1, x2, y2, this._queryCell, result, queryArgs, predicate);
        }
        return hitTest ? result.length > 0 : result;


Can this return now be moved into the else block?

Yeah, that might be a cleaner way to write it, but we need to make sure to return something in the first clause if predicate is undefined.

anandthakker · 2018-05-03T20:13:43Z

src/symbol/placement.js

+                (key) => {
+                    return key.collisionGroup === 0;
+                };
+        }


This could cause a name collision if a source id (and thus the corresponding groupName) were "undefined". Could the default group just be a separate defaultGroup property on the class, rather than stored in the collisionGroups map?

We could then also just reference defaultGroup instead of doing collisionGroups.get()

anandthakker · 2018-05-03T20:19:37Z

src/symbol/collision_index.js

        const grid = ignorePlacement ? this.ignoredGrid : this.grid;

-        const key = { bucketInstanceId: bucketInstanceId, featureIndex: featureIndex };
+        const key = { bucketInstanceId: bucketInstanceId, featureIndex: featureIndex, collisionGroup: collisionGroup };


Nit: it might be helpful to rename this (both the key property and function parameter) to collisionGroupID, since collisionGroup could also refer to the group name

anandthakker · 2018-05-03T20:24:40Z

test/integration/render-tests/icon-collision-group/default/style.json

+  "sprite": "local://sprites/sprite",
+    "layers": [
+        {
+            "id": "defaultGroup1",


Would this really use the "default" group? My understanding is that with crossSourceCollisions: false, we're giving each source a named group.

Updated the names of the tests and the IDs of these layers to get rid of the vestigial grouping logic.

anandthakker · 2018-05-03T20:26:08Z

src/symbol/placement.js

+        if (groupName && this.maxGroupID === 0) {
+            // Keep the predicate null until the first collision
+            // group gets added to avoid overhead in the case
+            // everything's in the default group.


Will this ever happen in the current implementation? It looks to me like if crossSourceCollisions is set false, then every layer gets a named group, and otherwise, every layer uses the default group.

Yeah, this is probably weird dead code to leave around if we're going to use the crossSourceCollisions: bool approach. We might try to add something to skip the expense if crossSourceCollisions is true and there's only one source, but it's probably not an important optimization.

I changed CollisionGroups to explicitly work with source IDs now, which avoids the dead code and any worries about how to manage a "default" group.

ansis · 2018-05-03T22:10:42Z

Studio team is uncomfortable with supporting the previous "collision group" proposal. The concern is not so much the immediate cost of building UI to support it as the concern that similar functionality will be available in the future as part of a larger "style components" feature

@samanpwbb how do you see components and collisions working? I haven't been following those discussions closely but my gut reaction is that sometimes you'd want two components to collide with each other and sometimes not. It seems like these are two separate concerns?

map.crossSourceCollisions = false

I pretty hesitant about adding a map option that affects the style of the map. If we're willing to make this a public ui I think we should put it in the spec.

If we have to make a breaking change, it's not as painful for the SDK as it is for the style-spec

It's easier on our side because we don't need to handle migration, but for users adapting to the change would be the same no matter what. Even if we don't migrate styles.

My gut reaction is that we should try to figure out how collision groups would work with style components. If collisions are not handled at the style component level we should probably do #6028 instead of this intermediate solution.

samanpwbb · 2018-05-07T17:19:09Z

@samanpwbb how do you see components and collisions working? I haven't been following those discussions closely but my gut reaction is that sometimes you'd want two components to collide with each other and sometimes not. It seems like these are two separate concerns?

Here's an example component, with a crossComponentCollisions property:

{
  "id": "my-component",
  "name": "My Component",
  "sources": { ... },
  "layers": [
    {"id": "layer-id-1", "type": "fill", "layout": {}, "paint": {} },
    {"id": "layer-id-2", "type": "fill", "layout": {}, "paint": {} },
    ...
  ]
  // Options allows user to configure layers/sources in component
  "options": {
      "water": {
          "title": "Water",
          "type": "color",
          "doc": "Color of oceans, lakes, rivers, and other bodies of water.",
          "default": "#8ccbf7"
      },
      "language": {
          "title": "Language",
          "type": "enum",
          "values": ["default", "en", "fr", "ar"]
      }
  },
  // This property would act the same as colliisionGroup property,
  // But for all layers within the component
  "crossComponentCollisions": false
}

Imagine an app that uses the collisionGroup layer property. The app involves frequently adding and removing layers. The implementor would need to manage both the adding/removing of layers themselves, and then they'd need to update which layers belong to which collision group. That makes the amount of state the app needs to grapple with higher than it needs to be, imo.

Components could also serve the purpose of managing which layers belong to which collision group. Simply create two components: One for each of your sources, and then place layers in the correct component. No longer need to manage this additional state property (the collision group).

I'm most concerned about a future where we have both components, and collision groups – essentially two competing state management systems.

ansis · 2018-05-08T21:38:29Z

Thanks @samanpwbb

Imagine an app that uses the collisionGroup layer property. The app involves frequently adding and removing layers. The implementor would need to manage both the adding/removing of layers themselves, and then they'd need to update which layers belong to which collision group. That makes the amount of state the app needs to grapple with higher than it needs to be, imo.

If I'm understanding the example correctly, the advantage of of specifying the collision group on the component instead of the layer is that you can set the property once and have it automatically apply to a whole bunch of layers. I think this reasoning could apply to other properties as well. What makes collision-group special when compared to other properties like text-font? Would setting the collision-group for each layer in the example be different than setting the font for each layer that gets added and removed?

I'm not sure I understand why collision group should be treated as an exception

One possible approach alternative approach to reducing verbosity/redundancy could be to provide some way of specifying default values at the component level. The component creator could then do "defaults": { "text-collision-group": "my-component-name", "text-font": "My Font" } and add layers without touching these. This adds complexity though

samanpwbb · 2018-05-08T21:51:02Z

What makes collision-group special when compared to other properties like text-font?

The component abstraction does benefit both properties. The difference between a Collision group string property and other properties is that collision group property is effected by the value in other layers, and would be much easier to work with if it was clear which other layers shared the value. Changing text-font on a layer is immediately apparent and won’t be effected by changes to other layers.

cammanderson · 2018-05-22T06:34:42Z

Hi Guys,

Keen to see a work around developed for this. We've lost some key information from our maps for our users by going > 0.42, but due to the fixes for iOS 11.3 contained in later releases we can't revert.

Just wanting to know if a solution is likely to fall in an upcoming release, or whether we have to act on an alternative work around until this issue is supported?

Thanks
Cameron

ChrisLoer · 2018-05-24T14:29:01Z

Hi @cammanderson sorry for the delay, we're still trying to figure out the most consistent way to implement this so that we don't introduce inconsistencies in the style-spec that may conflict with future functionality. It would help us reason about the problem if you could provide some detail about what you're trying to accomplish and which part gets blocked by the current behavior.

cammanderson · 2018-05-24T19:59:33Z

Thanks for the update. We are placing symbols on a map across a number of locations. We used to have symbols sit on top of towns labels etc, and the town labels would still be present as our symbol data was from another source (think store locator when zoomed out you still want to see the spread of stores but maintain the contextual labeling). Now all context is lost as all the map location labels have disappeared due to the collision behaviour changing. We can’t really change the location label behaviour because they were working as intended to resolve their conflicts with other location labels as desired. We also can’t use marker controls as we are using a lot of symbols from our data sources (eg vector tiles in some cases) and rely also on those symbols collision behaviour also. Our maps aren’t static enough for us to hardcode or have a reasonable workaround or a scalable workaround. My only option would be to revert to a version of mapbox that supports data driven expressions and then manually apply the major iOS bug fix that came in later releases by itself, which came after this global collision behaviour change. I’m not even sure if that’s possible but I guess I just need to know how long as we have several deadlines fast approaching. I can appreciate the challenge of updating the style spec and we don’t want to create junk, I’m an advocate of the collision groups PR. What I’m unclear about is why Studio UI support would drive the style spec? Many of us affected are using the API and various JS from your examples to build additional data sources and layers dynamically on top of base styles. I can appreciate the PR to change the map property in one place as a quicker work around, but either way it’s a tricky scenario where due to major compliance with iOS releases we are forced to adopt the versions where the global collision behaviour has changed with a dramatic effect of some existing use cases that I suspect are not niche.

… against the content of keys.

…ps but not between them.

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.

- Rename tests - Remove dead code from CollisionGroups, update parameter names - Simplify return pathways for _query

ChrisLoer · 2018-05-29T21:03:51Z

I just rebased on master and addressed @anandthakker's review comments, but we're still looking for consensus on the design direction.

Thanks for the extra context @cammanderson! From the input I've seen, it seems like a common thread is thinking of basemap labels as part of the "background" for symbols added at runtime to be displayed on top of. I think our default response to the "marker on top of a location label" case has been something along the lines of "isn't it better for legibility if the location label gets hidden since it's partially occluded". From your description of your case, it sounds like (1) it's frequently the case that enough of the location label is visible for it to be legible, (2) marker/location-label overlap isn't a visual problem because the marker has an opaque background, (3) if markers are allowed to collide out location labels, the location-label density ends up too low to be useful?

What I’m unclear about is why Studio UI support would drive the style spec?

We want Studio to be capable of creating/editing any Mapbox style. Blocking style spec changes that don't have a matching Studio UI design helps us achieve that goal, but I think it also forces us to have a higher bar for what we accept into the style spec (i.e. if we're not sure what the matching Studio UI for a change is, that implies we're introducing a change to the style spec that departs from existing conventions and may cause trouble for other users as well).

ChrisLoer · 2018-09-06T19:51:47Z

For the archaeological record: we deferred implementing this on gl-native because we weren't totally comfortable with the design direction and we were hoping that there might not be large gl-native users dependent on the old behavior. I think "wait and see" was a reasonable strategy, but in this case it only took ~2.5 months before the native version of this showed up as a high priority customer issue.

ChrisLoer requested review from ansis and anandthakker April 25, 2018 20:22

ChrisLoer force-pushed the per-source-collision branch from 5c09623 to 9097003 Compare April 25, 2018 21:06

ChrisLoer mentioned this pull request Apr 30, 2018

Rollover effect with 0.45 #6290

Closed

anandthakker reviewed May 3, 2018

View reviewed changes

ChrisLoer added 5 commits May 29, 2018 15:14

Make GridIndex query calls take an optional predicate that can filter…

69960eb

… against the content of keys.

Introduce "collision groups": collision detection happens within grou…

14d740e

…ps but not between them.

Render tests for text-collision-group and icon-collision-group.

4f920a5

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

1ea1c4a

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.

"collision group" -> "cross source collisions" conversion:

e9dd1b9

- Rename tests - Remove dead code from CollisionGroups, update parameter names - Simplify return pathways for _query

ChrisLoer force-pushed the per-source-collision branch from 9097003 to e9dd1b9 Compare May 29, 2018 20:30

ChrisLoer mentioned this pull request Jun 7, 2018

Collision groups #6028

Closed

anandthakker approved these changes Jun 8, 2018

View reviewed changes

ChrisLoer merged commit 38138a3 into master Jun 8, 2018

ChrisLoer deleted the per-source-collision branch June 8, 2018 15:44

ChrisLoer mentioned this pull request Jun 8, 2018

Add option to allow overlapping of labels from different sources #5836

Closed

ChrisLoer mentioned this pull request Sep 4, 2018

interpolate expression: strange numeric issues #7163

Closed

lilykaiser mentioned this pull request Sep 5, 2018

Add an option to restore per-source collision detection mapbox/mapbox-gl-native#12820

Closed

florianPOLARSTEPS mentioned this pull request Apr 30, 2020

Increase flexibility of collision detection options #4117

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "crossSourceCollisions" option to restore per-source collision detection #6566

Add "crossSourceCollisions" option to restore per-source collision detection #6566

ChrisLoer commented Apr 25, 2018

anandthakker commented Apr 27, 2018

ChrisLoer commented Apr 27, 2018

samanpwbb commented Apr 28, 2018

phiphou commented May 3, 2018

ChrisLoer commented May 3, 2018

nickidlugash commented May 3, 2018

anandthakker May 3, 2018

ChrisLoer May 7, 2018

ChrisLoer May 29, 2018

anandthakker May 3, 2018

anandthakker May 3, 2018

anandthakker May 3, 2018

anandthakker May 3, 2018

ChrisLoer May 29, 2018

anandthakker May 3, 2018

ChrisLoer May 7, 2018

ChrisLoer May 29, 2018

ansis commented May 3, 2018

samanpwbb commented May 7, 2018 •

edited

Loading

ansis commented May 8, 2018

samanpwbb commented May 8, 2018 •

edited

Loading

cammanderson commented May 22, 2018

ChrisLoer commented May 24, 2018

cammanderson commented May 24, 2018

ChrisLoer commented May 29, 2018

ChrisLoer commented Sep 6, 2018

Add "crossSourceCollisions" option to restore per-source collision detection #6566

Add "crossSourceCollisions" option to restore per-source collision detection #6566

Conversation

ChrisLoer commented Apr 25, 2018

anandthakker commented Apr 27, 2018

ChrisLoer commented Apr 27, 2018

samanpwbb commented Apr 28, 2018

phiphou commented May 3, 2018

ChrisLoer commented May 3, 2018

nickidlugash commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansis commented May 3, 2018

samanpwbb commented May 7, 2018 • edited Loading

ansis commented May 8, 2018

samanpwbb commented May 8, 2018 • edited Loading

cammanderson commented May 22, 2018

ChrisLoer commented May 24, 2018

cammanderson commented May 24, 2018

ChrisLoer commented May 29, 2018

ChrisLoer commented Sep 6, 2018

samanpwbb commented May 7, 2018 •

edited

Loading

samanpwbb commented May 8, 2018 •

edited

Loading