Increase flexibility of collision detection options

lucaswoj commented 7 years ago

From @ansis on March 18, 2016 0:53

There are currently two properties that affect how a symbol

-allow-overlap determines if previous symbols affect the placement of the current symbol
-ignore-placement determines if the current symbol will affect the placement of layer symbols

In most cases you want both to have the same value. I can't think of an example you would want them to be different.

We should remove -ignore-placement and make -allow-overlap do what both these properties currently do.

@nickidlugash @peterqliu can you imagine a case where you would want these two properties to be separate?

Copied from original issue: mapbox/mapbox-gl-style-spec#429

lucaswoj commented 7 years ago

From @nickidlugash on March 18, 2016 18:49

@ansis – @peterqliu has probably touched upon more varied styling use cases, but personally I don't have any important use cases for having different values for these two properties. I'm a fan of reducing the number of arcane properties pertaining to label placement/behavior :+1:

Not to open a can of worms, but this is what I actually want for collision properties (actual property names TBD):

Allow overlap with symbols in other layers
Allow overlap within symbols in current layer

I think the distinction between whether a symbol can overlap symbols within its own layer or with symbols in other layers is much more important for styling than the distinction between whether a symbol can overlap symbols in layers above or below the current layer. Since collision detection is per-feature, both our current properties essentially create identical behavior for symbols within the same layer (a true value for either makes symbols within a layer overlap each other). I think this has been the source of a lot of user confusion between the two properties. We have two properties to control overlapping symbols, but you still can't control what you actually want to control.

raphaelvigee commented 6 years ago

Any news on this feature ?

ChrisLoer commented 6 years ago

@nickidlugash I have a proposal in https://github.com/mapbox/mapbox-gl-js/issues/5836#issuecomment-358507433 that's aimed at restoring the ability to allow overlap between symbols from multiple sources. It's backwards compatible, which means no reduction in style-spec complexity. Of:

Allow overlap with symbols in other layers

Allow overlap within symbols in current layer

It would support (1), but not (2).

ChrisLoer commented 6 years ago

After discussion with @nickidlugash, I've decided to hijack this issue in favor of the "expand scope" part and give up on the proposal to make breaking changes. Even though they'd be a valuable simplification, I think the somewhat stable consensus is that we're not going to make breaking changes to the style spec anytime soon. Issue #5836 gives us motivation to add some functionality now, but we also want to make sure any changes we make are forward-compatible, so I'm going to lay out a two-phase proposal:

Allow overlap with symbols in other layers (`*-collision-group`)

The first phase is to add a "collision group" property that controls which layers a feature can collide with. Putting all of the layers from a given source within their own collision group can mimic the collision behavior we had before we introduced cross-source collision detection (#5150). The collision group property is compatible with the existing allow-overlap and ignore-placement settings, and if it's not set everything just goes in the "default" group.

This is implemented in PR #6028.

Allow overlap with symbols in current layer (`*-collides-with`)

The goal of the second phase is to allow maximal expressivity of collision logic. It would have two parts:

Add a *-collides-with property that determines which collision groups a layer should collide against.
Make both *-collides-with and *-collision-group into array properties, so that one layer can be inserted in multiple groups via *-collision-group and collide against multiple (different) groups via *-collides-with.

The default for *-collides-with is to be equal to the set of groups in *-collision-group, and the default for *-collision-group is empty/“the default group”. *-text-ignore-placement becomes a redundant way to express *-collision-group: [], and *-allow-overlap becomes a redundant way to express *-collides-with: []

"Allow overlap with symbols in the current layer, but not others" would become something like collision-group: 'my-group', collides-with: ['default-group','some-other-group'].

Open Questions/Criticisms

If we're going to do the two-phase approach, does it make sense to start with a property like *-collision-group as just a string and then turn it into an array later?
If we're using arrays of collision groups, we'd need some way to refer to the "default" group (maybe just give it a hardcoded name?)
Would we want some way to inspect the "collision groups" implied by a style, allowing expressions like "collides with all groups but mine"?

@nickidlugash brought up the concern that having these groups be implicitly formed by layer properties is somewhat "backwards" -- maybe the groups should exist as their own objects in the style-spec?

It would be very helpful to come up with some use cases for the "second phase" to see if it makes sense. If we decide we like it, maybe it makes sense to implement both phases at once. If we don't like it, maybe that's a blocker for merging PR #6028...

ansis commented 6 years ago

If we have a way to support all the cases covered by *-allow-overlap and *-ignore-placement will we be able to deprecate these properties? or are we past the point where we are able to do that?

This is definitely a nicer alternative to *-allow-overlap and *-ignore-placement but do we need this level of expressivity? What are the cartographic use cases for:

multiple collision groups for a label
a single collides-with group
multiple collides-width groups

mb12 commented 6 years ago

@ChrisLoer Can you please clarify if the following is the expected behavior of *-allow-overlap in presence of multiple collision groups?

1.) *-allow-overlap enables overlapping for objects in the layer only within the collision group to which it belongs. Since no collision detection is run across collision groups, this effectively means overlap is allowed across all layers in the style.

For Symbol layers, added explicitly in user code, we generally add -allow-overlap and -ignore-placement as true. So no changes would be required in the user code even if we modify the style to explicitly add a collision group per source.

Can you please also confirm if the following is correct or not regarding *-collides-with?

To enable overlapping with all layers in the style, one would need to add a *-collides-with property that contains all the collision groups including the default group.

If -allow-overlap and -ignore-placement are deprecated would the above be the right way to accomplish what we do today with these properties?

ChrisLoer commented 6 years ago

@mb12 The equivalent to allow-overlap: true (if it were deprecated) would be collides-with: [] (i.e. "collides with no groups"). The equivalent of ignore-placement: true would be collision-group: [] (i.e. "not inserted into any collision groups").

Adding a layer in a new collision group won't affect existing layers (e.g. the ones using the default collision group). In this proposal, it is somewhat cumbersome to add a new collision group and make items in the "default" group collide with items in the new group, because you would have to explicitly add collides-with: ['new-group'] to all of your (previously default) layers.

ChrisLoer commented 6 years ago

@nickidlugash/@lucaswoj will you have a chance to weigh in on the proposal at https://github.com/mapbox/mapbox-gl-js/issues/4117#issuecomment-360548291? Issue #5836 is a regression and it's been reported as a blocker by at least two users so far, so I want to merge #6028 or come up with an alternative solution soon.

lucaswoj commented 6 years ago

I would like to second @ansis' comments. This design may be more expressive & more complex than we need right now. Let's try to come up some use cases that couldn't be met by supporting 0 or 1 collision groups per layer.

nickidlugash commented 6 years ago

Let's try to come up some use cases that couldn't be met by supporting 0 or 1 collision groups per layer.

I'll try to think about this.

A few more points for discussion:

@nickidlugash brought up the concern that having these groups be implicitly formed by layer properties is somewhat "backwards" -- maybe the groups should exist as their own objects in the style-spec?

I do still feel uncomfortable with this method of group assignment, as it seems unwieldy to work with. What would a UI in Studio look like for this? I think we would need to implement something like Studio-only variables to manage the collision groups. /cc @samanpwbb

2. We currently have separate collision properties for text and icon – do we need to? Are there use cases for assigning different values for text and icon in the same style layer? I can't find/recall any previous discussions around this topic. If there are not use cases, if we move forward with this proposal, could we make just 2 new properties that apply to both?: symbol-collision-group, symbol-collides-with?

ChrisLoer commented 6 years ago

could we make just 2 new properties that apply to both?: symbol-collision-group, symbol-collides-with?

That seems fine to me -- I don't know how it's useful to manage those separately, either. And at least in terms of fixing the regression from introducing cross-source placement, we don't need that granularity.

Let's try to come up some use cases that couldn't be met by supporting 0 or 1 collision groups per layer.

Yeah really interested to hear ideas here. As far as I know we don't so far have any pressing need for the collides-with expressiveness/complexity.

What would a UI in Studio look like for this?

Maybe something like the "Labels" GitHub UI?

lucaswoj commented 6 years ago

Perhaps we could forgo the concept of "groups" entirely and simply have each layer list the other layers it collides with, with some keyword for "all symbol layers" available.

"collides": ["all"]
"collides": ["self"]
"collides": ["layer-id-1", "layer-id-2", "layer-id-3"]
"collides": ["self", "layer-id-1", "layer-id-2", "layer-id-3"]

There may be cases where this creates extra bookkeeping or reduces extensibility; however those problems exist with a "groups" approach too.

ChrisLoer commented 6 years ago

I think the problem with that approach is if you're using a core style and then adding a couple layers in another source, and you want to keep collision detection for the layers in that other source independent, you have to go through and modify the collides property of every single symbol layer in the core style with a list of all the other layers except for the ones you're adding. That seems like much more bookkeeping.

ttsirkia commented 6 years ago

At least in my use case, it would be important that I don't have to know anything about the base map which is static and never changes. If I added dynamic elements on top of the base map, I would like to just define that the dynamic elements cannot interfere with the base map without knowing any layer names or such that are provided by the base map.

This is extremely easy in #6028 by just adding a collision group for those layers which contain dynamic elements and are added by me on top of the base map.

ChrisLoer commented 6 years ago

@lucaswoj care to weigh in again? I'd like to get @ttsirkia unblocked on upgrading GL JS and while I think the concerns with the current implementation are legit, I haven't come up with or seen a more satisfying solution yet...

ChrisLoer commented 6 years ago

@lucaswoj and I talked about this a bit yesterday. One thing I didn't understand from his proposal in https://github.com/mapbox/mapbox-gl-js/issues/4117#issuecomment-371907469 is that it probably would include a "default group" for every layer that didn't specify a collides property. So that would address my concern about satisfying the @ttsirkia use-case without having to modify every layer in the core style, but at the cost of introducing the unfortunate "action-at-a-distance" behavior of groups (i.e. if you decide you want to make "layerX" collide with a "layerY" that's not in the "default group", you add a collides property to "layerX", but then that implicitly removes layerX from the "default group" which affects the collision behavior of all the other layers colliding against that group.

For the "@ttsirkia use-case", we think either proposal would be pretty straightforward -- it's just the difference between adding one named group to each layer in the source vs. adding an array of all the other layers in the source.
The collides proposal has the advantage that it has more expressivity built-in, because it's "unidirectional": eg if you want layer1, and layer2 to mutually block each other, you specify a collides property on both of them, but if you only want one to block another you just specify one collides property. One thing this means is that it would technically be possible to replace both *-ignore-placement and *-allow-overlap with collides lists -- I'm concerned that might be bad because having multiple ways to accomplish the same thing in an API is confusing. I don't know a compelling use case for this level of expressivity, but it supports @nickidlugash 's original request of "Allow overlap within symbols in current layer"
Using named collision groups within the core styles could be pretty dangerous, in that it would make it easy for there to be inadvertent naming collisions with user-created groups. The collides proposal avoids the collision problem, but also makes it so that if core styles use the feature they should be really confident that no one will want to do collisions against any layer that's not in the "default group" (because it would be a nightmare for users to have to track a potentially changing set of layer names to collide against).

We're not super satisfied with either approach and are waiting for inspiration to strike on a third option. Maybe "style components" could somehow give us the grouping we're looking for?

@mourner suggested trying to hide almost all of the collision-group functionality behind a single global switch (either in the map or the style) that just emulates the old cross-source placement behavior. The motivation there is: (1) avoid adding surface area to the style spec whenever possible, and (2) solve only the specific problem we know we need to solve.

ttsirkia commented 6 years ago

I'm open to any kind of technical approach. This is clearly something which appears only when you have dynamic data on top of the map so it shouldn't decrease the performance in the normal use cases.

I think one of the problems is that this must work without having any access to (or knowledge of) the background map and its style. So when adding dynamic elements by using setData, it must be the styles for those layers which control the behavior.

One approach without thinking the implementation would be that the user "freezes" the background map by adding a specific layer above the background map and then add above that the new feature layers which know nothing about the features in the layers below. In this way, you don't have to specify any collision groups, names etc. Of course, it doesn't give as much flexibility as other options.

samanpwbb commented 6 years ago

As a Studio team representative, I strongly urge us to keep simplicity and consistency in mind with new spec proposals. Here are a few thoughts:

To bring this back to the original topic of the issue, I think there's a big opportunity to simplify collision detection configuration in the spec. I'd love to see us combine icon and text collision, and move away from both -allow-overlap and -ignore-placement.

What about a single always-show boolean property. If true for a layer, that layer is always displayed. Layers with always-show: false will hide if they collide with the always-show: true. Two always-show: true layers are allowed to overlap each other. always-show: false would be the default. Besides that, control what shows via layer order.

We wouldn't need to deprecate -allow-overlap and -ignore-placement – but this new property would pave the way for simplifying the interface in studio – we could migrate layers over to always-show and hide the old collision properties in our UI.

I see there is a separate need for more nuanced collision detection, where user manages separate groups of layers, each group collides only within itself. I have two thoughts here. Short term, check out https://github.com/mapbox/mapbox-gl-sync-move. It's a great tool that lets you sync map state between two GL maps. Long term, components seem like a great abstraction for supporting this behavior. Rather than expecting each layer to manage it's collision, or using a separate root property for managing collision, we could add component-level collision properties that allow user to define if a component should respect symbol layers in other components or not.

Lastly, the Studio team does not have the capacity to build dedicated UIs for fine-grained collision detection, but we will be investing significant design + engineering efforts into supporting a component pattern.

ttsirkia commented 6 years ago

I think the always-show is a nice idea but have at least one major problem in my use case.

I would assume that the property would be false in the background maps provided by, for example, Mapbox. Now if there is a label for the town names in the backround map, I would like it to collide with other town names, but a town name label and a label for a dynamic feature should be both visible. This cannot be achieved by the suggestion if I understood it correctly.

ttsirkia commented 6 years ago

This is somehow very nice example: https://www.mapbox.com/mapbox-gl-js/example/animate-point-along-route/ What would be the simplest way to prevent the state names disappearing when the airplane is above them?

The current behavior is distracting in this kind of cases when you expect the background to be static and frozen.

andrewharvey commented 4 years ago

This is somehow very nice example: https://www.mapbox.com/mapbox-gl-js/example/animate-point-along-route/ What would be the simplest way to prevent the state names disappearing when the airplane is above them?

The current behavior is distracting in this kind of cases when you expect the background to be static and frozen.

@ttsirkia That example is fixed now.

ttsirkia commented 4 years ago

@ttsirkia That example is fixed now.

Did you change the example or was this problem already resolved earlier when the collision detection mechanism was changed?

andrewharvey commented 4 years ago

Did you change the example or was this problem already resolved earlier when the collision detection mechanism was changed?

@ttsirkia I updated the example #7044

florianPOLARSTEPS commented 4 years ago

I was wondering if an implementation similar to what was suggested here is still in consideration. Reading through some tickets, to me it seems that discussion was stopped in favor of an alternative implementation. Is that correct?

I am working on some features at the moment where I would love to be able to take full control over collisions on a presentation level since I need to combine quite a lot of dynamic data sources, and I do not really want to structure/combine those data sources so that it satisfies the visual design of the map. To me that seems like a wrong approach from an architectural point of view. In my opinion, the styling of the map, which includes what hides/collides when something overlaps, should not be driven by data (i.e. the sources), but by the components in the architecture that are responsible for presentation (i.e. SymbolLayers... etc).

EveWashington commented 1 year ago

In the past three years has there been any progress, or workarounds discovered, on the initial goal #1 of how to enable overlap with other layers but not within the same layer?

mapbox / mapbox-gl-js