Fixed office/amenity conflict

[andrzej-r]

RE: #3159 (comment)
Fixes #3195

POIs with both amenity and office tags triggered both code paths, resulting in points that shared some visual effects of both of these points.

Refactored the code to use feature field instead, as the coalesce() function guarantees it is unique. In the above example, amenity takes precedence over the office tag.

I've also, added extra checks in addresses and building_text layers so that they don't render labels when amenity/shop/office tags are defined.
In most (but not all) cases these labels were already hidden by Mapnik because they would have overlapped POI icons otherwise. However, even if not rendered, these labels would still be processed by Carto and Mapnik, increasing rendering time.

[andrzej-r]

Example illustrating the tag conflict and the fix.

[nebulon42]

Wouldn't this be prone to combinatory explosion problems (see mapbox/carto#20)? This was the reason why feature = office_ was introduced in the first place.

[andrzej-r]

Hi @nebulon42,

Is there any way to test performance, or at least to check the processing result (the mapnik xml file) for a specific tile?

Note that the check for a unique feature= tag is still there. In fact, [feature = 'office'] is now required for any office tag related rendering (unlike before, when some rendering could have been triggered when non-unique office tag was not null, which was causing rendering errors.

Having said that, I don't really understand the combinational explosion mechanism. Perhaps @gravitystorm could comment on that.

[matthijsmelissen]

If we have rules for amenity=foo, leisure=bar and shop=baz, then the resulting XML ouput (which is the input fir Mapnik) will have one rule for each combination of amenity, leisure and shop (8 in total).

[andrzej-r]

That should be sorted by having checked feature=* first, then.

How do I get to see the resulting XML, so that I can optimize its size?

[nebulon42]

You don't need to worry about tiles, but just about the generated XML. You can see the generated XML if you manually invoke carto. E.g.:

npm install -g carto
cd openstreetmap-carto
carto project.mml > output.xml

You can find more details at https://cartocss.readthedocs.io/en/latest/installation_usage.html.

Bascially you would need to look for a size explosion of the generated XML file before and after.

[andrzej-r]

I run carto on several versions of the code. Below I have listed the number of lines of some of them:

v4.9.0: 47743
v4.10.0: 71903 (+24160)
v4.10.0 + this PR: 48556 (+813)
v4.10.0 + this PR without extra checks in addresses and building_text layers: 48554 (+811)

So, if anything it is the existing implementation that is causing the output to swell quite a bit. Probably related to the fact it was testing non-unique tags.

[kocio-pl]

v4.10.0: 71903 (+24160)

Oh, it looks like we have a problem here, maybe even of the same type (combinatory explosion). Could someone find the source of such a big size expansion?

[andrzej-r]

It's definitely coming from the previous office tag implementation. Because:

1. Improving the implementation fixes it
2. Line counts just before and after the merge were respectively 48559 and 71903

But line count is not solely responsible for the execution time. For example, the revert (#3182) that fixed CPU use, had no impact on the xml file line count (71903 lines before and after).

By the way, the line count is calculated as part of Travis CI tests (with a slightly different formula and Carto version). I can only see the results of my PRs, but perhaps you can access all CI test results.

[kocio-pl]

I see. It might be however the reason why the amount of dropped metatiles is ~1,5 times higher this time than in previous few releases:

https://munin.openstreetmap.org/openstreetmap/render.openstreetmap/renderd_processed.html

@matthijsmelissen @nebulon42 Please give a nod when this is ready in your opinion. We should make a v4.10.1 and I can do it, but I'd be happy if somebody else make the release.

[matthijsmelissen]

I've also, added extra checks in addresses and building_text layers so that they don't render labels when amenity/shop/office tags are defined.

This also drops house numbers from amenity tags that are not rendered. I don't think we want that.

In general, PRs should be as small as possible: it makes the code change easier to review, and it prevents situations like this one where a PR contains two changes of which I would like to merge only one of them.

[matthijsmelissen]

Could someone find the source of such a big size expansion?

This seems to be #3163, based on the stats provided by @andrzej-r: as we have a Carto rule for [office != null], we get an additional XML rule for the combination of [office != null] with every individual feature. The current PR fixes this.

We should make a v4.10.1 and I can do it,

I don't think we necessarily need to make a v4.10.1 release. Rolling out a new release might put just as much stress on the server as just waiting a week or so until the next regular release.

[kocio-pl]

This is a visual problem and (I suspect) also the performance problem. However we can for example make a regular (v4.11.0) release 3 weeks after the v4.10.0 (~11.05) instead of typical 4 weeks if you think it's not worth the hassle.

[andrzej-r]

This also drops house numbers from amenity tags that are not rendered. I don't think we want that.

In general, do we even want to render house numbers/names for amenities? It only adds extra clutter and provides no information.
Buildings that are also amenities are a different story. But IMHO such more important amenities should be supported by this style anyway.

Anyway, this can be easily changed by testing supported amenity tag values in the SQL code of buildings labels.

In general, PRs should be as small as possible

The code is present in this PR because if fixes a specific issue visible in the first screenshot: the name tag is first rendered as a building label at z16 (no amenity icon to obscure it), and then as an amenity label at z17+.

Now, I am not particularly attached to this part of the fix so I am OK removing it, if you wish so. But IMHO we should do something about it. Relying on Mapnik to filter out the labels by obscuring them with icons is a wrong (not just inefficient) solution. What if a newer version of Mapnik gets smarter and starts rendering labels twice?

[matthijsmelissen]

Now, I am not particularly attached to this part of the fix so I am OK removing it, if you wish so.

In any case, let's split it off to a different PR, so we can separate the discussion.

[andrzej-r]

Done. I've tested it locally. It works the same as git master w.r.t. rendering building labels and unsupported amenity tags.

[matthijsmelissen]

Thanks!