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.

Sign up for GitHub

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

Merged
merged 19 commits into from
Oct 16, 2024
Merged

Rendering specific access tags #4952

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
f8488d2
Interpret additional access tags
dch0ph Apr 12, 2024
9c8098b
Function load in CI
dch0ph Apr 13, 2024
a413991
Add carto_path_primary_path
dch0ph Apr 13, 2024
911569e
Moving customers, permit
dch0ph Apr 26, 2024
8f386d2
Major changes in response to comments
dch0ph Apr 28, 2024
e04589c
Use foot primary mode for highway=pedestrian
dch0ph Apr 30, 2024
a300250
Typo fix
dch0ph May 1, 2024
8f92b8d
Remove incorrect END statements
dch0ph May 1, 2024
42a9e75
Fix regression on introducing explicit unknown
dch0ph May 4, 2024
f99920e
Fix regression for highway=path
dch0ph May 7, 2024
cdc9ace
Alter 'destination' outcome for 2-state access
dch0ph May 10, 2024
987258c
Merge remote-tracking branch 'upstream/master' into carto-new-access
dch0ph Jun 14, 2024
a1dfd94
Simplify functions.sql
dch0ph Jun 15, 2024
194d641
Tidy access functions
dch0ph Jun 22, 2024
c4c775c
Update functions.sql
dch0ph Jun 26, 2024
15c75d6
Remove obsolete comment from MML
dch0ph Jun 26, 2024
1d369dc
Avoid unknown overload
dch0ph Aug 9, 2024
b1f77c2
Extend code comments
dch0ph Aug 15, 2024
af8400a
Fix broken bridge on path
dch0ph Oct 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ jobs:
osm2pgsql -G --hstore --style openstreetmap-carto.style --tag-transform-script openstreetmap-carto.lua -d gis -r xml <(echo '<osm version="0.6"/>')
- name: Create indexes
run: psql -1Xq -v ON_ERROR_STOP=1 -d gis -f indexes.sql
- name: Load functions
run: psql -1Xq -v ON_ERROR_STOP=1 -d gis -f functions.sql
- name: Load empty shapefiles
run: scripts/get-external-data.py --no-update --cache -D scripts/empty_files
- name: Test queries are valid
Expand Down
7 changes: 7 additions & 0 deletions INSTALL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ The indexes can be created in parallel with
scripts/indexes.py -0 | xargs -0 -P0 -I{} psql -d gis -c "{}"
```

### Database functions
Some functions need to be loaded into the database for current versions. These can be added / re-loaded at any point using:

```sh
psql -d gis -f functions.sql
```

## Scripted download
Some features are rendered using preprocessed shapefiles.

Expand Down
92 changes: 92 additions & 0 deletions functions.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/* Additional database functions for openstreetmap-carto */

/* Access functions below adapted from https://github.com/imagico/osm-carto-alternative-colors/tree/591c861112b4e5d44badd108f4cd1409146bca0b/sql/roads.sql */


/* Simplified 'yes', 'restricted', 'no', NULL scale for access restriction
'no' is returned if the rendering for highway category does not support 'destination'.
NULL is functionally equivalent to 'yes', but indicates the absence of a restriction
rather than a positive access = yes */
CREATE OR REPLACE FUNCTION carto_int_access(primary_mode text, accesstag text)
RETURNS text
LANGUAGE SQL
IMMUTABLE PARALLEL SAFE
AS $$
SELECT
CASE
WHEN accesstag IN ('yes', 'designated', 'permissive', 'customers') THEN 'yes'
WHEN accesstag IN ('destination', 'delivery', 'permit') THEN
imagico marked this conversation as resolved.
Show resolved Hide resolved
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.

WHEN accesstag IN ('no', 'private', 'agricultural', 'forestry', 'agricultural;forestry') THEN 'no'
ELSE NULL
END
END
$$;

/* Try to promote path to cycleway (if bicycle allowed), then bridleway (if horse)
This duplicates existing behaviour where designated access is required */
CREATE OR REPLACE FUNCTION carto_path_primary_mode(bicycle text, horse text)
RETURNS text
LANGUAGE SQL
IMMUTABLE PARALLEL SAFE
AS $$
SELECT
CASE
WHEN bicycle IN ('designated') THEN 'bicycle'
WHEN horse IN ('designated') THEN 'horse'
ELSE 'foot'
END
END
$$;

/* Classify highways into access categories which will be treated in the same way
Default is NULL in which case only the access tag will be used (e.g. highway=track)
Note that bicycle, horse arguments are only relevant if considering highway=path */
CREATE OR REPLACE FUNCTION carto_highway_primary_mode(highway text, bicycle text, horse text)
RETURNS text
LANGUAGE SQL
IMMUTABLE PARALLEL SAFE
AS $$
SELECT
CASE
WHEN highway IN ('motorway', 'motorway_link', 'trunk', 'trunk_link', 'primary', 'primary_link', 'secondary',
'secondary_link', 'tertiary', 'tertiary_link', 'residential', 'unclassified', 'living_street', 'service', 'road') THEN 'motorcar'
WHEN highway = 'pedestrian' THEN 'pedestrian'
WHEN highway IN ('footway', 'steps') THEN 'foot'
WHEN highway = 'bridleway' THEN 'horse'
WHEN highway = 'cycleway' THEN 'bicycle'
WHEN highway = 'path' THEN carto_path_primary_mode(bicycle, horse)
ELSE NULL
END
END
$$;

/* Return int_access value which will be used to determine access marking.
Only a restricted number of types can be returned, with NULL corresponding to no access restriction */
CREATE OR REPLACE FUNCTION carto_highway_int_access(highway text, "access" text, foot text, bicycle text, horse text, motorcar text, motor_vehicle text, vehicle text)
dch0ph marked this conversation as resolved.
Show resolved Hide resolved
RETURNS text
LANGUAGE SQL
IMMUTABLE PARALLEL SAFE
AS $$
SELECT
CASE carto_highway_primary_mode(highway, bicycle, horse)
WHEN 'motorcar' THEN
carto_int_access('motorcar', CASE
WHEN motorcar IN ('yes', 'designated', 'permissive', 'no', 'private', 'destination', 'customers', 'delivery', 'permit') THEN motorcar
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>]

WHEN 'foot' THEN carto_int_access('foot', CASE WHEN foot IN ('yes', 'designated', 'permissive', 'no', 'private', 'destination', 'customers', 'delivery', 'permit') THEN foot ELSE "access" END)
WHEN 'bicycle' THEN carto_int_access('bicycle', CASE WHEN bicycle IN ('yes', 'designated', 'permissive', 'no', 'private', 'destination', 'customers', 'delivery', 'permit') THEN bicycle ELSE "access" END)
WHEN 'horse' THEN carto_int_access('horse', CASE WHEN horse IN ('yes', 'designated', 'permissive', 'no', 'private', 'destination', 'customers', 'delivery', 'permit') THEN horse ELSE "access" END)
ELSE carto_int_access(NULL, "access")
END
END
$$;

/* Uncomment lines below to create generated column for int_access
ALTER TABLE planet_osm_line DROP COLUMN IF EXISTS int_access;
ALTER TABLE planet_osm_line
ADD int_access text GENERATED ALWAYS AS (CASE WHEN highway IS NOT NULL THEN carto_highway_int_access(highway, "access", foot, bicycle, horse, tags->'motorcar', tags->'motor_vehicle', tags->'vehicle') ELSE NULL END) STORED;
*/
39 changes: 12 additions & 27 deletions project.mml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ Layer:
bicycle,
tracktype,
int_surface,
access,
int_access,
construction,
service,
link,
Expand All @@ -467,9 +467,7 @@ Layer:
WHEN surface IN ('paved', 'asphalt', 'cobblestone', 'cobblestone:flattened', 'sett', 'concrete', 'concrete:lanes',
'concrete:plates', 'paving_stones', 'metal', 'wood', 'unhewn_cobblestone') THEN 'paved'
END AS int_surface,
CASE WHEN access IN ('destination') THEN 'destination'::text
WHEN access IN ('no', 'private') THEN 'no'::text
END AS access,
carto_highway_int_access(highway, access, foot, bicycle, horse, tags->'motorcar', tags->'motor_vehicle', tags->'vehicle') AS int_access,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This reveals an inconsistency of how we're treating access tags. foot, bicycle, and horse get columns while motorcar, motor_vehicle, and vehicle don't.

If we're storing the raw tag text here, let's put them all in the tags column. If we're preprocessing we could have a smaller number of columns

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, our current database schema has foot, bicycle, and horse as columns (because these have been interpreted in rendering of highway=path for a long time) and not the other more specific access tags. I agree that with this change this distinction would not make that much sense any more and sourcing all of them from hstore would be reasonable. But since that would involve a database reload i would suggest to do this separately. Overall, i am not sure if this is worth breaking backwards compatibility of the database.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any issue with doing a reload. Being consistent on columns is a plus, even if we aren't able to reach a consensus on any preprocessing

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am fine with removing foot/bicycle/horse from the columns. I am just not sure if it is worth doing a database layout change breaking backwards compatibility just because of this.

We could also source foot/bicycle/horse from hstore in anticipation of such a change now and defer the actual removal of the columns to when we do a database reload anyway.

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 could also source foot/bicycle/horse from hstore in anticipation of such a change now and defer the actual removal of the columns to when we do a database reload anyway.

This would make sense. But if we are sourcing all the detailed access tags via hstore, I suggest passing the hstore tags as a function argument to both carto_highway_int_access and carto_path_type. As well as being neater, it is presumably slightly more efficient to selectively access tags as needed rather than unpack them all to pass as function arguments [but I'm no SQL expert]?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have to correct myself here: I wrongly remembered that tags in columns are also available in hstore, but they are not. Hence my suggestion would not work.

construction,
CASE
WHEN service IN ('parking_aisle', 'drive-through', 'driveway') THEN 'INT-minor'::text
Expand All @@ -496,10 +494,7 @@ Layer:
bicycle,
tracktype,
'null',
CASE
WHEN access IN ('destination') THEN 'destination'::text
WHEN access IN ('no', 'private') THEN 'no'::text
END AS access,
NULL,
construction,
CASE WHEN service IN ('parking_aisle', 'drive-through', 'driveway') THEN 'INT-minor'::text ELSE 'INT-normal'::text END AS service,
'no' AS link,
Expand All @@ -514,7 +509,7 @@ Layer:
z_order,
CASE WHEN substring(feature for 8) = 'railway_' THEN 2 ELSE 1 END,
CASE WHEN feature IN ('railway_INT-preserved-ssy', 'railway_INT-spur-siding-yard', 'railway_tram-service') THEN 0 ELSE 1 END,
CASE WHEN access IN ('no', 'private') THEN 0 WHEN access IN ('destination') THEN 1 ELSE 2 END,
CASE int_access WHEN 'no' THEN 0 WHEN 'restricted' THEN 1 ELSE 2 END,
CASE WHEN int_surface IN ('unpaved') THEN 0 ELSE 1 END
) AS tunnels
properties:
Expand Down Expand Up @@ -686,7 +681,7 @@ Layer:
bicycle,
tracktype,
int_surface,
access,
int_access,
construction,
service,
link,
Expand All @@ -704,9 +699,7 @@ Layer:
WHEN surface IN ('paved', 'asphalt', 'cobblestone', 'cobblestone:flattened', 'sett', 'concrete', 'concrete:lanes',
'concrete:plates', 'paving_stones', 'metal', 'wood', 'unhewn_cobblestone') THEN 'paved'
END AS int_surface,
CASE WHEN access IN ('destination') THEN 'destination'::text
WHEN access IN ('no', 'private') THEN 'no'::text
END AS access,
carto_highway_int_access(highway, access, foot, bicycle, horse, tags->'motorcar', tags->'motor_vehicle', tags->'vehicle') AS int_access,
construction,
CASE
WHEN service IN ('parking_aisle', 'drive-through', 'driveway') OR leisure IN ('slipway') THEN 'INT-minor'::text
Expand Down Expand Up @@ -741,10 +734,7 @@ Layer:
'concrete:plates', 'paving_stones', 'metal', 'wood', 'unhewn_cobblestone') THEN 'paved'
ELSE NULL
END AS int_surface,
CASE
WHEN access IN ('destination') THEN 'destination'::text
WHEN access IN ('no', 'private') THEN 'no'::text
END AS access,
NULL,
construction,
CASE WHEN service IN ('parking_aisle', 'drive-through', 'driveway') OR leisure IN ('slipway') THEN 'INT-minor'::text ELSE 'INT-normal'::text END AS service,
'no' AS link,
Expand All @@ -763,7 +753,7 @@ Layer:
z_order,
CASE WHEN substring(feature for 8) = 'railway_' THEN 2 ELSE 1 END,
CASE WHEN feature IN ('railway_INT-preserved-ssy', 'railway_INT-spur-siding-yard', 'railway_tram-service') THEN 0 ELSE 1 END,
CASE WHEN access IN ('no', 'private') THEN 0 WHEN access IN ('destination') THEN 1 ELSE 2 END,
CASE int_access WHEN 'no' THEN 0 WHEN 'restricted' THEN 1 ELSE 2 END,
CASE WHEN int_surface IN ('unpaved') THEN 0 ELSE 1 END,
osm_id
) AS roads_sql
Expand Down Expand Up @@ -918,7 +908,7 @@ Layer:
bicycle,
tracktype,
int_surface,
access,
int_access,
construction,
service,
link,
Expand All @@ -936,9 +926,7 @@ Layer:
WHEN surface IN ('paved', 'asphalt', 'cobblestone', 'cobblestone:flattened', 'sett', 'concrete', 'concrete:lanes',
'concrete:plates', 'paving_stones', 'metal', 'wood', 'unhewn_cobblestone') THEN 'paved'
END AS int_surface,
CASE WHEN access IN ('destination') THEN 'destination'::text
WHEN access IN ('no', 'private') THEN 'no'::text
END AS access,
carto_highway_int_access(highway, access, foot, bicycle, horse, tags->'motorcar', tags->'motor_vehicle', tags->'vehicle') AS int_access,
construction,
CASE
WHEN service IN ('parking_aisle', 'drive-through', 'driveway') THEN 'INT-minor'::text
Expand All @@ -965,10 +953,7 @@ Layer:
bicycle,
tracktype,
'null',
CASE
WHEN access IN ('destination') THEN 'destination'::text
WHEN access IN ('no', 'private') THEN 'no'::text
END AS access,
NULL,
construction,
CASE WHEN service IN ('parking_aisle', 'drive-through', 'driveway') THEN 'INT-minor'::text ELSE 'INT-normal'::text END AS service,
'no' AS link,
Expand All @@ -983,7 +968,7 @@ Layer:
z_order,
CASE WHEN substring(feature for 8) = 'railway_' THEN 2 ELSE 1 END,
CASE WHEN feature IN ('railway_INT-preserved-ssy', 'railway_INT-spur-siding-yard', 'railway_tram-service') THEN 0 ELSE 1 END,
CASE WHEN access IN ('no', 'private') THEN 0 WHEN access IN ('destination') THEN 1 ELSE 2 END,
CASE int_access WHEN 'no' THEN 0 WHEN 'restricted' THEN 1 ELSE 2 END,
CASE WHEN int_surface IN ('unpaved') THEN 0 ELSE 1 END
) AS bridges
properties:
Expand Down
Loading
Loading