gravitystorm / openstreetmap-carto

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

Render barrier=jersey_barrier #4923

Closed dch0ph closed 8 months ago

dch0ph commented 8 months ago

Fixes #4854

Changes proposed in this pull request: barrier=jersey_barrier on node is rendered as barrier=block/log . barrier=jersey_barrier on way is rendered as default barrier, i.e. same as wall, fence.

There doesn't seem any value in introducing a distinct signature for jersey barrier as a way; the point is that the blocks form a continuous barrier. Although personally there doesn't seem much point in even distinguishing a jersey barrier used as a block, from an "ordinary" block, there would be even less point in rendering them differently.

Test rendering with links to the example places:

Node

Current image

After image

Way

image

image

(Example of barrier between carriageways is not the prettiest, but nearest to hand and makes the point)

imagico commented 8 months ago

Thanks for the pull request.

I agree rendering this with the generic line signature for linear barriers is the best approach. If we should decide to differentiate barrier rendering by type of barrier we should first do this for the main linear barrier classes (fence/wall/retaining_wall).

I am not sure if rendering nodes with barrier=jersey_barrier is a good idea. This mapping concept is ambiguous in meaning - it is not clear if a node with that tag on a road physically blocks the road for all modes of transport (like a barrier=fence/barrier=wall across the road) or if it just prevents passage of larger vehicles (like barrier=block) or if it just constrains passage so it can easily be blocked completely with a single parked vehicle.

As a general note: Please always post examples in native resolution.

dch0ph commented 8 months ago

I'm inclined to agree about nodes with barrier=jersey_barrier. The wiki implies its use on a node corresponds to access=no, effectively a line of blocks across a road, and so it is arguably misleading to render in the same way as barrier=block. But I suspect that many mappers take the view, as implied in the original issue from @ppete2, that barrier=jersey_barrier on a node corresponds to a single block preventing just vehicles.

Personally I err to a more maximalist view of "showing something for the user", but I appreciate that OSMCarto takes a more conservative "only show what is well defined" line.

I'll leave things for comment for a week or so.

As a general note: Please always post examples in native resolution.

Apologies. My test render chain only outputs PDF via nic4.py. But I doubt there's any ambiguity over whether the PR works.

P.S. Although if we are not rendering barrier=jersey_barrier on nodes on the basis that it is poorly defined, we should surely stop rendering the dumb tag barrier=log!

imagico commented 8 months ago

I'll leave things for comment for a week or so.

Yes, input on the matter of if use of barrier=jersey_barrier is semantically well defined in practical use would be welcome.

But I doubt there's any ambiguity over whether the PR works.

The purpose of examples is not primarily to serve as a test for correct operation, it is to allow people following this issue tracker to quickly get an idea how a change will look like in the map.

For this change this is not a big deal because evidently it will look like the other barrier=* features already rendered. But as a general principle it is good to have a native resolution test.

P.S. Although if we are not rendering barrier=jersey_barrier on nodes on the basis that it is poorly defined, we should surely stop rendering the dumb tag barrier=log!

That is a separate issue - but i would not call barrier=log a dumb tag. The problem is more if it is well enough established and consistently used for what we render it as. barrier=log rendering was added in #3054 at a time when we had no consensus requirement on changes. There is the competing tag obstacle=fallen_tree - which is semantically much more clearly defined but less widespread in use.

dch0ph commented 8 months ago

In the absence of further comments, I've removed the node rendering, leaving a fairly trivial change to render a way as a default barrier.

Can you squash commits as part of the merge? I couldn't work out how to do this easily.