Use ST_PointOnSurface for junctions

[jeisenbe]

Related to #3201

Changes proposed in this pull request:

• Use ST_PointOnSurface for junctions layer in project.mml

Test rendering:

Before

After

This PR will need to be updated after #3915 is merged

[matkoniecz]

Sadly, this now has merge conflicts.

[jeisenbe]

I've rebased the PR and fixed the merge conflicts. The rendering examples above are unchanged.

(I wish PRs with merge conflicts would be marked on the list at https://github.com/gravitystorm/openstreetmap-carto/pulls - there isn't any way we can show this, is there?)

[jeisenbe]

Anyone available to review this?

[jeisenbe]

@pnorman - can you review this?

[pnorman]

Looks good, but I haven't run it.

[jeisenbe]

Thank you. Is that enough of a review to merge this PR?

[imagico]

I think the SQL code should have the form as demonstrated by

https://github.com/gravitystorm/openstreetmap-carto/blob/master/project.mml#L354-L371

to make it clear if the WHERE condition applies to the whole query or just to the second component of the UNION ALL.

Apart from that the change itself seems fine - though i would suggest to consider dropping the rendering of polygons with junction=yes - there are only four percent of these with a name tag (1300) compared to 56 percent (19000) for nodes. Incentivizing tagging named junctions on polygons instead of nodes is bad for all data users and against established mapping practice and counterproductive for consistent mapping.

[imagico]

Sorry - that was meant to be a 'request changes' review.

[pnorman]

way && !bbox! needs to be in both WHERE clauses.

When the bbox token appears in the query, Mapnik no longer adds it to the outer wrapping query, so with the current code it wouldn't have an index to use for the planet_osm_point part

[bhousel]

Apart from that the change itself seems fine - though i would suggest to consider dropping the rendering of polygons with junction=yes - there are only four percent of these with a name tag (1300) compared to 56 percent (19000) for nodes. Incentivizing tagging named junctions on polygons instead of nodes is bad for all data users and against established mapping practice and counterproductive for consistent mapping.

From what I understand, named junctions are somewhat important in Japan and Korea and an established mapping practice.

Their use is documented pretty well on the junction=yes page and we've been supporting them in iD for over 5 years (see openstreetmap/iD#2484).

[imagico]

I am not proposing to stop rendering names on junctions, i am proposing to drop doing this for junction polygons which are not consistently used to map something specific and where there is clearly no agreement if they are supposed to carry a name tag or not.

The wiki in this case quite accurately describes the current use:

Therefore the explicit tagging is only necessary if the node has further properties, e.g. a name=* for named junctions in some countries.

The tag is also used on highway areas that need to be declared to be junctions.

So use of junction nodes to map named junctions is well defined and consistently used. The tagging of polygons with junction=yes however is mostly re-purposing the tag for something completely different. It is also cargo culting a supposed 'need' - but that is beside the point here.

94 percent of the junction polygons have an area:highway tag indicating the junction=yes is a secondary tag here as explained on the wiki. One of the birth errors of area:highway tagging (which i pointed out several years ago already) is that it tacks between the idea to duplicate semantic tags like names from linear highways and not doing so. Despite that mappers have with their mapping practice clearly demonstrated that they don't want to duplicate names (less than 2 percent of area:highway features have a name tag) so we are here throwing a wrench into the mechanics of consistent tagging development within the community by displaying counterproductive incentives directly working against a tagging trend towards consistency.

[jeisenbe]

consider dropping the rendering of polygons with junction=yes ... there are only four percent of these with a name tag (1300) compared to 56 percent (19000) for nodes

I agree.

Would you mind opening an issue about this, so it's not forgotten?

[jeisenbe]

I've pushed a new commit with the recommended changes. I hope I got the parenthesis right.

[bhousel]

So use of junction nodes to map named junctions is well defined and consistently used. The tagging of polygons with junction=yes however is mostly re-purposing the tag for something completely different. It is also cargo culting a supposed 'need' - but that is beside the point here.

Are we talking about the same thing?

I see this on the wiki right now:

Isn't that encouraging people to do exactly the thing that you are dropping the rendering for?

[bhousel]

cc @sommerluk who seems to be the one who made the proposal, got it approved, updated the wiki, and opened the issue in iD.

[baerbock]

@jeisenbe How does ST_PointOnSurface work differently vs the old rendering?

Here is an example of a named junction=yes in Japan: Is the current rendering more like "before" or like "after"?

[jeisenbe]

ST_PointOnSurface is a different algorithm than the interior placement which we previously used in Mapnik (the rendering software which we use to render this style), but the main advantage is that ST_PointOnSurface is used to calculate a node instide of the polygon, and then this node is sent to Mapnik for rendering. This is often faster and will also work for vector tiles. Sometimes the placement was better with interior but sometimes the new ST_PointOnSurface rendering is better - there is not a clear postive or negative change overall. Both options sometimes create poor labeling points, unfortunately)