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 rendering for man-made cranes #3501

Merged
merged 31 commits into from
Dec 14, 2018
Merged

Conversation

jeisenbe
Copy link
Collaborator

@jeisenbe jeisenbe commented Nov 8, 2018

Fixes #3478

Changes proposed in this pull request:

  • Add svg crane icon, rendered in man-made-color (gray) thanks to @Tomasz-W
  • Add man_made=crane to project.mml file
  • Render man_made=crane with initial zoom level based on height, following same zoom levels as man_made=tower
  • Render name label at z17

EDITED: initial proposal was to render at z14 and z15 based on height >100 and >50, now changed to only z16 for height >50 and z17 otherwise.

Port of Singapore
https://www.openstreetmap.org/#map=17/1.2655/103.7928
z17
z17singapore
Future rendering at z14 (assuming height tags of 51 to 99 meters are added to these cranes; currently they would only render at z17 as above)
z14singapore-future

Milan train station
https://www.openstreetmap.org/#map=15/45.4851594/9.1832991
z14
z14milano
z15
z15milano
z16
z16milano

Port of Venice
https://www.openstreetmap.org/#map=15/45.4729014/12.2435279
z13
z13venice
z15
z15venice
z17
z17venice

River port cranes with name, Blauer Kran, height 24m
https://www.openstreetmap.org/#map=16/50.1129065/8.7519599
z16 Blauer Kran
z16blauer
z17 Blauer Kran
z17blauer
z17 Evo - height untagged; mapped as area (closed way)
z17evo

Big Willi river boat crane - height 12m
z17willi

jeisenbe and others added 21 commits October 17, 2018 22:06
update with changes from master in past month
Follows pattern of amenity=bank
Followed pattern of amenity=bank
Added internet_cafe to amenitypoints and project.mss,  and added icon to symbol/amenity


Merging local copy with changes made previously online only. Added username and email
…p-carto

Updating my fork with the recent changes in the original master
Merging new changes from gravitystorm/openstreetmap-carto.
merge upstream changes into fork
Cranes will render at the same zoome levels as towers, based on height, or at z17 by default, with name lable at z17
@Tomasz-W
Copy link

Tomasz-W commented Nov 8, 2018

@jeisenbe Please test icons for all cranes types: #3478 (comment)

@Tomasz-W Tomasz-W mentioned this pull request Nov 8, 2018
26 tasks
@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Nov 9, 2018 via email

Fixed merge conflict by adding optical and radio telescope code and fixing whitespace
@jeisenbe
Copy link
Collaborator Author

I added marker-transform: 'translate(0,-6)'; so the base of the crane will be near the node or center of the area, as suggested in #3176 "Tower Baseline Alignment"

Reduce text-dy from 10 to 6 for crane due to using translate (0,-6), which moves the icon up 6 pixels
@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Nov 11, 2018

Renderings with marker-transform translate (0,-6) and reduced text-dy (can't forget that!):

Big Willi previous:

Big Willi new (up 6 pixels) z17:
willi-up6-z17

Singapore previous:

Singapore up 6 pixels z14:
z14-singapore-up6

I now think it might be better to limit the zoom level to z14 for height >100m, instead of 13m as with towers. Chimneys and masts do not render above z16 and z15 respectively; only towers render at z13.

I find the tall towers a bit distracting on a city map at z13, especially in areas near the equator like Honolulu and Singapore, but at least there are not dozens of them in one place like cranes at a port.

Towers at z13 are probably fine in Stockholm or St. Petersburg near 60 degrees N, where z13 is the same as z14 at the equator.

Lebanon (tested with zoom z13 as initial level for all cranes as an example)
z13
test - rather too prominent
lebanon-z13
z14* test - probably ok
lebanon-z14

So I'm changing the code to ```

[zoom >= 14][height >= 100],
[zoom >= 15][height >= 50],
[zoom >= 16][height >= 30],
[zoom >= 17]
Agreed?

@dieterdreist
Copy link

dieterdreist commented Dec 2, 2018 via email

@dieterdreist
Copy link

dieterdreist commented Dec 2, 2018 via email

@Adamant36
Copy link
Contributor

Adamant36 commented Dec 2, 2018

I wasn't taking issue with height in general. Yes it has importance for some objects, like crosses or memorials, only in this case. Where the difference between a 100 foot versus a 60 foot crane is minimal. In the case of how it should be rendered. Either cranes have importance in a landscape or they don't. The 40 foot difference probably doesn't matter.

They aren't a universally agreed on item of landscape or cultural importance either (unlike memorials). So there's to much variance in where people would care about them versus not for me to be comfortable with saying they should be rendered as such. I don't think map rendering should be based on the few outlier examples of places in Europe that usually get used to justify things either.

@dieterdreist
Copy link

dieterdreist commented Dec 2, 2018 via email

@Adamant36
Copy link
Contributor

Adamant36 commented Dec 2, 2018

Totally. There should be a different tag for them depending. Height alone isn't a good indicator of that IMHO.

@polarbearing
Copy link
Contributor

polarbearing commented Dec 2, 2018 via email

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Dec 2, 2018 via email

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Dec 2, 2018

Not many cranes are tourist sites, but a few of the big ones have become icons:
https://en.wikipedia.org/wiki/Samson_and_Goliath_(cranes)

Belfast mentions these cranes on their tourist info website:
https://visitbelfast.com/see-do/partners/samson-goliath-cranes

And this crane, which I showed above, has been tagged as tourism=attraction

@Adamant36
Copy link
Contributor

So render all cranes at the same starting zoom level. Except add an exception that ones labeled as tourism=attraction get rendered a level higher. That seems like a more sane less arbitrary thing then height for me and it will also get people to tag the few significant ones as such.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Dec 2, 2018

Tourism=attraction is less arbitrary than height?
Certainly not less subjective.

@Adamant36
Copy link
Contributor

Both are subjective. Tourism=attraction is at least less arbitrary then height to determine what counts as a tourist attraction since its based on an actual tagging scheme for it. Unless of it being based on another tag thats a level of abstraction away.

Serious question, would you recomend rendering paths based on width instead of path type, or roads based on speed limit instead of what road clissification tag they have?

In other words, we know a road is residential road because its tagged as such. Maybe some are misstaged or the meaning/classifiction of a "residential road" isnt quit clear in places, but its at least based on an estiblashed tagging criteria for that thing. So its grounded in OSM precedence.

Height in OSM is simply meant to construe height. So although we could bicker back and forth endlessly about what height infers as far as importance, because its based on the particular objectt and different everywhere, we can at least all agree that the tourism tag is how OSM intends "tourist attractions" to be tagged. Be it a crane or any other object.

Anyway, tourism tag or not, cranes still shouldnt be tagged any earlier then z17. Thats the clear consenus. We can wait until more people come along and see if they disagree. Im fine with that. Maybe Tomasz-W would be willing to come up with a smaller icon. I dont think it would help, but at least its something. Or I could always do a PR with it rendered at z17 so it gets rendered at least and we could debate this in another issue specific to if your height thing has merit or not.

As ive said before though, theres no reason you cant just modify the code how we requested and show some examples of where it would benefit from height an other issue later if need be. That seems like the sane, fair thing to do instead of holding it up in endless debate. Especially since it seems like we just arent going to agree.

@dieterdreist
Copy link

dieterdreist commented Dec 2, 2018 via email

@dieterdreist
Copy link

dieterdreist commented Dec 2, 2018 via email

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Dec 4, 2018

Here are more test renderings, from Jakarta. This huge city is quite close to the equator, so z16 at this level is the same as z17 at 60 degrees north, and is about half-way between z16 and z17 for cities at 40 to 45 degrees. The cranes that I found are all located in the port terminal area.

z17 current master
jakarta-port-z17-master

z17 cranes - after PR
jakarta-port-z17-cranes

z18 cranes - after
jakarta-z18-cranes

z16 (no change with this PR based on current data)
jakarta-port-z16

@kocio-pl
Copy link
Collaborator

Since this discussion is inconclusive, I will merge it as it is and let's examine it on the real map for some time. I think rendering cranes is a good thing in general and such secondary problems should not eclipse that.

Thanks for both creating (icon + code) and discussing this problem!

@kocio-pl kocio-pl merged commit 6afcc4d into gravitystorm:master Dec 14, 2018
@Adamant36
Copy link
Contributor

Kocio-pl, although I agree that we shouldn't let minor details get in the way of rendering something, doesn't merging a PR like this risk making people think they can get their way by just stalling on implementing, or just ignoring, a request for something to be changed? That's essentially what happened here.

There's no reason Jeisenbe couldn't have just implement rendering at z17/z18 and then it if z16 was needed could have been decided later like multiple people requested instead of ignoring it and forcing it to now be the other way around. By merging it anyway, without him listening to feedback, your just incourging him to continue ignoring other peoples wishes. Which he's done on several issues already as it is. Not just this one. Personally, I don't think its a good way to go.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 14, 2018

I see. My intention is not to help ignore anything. I stated before that both ways are acceptable for me and cranes are not that popular to create immediate problem, so I guess it's safe to change it if we see it does not work the way he thinks it does.

From my experience it's more important to allow a bit of fuzziness in the project, including imperfect design, than allow discussing every bit to death to avoid any problem, because first one might be fixed in the future (or accepted eventually), while the second approach can simply paralyze team. I worry to not loose the nice balance between designing/discussing and coding we have currently.

@Adamant36
Copy link
Contributor

Ok. I can understand that. I know its not an easy thing to balance everything perfectly. Especially in a project like this. Thanks for the explanation.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Dec 14, 2018 via email

@kocio-pl
Copy link
Collaborator

Thanks, nice to hear that. This is also one of my reasons to not get deeper into this problem - I have some big changes proposals pending at the table, which will affect the style much more than all cranes combined...

@Adamant36
Copy link
Contributor

@jeisenbe So if me or someone else starts a new issue about it (or even requests a change to another issue), you'll blow it off because they aren't "maintainers"? Seems like a good way to get people to not to work with you anymore and end up with a bad map. Maintainers might merge the PRs, but they only constitute a small part of the feedback on the and make even less design decisions then the contributors who arent maintainers do. We are relient on feedback from others. Including myself. Personally, I dont priortize feedback based on the "status" the person giving it. Otherwise, its a good way to alienate myself (probably not anymore then writting these types of messages though..But whatever).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new features Requests to render new features POI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add rendering for man_made=crane
6 participants