Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase flexibility of collision detection options #4117

Open
lucaswoj opened this issue Feb 1, 2017 · 25 comments
Open

Increase flexibility of collision detection options #4117

lucaswoj opened this issue Feb 1, 2017 · 25 comments
Labels
cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.)

Comments

@lucaswoj
Copy link
Contributor

lucaswoj commented Feb 1, 2017

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 lucaswoj added the breaking change ⚠️ Requires a backwards-incompatible change to the API label Feb 1, 2017
@lucaswoj
Copy link
Contributor Author

lucaswoj commented Feb 1, 2017

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 👍

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

  1. Allow overlap with symbols in other layers
  2. 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.

@1ec5 1ec5 added the cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.) label Feb 3, 2017
@raphaelvigee
Copy link

raphaelvigee commented Nov 28, 2017

Any news on this feature ?

@ChrisLoer
Copy link
Contributor

@nickidlugash I have a proposal in #5836 (comment) 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:

  1. Allow overlap with symbols in other layers
  2. Allow overlap within symbols in current layer

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

@ChrisLoer ChrisLoer changed the title Remove "*-ignore-placement" properties and expand the scope of "*-allow-overlap" properties Increase flexibility of collision detection options Jan 25, 2018
@ChrisLoer ChrisLoer removed the breaking change ⚠️ Requires a backwards-incompatible change to the API label Jan 25, 2018
@ChrisLoer
Copy link
Contributor

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
Copy link
Contributor

ansis commented Jan 25, 2018

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
Copy link

mb12 commented Jan 25, 2018

@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
Copy link
Contributor

@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
Copy link
Contributor

@nickidlugash/@lucaswoj will you have a chance to weigh in on the proposal at #4117 (comment)? 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
Copy link
Contributor Author

lucaswoj commented Mar 8, 2018

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
Copy link

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

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
Copy link
Contributor

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
Copy link
Contributor Author

lucaswoj commented Mar 9, 2018

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor

@lucaswoj and I talked about this a bit yesterday. One thing I didn't understand from his proposal in #4117 (comment) 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
Copy link

ttsirkia commented Apr 17, 2018

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
Copy link
Contributor

samanpwbb commented Apr 17, 2018

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
Copy link

ttsirkia commented Apr 17, 2018

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
Copy link

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
Copy link
Collaborator

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
Copy link

@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
Copy link
Collaborator

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
Copy link

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
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-platform 📺 Requires coordination with Mapbox GL Native (style specification, rendering tests, etc.)
Projects
None yet
Development

No branches or pull requests