gravitystorm / openstreetmap-carto

A general-purpose OpenStreetMap mapnik style, in CartoCSS
Other
1.55k stars 822 forks source link

Tag:tourism refrains turntable from rendering #3812

Closed IgorEliezer closed 4 years ago

IgorEliezer commented 5 years ago

Expected behavior

Tag tourism should not affect the rendering of railway turntables.

Actual behavior

I'll use this turntable (https://www.openstreetmap.org/way/444942851/history) as example.

Changeset 71486896: before adding tourism=yes.

turntable_1

Changeset 71487174: after adding tourism=yes.

turntable_2

I also tried removing name and wikimedia_commons: no effect.

jeisenbe commented 5 years ago

This behavior is the result of how we have to import data from the openstreetmap database into a rendering database prior to rendering. Since there is no "area" data element in Openstreetmap, mappers use closed ways (ways that start and end at the same node) to represent areas with many features. When we import data, there is a list of keys which are considered to be areas by default, so all features with these keys are imported as polygons if they are mapped as a closed way.

All features with tourism=* are treated as polygons when mapped on closed ways because most values of tourism are things like =hotel, =motel, =guesthouse, =attraction which can be mapped as nodes or areas. Features with the key railway=* can be lines like railway=rail or railway=turntable, or areas like railway=station so we specifically check for just the tag like =station which should be imported as polygons.

Each database object mapped as a way is only imported as a single linestring or a single polygon. So when an object has "tourism=" and "railway=" it gets imported as a polygon when mapped as a closed way. We don't have a special rule that says railway=turntable -> linestring, so the tourism= tag causes your example to be imported as a polygon, into a rendering database named planet_osm_polygon. This is used for things like icons, text labels, and landcover rending.

However, the line rendering for turntables is based on planet_osm_line which only includes features that were imported into the rendering database as linestrings. Since your feature was imported as a polygon, due to the tourism= key, it is not in planet_osm_line and does not render.

jeisenbe commented 5 years ago

We could try to fix this by treating tourism=yes in a special way at the time of rendering database import, since this tag isn't meant to be a main feature tag. However, this problem would still occur if mappers add tourism=attraction or tourism=<some other tag> to another feature that should be a linestring but is mapped as a closed way.

The wiki page for tourism=yes (actually just the Key:tourism page) suggest that both tourism=yes and tourism=attraction should only be used for nodes and areas, not for linear way features like railways; this also suggests that neither of these tags should be added to this feature.

The tag tourism=yes is only used 5428 times and only 1988 times on any kind of way, compared to 183,000 tourism=attraction features, so the latter tags is more likely to cause problems, especially the 1900 with highway=*

I believe in that case it is also good to read and follow the recommended Good Practice wiki page: One Feature, One OSM Element - this explains why each OSM object (especially closed ways) should only be tagged with one main feature tag, including things like tourism=attraction or tourism=<some other tag> which can be used as a feature. But I understand that tourism=yes isn't meant as a main feature tag.

matkoniecz commented 5 years ago

tourism=attraction is a property, not main feature tag so it also is not supposed not cause problems.

jeisenbe commented 5 years ago

Tourism=attraction is often used as a feature tag. We render it when used alone, though it's now lower priority than anything else, so another feature will be rendered if it's double-tagged.

I don't think there is an easy way to fix problems with tourism=attraction unless we stop rendering it as a feature (perhaps not a bad idea?).

But we can fix the issue with tourism=yes at the next database reload by including it as a linestring exception in openstreetmap-carto.lua - I've tested this with various combinations of tourism=yes and other features meant to be lines or areas, and it works properly.

(Interestingly, the wiki documentation for railway=turntable currently suggests it should be tagged as a node or area, not a way, but we render it as a linear feature and this tagging seems common, though it's debated on the Talk page)

matkoniecz commented 5 years ago

It is rendered on its own, as it allows rendering of interesting points with unusual/rare primary features but it is never enough to define object on its own.

IgorEliezer commented 5 years ago

I've made another edit on the turntable. I used tourism=attraction instead of tourism=yes. The turntable stops rendering but this time it renders the name.

map (2) https://www.openstreetmap.org/way/444942851

As to the comments:

  1. I thought tourism=yes was used to indicate that an object is also a generic tourism item, not a main/standalone thing. You can use building=yes + tourism=museum, by the way.
  2. The turntable is one of the attractions in the historic railway town offered by the local city (board map, item 10) and there's a sign with touristic info, so I decided to use tourism=attraction. I could remove it, if it's still an issue. I got the key:tourism idea as a feature tag but I would like to keep it searchable as touristic attraction in the data consumer apps/sites.
  3. I don't think a turntable is a linear feature, pretty much like railway=station or railway=roundhouse are not. The turntable depicted in my 1st post is big enough to fit a small station building, as it is almost 20 m in diameter.
jeisenbe commented 5 years ago

"I thought tourism=yes was a property tag to indicate that an object is also a generic tourism item"

Yes, that's correct. Because of this, we can fix the problem with tourism=yes by excluding it from the tourism features that are imported as polygons, since tourism=yes shouldn't be the only tag on a feature, and if it is, we don't render it.

We can't fix the problem with tourism=attraction, because sometimes it is the only tag on a feature, and it is currently rendered.

jeisenbe commented 5 years ago

I've looked into the current rendering of railway=turntable. The recommended mapping, according to the wiki, is as a node or area, so it's bit odd that we render it from the line features (planet_osm_line) rather than importing it as a polygon and rendering from that.

Currently there are 2298 ways with railway=turntable. I downloaded them and searched with JOSM: there are only 31 unclosed ways, and of these only 18 are just mapped as an unclosed ways (the others also are mapped as a node or as a closed way in addition). So we could drop the rendering of linear ways tagged railway=turntable to support the recommended mapping method, as a node or area.

This would require importing these features as polygons when mapped as closed ways, so such a change could not take effect until after the next database reload is planned for the servers at openstreetmap.org

jeisenbe commented 5 years ago

PR #3833 "Add tourism=yes as a linestring exception" will fix this issue, but only after the next database reload, so it will stay open for now.

Also, any thoughts about changing railway=turntable rendering to be based off of the polygon instead? This will drop rendering of 18 turntables incorrectly mapped as linear ways, but 2280 will continue to render correctly.

pnorman commented 5 years ago

Also, any thoughts about changing railway=turntable rendering to be based off of the polygon instead? This will drop rendering of 18 turntables incorrectly mapped as linear ways, but 2280 will continue to render correctly.

+1

jeisenbe commented 4 years ago

Closed by #3833 in v5.0.0 which is now released