Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rendering specific access tags #4952

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

dch0ph
Copy link
Contributor

@dch0ph dch0ph commented Apr 13, 2024

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
image

After
image

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

After
image

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

Before
image

After
image

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

After
image

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

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!

mode specific access tags relevant to primary mode of highway interpreted to determine access marking for:
Road types (motorcar > motor_vehicle > vehicle)
Footway (foot)
Cycleway (bicycle)
Bridleway (horse)
Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is first look at the implementation, i have not actually tested it yet and this definitely requires thorough testing.

Overall, i think this is a coding wise a good design for the initial take on the subject. But this is my personal opinion based on me being fine with the concept of using SQL functions to modularize SQL functionality in the style. Others might see this differently.

If we use this approach what we should be aware of is that if we ever change the parameter lists or the names of the functions we will need to make sure our installation procedure drops the old functions - because that is not taken care of by the CREATE OR REPLACE. This should probably be mentioned in our developer documentation.

Will separately comment further on the handling of highway=track in #214.

functions.sql Outdated
CASE
WHEN accesstag IN ('yes', 'designated', 'permissive', 'customers') THEN 'yes'
WHEN accesstag IN ('destination', 'delivery', 'permit') THEN
CASE WHEN primary_mode IN ('motorcar', 'pedestrian') THEN 'restricted' ELSE 'no' END
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You here distinguish between a two class and a three class access restriction model based on the primary mode - while we make this distinction based on road class. While this might not be significant in the exact design model you propose, it would be in many styling variation (like for example if you render highway=track with primary mode motorcar).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You here distinguish between a two class and a three class access restriction model based on the primary mode - while we make this distinction based on road class. While this might not be significant in the exact design model you propose, it would be in many styling variation (like for example if you render highway=track with primary mode motorcar).

Yes and no. I renamed the functions to carto_highway_primary_mode etc. and returned a "primary mode" for clarity, whereas it's really an "access type", so you could classify into motorcar_track if you wanted to have a 2 class access style for track. I could change it back to say access_mode. I was keen not to "reparse" all the way from highway to avoid repetition and improve efficiency.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Primary mode of use and road class are well defined concepts - you will need to explain what you exactly mean by "access type".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fundamentally the "access category" is a data abstraction that keeps the code simple and flexible. A given way is classified into an access category, which mostly depends on the highway type, but for highway=path also depends on access tags. All the decisions can then be made on the "access category", e.g. this determines which mode-specific access tags are to be used, but it is also used to distinguish between 2-state and 3-states access renders. So it is a single value that encapsulate both the "primary mode" and how the different access values will be translated into yes|restricted|no. So in retrospect it was confusing to conflate the "access category" with "primary mode". Hopefully most of this is documented in the comments.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fundamentally the "access category" is a data abstraction that keeps the code simple and flexible.

I think it is not a good idea to invent such concepts as parameters of implementing styling logic, especially if their meaning changes when changing the design logic.

The primary concern when implementing this should be that other style developers can easily understand and modify the tag interpretation logic. The secondary concern should be code performance since we are going to use it on the fly during rendering. Code complexity itself is lower priority.

As i said - we actually want to make this distinction based on road class, because it depends on the road class and its line signature if we can only show a two class access classification or if we can show three classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can call it road_class rather than access type/category. That's all it is really. highway=cycleway and highway=path, bicycle=designated belong to the same "road class" because the access marking works in the same way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then i'd suggest to simply re-cast highway=path into highway=cycleway/bridleway/footway in SQL and work with that as parameter. You essentially already have the function for that (carto_path_primary_mode()). We currently do this re-casting in MSS code - but since you need it for the access restriction processing you can do it in SQL and then simplify the MSS code as a bonus.

You'd need an additional query nesting level for that or move the carto_highway_int_access() call to the outer query.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then i'd suggest to simply re-cast highway=path into highway=cycleway/bridleway/footway in SQL

Yes, I was thinking along these lines - an effective int_highway. I am keen to collapse the different road types in one place, and will have to check that this works across the wider framework.

functions.sql Outdated Show resolved Hide resolved
functions.sql Outdated
WHEN motor_vehicle IN ('yes', 'designated', 'permissive', 'no', 'private', 'destination', 'customers', 'delivery', 'permit') THEN motor_vehicle
WHEN vehicle IN ('yes', 'designated', 'permissive', 'no', 'private', 'destination', 'customers', 'delivery', 'permit') THEN vehicle
ELSE "access" END)
WHEN 'pedestrian' THEN carto_int_access('pedestrian', 'no')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is odd - if you have a distinct primary mode pedestrian (and it is unclear what this means relative to foot): why does this universally mean access=no. Note that while we currently do not render highway=pedestrian with access dashing, access restricted pedestrian roads of course do exist - like in private parks or in military facilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, perhaps renaming "access type" as "primary mode" has hindered rather than helped. The idea of a "pedestrian" access mode was an implicit motorcar/motor_vehicle=no. Putting this no into int_access simplifies later queries about whether to add colour for bus=yes etc.; only ways with int_access=restricted|no are considered.
The alternative is to set int_access=NULL and to treat highway=pedestrian as a special case [IF int_access=restricted|no OR highway=pedestrian THEN <consider bus=yes marking>]

Copy link
Collaborator

@pnorman pnorman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

All we need to do is coalesce the default value and access tag hierarchy in the appropriate order and compare that with the values we're considering.

@imagico
Copy link
Collaborator

imagico commented Apr 18, 2024

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
Copy link
Collaborator

pnorman commented Apr 19, 2024

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
Copy link
Contributor Author

dch0ph commented Apr 19, 2024

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
Copy link
Collaborator

imagico commented Apr 19, 2024

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
Copy link
Collaborator

pnorman commented Apr 20, 2024

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
Copy link
Collaborator

imagico commented Apr 21, 2024

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
Copy link
Contributor Author

dch0ph commented Apr 21, 2024

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
Copy link
Collaborator

imagico commented Apr 21, 2024

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
Copy link
Contributor Author

dch0ph commented Apr 22, 2024

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
Copy link
Collaborator

pnorman commented Apr 22, 2024

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
Copy link
Collaborator

imagico commented Apr 22, 2024

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 #4952 (comment) 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
Copy link
Collaborator

imagico commented Apr 25, 2024

@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.

Following discussion moving:
access=customers -> "restricted" marking
access=permit -> "no" marking
Functions renamed for clarity

Changed logic for mode-specific tags, only ignoring 'unknown' values

unknown access type return for unknown/uninterpretable

path promoted to cycleway/bridleway in SQL rather than MSS
@dch0ph
Copy link
Contributor Author

dch0ph commented Apr 28, 2024

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
Copy link
Collaborator

imagico commented Apr 28, 2024

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
Copy link
Contributor Author

dch0ph commented Apr 28, 2024

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
Copy link
Collaborator

imagico commented Apr 28, 2024

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
Copy link
Contributor Author

dch0ph commented Apr 29, 2024

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:
image
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
Copy link
Collaborator

imagico commented Apr 29, 2024

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
Copy link
Contributor Author

dch0ph commented Apr 30, 2024

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

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pedestrian road part looks good to me now.

Overall this seems essentially untested so far: functions.sql does not run:

psql:functions.sql:25: ERROR:  syntax error at or near "END"
LINE 22: END
         ^
psql:functions.sql:41: ERROR:  syntax error at or near "END"
LINE 14: END
         ^
psql:functions.sql:59: ERROR:  syntax error at or near "END"
LINE 16: END
         ^
psql:functions.sql:83: ERROR:  syntax error at or near "END"
LINE 22: END
         ^

Anyone who wants to see this move forward can help by testing and reporting any issues. Useful would in particular an analysis if the introduction of the functions leads to substantial performance decrease. This can be looked at through pg_stat_statements.

functions.sql Outdated Show resolved Hide resolved
@dch0ph
Copy link
Contributor Author

dch0ph commented May 1, 2024

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:

image

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

@imagico
Copy link
Collaborator

imagico commented May 1, 2024

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

@dch0ph
Copy link
Contributor Author

dch0ph commented May 1, 2024

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
Copy link
Collaborator

pnorman commented May 1, 2024

@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
Copy link
Contributor Author

dch0ph commented May 1, 2024

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
Copy link
Collaborator

imagico commented May 1, 2024

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 #4952 (comment).

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 #4952 (review)). 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
Copy link
Collaborator

imagico commented May 7, 2024

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:

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
Copy link
Contributor Author

dch0ph commented May 7, 2024

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!

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had overseen the change in interpretation of access=destination in my previous review - see inline comment.

Apart from that i could not see any further issues but it would not hurt if a few other people would carefully check this.

I also superficially checked if there are likely any performance implications due to the introduction of the functions and i do not have the impression of this being the case. There does not seem to be any reproducibly measurable difference in render times between current master and this PR (or even the approach 3+ - which is the most complex one).

I also updated the branches with the alternative approaches with the fix for highway=path:

https://github.com/imagico/osm-carto-alternative-colors/tree/carto-new-access-approach1
https://github.com/imagico/osm-carto-alternative-colors/tree/carto-new-access-approach2
https://github.com/imagico/osm-carto-alternative-colors/tree/carto-new-access-approach3
https://github.com/imagico/osm-carto-alternative-colors/tree/carto-new-access-approach3+

functions.sql Outdated Show resolved Hide resolved
'destination' on path / footway etc. interpreted as 'yes' (matching current behaviour)
@pnorman
Copy link
Collaborator

pnorman commented May 10, 2024

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
Copy link
Collaborator

imagico commented May 10, 2024

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.

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this a bit more (including confirming that the logic on highway=path is exactly as before) and am satisfied with it. The SQL implementation is done in a way that allows easy customization and is fairly strait away to understand for map designers and moving the path interpretation logic to an SQL function also makes the MSS code better readable and maintainable. There do not appear to be significant impacts on performance in rendering due to the added function calls in the road layers.

In terms of the exact logic of tag interpretation I still think approach 3+ (https://github.com/imagico/osm-carto-alternative-colors/tree/carto-new-access-approach3+) is the better approach to serve our goals. This involves

  • showing a distinct unknown access conditions line signature at z18+ for invalid values as well as explicitly tagged access=unknown without a fallback.
  • showing the fallback according to the formal access hierarchy for explicitly tagged unknown values when a fallback is tagged (like this PR currently does as well).
  • showing the fallback according to the formal access hierarchy for invalid values at z17 and lower.

See z17,z18.

But i am not tied to this - if there is agreement otherwise that what this PR implements right now is preferable i can live with that. The number of cases in practical tagging where this makes a difference is small.

@imagico imagico requested a review from pnorman May 13, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Render more specific access tags i.e. bicycle, motor_vehicle etc
3 participants