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

Rendering man_made=works #2700

Merged
merged 2 commits into from
Sep 15, 2017
Merged

Conversation

kocio-pl
Copy link
Collaborator

Resolves #887.

Warsaw, z18
Before
s5pama4i
After
vvfegrv5

@kocio-pl kocio-pl mentioned this pull request Jul 23, 2017
@imagico
Copy link
Collaborator

imagico commented Jul 23, 2017

As previously said multiple times i am against point label rendering of area features based on way_area if those are not at the same time rendered as areas. This would create a bad mapping incentive.

@matthijsmelissen
Copy link
Collaborator

matthijsmelissen commented Jul 23, 2017

As previously said multiple times i am against point label rendering of area features based on way_area if those are not at the same time rendered as areas.

👍, this should be changed. However, this is an existing problem which is IMO outside of the scope of this PR.

@imagico
Copy link
Collaborator

imagico commented Jul 23, 2017

However, this is an existing problem which is IMO outside of the scope of this PR.

It is not, as far as i can see man_made=works is currently not rendered in any way, this PR adds rendering on point features (which is fine) and on area features with rendering being influenced by way_area (which i do not support).

@matthijsmelissen
Copy link
Collaborator

matthijsmelissen commented Jul 23, 2017

As that is wider than only man_made=works, could you create a new issue for that?

@imagico
Copy link
Collaborator

imagico commented Jul 23, 2017

I am not aware of any case currently where label styling is based on way_area for features that are not rendered as areas - unless i missed some changes that introduced this.

You could formally count place=island since the tag itself is not rendered but it is universally rendered as negative space through the coastline and water areas.

@matthijsmelissen
Copy link
Collaborator

Sorry, I totally misunderstood you. I think shop=mall is another current example.

@imagico
Copy link
Collaborator

imagico commented Jul 23, 2017

Good point, did not notice that one.

Could be fixed by rendering shop=mall (when not also building=yes) like landuse=retail in the landcover layer - see #2702. Same could be done for this one (i.e. rendering it like landuse=industrial).

@matthijsmelissen
Copy link
Collaborator

👍 for both suggestions.

@HolgerJeromin
Copy link
Contributor

HolgerJeromin commented Jul 23, 2017

works should be inside industrial area. So rendering only the name and not the area fill is on purpose. same as power plant.
for example one big industrial with many works inside. but rendering the border (but no fill) could be tested.

@kocio-pl
Copy link
Collaborator Author

@imagico Would rendering the no-building works area the same as industrial be OK for you?

@polarbearing
Copy link
Contributor

rendering the border (but no fill)

That would mean a landuse=industrial without a man_made tag has a different border than with, which is inconsistent. It would also make it more difficult to recognise a mapped barrier=fence/wall.

I find rendering the landuse with a fill sufficient. If it is a large plant with a name, it already has the name in way size from the landuse. If there are several small man_made=works in one larger landuse, rendering the small names is sufficient, as @HolgerJeromin pointed out.

@dieterdreist
Copy link

dieterdreist commented Jul 24, 2017 via email

@HolgerJeromin
Copy link
Contributor

That would mean a landuse=industrial without a man_made tag has a different border than with, which is inconsistent.

This is not correct.
Two industrials which are sharing the same border are visible. This would not be the case if there was no border color. See the north border of:
https://www.openstreetmap.org/way/91224778

@pnorman
Copy link
Collaborator

pnorman commented Sep 11, 2017

I think the concerns raised have been resolved. @imagico, is this so?

@HolgerJeromin
Copy link
Contributor

After read the code this i think features with only man_made=works without landuse=industrial would render only the name. No border and no fill. Am I right?

The dimension of this would not be visible, or would it?

@imagico
Copy link
Collaborator

imagico commented Sep 11, 2017

I think the concerns raised have been resolved. @imagico, is this so?

No changes were made since the PR was originally started so the answer is no.

@kocio-pl
Copy link
Collaborator Author

What is desired solution then? Do we have an agreement here?

@kocio-pl
Copy link
Collaborator Author

After read the code this i think features with only man_made=works without landuse=industrial would render only the name. No border and no fill. Am I right?

Yes. Because there are no changes in landcover.mss, no such area would be colored nor outlined.

The dimension of this would not be visible, or would it?

It wouldn't.

@HolgerJeromin
Copy link
Contributor

Oh, i thought that power=plant got a text and outline (example). I was wrong.

So with the current PR state, works will be rendered exactly like plant. Kind of ok for me.

@imagico
Copy link
Collaborator

imagico commented Sep 12, 2017

Interesting observation about power=plant - did not realize that. That is the same situation as shop=mall.

My opinion is as before:

As previously said multiple times i am against point label rendering of area features based on way_area if those are not at the same time rendered as areas. This would create a bad mapping incentive.

Could be fixed by rendering shop=mall (when not also building=yes) like landuse=retail in the landcover layer - see #2702. Same could be done for this one (i.e. rendering it like landuse=industrial).

Other possibilities would be rendering with an icon or rendering with a point label independent of way_area for both point and area features.

@HolgerJeromin
Copy link
Contributor

Rendering (on plant and works) an outline (same color like industrial outline) would show the dimension of a feature but still give a feedback that an landuse=industrial is missing.

@daganzdaanda
Copy link

Rendering (on plant and works) an outline (same color like industrial outline) would show the dimension of a feature but still give a feedback that an landuse=industrial is missing.

That would be a good solution, also for offshore wind parks like https://www.openstreetmap.org/way/186609785 where landuse=industrial is just wrong.

@kocio-pl
Copy link
Collaborator Author

Thanks for a good example - it's hard to find them, because with so many rules Overpass Turbo is hitting the RAM limits. There's only a name and the outline, no filling, but if you want to make sure, I need another location where there's no industrial area. In the meantime I have removed industrial area filling+outline rendering rule just to show you how would it look like:

Verl, z18
Before
87zvg9zp
After
jrplgd_j
After (without industrial filling)
vrq ne50

@kocio-pl kocio-pl changed the title Rendering name for man_made=works Rendering man_made=works Sep 13, 2017
@HolgerJeromin
Copy link
Contributor

HolgerJeromin commented Sep 13, 2017

The plant https://www.openstreetmap.org/way/53243836 has no underlying industrial (but should :-)

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Sep 13, 2017

But this is a different tag than man_made=works, so it won't fly with this code. Of course we can make another PR for power plants and other possible objects, but for now I'd like just to make sure if name+outline is the way to go for such cases.

@HolgerJeromin
Copy link
Contributor

Here you go: https://www.openstreetmap.org/way/29388213 :-)

@kocio-pl
Copy link
Collaborator Author

So here you are 😄 :

Erkrath, z19
Before
giiifyvg
After
pqebzzty

@kocio-pl kocio-pl merged commit 7dd108b into gravitystorm:master Sep 15, 2017
@kocio-pl kocio-pl deleted the manmade-works-name branch September 15, 2017 09:46
@HolgerJeromin
Copy link
Contributor

One question: does this render nodes with this tag?

@kocio-pl
Copy link
Collaborator Author

Yes, the original example is a node:

http://www.openstreetmap.org/node/3428336388

@HolgerJeromin
Copy link
Contributor

Btw, this is missing in the changelog

@kocio-pl
Copy link
Collaborator Author

Also some other things (like pattern on natural=sand), but Paul never did very detailed changelogs IIRC.

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.

8 participants