-
Notifications
You must be signed in to change notification settings - Fork 826
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
Added combined placement option #2687
base: master
Are you sure you want to change the base?
Conversation
Thanks for the contribution! I can see the value in this. I'll try to find time to review in detail early next week. |
* This file is part of Mapnik (c++ mapping toolkit) | ||
* | ||
* Copyright (C) 2015 Walid Ibrahim | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright should be Copyright (C) 2015 Artem Pavlenko
as per https://github.com/mapnik/mapnik/blob/master/docs/contributing.markdown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no worries will change it, thanks for sharing the links
{ | ||
apply_simple_placement(); | ||
} | ||
else if(list_placement_info_->next()) //try the list placement options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should discard the first list placement before first call of this list_placement_info_->next()
. The first list placement is default
placement so you run through default placement twice.
I see |
@talaj it is a functionality I have been working on for awhile to fix marker placement across tile edges as they appear clipped. So far I got it working fine with MarkersSymbolizer, ShieldSymbolizers with point placement, and struggling to make it work with GroupSymbolizer. Here is the original issue I reported before #2577 |
@wibrahim - sorry I've not had a chance to review this properly. I know I won't before upcoming 3.0.0 release, but I've assigned to |
List and simple placements were already supported by Shield and Text Symbolizers, but we can only use one of them at a time.
In the proposed changes, I combined both placements options into a single "combined" placements, were the engine tries the simple placement algorithm as specified first and if no placement is found, then it check its list placement options and re-applies the simple placement again.
A possible test case would be
And the output is: