Set polygon/linestring differently for some key values differently than the key in default style.lua

[matthijsmelissen]

This allows the decision whether an object is treated as polygon or linestring
to be set in the style.lua file.

In particular, it treats the following values as linestring:
man_made=embankment, man_made=breakwater, man_made=groyne, natural=cliff, natural=tree_row, historic=citywalls, waterway=derelict_canal, waterway=ditch, waterway=drain, waterway=river, waterway=stream, waterway=wadi, waterway=weir, power=line, power=minor_line. It also treats highway=services as polygon.

Including this in the default style.lua file has the following advantages:

• It serves as an easy-to-extend example of how the polygon/linestring decision
  can be made based on key.
• It provides sane defaults - it can be expected that most users of the style
  want to treat man_made=embankment and natural=cliff as linestring, and
  highway=services as polygon.

This resolves the following downstream bugs in openstreetmap-carto:

• man_made=embankment is not rendered if it's a closed way gravitystorm/openstreetmap-carto#892
• circular cliffs are not rendered gravitystorm/openstreetmap-carto#268
• highway=services is an area gravitystorm/openstreetmap-carto#137

This builds on #345, and thus assumes #345 is merged first.

[kreed]

power=line as linestring would be nice too. Usually power lines aren't closed loop so it isn't an issue but sometimes it comes up with busbars inside substations.

[matthijsmelissen]

Yes, good suggestion.

[matthijsmelissen]

I added power=line now, as well as some other key/value combinations.

[pnorman]

changes to filter_tags_relation_member should also be mirrored in multi.lua, as they are basically the same function.

[mboeringa]

@math1985 , you do take into account possible secondary building NOT IN (NULL,'no') tags? Historic citywalls are sometimes mapped as buildings with individual parts as closed loops / ways. So they may have both a historic = citywalls and building = yes/x tag.

Anything tagged with a building tag should be excluded from this evaluation I think and default to polygon.

I am also not sure about defaulting closed ways of man_made = breakwater and man_made = groyne as linestrings. I agree these are "linear" features by nature protruding from the coast or riverbank, _but if mapped as a closed way_, they almost certainly represent a micro-mapped polygon representation of the feature, so should likely be polygons.

[pnorman]

Renamed the issue - the polygon/linestring decision has always been allowed be set by value in Lua.

[matthijsmelissen]

changes to filter_tags_relation_member should also be mirrored in multi.lua, as they are basically the same function.

I don't really see how that works - there is not even an polygon_keys in multil.lua? What should exactly be changed?

[matthijsmelissen]

1. Rebased and solved a bug.

2. AppVeyor seems to fail, but I only touched the Lua files. What could be the cause of the build fail?

3. @pnorman In response to the following:

   changes to filter_tags_relation_member should also be mirrored in multi.lua, as they are basically the same function.

It's not yet totally clear to me how this works. Both style.lua and multi.lua add tags from the outer polygons to the multipolygons under some circumstances. However, these circumstances are quite different. The file style.lua adds the tags from the outer polygons to the multipolygon if the multipolygon has zero polygon tags. However, multi.lua adds the tags from the outer polygons to the multipolygon if the multipolyon is not a building. That's quite a different condition! The only change I made to filter_tags_relation_member is the way the number of polygon tags is counted, and as we don't count polygon tags in multi.lua, it does not seem like we need a similar change in multi.lua at the moment.

[matthijsmelissen]

@mboeringa Good point about historic=citywalls.

So basically the problem is that a closed linestring tagged with historic=citywalls can mean two things. Either it is a circular wall, or a wall of which the outline is tagged. Unfortunately, both of them are used commonly in practice. The question is now what we should assume as default.

Unfortunately the presence of a building tag is not sufficient to tell the two apart. For example Templin has a citywall mapped as area, but no building tag. And conversely, I could also imagine that an area tagged with both a building and citywall tag might signify a building with a city wall around it (like a fortress).

To repeat the question: a citywall as an open linestring will in any case be interpreted as line. What should we do with citywalls as closed linestring?
A. Treat as line, unless tagged area=yes.
B. Treat as area, unless tagged area=no.

I think personally I'd prefer option A. It seems counterintuitive to me that closing a linestring would suddenly change the semantics. Also, I don't think we currently have any tags that can refer both to a line or area that do not need an area=yes tag when used as area, except the tags in this PR.

What do others think?

For breakwater and groyne, the situation is similar, except that closed linestrings for these objects in almost all cases do refer to an area. How should we treat these objects?

[sommerluk]

@math1985 Could you also add junction=yes as polygon? (Downstream gravitystorm/openstreetmap-carto#1253 )

[lonvia]

A general remark: You do say that the PR is intended as an example implementation but at the same time I see a tagging discussion unfold here with plenty of references to the carto style. That tells me this might be the wrong repository for that change. Shouldn't you better start maintaining your own lua style file like you already do for the C style file? I'm just afraid that the more fine details of tagging are lost on the osm2pgsql maintainers as evident from the fact that this PR hasn't been merged in almost a year.

[matthijsmelissen]

I have thought a bit more about this, and I came to the following conclusions:

• An object with both a polygon-key and a line-key are currently treated as polygon. Therefore, the logical thing to do is to treat an object with both a polygon-key and a line-tag (e.g. building=yes and historic=citywalls) also as polygon. I have changed the code to handle this correctly. Thanks again @mboeringa.
• Current practice is that objects that can be interpreted as a line, such as highways, are interpreted as lines even when they form a closed linestring. To treat them as an area, the tag area=yes is needed. I'd like to follow that practice, so with this PR, tags like historic=citywalls or waterway=weir will be interpreted as line even for a closed linestring. This can be overruled by adding either area=yes or an area tag, such as building=yes. No change is needed to the PR.

[matthijsmelissen]

@math1985 Could you also add junction=yes as polygon? (Downstream gravitystorm/openstreetmap-carto#1253 )

Added.

[matthijsmelissen]

This is now ready to be merged. I can still squash the commits if you like.

A general remark: You do say that the PR is intended as an example implementation but at the same time I see a tagging discussion unfold here with plenty of references to the carto style. That tells me this might be the wrong repository for that change.

I understand your point, but I'm not sure if I agree. I think it would be good if osm2pgsql provides sane defaults. The decision whether to treat a certain tag, like natural=tree_row, as polygon or line is a data representation decision, not a rendering decision. I would therefore expect this to be solved within osm2pgsql. For this decision, it doesn't matter whether a user wants to use a PostGIS-database for openstreetmap-carto, an other style sheet, or perhaps not for rendering purposes at all. I'd expect all applications would require the same defaults for the line/area decision. I'd consider the current default behaviour, where a closed natural=tree_row is treated as a polygon, as a default that's not sane..

However, if the osm2pgsql maintainers feel this repository is not the right place for this PR, I would understand. Maintaining an openstreetmap-carto specific .lua-file would work for us too, and might be a good idea anyway, independent on whether this PR is or is not merged.

[lonvia]

This file seems accidentally committed, it is an artifact from autoconf times.

[lonvia]

Well, I see the argument that style.lua should do something sane. I'm saying that I'm not able to judge, if this PR counts as bug fixing or as a carto-osm-specific change. I've had a look at the code-part now and added a few comments but I would leave the final decision to @pnorman, if this makes sense to have this change in osm2pgsql or not.

From a practical point of view, you'd want your own lua style anyway because otherwise carto devs are forced to compile their own osm2pgsql all the time just to stay in sync with the style file.

[matthijsmelissen]

@lonvia Thanks for your feedback, I think I addressed all issues now.

[matthijsmelissen]

@pnorman Would you be able to review this PR? Let me know if you need anything else.

[lonvia]

We had a lengthy discussion about the lua style file at the last hackweek end. The general conclusion was that the lua style in its current state is not really ready to be used. The code is unnecessarily complicated and often terribly inefficient. So we are now seriously considering the idea of rewriting it from scratch. I'm afraid this will leave this PR (and possibly also #532) a bit in the hanging until there is decision on that matter.

I've also noticed that the code is still not quite right. I'll add a comment about that.

[lonvia]

Ignore the last part of my comment. The code is probably okay. But that reminds me of the second observation we made at the hackweekend: we need proper tests for the lua code.

[matthijsmelissen]

Thanks for the update. I know it's always hard to tell, especially when volunteers are involved, but do you have any idea how long it will take before the new lua file is ready?

Do you think the problems are so serious that you'd recommend openstreetmap-carto (and therefore the openstreetmap.org tile servers) to wait for the new lua file, or do you think we could use the current lua file within openstreetmap-carto?

[pnorman]

style.lua is an example, not a default. I wouldn't recommend using it as-is for any project.

[matthijsmelissen]

Rebased.

As far as I'm aware, this is still awaiting review from the osm2pgsql team.

[pnorman]

Closing as stale. style.lua is an example, and if a style needs a full transform, the osm-carto one is a good starting point.

We should probably rewrite style.lua based on osm-carto's experience, and add tests.

[matthijsmelissen]

We should probably rewrite style.lua based on osm-carto's experience, and add tests.

Yes, that would be great.

I don't think it make sense that each individual style decides for themselves which osm objects represent areas and which represent linestrings. This is a decision about the semantics of objects in OSM, not a styling decision. Competing styles are great, but competing semantics make no sense. So I think this is really something that should be done in this repository.