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 power=plant outline #2839

Closed
wants to merge 1 commit into from

Conversation

kocio-pl
Copy link
Collaborator

Follow up to #2700.

Umspannwerk Dieringhausen, z19

Before
qd357zog
After
x6xa0sx7

@kocio-pl
Copy link
Collaborator Author

Any other such objects with invisible areas which should be visible?

@HolgerJeromin
Copy link
Contributor

plant was the first and only object i remember which should be inside industrial area.
hotel/museum outline are a special case which should be handled separate.

@pnorman
Copy link
Collaborator

pnorman commented Sep 16, 2017

I'm not sure what this outline accomplishes.

@kocio-pl
Copy link
Collaborator Author

It shows the area without the landuse, indicating that it should be added.

@matthijsmelissen
Copy link
Collaborator

What does it look like on an industrial landuse background? Also in that case it might be nice to have the outline of the actual plant.

@kocio-pl
Copy link
Collaborator Author

What does it look like on an industrial landuse background?

It's the same code as in #2700, so look at #2700 (comment).

Also in that case it might be nice to have the outline of the actual plant.

What do you mean?

@matthijsmelissen
Copy link
Collaborator

What do you mean?

If we have power=plant within landuse=industrial, it would be good if the power=plant outline is still visible, but I believe your PR accomplishes that.

I would also be fine (and maybe even prefer) with giving power=plant a purple background like landuse=industrial.

@kocio-pl
Copy link
Collaborator Author

What would be the reason for rendering (or not rendering) purple background? I don't have an opinion now.

@pnorman
Copy link
Collaborator

pnorman commented Sep 18, 2017

It shows the area without the landuse, indicating that it should be added.

If the purpose is to reveal this as an error, that's something for a debugging layer, not a general purpose layer. If we want to render power plants like industrial areas we could do that.

@kocio-pl
Copy link
Collaborator Author

Well, for the man_made=works it was to show both - that's incomplete tagging and showing that it's not a node, but a bigger object.

It all depends on how we see the problem of area objects and landuse - if I understand correctly (I'm not sure if I really do):

  • @imagico desn't like such object to resemble a node and requests something more
  • @dieterdreist doesn't like filling
  • you don't like just the outline

So:

  1. Which solution would be something everybody (or just majority) could accept?
  2. Is man_made=works and power=plant the same case or a different one?

@mboeringa
Copy link

mboeringa commented Sep 21, 2017

As to fill for power=plant, be aware that there are also dispersed power plants, tagged as one entity. E.g. groups of windmills at sea, or solar panels, may be tagged with one closed way and power=plant.

See the wiki also:
http://wiki.openstreetmap.org/wiki/Tag:power%3Dplant

A fill is not appropriate in this case, only outline rendering makes sense.

@daganzdaanda
Copy link

@imagico desn't like such object to resemble a node and requests something more

Actually, he wrote he doesn't like to have the label styled based on way_area if there is nothing that shows the extent of the area. It's a subtle difference, but it does make sense.

@kocio-pl
Copy link
Collaborator Author

I will put this information also here: If nobody help me fix this code to the acceptable form, I plan to close it without merging in a few days.

@kocio-pl kocio-pl closed this Nov 18, 2017
@kocio-pl kocio-pl deleted the power-plant-outline branch November 18, 2017 06:22
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