gravitystorm / openstreetmap-carto

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

Double rendering name for some features on buildings #3780

Closed kocio-pl closed 5 years ago

kocio-pl commented 5 years ago

Probably regression after #3712, which was replacing centroid algorithm for amenities from the one used by current Mapnik to PostGIS ST_PointOnSurface.

Expected behavior

When building contains some feature we render, the name should be rendered only for this feature.

Actual behavior

A name can be rendered both for the feature and for the building in different places if the shape is non-trivial.

Links and screenshots illustrating the problem

Example with amenity=library+building=*+name=*:

Screenshot_2019-05-10 OpenStreetMap Carto — Kosmtik

Proposed solutions

Possible solutions might be:

  1. Remove selecting buildings for name rendering when they share the geometry with amenity tag.
  2. Pass the building name rendering also to PostGIS.
kocio-pl commented 5 years ago

The same problem with office+building combinations with non-trivial shapes (the one with almost square shape renders properly):

Screenshot_2019-05-10 OpenStreetMap Carto — Kosmtik(1)

[ https://www.openstreetmap.org/way/35008161 ]

kocio-pl commented 5 years ago

The same with historic example, so there are more tags with this problem probably.

pnorman commented 5 years ago

We'll need to move building names to st_pointonsurface too

kocio-pl commented 5 years ago

What about limiting building name rendering? Wouldn't it make the database response smaller (and maybe faster)?

pnorman commented 5 years ago

What about limiting building name rendering? Wouldn't it make the database response smaller (and maybe faster)?

What do you mean?

kocio-pl commented 5 years ago

Database query for rendering building name could just exclude the cases when there is also some other tag we render (amenity/office/historic etc).

imagico commented 5 years ago

We'll need to move building names to st_pointonsurface too

Yes, and we should check if there are other labels which potentially have the same problem.

Generally speaking this bug is also an indicator for ambiguous tagging - it is simply not clear if the name is the name of the building or the name of the amenity/office/etc. But it of course has become the implicit assumption of mappers that the name applies to the feature it is rendered for in the standard style.

matkoniecz commented 5 years ago

I am not sure is it something that should be fixed.

Such tagging (though common) is IMHO violation of https://wiki.openstreetmap.org/wiki/One_feature,_one_OSM_element

Maybe rather validators should complain about merging POI and building into one element?

kocio-pl commented 5 years ago

Such a big rendering change would require discussion first, so people can think about it, comment for some time and probably make changes in documentation and tools. I would not just slip that change without the intention into release.

imagico commented 5 years ago

I think this should be fixed independent of the question if this is correct mapping or not because the two labels are the result of the mapnik label location and ST_PointOnSurface by chance resulting in locations sufficiently distant from each other for both labels to show. This random nature of the double labels is highly confusing for mappers.

If we'd deliberately try to label both the building and the POI by moving the labels we could indeed try to display both since that is a valid interpretation of the data. Same as for something tagged both as an amenity and a shop for example. I am not sure if that is ultimately a good idea but it could be worth discussing.

pnorman commented 5 years ago

Database query for rendering building name could just exclude the cases when there is also some other tag we render (amenity/office/historic etc).

We don't know what is labeled at query time. If we de-duplicated at query time we'd need to reproduce most of the amenity query in the building query which isn't an option.

kocio-pl commented 5 years ago

We have no clear idea how to attribute name to buildings, though there are some tags for this:

taghistory(54)

taghistory(52)

taghistory(53)

Also I don't think it's really not proper tagging when more objects share the same database object. For example I would tag shop with cafe functions as one object. I believe object occupying a whole building can be seen as similar case.

jeisenbe commented 5 years ago

address:housename is not for the name of a building, but for a postal / mailing address that uses a name for a house instead of a housenumber. This is common in some rural areas in Britain.

imagico commented 5 years ago

Note building=yes + name=* is perfectly valid tagging for a named building. If there are other tags on the same feature it is not clear which of them the name applies to though. The solution to this is not to create additional name tags but to separate the building from its uses in mapping and to have in rendering a clear and consistent prioritization.