Collision groups

[ChrisLoer]

Fixes issue #5836.

This PR introduces two style properties, text-collision-group and icon-collision-group. Under the hood, these get mapped to numbers that are stored with each entry in the CollisionIndex. The default (aka unspecified value) is for properties to go in group "0". Collision detection is then performed only within that group.

I haven't done performance testing yet, but the idea is that the overhead should be pretty low if grouping isn't being used, because we omit the "predicate" function when we query the GridIndex -- the only cost should be looking up style properties and storing a tiny bit of extra data in the GridIndex. For styles with a large number of symbols split between multiple collision groups, it might be faster to try an alternative approach where we maintain one GridIndex for each group, but the current approach has the benefit of relative simplicity and flexibility in the future to do collision testing across multiple groups in one query. Going in the other direction, we could easily use this approach to fold the ignoredGrid into the main collision grid, but there'd be a potential perf hit.

I haven't thought much about how these properties should tie in with DDS/expressions. I think it would be straightforward to enable them as zoom-and-property functions, but I can't immediately think of the use case to justify the cost.

I currently have a modified debug.html checked in that puts two layers from a GeoJSON source into their own collision-group, in order to play with the functionality. No automated tests implemented yet.

/cc @ansis @ttsirkia @anandthakker

[ttsirkia]

Great job! I experimented the new feature in my original use case and this would perfectly solve it. This is very easy to use and understand the functionality.

[ansis]

@ChrisLoer the approach here sounds good to me. Should we get feedback from the cartography team on this before adding it to the spec?

[ChrisLoer]

Yeah, good idea: @nickidlugash or other @mapbox/cartography, any feedback on the idea of adding text-collision-group and icon-collision-group to the style spec?

[ChrisLoer]

No obvious change in the relevant benchmarks (which test the case where the feature isn't being used):

https://bl.ocks.org/anonymous/raw/1703e58f5ba2ab8be913d2b8c2c4c3f1/
https://bl.ocks.org/anonymous/raw/559a1fd9c99292f2cdf61e5868e888b3/
https://bl.ocks.org/anonymous/raw/e30057107af793d831106e0121dea3bf/

[nickidlugash]

@ChrisLoer per chat, it would be great to get on this ticket your thoughts on how this new feature could expand to support other requested collision behavior. I think having more control over collisions would be a valuable addition to the spec, and it probably makes sense to consider the full scope of needs now to help us create the most flexibility with the least spec complexity.

[ChrisLoer]

Should we get feedback from the cartography team on this before adding it to the spec?

There's a lot of discussion in #4117, pointing in several directions:

• Instead of adding collision options, remove options which will allow simplifying/improving Studio support (see Increase flexibility of collision detection options #4117 (comment)).
• Maybe don't support the previous per-source collision detection at all? This could be "wait for this to be part of style components" or just "find some other way to adapt".
• Very specifically restore the previous per-source collision detection behavior as a global option, without making it part of the style spec.
• An alternative collides proposal that would cover more potential use cases with a single style-spec property, at the cost of increased style complexity in some of the simple cases (apologies if I oversimplified/distorted the tradeoffs here)
• The collision-group proposal embodied in this PR, along with a speculative collides-with proposal that could extend it to support all of the same use cases as the collides proposal (but: I think there's a fair chance we won't need to do this in at least the medium-term future).

At the highest level, the different proposals lie at different points on the "keep the style spec simple" vs "add more functionality" spectrum, and there are good arguments for all of them. Unfortunately, I don't think we're going to be able to settle on a single right answer here, and I don't want to succumb to decision paralysis -- it's already been three months since this PR was opened.

So: unless someone pushes back harder, I'm planning to push through this PR as is. There is downside risk on supportability here, and I'm gambling that the upside is worth it. @ansis or @anandthakker, could you give the code another review (it's changed a bit since the PR first went up after being rebased a few times)?

[samanpwbb]

It is unlikely that Studio will have capacity to build a dedicated UI for managing collision groups. Without a dedicated UI, we can offer "collision group" text inputs on layers, but I think that will be confusing for 95% of our users, and I'd strongly urge us to seek a simpler solution.

I described my position in #4117 (comment).

[ChrisLoer]

Without a dedicated UI, we can offer "collision group" text inputs on layers, but I think that will be confusing for 95% of our users, and I'd strongly urge us to seek a simpler solution.

I think no explicit Studio support at all seems like a good approach, given the expected use case is more oriented towards run-time additions to the map.

[ChrisLoer]

After further discussion with @samanpwbb et al, I'm going to try to make a version of this PR that side-steps the style-spec concerns by making "per source collision detection" a map option. I think it's probably less elegant, but with the fallout more limited to GL (ie if we supersede it in the future with "style component" functionality, Studio won't need to maintain backwards compatibility with an older way of accomplishing the same thing).

[ChrisLoer]

We've decided against this approach in favor of the more limited approach in #6566.