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

Add allow-overlap and ignore-placement icon/text options #486

Merged
merged 1 commit into from Jun 24, 2014
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented Jun 24, 2014

Closes #477

@yhahn
Copy link
Member

yhahn commented Jun 24, 2014

@mourner want to make a PR for https://github.com/mapbox/mapbox-gl-style-spec as well?

@mourner
Copy link
Member Author

mourner commented Jun 24, 2014

@yhahn already did mapbox/mapbox-gl-style-spec#64

@yhahn
Copy link
Member

yhahn commented Jun 24, 2014

o

@mourner mourner added this to the v1.0.0 milestone Jun 24, 2014
@ansis
Copy link
Contributor

ansis commented Jun 24, 2014

I'm wondering if we could make this one option instead of two. Is there any case where you'd want to allow overlap with the current/previous layers, but not allow overlap with later layers?

@mourner
Copy link
Member Author

mourner commented Jun 24, 2014

Maybe, — that's a question to style designers. cc @nickidlugash
Also, who did introduce those props in carto? He should know cc @tmcw

@tmcw
Copy link
Contributor

tmcw commented Jun 24, 2014

Also, who did introduce those props in carto?

Carto's support for this is just a pass-through to Mapnik XML, no real magic or complexity there.

@mourner
Copy link
Member Author

mourner commented Jun 24, 2014

OK, maybe @artemp @springmeyer can weigh in here :)

@ansis
Copy link
Contributor

ansis commented Jun 24, 2014

@ajashton do you have any thoughts on whether allow-overlap and ignore-placement could be combined, or if they are useful to have separate?

@ajashton
Copy link
Member

There are use cases where you don't want things within a single layer overlapping, but you want to be able to draw things in subsequent layers on top. One example is big semitransparent labels like you see in Bing or some print maps:

I can also see it being useful for icons-as-texture, like what Saman was experimenting with a while back (tho ignore-placement is not used in this particular style):

@ansis
Copy link
Contributor

ansis commented Jun 24, 2014

awesome, thanks!

ansis added a commit that referenced this pull request Jun 24, 2014
Add allow-overlap and ignore-placement icon/text options
@ansis ansis merged commit 9ff464f into master Jun 24, 2014
@ansis ansis deleted the overlap branch June 24, 2014 22:44
@springmeyer
Copy link
Contributor

tl;dr - I agree with @ansis - seems wise to start with just allow-overlap and hide the complexity of ignore-placement until absolutely needed. Although @AJ or @nickidlugash overrule here.

The longer winded version is: Having them as separate options is important only in the rare case where you want icons to overlap themselves but you do not want text or icons from other layers (rendered after) to overlap. E.g. marker-allow-overlap:true; marker-ignore-placement:false; This was added to Mapnik at the request of the OSM stylesheet designers (mapnik/mapnik#564).

The sad part of this design (having two options) in TileMill has been that users often set allow-overlap:true but do not know about the obscure ignore-placement and hit performance problems rendering millions of markers. This is because the ignore-placement value does not change automatically (if unset) depending on your allow-overlap setting and Mapnik then ends up collecting bbox's for collision even if the style does not need to care about collision with features rendered later. To lessen this problem we basically default to recommending ignore-placement:true in TileMill latest: https://github.com/mapbox/tilemill/commit/d612c073d8dbbd23ff77924bb9fde5990887284e, which just feels duplicative.

Also, I just did a quick grep of tm-custom-styles and see a ton of cases where the intention is for them both to be true, so keeping as one option seems wise:

~/projects/tm2-custom-styles[master]$ grep -r 'marker-allow-overlap: true; marker-ignore-placement: true' ./ | wc -l
     194

@springmeyer
Copy link
Contributor

ha. @AJ's comment just appeared once I posted (had this open for a while stewing).

bensleveritt pushed a commit to bensleveritt/mapbox-gl-js that referenced this pull request Oct 24, 2016
stepankuzmin pushed a commit that referenced this pull request Jun 21, 2023
* Add measure light example

* Add measure light parsing

* Add measure light brightness evaluation

* Fix calculateLightsBrightness

* Propagate brightness changes

* Fix stale brighness values in expression evaluation context

* Fix flow and lint

* Fix invalid leftover

* Fix unit tests

* Add render tests

* Fix invalid ignore path

* Update to correct baselines

* Fix lint and increase render test allowed due to CI aliasing

* Review comment

* Fix small rebase issue

* Review suggestion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

option to allow marker overlap
6 participants