Add icon-overlap, text-overlap layout properties #347
Conversation
|
Bundle size report: Size Change: +252 B
ℹ️ View Details
|
|
A few things to note here:
I haven't looked deep into the code, but I saw some good changes related to typings, kudos! Overall, I think this is a good step forward! I guess it's worth talking about it in the committee. |
|
Existing styles should be unaffected. -allow-overlap behaves the same as before, and the new -overlap properties are optional add-ons. I'm not thrilled with the property values either; happy to entertain other suggestions. |
|
Can you please resolve conflicts. |
Add new symbol layout properties to allow finer control over symbol overlap behavior. New 'cooperative' mode allows opt-in behavior for overlap. A 'cooperative' symbol may only overlap another symbol that also uses 'cooperative' or 'full' overlap modes.
Default should be undefined, not literal string 'unset'.
drwestco
force-pushed
the
symbol-overlap
branch
from
83aa518
to
abfea4d
Compare
|
I have trouble thinking of a use-case for this, and I wonder if there might already be other existing mechanisms to support this. Wasn't https://docs.mapbox.com/mapbox-gl-js/style-spec/layers/#layout-symbol-symbol-sort-key meant to affect behaviour like this? That said, there appears to be some criticism there, too: mapbox/mapbox-gl-js#9368, and it requires modifications of the data to add the sort-key to each feature. |
|
Had an alternate idea for |
|
For me this seems too complicated, but other people who might have more complicated cases may need this flexibility, I don't know. |
|
Having both So if the idea is to add a new group name/ID property later, I'd still rename Also happy to just leave it as-is... |
|
I think the code is good, I'm just not sure how to signal the library users that something is now deprecated, and they should start migrating to the new "syntax", that's the only concern I got related to this PR. |
|
The style specification has seen a series of feature deprecations and breaking changes. A lot happened around version 8.0.0, see https://github.com/maplibre/maplibre-gl-js/blob/main/src/style-spec/CHANGELOG.md#800. From reading the style spec changelog, it seems the way to go about feature deprecation is to add a note in the changelog. Once the feature gets removed, the major version of the style spec has to increment, the changelog has to include a note about the breaking change, and the |
|
@drwestco TODOs are:
|
|
Noticed this wasn't included in the 2.0.0 release. What's the next step for getting it merged? |
|
I would say we should merge this and bump to v2.1.0, what do you think @HarelM? |
|
Sure :-) |
|
@drwestco, can you add a render test for this new functionality? https://github.com/maplibre/maplibre-gl-js/tree/main/test/integration#writing-new-tests |
src/style-spec/reference/v8.json
Outdated
| ], | ||
| "sdk-support": { | ||
| "basic functionality": { | ||
| "js": "2.0.0" |
There was a problem hiding this comment.
@HarelM can we merge this pull request for v2.0.6? Or should it be v2.1.0?
There was a problem hiding this comment.
In any case, when we merge this pull request, we should probably release a new version of GL JS and bump the version number in package.json
There was a problem hiding this comment.
OK, let's use 2.1.0. @drwestco please change the version here to 2.1.0 here. Also increment the version in package.json to 2.1.0:
Line 4 in 00e094c
And add a note in the changelog under a new subsection called 2.1.0 (https://github.com/maplibre/maplibre-gl-js/blob/main/CHANGELOG.md#main)
|
Thanks @drwestco for the render tests. |
|
You need to resolve conflicts... |
|
Regarding deprecating features: Mapbox took the stance to effectively never deprecate features or introduce breaking changes to the style specification. Whether or not we want to do the same is up to us. See mapbox/mapbox-gl-js#6584 |
This change allows for finer control of symbol overlap behavior than offered by
icon-allow-overlap,text-allow-overlap. It meets my needs, but I'm offering this draft PR for further refinement - do the changes make sense in general, are there clearer ways to present these options, etc.I still need to add tests for this new behavior. But wanted to get some eyes on this before spending time doing so.
This new feature is to address #249
Current overlap behavior
When a symbol is placed, and collides with a previously-placed symbol, one of two things will happen:
-allow-overlap: false, the placement is rejected.-allow-overlap: true, the placement is accepted, and the new symbol overlaps the old. This happens regardless of whether the previously-placed symbol has-allow-overlapor not.So collision is a one-way process. A symbol in a foreground layer gets placed first. A symbol in a background layer gets placed second. The foreground symbol has no say in whether the background symbol overlaps (underlaps) or not.
New overlap behavior
icon-overlap/text-overlap has three states:
never,always, andcooperative. When a symbol is placed, and collides with a previously-placed symbol, there are now three options:-overlap: 'never', the placement is rejected (same as-allow-overlap: false).-overlap: 'always', the placement is accepted (same as-allow-overlap: true).-overlap: 'cooperative', the overlap mode of the previously-placed symbol is checked. The placement is accepted if the previously-placed symbol is also using-overlap: 'cooperative'or-overlap: 'always'. If the previously-placed symbol is using-overlap: 'never', the new symbol's placement is rejected.The new behavior is opt-in. If
-overlapis set, it's used; otherwise the code falls back to the existing-allow-overlapproperties.