Restore rendering of text labels on points

[jeisenbe]

Fixes #3876
Fixes #3868

Changes proposed in this pull request:

• Restore rendering of text labels on points, including landcover and islands

Explanation:

Prior to #3764 most features in landcover had text labels rendered at high zoom level when tagged on nodes. For islands this was the case before #1444. It appears that text labels of the name for points was unintentionally lost in these two PRs.
This PR restores this rendering by adding a check for way_pixels=null.
Suprisingly, way_pixels != null has to be checked for each instance of [way_pixels > 12000] or else the points render with the large lancover text size

Test rendering:

Old rendering, before #3764 but after #1444

Current test rendering:

After this PR:

[kocio-pl]

I remember that you have made similar rendering tests for different features. Do you have the idea how could we make a standard (possibly automated) test for indicating visual changes in every feature we render, to avoid most basic problems, like this one?

[jeisenbe]

I don't think there is a universal solution. Often bugs are found in the things that we forget to test.

[kocio-pl]

I mean a list of objects and dumb automated file diff, not high level tests. It is trivial to catch the difference between tile with object and without it. The idea is to do it for every commit (possibly by Travis), so we do not have to remember about running it.

[jeisenbe]

Oh, that's interesting. I have no idea how to implement something like that. It would be difficult to get all the possible combinations in one test file: many changes have to do with one feature layered on top of another.

[kocio-pl]

@pnorman You have some experience with Travis checks, do you have some idea for implementing such test?

Maybe every feature should be in separate file, to make diffs atomic. If anybody else has some suggestion how can we deal with it, it would be great.

[pnorman]

If someone comes up with checks I can help get Travis running them.

[HolgerJeromin]

Ref #3446 (Regression tests for tag order on the same object)

[imagico]

Works fine.

Since this would significantly reduce code complexity (essentially it would make this change unnecessary) i wonder if it would not make more sense to simply set way_pixels to 0 instead of NULL in SQL since we essentially treat (and will likely always treat) point features the same way as very small polygon features in cases where both are rendered identically in principle.

[jeisenbe]

Thanks. I'll merge this in a couple of days if there are no other comments or objections.

[jeisenbe]

... simply set way_pixels to 0 instead of NULL in SQL since we essentially treat (and will likely always treat) point features the same way as very small polygon features in cases where both are rendered identically in principle.

@pnorman - any opinion on this idea? Would it slow things down to have more non-NULL fields in the rendering database?

[pnorman]

@pnorman - any opinion on this idea? Would it slow things down to have more non-NULL fields in the rendering database?

We would not be altering the database. We could go for 0.

[jeisenbe]

Should we merge this PR now, and then try out the solution with 0 in the next release, or would it be better to switch now?

[sommerluk]

Sorry for long waiting time. (I'm really busy currently.)

I approve this PR.

Going to way_pixels = 0 instead of way_pixels = null for nodes would not allow us anymore to have different rules for nodes and areas, but as @imagico said essentially we treat nodes like small areas anyway. It would not only make the MSS code shorter, but also much more natural to read (and prevent that someone else does the same error I did when introducing this bug). So I would go for the way_pixels = 0 on nodes solution right now instead of doing it later.

[jeisenbe]

OK, I've pushed a new commit which changes from NULL as way_area to O as way_area for the points, and removed the other changes. The test renderings above are unchanged, and all of the amenity-points.mss features in my test rendering appear to still render correctly, including subway entrances and stations (that layer is also affected by the change).

[imagico]

I don't mind this approach but what i actually had in mind is changing

openstreetmap-carto/project.mml

Line 1537 in eae0909

way_area/NULLIF(POW(!scale_denominator!*0.001*0.28,2),0) AS way_pixels

using COALESCE(..., 0).

The differentiation between NULL and zero is not practically that relevant because polygons with zero area are not valid and AFAIK osm2pgsql does not generate those. But for clarity selecting the node only feature types with way_area IS NULL is still clearer.

[jeisenbe]

I thought about that but didn't know how to do it. :-)

I've pushed a new commit using COALESCE(way_area/NULLIF(POW(!scale_denominator!*0.001*0.28,2),0), 0) AS way_pixels and switched back to way_area IS NULL

[jeisenbe]

@imagico or @sommerluk do you have time to review this before Friday?

[imagico]

It works fine.