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

Adding rendering for amenity=ferry_terminal #2385

Merged
merged 1 commit into from
Aug 20, 2017

Conversation

kocio-pl
Copy link
Collaborator

@kocio-pl kocio-pl commented Sep 29, 2016

Related to #2094.
Resolves #869.

Looks like airport area rendering is rather for large area and ferry terminal is more like a railway station, so I took it. Labels are however the same like in the airport, but rendered later.

I also made some cleaning in color variables.

Examples:

Warsaw, z15 (nodes only, compared to some other labels)
p7dbtip8

Victoria (nodes and typical area)
z14 (not rendered)
bjher_1n
z15
pkcactt9
z16
x5e7i lb
z17
g0l1qrzl
z18
cc36kprk
z19
faamqado

@kocio-pl
Copy link
Collaborator Author

Another possibility - labels the same as the bus stop - doesn't work for me:
cxyhw1qv

BTW: are there really so large railway stations which should be rendered from z10? If not, maybe we can adjust SQL to optimize query for some higher zoom levels?

@pnorman
Copy link
Collaborator

pnorman commented Oct 3, 2016

Can review when I get home.

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Oct 3, 2016

As the station color seems to be designed for buildings, it can be made lighter (here or in separate PR). We could also go with airport area color, but it's too pale for this usage IMO:

z17
gj9efxea

z18
q1snoghk

z19
qs l225q

@kocio-pl kocio-pl changed the title Adding rendering for amenity=ferry_terminal Adding rendering for amenity=ferry_terminal [WIP] Oct 4, 2016
@matthijsmelissen
Copy link
Collaborator

@kocio-pl Could you rebase this? I have the impression something went wrong, looking at the diff (or I'm misunderstanding the diff algorithm again).

@kocio-pl
Copy link
Collaborator Author

After some time away from this PR I've changed my mind and like to use the lighter version for a background, because current station color seems to be designed with station building in mind, not for the area (merging #2443 will have some implications there).

I don't need to find it from a distance (ferry lines are good hint), it's enough to see its shape to understand how the space is used. Also using the same color for airport, ferry and station area (and possibly other transportation areas) would be more consistent.

I will fix the code soon.

@kocio-pl kocio-pl changed the title Adding rendering for amenity=ferry_terminal [WIP] Adding rendering for amenity=ferry_terminal Nov 19, 2016
@kocio-pl
Copy link
Collaborator Author

The code is updated and ready to be merged.

General notes:

  • The area is now rendered as the aerodrome (not as the railway station), the rest remains the same
  • There was variable sections cleaning done

@nebulon42
Copy link
Contributor

nebulon42 commented Nov 19, 2016

I think it is not a good thing to mix cleanups with changes. It makes diffs harder to read (you could still leave it in this PR but move it to a separate commit). Since there were some changes of design over the course of the PR I don't get the changes at first glance. Could you please show a proper before after comparison?

@pnorman
Copy link
Collaborator

pnorman commented Nov 22, 2016

I think it's more worthwile to render an icon for a ferry terminal than a fill. These are features that people are often looking for, and an icon is better for that.

@kocio-pl
Copy link
Collaborator Author

The fill and the icon play a different role, just like in the airport: icon make it easier to find on the big picture, while the fill is important when zoomed in to know which roads and buildings belong here. This is related to #2094.

Unfortunately we have no such icon available, because none of my propositions was approved by the community (#1673), but if we have one, I would like to show it too.

@kocio-pl
Copy link
Collaborator Author

Code is cleaned now and here are basic before/after renderings (I won't make it extensive, because most of previous are correct except those with red background):

Warsaw, z15
Before:
e_ndw nu
After:
zvjyqtxs

Victoria, z18
Before:
4xbciwk5
After:
qw5 eoil

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Dec 5, 2016

Code is updated now and ready to be merged.

@matthijsmelissen
Copy link
Collaborator

I agree this feature would need an icon for it to be useful (sorry for slow review).

Copy link
Collaborator

@matthijsmelissen matthijsmelissen left a comment

Choose a reason for hiding this comment

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

Requesting changes.

@kocio-pl
Copy link
Collaborator Author

Any hints about the icon design? Maybe the discussion in the #1673 could be useful here.

I think the icon should be visible only to some zoom level (like the airport, but later, since typical ferry terminal is smaller). It wouldn't be visible on z18 then, so the rendering above would be the same.

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Dec 16, 2016

This is how original icon works in reality (but it's black here):
https://maps.wikimedia.org/#18/52.23707/21.03863
https://maps.wikimedia.org/#18/48.42213/-123.37224

I also think we could try with something more symbolic and easier to recognize, like the anchor:

  1. https://openclipart.org/detail/10145/aussersihl-coat-of-arms
    ferry-14-19

  2. https://openclipart.org/detail/190960/anchor
    1392496694-14px

  3. https://openclipart.org/detail/18387/simple-anchor
    bogdanco-simple-anchor-14px

What do you think?

@kocio-pl kocio-pl changed the title Adding rendering for amenity=ferry_terminal Adding rendering for amenity=ferry_terminal [WIP] Dec 22, 2016
@matthijsmelissen
Copy link
Collaborator

To me, the anchor doesn't really remind me of a ferry terminal. Not sure what others think.

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Mar 22, 2017

I would be happy to at least have some hint what could work for the others, since my ideas were not accepted till now and I ran out of them. However first anchor is still good for me.

@dieterdreist
Copy link

dieterdreist commented Mar 22, 2017 via email

@matthijsmelissen
Copy link
Collaborator

Not all ferries carry cars. Maybe a generic icon of a ship?

@dieterdreist
Copy link

dieterdreist commented Mar 22, 2017 via email

@pnorman
Copy link
Collaborator

pnorman commented May 10, 2017

As this has cartography changes, this PR is currently held up by the feature freeze until 4.0.0 has been released.

@matthijsmelissen
Copy link
Collaborator

@kocio-pl What is the status of this PR?

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Aug 12, 2017

I took some time to let the idea establish a bit in my head. Now I plan to eventually update it with 1. anchor icon, because:

  • it's generic enough and much easier to recognize than our previous attempts to depict ships,
  • having icon here conforms our style for transport amenities (blue icons for bus/tram stops or the airports) and having consistent system is enough to give user a hint what is the context of the anchor (transport),
  • it's probably not going to be used for any other water-related feature (like marina or port).

@kocio-pl
Copy link
Collaborator Author

@dieterdreist I think that current tagging is just generic, I wouldn't call it immature - this problem applies to many old features (like healthcare amenities or public transport). However your seaway=* proposition looks quite interesting to me, but it's not yet practically used (6 uses now) nor even proposed for discussion (it's still a draft), so we can re-examine it later.

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Aug 12, 2017

Current rendering (with icon):

Warsaw, z15
Before
sfhncvqu
After
yi8iztue

Sidney (Canada)
z15
Before
rz_0nkmo
After
vo2mmc f

z18
Before
h0wm1cah
After
3szsjaex

@kocio-pl kocio-pl changed the title Adding rendering for amenity=ferry_terminal [WIP] Adding rendering for amenity=ferry_terminal Aug 12, 2017
@kocio-pl
Copy link
Collaborator Author

Are there any other issues not discussed before?

@pnorman
Copy link
Collaborator

pnorman commented Aug 16, 2017

It's worth checking an inner harbor dense with small ferry terminals. http://www.openstreetmap.org/#map=15/49.2726/-123.1264 has some

@kocio-pl
Copy link
Collaborator Author

Vancouver, z15 (click to see unscaled images):
jzcaxgp5

@kocio-pl
Copy link
Collaborator Author

I feel this code is ready to be merged, but I don't know how long should I wait and what for, because it's quite old, but formal requirement (using icon) is fulfilled for just a few days. On the other hand nobody was opposing. Our decision rules are hard to apply to long and evolving solutions.

@kocio-pl kocio-pl merged commit 7fa948b into gravitystorm:master Aug 20, 2017
@kocio-pl kocio-pl deleted the ferry_terminal branch August 20, 2017 10:57
@vss-devel
Copy link

I've noticed that Mapnik started rendering amenity=ferry_terminal with an anchor symbol anchor symbol.

At nautical maps a similar symbol denotes an anchorage, so this usage of an anchor for the amenity=ferry_terminal is quite misleading.

@kocio-pl
Copy link
Collaborator Author

It's not a nautical map and none of the ship icon was clear enough for osm-carto community (even if they are used on some other styles). I also noticed using the anchor symbol in some other general map, but unfortunately I don't remember which one.

@imagico
Copy link
Collaborator

imagico commented Aug 29, 2017

@Vadp - you should best open a new issue for this.

Not only in nautical maps by the way, see for example chapter 2, symbol 226 in http://www.lib.berkeley.edu/EART/pdf/soviet.pdf - likewise for some pre-digital German TK100/TK200 (don't have a digitized sample at the moment).

@dieterdreist
Copy link

dieterdreist commented Sep 6, 2017 via email

@vss-devel
Copy link

I agree an anchor is not a good symbol for a ferry

@dieterdreist I've opened #2785 for this issue.

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

Successfully merging this pull request may close these issues.

7 participants