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 area rendering for amenity=prison #772

Merged
merged 2 commits into from
Jan 14, 2015

Conversation

polarbearing
Copy link
Contributor

Isolating undisputed style from combined proposal #771
This provides rendering for areas of amenity=prison, and closes

https://trac.openstreetmap.org/ticket/2512

Prison was apparently implemented in xml and did not make it to carto.

The design adopts the hatching style we have for military areas/barracks, just in a neutral grey and vertical, associating bars, as in the original implementation. Hatching being used, as @mkoniecz pointed out, for rather inaccessible areas. Or, in this case, difficult to leave ;-)

Icons is powers of two and allows global alignment.

pr

@matthijsmelissen
Copy link
Collaborator

I tested this and checked the code, which looks good.

The only thing I'm concerned about is that it adds yet another type of symbology to the map that might not be immediately clear to the user, for something that does not occur very often. A symbol combined with a neutral colour fill with a dashed outline might be clearer. Anyway that's for @gravitystorm to decide.

@polarbearing
Copy link
Contributor Author

Relying too much on the outline might be a problem since these facilities would typically come with a wall or fence tagged on their border. Also, from Z17 the symbol amenity_prison.p.20.png is still being rendered in the middle, with a name that should be unambigious, see example here:
pr_42982

@vincentdephily
Copy link

FWIW I like the dashed rendering, which we already use for millitary. Since we render so many things, I don't think we can afford solid fill for everything.

@matthijsmelissen matthijsmelissen changed the title Prison area Add rendering for amenity=prison Sep 24, 2014
@matthijsmelissen matthijsmelissen changed the title Add rendering for amenity=prison Add area rendering for amenity=prison Sep 24, 2014
@matthijsmelissen matthijsmelissen modified the milestones: 3.x - Needs upgrade to mapnik or openstreetmap-carto.style, Bugs and improvements Sep 26, 2014
@matkoniecz
Copy link
Contributor

Can you rebase this pull request to a single commit?

@polarbearing
Copy link
Contributor Author

rebased

@matkoniecz
Copy link
Contributor

I like results, I tested it on prisons in my city and it works well. One more request - can you rebase with amending commit description? Now it is rather confusing as police landuse is not affected by this PR.

After this change I would recommend merging.

@matkoniecz
Copy link
Contributor

@math1985

The only thing I'm concerned about is that it adds yet another type of symbology to the map that might not be immediately clear to the user, for something that does not occur very often. A symbol combined with a neutral colour fill with a dashed outline might be clearer.

Symbol is currently also appearing. I am not considering it as a "yet another type of symbology", rather as a variant of symbology used for landuse=military. I also encountered this kind of symbol used as "area with highly restricted access" on many other maps.

@polarbearing
Copy link
Contributor Author

amended message

@matkoniecz
Copy link
Contributor

@gravitystorm Are you opposed to hatching also in this case? This one is not so eye catching, and in my opinion style fits feature.

#1197 (comment)

@matthijsmelissen matthijsmelissen merged commit 9454602 into gravitystorm:master Jan 14, 2015
@nebulon42
Copy link
Contributor

The pattern here introduced a grey semi-transparent background. Other backgrounds of patterns were removed recently (see #1114). Could the transparency interact with other landuse colours? I think this should be changed to use polygon-fill.

edit: Ok, I saw that the transparency is required.

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.

6 participants