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

refactor and fix amenity-points #1093

Merged
merged 2 commits into from
Oct 29, 2014
Merged

refactor and fix amenity-points #1093

merged 2 commits into from
Oct 29, 2014

Conversation

matkoniecz
Copy link
Contributor

next part of #710

  • ensures that all objects from amenity-points that can display name "a" as node on z20 can also display name "ÉÉÉÉÉÉ".
  • wrap-width is set for all labels in amenity-points

This is proposed as single pull request to prevent merge conflicts, other alternative was to make single PR after every merge cycle what would slow down fixing #710 what blocks changes like #1040 and #1041.

Resolves #870, resolves #959.

fixes #870, fixes #959
script used to find bad text-dy is available at https://gist.github.com/mkoniecz/9451a41ec0ae38b23b54
the proper way to test it is using mapnik-legendary, but it depends on currently broken Ruby-Mapnik
@matkoniecz
Copy link
Contributor Author

there is also small text-dy fix in roads.mss

Maybe I will have time to add more #710 related commits into this PR (text-wrap-width would be the next issue).

@@ -1153,6 +1160,7 @@
[way_pixels > 12000] { text-size: @landcover-font-size-big; }
[way_pixels > 48000] { text-size: @landcover-font-size-bigger; }
text-fill: darken(@campsite, 50%);
text-dy: 15;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this also sufficient for the larger font sizes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably no, my script was checking only for rendering of nodes. I am planning on making next one that will be able to do the same with areas.

@matthijsmelissen
Copy link
Collaborator

Out of curiosity, which method did you use to calculate the text-dy?

@matkoniecz
Copy link
Contributor Author

Out of curiosity, which method did you use to calculate the text-dy?

I used hackish script (linked in the commit description) to check whatever name appears. On object where name "a" appeared and name "ÉÉÉÉÉÉ" was not appearing I manually increased text-dy by one and repeated it till also "ÉÉÉÉÉÉ" appeared.

Main improvement is that I automated reporting of the problem that was affecting most POIs.

@matkoniecz
Copy link
Contributor Author

I added commit setting standard wrap-width - in some cases it was previously not set, or was really small (for shops it caused wrapping for names like "Pasaż 13"[1], or was really big).

[1] http://www.openstreetmap.org/?mlat=50.06048&mlon=19.93801#map=19/50.06048/19.93801

In dense areas really long lines were easier to display than better looking blocks of text so some labels disappeared.

http://www.openstreetmap.org/#map=18/50.06218/19.93544

selection_006

http://www.openstreetmap.org/#map=18/50.05973/19.94155
selection_007

@matkoniecz
Copy link
Contributor Author

NOTE: removed from this PR as for for some reason some people are convinced that bays should be tagged as nodes.

Now natural=bay label also depends on way_area like other waterbody labels what fixes problem with labels for big and really small bays appearing at the same time. This change should encourage mappers to map bays, rather than placing natural=bay nodes where they want label to be rendered - as natural=bay nodes will appear at z17 rather than as it is currently on z14. Also, code duplication is now reduced.

http://www.openstreetmap.org/#map=14/60.1678/5.0645
http://overpass-turbo.eu/s/5CQ

selection_009

@matkoniecz
Copy link
Contributor Author

I am not planning on making additional changes in this PR, I will make next PR related to #710 after this PR is merged/rejected.

@matthijsmelissen
Copy link
Collaborator

Also solves #870 and #959 (updated PR description).

@matthijsmelissen
Copy link
Collaborator

I have tested this PR, and I think it can be merged.

@matkoniecz
Copy link
Contributor Author

Also solves #870 and #959 (updated PR description).

I thought that placing it in commit message will be enough for Github to close tickets.

@matthijsmelissen
Copy link
Collaborator

It is, didn't notice it was in the commit message already.

@imagico
Copy link
Collaborator

imagico commented Oct 27, 2014

Note starting to render node based natural=bay not earlier than z=17 essentially means ceasing to render them since bays are typically in the km to hundred km size range. This would mean removing support for more than 30000 features currently rendered. I don't want to repeat arguments from the discussion on the tagging ML here but this is clearly not an improvement for the map and has nothing to do with the fix this pull request is about.

The distance of the node to the coastline would be a perfectly good indicator for the size of the bay - and much less prone to manipulation than the polygon area since you cannot manipulate it without also moving the label. Of course the coastline is currently not available in the database but this is not a good reason to pressure mappers to generate polygons for >30000 nodes.

@matkoniecz
Copy link
Contributor Author

As this PR was supposed to be about obvious and completely uncontroversial changes I will remove the natural=bay commit.

@imagico
Copy link
Collaborator

imagico commented Oct 27, 2014

Thanks, much appreciated. In principle i see nothing wrong with showing the large bays mapped as polygons at z<14. This would be an improvement for the map although i personally would dislike the side effect of encouraging mappers to map large bays as areas.

gravitystorm added a commit that referenced this pull request Oct 29, 2014
refactor and fix amenity-points
@gravitystorm gravitystorm merged commit 323b392 into gravitystorm:master Oct 29, 2014
@matkoniecz matkoniecz deleted the ce branch October 29, 2014 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix text-dy for amenity=restaurant Fix text-dy for amenity=embassy
4 participants