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

Venice is under water #951

Closed
nvkelso opened this issue Aug 8, 2016 · 6 comments
Closed

Venice is under water #951

nvkelso opened this issue Aug 8, 2016 · 6 comments
Assignees
Milestone

Comments

@nvkelso
Copy link
Member

nvkelso commented Aug 8, 2016

  • What did you see? Venice is under water, so is the island west of Athens.
  • What did you expect to see? Roads should be on land.

We think this is caused because "land" kind sorts below water, but should instead sort above water.

@nvkelso nvkelso added this to the v1.0.0 milestone Aug 8, 2016
@zerebubuth
Copy link
Member

In the case of Venice, it's because the Adriatic sea, which gets kind:sea, sort_key:200, covers Venice island, which is kind:earth, sort_key:10.

In the case of Salamina, near Athens, the Aegean Sea, also kind:sea, sort_key:200, covers Salamina island, also kind:earth, sort_key:10.

If we alter the sort key of seas to be below earth, then we may get the opposite problem happening elsewhere in the world; bays which are part of seas "draining" because the land now "floats" above them. Related to #446.

There's another possible fix: both of these sea polygons come from a multipolygon tagged place=sea and, although it is a polygon, perhaps it would be better to use it only for labelling as we already do for bays, straits and fjords #400 #515? In both cases above there were other polygons (Venice Lagoon and Salamina's coastline) which cover the water areas, without needing changes to the sort key.

@nvkelso
Copy link
Member Author

nvkelso commented Aug 9, 2016

When we added islands I made a bad assumption that their landmass would already be present in the earth data we get from openstreetmapdata.com (and they'd be cookie-cutttered with at least ocean-like water), so this logic removes their polygons (after creating label placements for them): https://github.com/tilezen/vector-datasource/blob/master/queries.yaml#L478-L484.

Is another option to not remove island land features (to keep kind IN (island,islet) in the earth layer, and sort that above the water but below other man_made features? Example: https://www.openstreetmap.org/relation/2683672#map=18/45.42440/12.32811.

Looks like for seas we generate label placements but do not drop the sea polygon. Seems like we need the sea polygon starting at the zoom we transition from NE to OSM, but we're always showing them from zoom 3 earlier now? Which is also creating tons of duplicate sea labels :\

@zerebubuth
Copy link
Member

Venice is all land, from the land_polygons and water_polygons tables. The coastline is outside the lagoon, which is modelled as a large lake, so the "background" of the tile is all land.

Floating the island above the place=sea might lead to other issues, as there's other kinds of water on Venice island, for example Rio del Battello (way 258639622) and this natural=water (way 237920752), which need to be above the island. So now we've got at least 4 different layers:

  1. base earth,
  2. "sea" water,
  3. island earth,
  4. non-"sea" water.

On the other hand, the lagoon (relation 3049430) is cookie-cuttered, and has a hole for Venice island, which allows the land to show through. The problem is that the place=sea polygon for the Adriatic isn't cookie-cuttered and covers the land. So dropping the place=sea polygon (but keeping the centroid for labelling) would solve the problem.

The story is similar for Salamina; it's covered by the Aegean polygon, but cookie-cuttered from the water_polygons, so it would render properly if the sea polygon were dropped.

I don't think we need the sea polygon at all.

@zerebubuth
Copy link
Member

For what it's worth, I couldn't find anywhere in osm-carto where the polygon is rendered from place=sea.

@nvkelso
Copy link
Member Author

nvkelso commented Aug 9, 2016

Less is more, agree we should drop sea polygons. Also looking in osm-carto islands are only ever labeled and not rendered as land so I think we're fine there (except for performance issues).

Looks like we add sea to list of water features here: https://github.com/tilezen/vector-datasource/blob/master/queries.yaml#L471

Let's track removing duplicate sea labels (e.g. Adriatic Sea, Adriatic Sea, Adriatic Sea) in #953.

/cc @burritojustice who originally reported this issue.

@nvkelso
Copy link
Member Author

nvkelso commented Aug 25, 2016

Working for Athens case!

dev
screen shot 2016-08-25 at 00 45 45

prod
screen shot 2016-08-25 at 00 45 56

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

No branches or pull requests

3 participants