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
base: master
Are you sure you want to change the base?
Conversation
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)
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
).
There was a problem hiding this comment.
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 modemotorcar
).
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.
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
intohighway=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
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') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
]
There was a problem hiding this 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.
That depends on our strategy of dealing with unknown tags in general. If we'd show 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 |
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.
I don't think a 4th class is possible without becoming confusing. |
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 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.)
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. |
Currently, if a road is tagged 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 This is different here,
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. |
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.
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. |
#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.
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:
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 For both of these the explicit |
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 Return 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
My personal preference would be to strip out "access"= |
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 |
That would work for me. Rather than This would be functionally equivalent to a simple COALESCE, but ignoring There is the issue of what to do with |
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.
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.
There still is. If someone tags highway=residential access= they get the feedback that their tag worked, because it still displays as having access. |
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
This is exactly why i brought up the mixture approach as a fourth option.
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
No, because adding (for example) |
@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
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.
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 |
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 You have not yet made any changed to the pedestrian logic - you still universally have pedestrian roads as |
Thank for you the encouraging comments. I'll admit that limited thought went into the 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 I would counter with something like:
i.e. use the vehicle tags as for roads, e.g. It might be interesting (separately) to develop a marking for If the outcome were |
No, that is a wrong understanding of
|
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 As far as a hypothetical rendering of exceptions from implicit access restrictions (like on 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. |
OK. In the interests of keeping things moving I've adjusted the access interpretation of
i.e. using |
There was a problem hiding this 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.
Well - independent of if you universally get an error for that, the |
Fixed. Hopefully |
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. |
That's essentially what we have now. All the "combination" is now of the form:
which is functionally coalescing
Users wanting just to install OSMCarto, developers of OSMCarto, or both? |
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.
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.
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. |
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 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
|
Thanks for showing these contact sheets. They highlight a reversion on So it retains the current logic of only "promoting" to cycleway/bridleway. But I need to add the access logic for You would only save ~1% of lines of XML from removing I'll stay out of commenting on the different options! |
There was a problem hiding this 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+
'destination' on path / footway etc. interpreted as 'yes' (matching current behaviour)
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
and you would need to add to that an additional line of exactly the same form:
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. |
There was a problem hiding this 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.
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.
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 byaccess
]In this initial PR, the interpretation of path is unchanged, i.e. path is considered to be a cycleway or bridleway if
bicycle=designated
orhorse=designated
respectively.The access tags are reduced to a single
int_access
tag tagging the valuesno
,restricted
andyes
(which is equivalent to NULL).restricted
used to indicate an intermediate "some restrictions apply" for vehicles. The access marking used is the the currentaccess=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 tono
).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 specificbicycle=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!