Rendering specific access tags

[dch0ph]

Fixes #214

Changes proposed in this pull request:

The code proposed has been extensively discussed in #214, but in summary:

SQL functions introduced that interpret mode-specific access tags in addition to the overall access tag.

Tags considered are determined by a "primary mode" inferred from the highway types: Vehicle road (primary mode: motorcar): motorcar > motor_vehicle > vehicle cycleway: bicycle bridleway: horse footway, steps: foot [Access tagging on track is unchanged, i.e. only determined by access]

In this initial PR, the interpretation of path is unchanged, i.e. path is considered to be a cycleway or bridleway if bicycle=designated or horse=designated respectively.

The access tags are reduced to a single int_access tag tagging the values no, restricted and yes (which is equivalent to NULL). restricted used to indicate an intermediate "some restrictions apply" for vehicles. The access marking used is the the current access=destination, but the name change reflects the fact that other values are included, e.g. delivery.

The int_access is generated, as normal practice for this style, on-the-fly. An option to use a generated column to pre-calculate these values is commented out. In practice, the overhead of combining the access tags is likely to be small given the vast amount of in-line SQL in the roads query. Note that some cruft in the railway side of the roads query has been removed.

Other global access tags, such as access=forestry, are also now interpreted (equivalent to no). access=agriculture;forestry is also accepted, although we don't typically interpret multiple-value tags.

The code does not require a database re-load, but does require additional functions to be installed in the Postgres database. These have been placed into a file functions.sql so that there is a place where functions for the style can be gathered. This does require an additional step in installing the style, and so installation instructions and the CI test have been adjusted. The Docker set-up has not been touched and will need fixing by somebody who understands/uses it.

Test rendering with links to the example places:

Destination marking Residential highway tagged with motor_vehicle=destination.

Before

After

No marking Before access=yes, motor_vehicle=no, psv=yes

After

Honouring foot North-south footway tagged with highway=footway, foot=private

Before

After

Honouring bicycle highway=cycleway, bicycle=designated, access=no Before

After

The last example needs discussion since it is a case where access=no has been added because the bridge is closed:

The logic of access tagging is that the general access=no is overridden by the specific bicycle=designated, and a router should allow bicycles across. So this usage is arguably tagging for the renderer: retaining perhaps a formal right of way (bicycle=designated) but indicating that the way is closed by exploiting Carto's simple approach to access tagging.

It is inevitable that a wider and more correct usage of the access tags will throw up such cases!

[imagico]

This seems flawed, as if you have, for example, access=foo motor_vehicle=bar hgv=baz then the access restriction for hgv is the value of the hgv tag, regardless of if we understand that value or not.

That depends on our strategy of dealing with unknown tags in general.

If we'd show hgv=* in that scenario then we would have to interpret hgv=baz as either of the three access classes we render. We could do that, but that would functionally introduce a kind of catch-all - interpreting all unknown values as synonyms for a specific known value. And i am not sure we'd want that.

The simplest alternative to introducing a catch-all would be to simply ignore unknown values (because we don't know what they mean - hence any interpretation we'd make would be wrong). That is what this PR currently does. I am not saying this is the only viable approach. But it is not clearly wrong either. In case of an unknown tag the reason might be a typo and the typo might be in the key or in the value. If it is in the key the approach of treating unknown tags as fully invalid and void would be the better.

The other alternative would be to introduce an additional visualization class meaning unknown - indicating that an access restriction is tagged for the primary mode of use but it is undefined - and therefore distinctly different from the case with no access restriction. This would go a bit in direction of #4723.

[pnorman]

We have to interpret unknown values regardless. If we have access=baz and it's unknown we still have to render the road one of the three ways.

The other alternative would be to introduce an additional visualization class meaning unknown

I don't think a 4th class is possible without becoming confusing.

[dch0ph]

This seems flawed, as if you have, for example, access=foo motor_vehicle=bar hgv=baz then the access restriction for hgv is the value of the hgv tag, regardless of if we understand that value or not.

I don't follow this specific point, since we don't consider the "access restriction for hgv".

But, I guess the general point is about access=no, motor_vehicle=baz since the two approaches will give different results.

Personally I feel that the outcome of the COALESCE approach will be more obvious to mappers (if motorcar is set, it's value will determine access marking, if not, motor_vehicle etc.)

We have to interpret unknown values regardless.

Good point. Currently there is an asymmetry between the mode-specific tags which are "vetted", and the overall access tag, which is not, primarily because we have to do something with unknown values.

[imagico]

We have to interpret unknown values regardless. If we have access=baz and it's unknown we still have to render the road one of the three ways.

Currently, if a road is tagged access=baz that tag is ignored, it is rendered as if the tag was not there.

So far the style universally ignores unknown tags (i.e. it treats them as if they were not there) - with the exception of the few catch-alls we have in the style (which we try to reduce - see #4568 and #4725).

So far all the catch-alls we deliberately have in the style are cases where an unknown value naturally is to be engrossed in a common design with other known values. Like unknown building=* being rendered like building=yes and unknown shop=* like known shop types without a distinct symbol.

This is different here, motor_vehicle=baz does not naturally fit into either of the three classes we render.

I don't think a 4th class is possible without becoming confusing.

I would agree if that 4th class was indeed another class of access restriction. But it would not be, it would be an error indicator. It would not transport information about the geographic reality, it would indicate that there is an error in the data that prevents us from reliably providing such information even though the other data clearly indicates that such information should be shown.

I had been sceptical about #4723 in general (and i still am) - but this specific constellation where there is no good solution other than explicitly showing cases where there is an error in the data is one where i would be in favor of an explicit error indicator.

[pnorman]

I don't think a 4th class is possible without becoming confusing.

I would agree if that 4th class was indeed another class of access restriction

If you don't like the word class, call them a distinct symbology. I don't think we can have four given the number of other things we render on roads.

Currently, if a road is tagged access=baz that tag is ignored, it is rendered as if the tag was not there.

So far the style universally ignores unknown tags (i.e. it treats them as if they were not there) - with the exception of the few catch-alls we have in the style (which we try to reduce - see #4568 and #4725).

A catch-all is when we're rendering all features regardless of what they are. That's what we've tried to reduce.

Deciding what to do with unknown access values is, on the other hand, something we must do if we render any. If for each road we render it with three different symbologies to represent different access restrictions, we must then have some way to decide which of the three to use given the access related tags. This includes when the access tags make no sense to us - unless we completely stop rendering the road, we still have to decide which of the three.

[imagico]

A catch-all is when we're rendering all features regardless of what they are. That's what we've tried to reduce.

4568 and #4725 are cases of catch-alls in secondary tags where any and all values with a certain key other than a few explicitly handled otherwise have a distinct effect on rendering compared to no such tag being present.

Our goal to support mappers in consistent mapping practice IMO mandates us to reduce these because it is not only ill-defined primary (feature) tags being rendered without there being a consistent mapping practice in use that works against this goal, the same applies for secondary tags.

Deciding what to do with unknown access values is, on the other hand, something we must do if we render any.

As already said - our standing practice universally implemented in this style except for the few catch-alls we have is to ignore them.

If we are considering changing that we should look at it from the perspective of our goals, specifically here (a) the goal to support mappers in consistent mapping practice and (b) the goal for our design being intuitively understandable for mappers. Looking at the suggestions we have so far:

1. ignoring unknown access values (this is what this PR implements at the moment)

   • support of mappers in consistent mapping practice is good because only tags that we know are applied consistently go into the rendering - hence there is no counterproductive positive feedback on ill-defined tags.
   • intuitive readability is non-ideal because the tag interpretation breaks with the strict logic of access tag overrides in case of unknown values. It is, however, clearly not worse than the status quo where we treat all more specific access restrictions as unknown (i.e. completely ignore the hierarchical structure of the access tagging system).

2. interpreting unknown values universally as access=yes

   • support of mappers in consistent mapping practice is not ideal because the mapper get positive feedback in the form of a distinct effect on rendering results of undefined tags, potentially leading to proliferation of access values for only weakly restricted access (because those receive the semantically fitting positive feedback)
   • intuitive readability is good because it follows the consensus logic of acccess tagging strictly. It is, however, confusing in case of access values unknown to us that are either evidently non-sensical (why is this access=no road shown as open? Ah, because of the motorcar=who_cares), quite clearly severely restricted (from the list of somewhat common values on taginfo for example bus, emergency, restricted, discouraged) or explicitly unknown (like access=no + motorcar=unknown/motorcar=FIXME)

3. an explicit error indicator signature for unknown values

   • support of mappers in consistent mapping practice is good
   • intuitive readability is good if a suitable signature can be developed

Personally i am confident that an intuitively readable design could be found for the third option (under the paradigm of being an explicit error indicator and not simply a fourth class in addition to the three we have). But developing such is delicate design work and this PR was started under the premise of changing tag interpretation only and not the actual style design. So i am hesitant to suggest to @dch0ph to work on this.

Everyone should also keep in mind that, compared to other tags, completely undefined values with no consensus meaning are really rare in access tagging:

https://taginfo.openstreetmap.org/keys/motor_vehicle#values https://taginfo.openstreetmap.org/keys/motorcar#values

For both of these the explicit unknown value is the most common one that we could not reasonably put into one of the three classes we have based on established mapping practice. These observations IMO also favors option one and three.

[dch0ph]

I would suggest the following for this PR:

Switch to the COALESECE approach so that unknown (=invalid + non-interpreted) tags are not ignored, but reach the "interpretation" level (the carto_int_access function).

Return int_acess=unknown (rather than NULL) for unknown tags. Alternatively UNKNOWN to indicate that this is a value generated by Carto rather than a user tag (access=unknown) [but see below].

It is then devolved to the MSS whether to show a render for unknown/UNKNOWN access. [In the current PR/MSS, unknown values would be ignored as only int_access=restricted or int_access=no are associated with access rendering.]

For both of these the explicit unknown value is the most common one that we could not reasonably put into one of the three classes we have based on established mapping practice.

My personal preference would be to strip out "access"=unknown in Lua as junk tagging, but the easy option would to return int_access=unknown for an unknown access tag. This would automatically include cases where unknown has been used.

[imagico]

I would strongly suggest first to develop consensus on the desired tag interpretation and then think about implementation details. Choosing a certain tag interpretation because it is more convenient to implement would be a bad idea.

In principle a mixture between approach 1 and 2 would be possible in the form that the explicit unknown value is ignored (i.e. access=no + motor_vehicle=unknown would yield no) while other values with undefined meaning are treated according to Paul's suggestion (i.e. something like access=no + motor_vehicle=restricted would yield yes).

[dch0ph]

In principle a mixture between approach 1 and 2 would be possible in the form that the explicit unknown value is ignored (i.e. access=no + motor_vehicle=unknown would yield no) while other values with undefined meaning are treated according to Paul's suggestion (i.e. something like access=no + motor_vehicle=restricted would yield yes).

That would work for me. Rather than WHEN motorcar IN ('yes', 'designated', 'permissive', 'no', 'private', 'destination', 'customers', 'delivery', 'permit') THEN motorcar we would have WHEN motorcar <> 'unknown' THEN motorcar

This would be functionally equivalent to a simple COALESCE, but ignoring unknown on the mode specific tags.

There is the issue of what to do with access=unknown. It could yield int_access=unknown (along with other uninterpreted tags, such as motorcar=baz). Alternatively we could distinguish between these outcomes.

[pnorman]

interpreting unknown values universally as access=yes (Paul's suggestion)

This is not what I suggested. I made no suggestion as to what a way with unknown access for the primary method is.

There is some justification for treating unknown differently, as it's an explicit unknown as opposed to a value we don't understand. access=no motor_vehicle=unknown truly indicates an unknown value, as opposed to access=no motor_vehicle=foo indicates a value we don't understand. Falling back to the access=no is wrong in the second case because the access tag doesn't tell us anything about what the mapper specified for motor_vehicle access.

3. an explicit error indicator signature for unknown values

You're proposing not unknown values, but invalid ones. Do we do this elsewhere? In the past we have rejected becoming a QA layer as it is clearly incompatible with 3/4 purposes. What we do with the tracktype is different - we have a symbology we show for unknown values where no tracktype is supplied, but we're not aiming for a different symbology for values that we believe are errors.

• support of mappers in consistent mapping practice is good because only tags that we know are applied consistently go into the rendering - hence there is no counterproductive positive feedback on ill-defined tags.

There still is. If someone tags highway=residential access= they get the feedback that their tag worked, because it still displays as having access.

[imagico]

This is not what I suggested. I made no suggestion as to what a way with unknown access for the primary method is.

Ok, i removed the attribution to you for the approach 2 in my list. But since my aim here is to develop consensus on a concrete approach to access rendering this does not really bring us forward. It would be helpful if you'd indicate which concrete approach you'd favor and which approaches you would find acceptable. So far we have discussed approaches 1-3 from https://github.com/gravitystorm/openstreetmap-carto/pull/4952#issuecomment-2067980951 and the mixture between 1 and 2 with explicit unknown being ignored while other unknown/invalid values are treated according to approach 2. If you have a different concrete suggestion please say so.

There is some justification for treating unknown differently, as it's an explicit unknown as opposed to a value we don't understand.

This is exactly why i brought up the mixture approach as a fourth option.

You're proposing not unknown values, but invalid ones. Do we do this elsewhere?

No, as said, this would be a first in this style - but so is the combined interpretation of several tags in this form that leads to this problem in the first place.

The tracktype case is different because we have no implicit default there (while we have an implicit default of access=yes on roads). And we have no complex combined interpretation of several tags, hence ignoring invalid tracktype values and treating them as explicit unknown is functionally the same (in other words: approach 1 and 3 would be functionally identical).

• support of mappers in consistent mapping practice is good because only tags that we know are applied consistently go into the rendering - hence there is no counterproductive positive feedback on ill-defined tags.

There still is. If someone tags highway=residential access= they get the feedback that their tag worked, because it still displays as having access.

No, because adding (for example) access=yes_except_for_tanks tag does not change the map rendering compared to not having the access=yes_except_for_tanks tag - hence no positive feedback. While in approach 2 tagging access=no + motor_vehicle=yes_except_for_tanks would change the rendering compared to just access=no. Hence the mapper adding motor_vehicle=yes_except_for_tanks would get positive feedback on their change.

[imagico]

@pnorman - reacting with a :-1: to my comment but not explaining what you disagree with is decidedly non-helpful. If there is anything unclear about the list of options i sketched please ask. If i misunderstood your comments and you think you have made a concrete suggestion different from the ones i listed please explain. If you disagree with my approach to developing consensus on this matter overall please take the initiative and explain what concrete solutions you deem viable.

[dch0ph]

To simplify further discussion (but not to prejudge consensus on tag interpretation), I've committed a bunch of changes that emerged from comments to date.

1. Clarified the purpose of the functions by returning an effective int_highway, a consolidated highway indicator which determines how access will be handled, e.g. all road types consolidated under road.

2. Changed logic of "coalescing" mode-specific tags so that only motorcar=unknown etc. are ignored. carto_int_access determines how the final coalesced tag is to be interpreted, with unknown values (including from access=unknown) returning 'unknown', potentially allowing this error result to be rendered.

3. The "promotion" of highway=path depending on bicycle and horse is now handled in SQL by returning e.g. feature=highway_cycleway for highway=path, bicycle=designated. This puts all the access handling in SQL, and the horse, bicycle (and unused foot) tags don't need to be passed into MSS. highway=path is still distinguished from highway=footway in SQL, even though the rendering in MSS is the same. The simplification of the MSS removes 2k lines of XML (4%).

I have checked that update code works as expected for the previous test cases, but have not exhaustively considered the various edge cases discussed. Ideally this would involve a "test sheet" or a shareable demo server.

I noticed that the roads query seems to include some cruft on the railway branch, e.g. evaluation of an int_surface! This could be a separate PR alongside other follow-up PRs.

[imagico]

From a quick look this seems like a structure that can be adjusted to any of the options in tag interpretation discussed so far so works well in terms of our goal of adaptability. Same applies to adjusting the tag re-casting of highway=path into other types (which can be done by modifying the carto_path_type() function).

You have not yet made any changed to the pedestrian logic - you still universally have pedestrian roads as no. Considering we don't show access restrictions on pedestrian roads at the moment this might seem a reasonable shortcut - I would still suggest treating them like footways in terms of primary mode and like roads in terms of having a separate restricted class. This would allow customization (or reuse of the SQL code in other styles) where restrictions on pedestrian roads are shown.

[dch0ph]

Thank for you the encouraging comments.

I'll admit that limited thought went into the pedestrian case; the automatic no access marking aimed at providing a "base" marking which would be coloured for permitted traffic, e.g. psv=yes.

I'm not convinced that a primary foot mode works here. They are basically vehicle roads with signage to restrict vehicles in much the same way as ordinary roads. I don't see how you would interpret/use the foot tag.

I would counter with something like:

  CASE
      WHEN motorcar <> 'unknown' THEN motorcar
      WHEN motor_vehicle <> 'unknown' THEN motor_vehicle
      WHEN vehicle <> 'unknown' THEN vehicle
      ELSE COALESCE("access", 'no') END

i.e. use the vehicle tags as for roads, e.g. motorcar=no, motorcar=destination, but default to no if there is no access tagging.

It might be interesting (separately) to develop a marking for restricted access on pedestrian ways.

If the outcome were int_access=yes (e.g. access=yes or motorcar=yes) + psv=yes, then there would be no psv allowed marking. But this is probably the Right Thing, since this would be fairly nonsense tagging for a pedestrian way.

[imagico]

I'm not convinced that a primary foot mode works here. They are basically vehicle roads with signage to restrict vehicles in much the same way as ordinary roads. I don't see how you would interpret/use the foot tag.

No, that is a wrong understanding of highway=pedestrian i think. These are footways that are wide enough to be used with a normal car but where cars are not allowed (and which therefore have an implicit motor_vehicle=no - or vehicle=no - depending on local conventions). Signage is not a requirement for highway=pedestrian - many are simply physically impossible to be used with a car.

foot=yes would be redundant on a highway=pedestrian and foot=no would be troll tagging. But many other values can be sensible in certain situations. foot=private on highway=pedestrian in a private park i already mentioned. Of course you could equally use access=private in such cases (and this is more common in practical use) - but foot=private is equally valid. foot=designated is also meaningful tagging (and should yield yes when used in combination with an explicit access=no)

[dch0ph]

No, that is a wrong understanding of highway=pedestrian i think. These are footways that are wide enough to be used with a normal car but where cars are not allowed (and which therefore have an implicit motor_vehicle=no - or vehicle=no - depending on local conventions). Signage is not a requirement for highway=pedestrian - many are simply physically impossible to be used with a car.

Agree that signage isn't necessary. I suspect that usage varies a lot between mappers (not least based on the wiki examples). But personally I only use highway=pedestrian where vehicle usage is possible (i.e. part of the road network, not separated by kerbs), e.g. a shopping centre/mall route would be highway=footway, but clearly some mappers would use highway=pedestrian for this.

From a quick scan on taginfo (743k uses overall) and Overpass:

Most access tagging is for bicycle (e.g. 62k bicycle=yes). Separate (but interesting) issue.

foot=yes has 24k. This is mostly fairly redundant highway=pedestrian, foot=yes.

access=private has 7k uses. This seems mostly on pedestrian areas (area=yes) on private grounds (which would be off topic).

There's a bunch of vehicle-related tags: vehicle=no (5k), motor_vehicle=private (4k), motor_vehicle=destination (4k).

But also motor_vehicle=yes (!). These mostly seem to correspond to "shared use" ways in public areas, where perhaps living_street is more appropriate.

access=no (5k): These mostly seem to be poor tagging, where a mode specific tag should be used (highway=pedestrian, access=no). But also includes examples on military land (although obvious from military hatching pattern).

foot=private (4k): on private land.

So there arguments for both routes:

Treating as a footway, and considering foot and access, in order to distinguish private ways. Personally I feel this is just less "interesting", and would need a careful choice of rendering that was different from vehicle access.

OR

Focussing on vehicle access. That would immediately allow things like: for no general vehicle access, but psv allowed.

It would be useful to distinguish between motor_vehicle=no and motor_vehicle=destination. You could then add a render for "vehicle=restricted" (e.g. invert the current destination marking to white circles?).

In principle you could support both, with an additional int_access result, e.g. int_access=footno. Given the multiple uses of highway=pedestrian this might be a reasonable (if inelegant) option.

[imagico]

I think this is going too far outside the topic of this PR and #214. As indicated at the moment we are not rendering access restrictions on highway=pedestrian. We are discussing what would be a sensible default for the tag interpretation functions we introduce here for this road type based on the paradigm of visualizing restrictions for the primary mode of transport. I don't see a basis for considering the primary mode of transport for highway=pedestrian to be something other than foot as far as these functions are concerns.

As far as a hypothetical rendering of exceptions from implicit access restrictions (like on psv=yes on highway=pedestrian) is concerned - i mentioned the possibilities to do that here. The idea you sketch would mean stopping to render road types with implicit access restrictions as a distinct road classes and not rendering access restrictions based on primary mode of transport but based on a universally assumed motorcar default. That would mean all road types with implicit restrictions for motorcars would universally get a restriction dashing - which, as mentioned in the linked to blog post, would make larger pedestrian areas in urban centers very noisy (because they are all disallowed for motorcar).

But again - this is a discussion we should not have here. If and when the topic practically comes up the proper way to decide this would be to test how this would look like in practical rendering on a larger scale.

[dch0ph]

OK. In the interests of keeping things moving I've adjusted the access interpretation of highway=pedestrian:

WHEN 'pedestrian' THEN carto_int_access('pedestrian', CASE WHEN foot <> 'unknown' THEN foot ELSE "access" END)

i.e. using foot (as footway), but keeping the 3-state "rendering" associated with roads. Hence foot=delivery will result in int_access=restricted, although this will have no effect since there is no access marking on highway=pedestrian

[dch0ph]

That's strange. I've been running the code on my own setup for some while, but the bridleway typo (now fixed) had slipped through.

The function load works in the github CI:

This step does need to be 100% reliable. I hope it's not a postgres version issue.

[imagico]

Well - independent of if you universally get an error for that, the END after the SELECT does not make sense.

[dch0ph]

Well - independent of if you universally get an error for that, the END after the SELECT does not make sense.

Fixed. Hopefully functions.sql will work properly on all postgres setups now.

[pnorman]

@pnorman - reacting with a 👎 to my comment but not explaining what you disagree with is decidedly non-helpful. If there is anything unclear about the list of options i sketched please ask.

If i misunderstood your comments and you think you have made a concrete suggestion different from the ones i listed please explain. If you disagree with my approach to developing consensus on this matter overall please take the initiative and explain what concrete solutions you deem viable.

We disagree on if there is no feedback or not. Having re-read what you said, I still conclude that we're offering some feedback no matter how we do this.

I still believe the correct logic is coalescing the values and comparing the result. Additionally, no consideration has been given to the users of our code and what the added complexity will be like.

[dch0ph]

I still believe the correct logic is coalescing the values and comparing the result.

That's essentially what we have now. All the "combination" is now of the form:

CASE WHEN foot <> 'unknown' THEN foot ELSE "access" END

which is functionally coalescing foot and access, but ignoring tags like foot=unknown

no consideration has been given to the users of our code and what the added complexity will be like.

Users wanting just to install OSMCarto, developers of OSMCarto, or both?

[imagico]

We disagree on if there is no feedback or not. Having re-read what you said, I still conclude that we're offering some feedback no matter how we do this.

There is no disagreement on that - we provide some feedback even by ignoring certain tags - that is what #214 is largely about. My point is that there is a fundamental difference in the nature of the feedback between a rendering logic where a tag is ignored (i.e. its presence leads to the same result as its absence) and one where the presence of a tag affects the rendering result compared to its absence.

I still believe the correct logic is coalescing the values and comparing the result.

My understanding is that if you combine that with the current MSS logic the result will be approach 2 in https://github.com/gravitystorm/openstreetmap-carto/pull/4952#issuecomment-2067980951.

I explained above why - despite acknowledging the strict adherence to the formal logic of access tagging as a relevant argument - i think other methods would be preferable in the overall look at our goals. But i am open to arguments that suggest a different conclusion.

Additionally, no consideration has been given to the users of our code and what the added complexity will be like.

I suppose by users of the code you mean style deployments (in particular OSMF operations). It is not true that no consideration was given to those - on the contrary, i explicitly mentioned that we should look at the effect this has on query performance and that we need to keep in mind how future changes to the SQL functions we add can be correctly managed (in https://github.com/gravitystorm/openstreetmap-carto/pull/4952#pullrequestreview-2000181884). If you have additional considerations in that regard that should be taken into account i would very much like to hear them.

I also like to point out that - as far as i remember, and with the exception of unforeseen massive performance regressions (which we usually reverted in a follow-up release right away) - we have never received any sustained complaints from style users about issues related to the technical complexity of our style. OSM-Carto is - despite a relatively large body of code - structurally relatively simple compared to many other styles. And @dch0ph even pointed out above that this change might lead to a substantial reduction in the mapnik xml code volume as a result of moving the path to footway/cycleway/bridleway logic to SQL.

[imagico]

Ok, i have tested this a bit more.

At the moment this implements an approach 2+ according to the discussed options, i.e. interpreting unknown values universally as access=yes - except for the explicit unknown - which is ignored.

How this looks like can be seen here.

I also implemented the other variants discussed for comparison:

• approach 1 ignoring unknown access values. Rendering sample

• approach 2 interpreting unknown values universally as access=yes. Rendering sample

• approach 3 and explicit line signature to indicate unknown value. Rendering sample

The design demonstrated is used only at z18+ and only on the roads wide enough for a readable rendering (i.e. not on minor service roads). I also tested another option that is essentially an approach 3+, i.e. it ignores the explicit unknown in a similar way as this PR currently does. In addition at z<18 where the explicit unknown line signature is not shown it displays the fallback value (as far as this is tagged).

• approach 3+ and explicit line signature to indicate unknown value while ignoring explicit unknown and showing fallback at z<18. Rendering sample

[dch0ph]

Thanks for showing these contact sheets. They highlight a reversion on highway=path (as footway) which should be the same as highway=path. I was originally "dissolving" highway=path into cycleway/bridleway/footway, but worried that it was piling too much change into one PR.

So it retains the current logic of only "promoting" to cycleway/bridleway. But I need to add the access logic for highway=path.

You would only save ~1% of lines of XML from removing path from the MSS, and conceivably someone might want to render path and footway differently.

I'll stay out of commenting on the different options!

[pnorman]

I suppose by users of the code you mean style deployments (in particular OSMF operations).

No, I'm thinking mainly of switch2osm level users. Sophisticated users like the OSMF, Geofabrik, etc won't have a problem with functions although they may find them annoying.

[imagico]

No, I'm thinking mainly of switch2osm level users. Sophisticated users like the OSMF, Geofabrik, etc won't have a problem with functions although they may find them annoying.

Do you have any information, like in the form of concrete feedback, supporting the hypothesis that installing some functions from a static sql file poses an additional hurdle to people with limited experience who try to set up a tile server compared to just generating the indices. Specifically on

https://switch2osm.org/serving-tiles/manually-building-a-tile-server-ubuntu-24-04-lts/

you currently have the line

sudo -u _renderd psql -d gis -f indexes.sql

and you would need to add to that an additional line of exactly the same form:

sudo -u _renderd psql -d gis -f functions.sql

which - in contrast to the first - does not take any substantial additional disk space and does not need any substantial time to run.

Even if - for the sake of the argument - i assume for the moment that installing a few functions will slightly increase deployment complexity and therefore slightly increase the difficulty level for some people who try to install the style, the benefits of using functions for our other goals would by far outweigh this. Or in other words: If we'd forgo the possibility to use custom SQL functions in our queries for reason of not even marginally increasing the deployment complexity we'd elevate a small part of one of our goals (not too difficult [...] to set up a tile server for this style) above the rest of this goal (should be easy to customize) and all the other goals. We have already seen during development of this PR how much easier it is to adjust the rendering logic for access restrictions if this is implemented in a function rather than duplicated multiple times across the different road layers.

I also like to point out that in the past when we made changes that much more massively affected the ease of deploying the style (#1540, #4606) the question of this creating additional difficulties for people with limited experience who try to set up a tile server did not play a role. This might be because this is a problem that became only known more recently - but then again: I would like to see some concrete indicators that this is actually a real problem for something as simple as installing a few functions in addition to the indices we already have.

Finally: If ease of deployment is a serious concern for the switch2osm initiative it might be worth considering to have a more minimal style with as little installation complexity and external dependencies as possible as the basis of the beginners tutorials there. OSM-Carto tries to create a rich map that is understandable and supportive for mappers and that represents the diversity of the OSM community and geography in general. That in combination inevitably leads to a substantial design and technical complexity as well as resource requirements.

[dch0ph]

Some quick comments.

I do like the idea of a "debug view" at high zoom that highlights problem tagging ("approach 3+"). But hopefully this step will generate some feedback and more community input on future directions.

As noted at the start: The Docker set-up has not been touched and will need fixing by somebody who understands/uses it. So this shouldn't be pulled as-is to the main branch.

On a related topic, the plan is for a follow-up PR that "promotes" highway=path to cycleway/bridleway on the basis of relevant yes|permissive (rather than just designated). We agreed that this change of behaviour (which would bring more consistency into tag interpretation) should be a separate PR. But, from a user point of view, it might well be irritating to change the access code/functions twice.

[imagico]

I do like the idea of a "debug view" at high zoom that highlights problem tagging ("approach 3+"). But hopefully this step will generate some feedback and more community input on future directions.

Note this is not intended as a debug view in the sense of the primary purpose of the design being to highlight errors to the mappers but as a visualization of cases where data does not allow us to make a proper decision between yes, no and restricted so either of these would be factually incorrect and misleading. Limiting this to z18+ is a pragmatic way to ensure that the design is discernible from the others and intuitively understandable for all road types.

The idea of providing more explicit visualization of errors or missing information in the data is discussed in #4723.

As noted at the start: The Docker set-up has not been touched and will need fixing by somebody who understands/uses it. So this shouldn't be pulled as-is to the main branch.

I have no knowledge of docker so this is not something i can help with. It would be good if someone familiar with it could provide help here but i also don't think this should ultimately block this PR.

On a related topic, the plan is for a follow-up PR that "promotes" highway=path to cycleway/bridleway on the basis of relevant yes|permissive (rather than just designated). We agreed that this change of behaviour (which would bring more consistency into tag interpretation) should be a separate PR. But, from a user point of view, it might well be irritating to change the access code/functions twice.

There are multiple potential follow-up changes - making the re-casting of path to cycleway/bridleway less confusing is one of them, another is solving #214 also for highway=track in some form (potentially together with #4321). But first we need to achieve consensus on this.

[imagico]

@pnorman - please have another look at this after the changes made based on the discussion.

[imagico]

I agree in-lining functions where this is possible without duplicating logic extensively is a good idea since it makes the logic easier to understand. The reason why i did not suggest this before was that where this is possible depends on the approach in tag interpretation logic we use and we have not come to a definitive agreement on that yet.

[dch0ph]

Seeing the changes to the MML I'm not 100% opposed to functions that would reduce complexity. In their current iteration, these do not reduce complexity, they just reduce duplication.

Some complexity is inevitable since we are combining a bunch more tags, and simultaneously "resolving" highway=path. The functions would be simpler if highway=path was being "dealt with" elsewhere. But the options here are unattractive: introducing another SELECT layer in SQL, or fixing things (too?) early in Lua.

Should we preprocess the access values in some way through the lua? We could normalize values and handle unknown ones more easily at that point.

It would be logical / efficient to remove the unknown troll tag at import.

It would make sense to do some normalisation at import, e.g. the code currently interprets agricultural;forestry, but not (the extremely rarely used, but valid) forestry;agricultural. But does it make sense to add Lua code for extremely rare usages? Or instead declare that OSMCarto will only interpret X;Y when alphabetically ordered?

Otherwise I don't think much is gained, e.g. by merging designated into yes. It helps if access tags are being interpreted in MSS, but not so much in SQL.

[imagico]

I would suggest to keep the strategic discussion if to do tag interpretation during data import out of this PR. That is a matter where it will be hard to achieve consensus among the maintainers on and it is a matter that should be discussed not exclusively based on the specific problem of access restriction rendering but based on a broader look at how we currently interpret tags and what new types of tag interpretation we might want to introduce in the future.

It would be logical / efficient to remove the unknown troll tag at import.

To be clear: I do not consider access tags with value unknown a troll tag. It is simply a way of mappers documenting that there is an explicit lack of clarity on the ground if a certain mode of transport is permissible or not. That this makes tag interpretation somewhat more difficult for us is not a reason why mappers should not use this kind of tagging.

Regarding agricultural;forestry and forestry;agricultural - we decide that based on practice of use. Right now the latter is very rare, more common is combinations of agricultural, forestry and destination in some form. But this is not really a matter inherently connected to this PR (which is about interpreting additional access keys, not values). If and when we have merged this change we will need to look at highway=track access restriction rendering and then we can discuss this matter more in depth. It will also be significant to look at how routing engines interpret this kind of tagging of course.

[pnorman]

I agree in-lining functions where this is possible without duplicating logic extensively is a good idea since it makes the logic easier to understand. The reason why i did not suggest this before was that where this is possible depends on the approach in tag interpretation logic we use and we have not come to a definitive agreement on that yet.

I can't really comment on the logic since I can't figure it out

[imagico]

I can't really comment on the logic since I can't figure it out

You mean the one currently implemented by this PR or any of the other approaches discussed?

[dch0ph]

I've committed an updated version which removes the intermediate int_highway (along with a function). This made more sense in the original implementation which used PL/pgSQL functions (but which was felt to be a step too far).

There is some minor code/logic duplication as a result, but overall the code should be easier to follow.

I feel it is still useful for clarity / ease of maintenance to retain carto_int_access as a separate function, rather than further in-lining.

The internal function carto_int_access has changed signature. This shouldn't cause problems even if reloading functions.sql to an existing database, but DROP FUNCTION carto_int_access(text, text) would remove the old version if necessary.

The merge conflicts with the changed railway code have been resolved.

I've deleted the commented-out code for adding a generated column as this doesn't seem useful.

P.S. I retract my comment above about unknown being troll tagging. It would have been better described as a non-rendered tag. We do remove a lot of these at the import stage already.

[imagico]

Have not tested but looks good at a first glance. I think it would be good if you'd

• unify the indention of the CASE statements (the variant used in carto_path_type() is the most readable IMO)
• consider using COALESCE(NULLIF(tag, 'unknown'), ...) instead of CASE WHEN tag <> 'unknown' THEN tag ... END. I think this is what @pnorman prefers, i am undecided on the matter (the CASE version might be easier to understand for people unfamiliar with SQL but significantly less compact).

P.S. I retract my comment above about unknown being troll tagging. It would have been better described as a non-rendered tag. We do remove a lot of these at the import stage already.

We currently remove only metadata tags that do not contain information about the geography.

[pnorman]

• consider using COALESCE(NULLIF(tag, 'unknown'), ...) instead of CASE WHEN tag <> 'unknown' THEN tag ... END. I think this is what @pnorman prefers, i am undecided on the matter (the CASE version might be easier to understand for people unfamiliar with SQL but significantly less compact).

I'd rather use nullif because it's the standard way to do this logic and easier to mentally see what it's trying to do. As a less preferred option I'd use CASE WHEN NULLIF(tag, 'unknown') THEN ... END

I'll have a look at the code later.

[imagico]

I wanted to evaluate if the other approaches to access tag interpretation can equally be implemented using COALESCE()/NULLIF() so i implemented approach 3+ (explained in detail in https://github.com/gravitystorm/openstreetmap-carto/pull/4952#pullrequestreview-2052641197) using this technique:

https://github.com/imagico/osm-carto-alternative-colors/blob/carto-new-access-approach3%2B/functions.sql

You essentially have to do the carto_int_access() before the NULLIF() to get the desired logic of ignoring all unknown values (instead of just ignoring the explicit unknown value). With the difference between z<18 and z>=18 and the undefined line signature this of course gets a bit more complicated overall.

For comparison the current logic of this PR (which is approach 2+ in that terminology) implemented using COALESCE()/NULLIF():

https://github.com/imagico/osm-carto-alternative-colors/blob/carto-new-access-approach2%2B/functions.sql

[dch0ph]

Requested formatting changes and switch to COALESCE / NULLIF implemented.

[imagico]

When we are restricting a key value to a small number of values with only 3 meanings this is a case that screams out for preprocessing.

Let us not have the strategic discussion how to best do tag interpretation on a PR.

[pnorman]

Let us not have the strategic discussion how to best do tag interpretation on a PR.

This is not a discussion on how to best interpret access tags. This is a discussion on where to implement the logic of turning access values into the internal values we work with

[imagico]

Yes, let us not discuss this strategic topic on this PR, i already mentioned that above in https://github.com/gravitystorm/openstreetmap-carto/pull/4952#issuecomment-2168549541:

I would suggest to keep the strategic discussion if to do tag interpretation during data import out of this PR. That is a matter where it will be hard to achieve consensus among the maintainers on and it is a matter that should be discussed not exclusively based on the specific problem of access restriction rendering but based on a broader look at how we currently interpret tags and what new types of tag interpretation we might want to introduce in the future.

What you call restricting a key value to a small number of values with only 3 meanings is an example of what i call tag interpretation (a classical case in map design - reducing a large number of classes to a smaller number of classes to differentiate in visual design). Hence what you suggest (doing this in preprocessing, i.e. during data import) amounts to the suggestion of doing tag interpretation during data import.

I would gladly discuss this matter in general with all the context that needs to be considered, but lets have this in a separate issue and not tie it to this PR.

[dch0ph]

Hence what you suggest (doing this in preprocessing, i.e. during data import) amounts to the suggestion of doing tag interpretation during data import.

My initial proposal involved a generated column as it did seem more elegant to pre-compute int_access rather than re-calculate every time. But in practice the time penalty seems to be unmeasurable - despite the length of code, the number of operations for an individual set of tags is quite limited - and it was quite a pain regenerating the int_access column after a database reload or if changing the function definitions. Keeping tag interpretation in SQL does seem the way to go.

[dch0ph]

Changes discussed above made.

PostgresSQL doesn't like the argument names being changed, and so the previous version of carto_int_access will first need to be dropped if present.

[pnorman]

@dch0ph do you have the ability to mark conversations as resolved? If so, can you indicate they are for the old comments, and if not, can you leave a comment saying it's fixed.

I consider it more important to have our code easy to read and understand than for it to be adaptable to different styles. I don't believe we should make the code more complex in order to make it easier for other people to potentially make changes.

I might rank this differently if it were a different section of the code, but our roads code is already complex to the point of being incomprehensible and the largest barrier to customization is understanding it.

My strong preference would be to release 5.9.0, merge the flex PR, make the necessary column changes and normalization at import time, and then merge the style changes for being part of 6.0.0

[imagico]

Having done a lot of work based on the road layers during the past years i can say that my experience here is very different. The main barrier to do any meaningful work in the road layers beyond fiddling with minor styling adjustments is the lack of structure, modularization and functional separation in the code, the extensive duplication of SQL in the road layers and the endless spaghetti MSS with interleaved use in different layers. What this PR is doing in that regard is trying - in a very small way - to not add to this mess but to start using new means (in this case SQL functions) to take a first step to break out this dead end.

Anyway - even if we agree to disagree on that: You have not presented any alternative approach for @dch0ph to pursue in solving #214 compared to the approach they took. In other words: Your suggestion seems to be to forgo addressing #214. I don't consider that acceptable.

My strong preference would be to release 5.9.0, merge the flex PR, make the necessary column changes and normalization at import time, and then merge the style changes for being part of 6.0.0

Let's discuss release planning in #4981. And let's discuss the question of if to do styling specific tag interpretation at import time in a separate issue.

[dch0ph]

@dch0ph do you have the ability to mark conversations as resolved? If so, can you indicate they are for the old comments, and if not, can you leave a comment saying it's fixed.

@pnorman I've resolved the queries raised in your last review where the query is clearly closed / resolved / superceded. There look to be two minor unresolved points (minor in the sense that they do not affect the outcomes and are more about the interpretative framework):

1. Whether to return, say, unrecognised when the access tags could not be sensibly resolved (rather than overloading the meaning of unknown).
2. The handling of the outcome of carto_path_type. I am happy with the current simplified code.