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

Jump to bottom

Crossing refactoring #1201

Merged
merged 27 commits into from
Nov 13, 2024
Merged

Crossing refactoring #1201

merged 27 commits into from
Nov 13, 2024

Conversation

tordans
Copy link
Collaborator

@tordans tordans commented Apr 24, 2024 •
edited
Loading

This is a first follow up to the community meeting 2024-02-28 where we discussed how to improve the crossing presets.

  • The commits in this PR are mostly refactoring by streamlining the presets to use @template fields lists. Each commit message documents possible side effects.
  • The PR also adds unsearchable crossings for highway=path+path=crossing, which is missing is missing ATM.
  • The PR makes a few decision on which fields to display when. For example lit is displayed on all way type crossings. But tactile_paving is only part of the node crossings (and should be tagged on the barrier=kerb when separate ways are mapped)

Testing

Show testcases and testing log

node presets/highway/crossing.json

testing

highway=crossing

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];node["highway"="crossing"]({{bbox}});out body;>;out skel qt;

testing

node/8677399770

==> All good

node presets/highway/crossing/_marked.json

highway=crossing
crossing=marked

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];node["highway"="crossing"]["crossing"="marked"]({{bbox}});out body;>;out skel qt;

testing

node/7896056627

=> All good

addTags => shows yellow box

    "tags": {
        "highway": "crossing",
        "crossing": "marked"
    },

node presets/highway/crossing/_zebra.json

highway=crossing
crossing=zebra

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];node["highway"="crossing"]["crossing"="zebra"]({{bbox}});out body;>;out skel qt;

testing

node/9446412638

=> All good

node presets/highway/crossing/traffic_signals.json

highway=crossing
crossing=traffic_signals

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];node["highway"="crossing"]["crossing"="traffic_signals"]({{bbox}});out body;>;out skel qt;

testing

node/7242739087

=> All good

node presets/highway/crossing/uncontrolled.json

highway=crossing
crossing=uncontrolled

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];node["highway"="crossing"]["crossing"="uncontrolled"]({{bbox}});out body;>;out skel qt;

testing

node/9435519215

=> All good

node presets/highway/crossing/unmarked.json

highway=crossing
crossing=unmarked

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];node["highway"="crossing"]["crossing"="unmarked"]({{bbox}});out body;>;out skel qt;

testing

node/2289508839

=> All good

addTags => Works as expected

    "addTags": {
        "highway": "crossing",
        "crossing": "unmarked",
        "crossing:markings": "no"
    },

way presets/highway/cycleway/_crossing.json

cycleway=crossing

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["cycleway"="crossing"]["crossing"!~".*"]({{bbox}});out body;>;out skel qt;

testing

way/1014361250

=> All good

addTags => All good

    "addTags": {
        "highway": "cycleway",
        "cycleway": "crossing"
    },

way presets/highway/cycleway/crossing/_marked.json

highway=cycleway
cycleway=crossing
crossing=marked

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="cycleway"]["cycleway"="crossing"]["crossing"="marked"]({{bbox}});out body;>;out skel qt;

testing

way/1009387182

=> All good

addTags => All good

    "addTags": {
        "highway": "cycleway",
        "cycleway": "crossing",
        "crossing": "marked",
        "crossing:markings": "yes"
    },

way presets/highway/cycleway/crossing/bicycle_foot.json

"exclude": ["fr","lt","pl","il","ps"]

highway=cycleway
cycleway=crossing
foot=designated

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="cycleway"]["cycleway"="crossing"]["foot"="designated"]({{bbox}});out body;>;out skel qt;

testing

way/1015325150

=> All good

addTags => All good

    "addTags": {
        "highway": "cycleway",
        "cycleway": "crossing",
        "foot": "designated",
        "bicycle": "designated"
    },

way presets/highway/cycleway/crossing/traffic_signals.json

highway=cycleway
cycleway=crossing
crossing=traffic_signals

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="cycleway"]["cycleway"="crossing"]["crossing"="traffic_signals"]({{bbox}});out body;>;out skel qt;

testing

way/1189207983

=> All good

way presets/highway/cycleway/crossing/uncontrolled.json

highway=cycleway
cycleway=crossing
crossing=uncontrolled

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="cycleway"]["cycleway"="crossing"]["crossing"="uncontrolled"]({{bbox}});out body;>;out skel qt;

testing

way/1197740324

=> All good

way presets/highway/cycleway/crossing/unmarked.json

highway=cycleway
cycleway=crossing
crossing=unmarked

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="cycleway"]["cycleway"="crossing"]["crossing"="unmarked"]({{bbox}});out body;>;out skel qt;

testing

way/1225892530

=> All good

addTags => All good

    "addTags": {
        "highway": "cycleway",
        "cycleway": "crossing",
        "crossing": "unmarked",
        "crossing:markings": "no"
    },

way presets/highway/footway/crossing.json

highway=footway
footway=crossing

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="footway"]["footway"="crossing"]["crossing"!~".*"]({{bbox}});out body;>;out skel qt;

testing

way/895119395

=> All good

way presets/highway/footway/crossing/_marked.json

highway=footway
footway=crossing
crossing=marked

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="footway"]["footway"="crossing"]["crossing"="marked"]({{bbox}});out body;>;out skel qt;

testing

way/887712501

=> All good

addTags => All good (cannot handle crossing_ref=zebra from this way, but that is expected)

    "addTags": {
        "highway": "footway",
        "footway": "crossing",
        "crossing": "marked",
        "crossing:markings": "yes"
    },

way presets/highway/footway/crossing/_zebra.json

highway=footway
footway=crossing
crossing=zebra

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="footway"]["footway"="crossing"]["crossing"="zebra"]({{bbox}});out body;>;out skel qt;

testing

way/578831990

=> All good

way presets/highway/footway/crossing/traffic_signals.json

highway=footway
footway=crossing
crossing=traffic_signals

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="footway"]["footway"="crossing"]["crossing"="traffic_signals"]({{bbox}});out body;>;out skel qt;

testing

way/1227311404

=> All good

way presets/highway/footway/crossing/uncontrolled.json

highway=footway
footway=crossing
crossing=uncontrolled

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="footway"]["footway"="crossing"]["crossing"="uncontrolled"]({{bbox}});out body;>;out skel qt;

testing

way/1165352272

=> ALl good

way presets/highway/footway/crossing/unmarked.json

highway=footway
footway=crossing
crossing=unmarked

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="footway"]["footway"="crossing"]["crossing"="unmarked"]({{bbox}});out body;>;out skel qt;

testing

way/1026900725

=> All good

addTags => All good

    "addTags": {
        "highway": "footway",
        "footway": "crossing",
        "crossing": "unmarked",
        "crossing:markings": "no"
    },

way presets/highway/path/_crossing.json

highway=path
path=crossing

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="path"]["path"="crossing"]["crossing"!~".*"]({{bbox}});out body;>;out skel qt;

testing

way/947039863

=> All good

way presets/highway/path/crossing/_marked.json

highway=path
path=crossing
crossing=marked

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="path"]["path"="crossing"]["crossing"="marked"]({{bbox}});out body;>;out skel qt;

testing

way/1252788055

=> All good

way presets/highway/path/crossing/_traffic_signals.json

highway=path
path=crossing
crossing=traffic_signals

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="path"]["path"="crossing"]["crossing"="traffic_signals"]({{bbox}});out body;>;out skel qt;

testing

way/1122016511

=> All good

way presets/highway/path/crossing/_uncontrolled.json

highway=path
path=crossing
crossing=uncontrolled

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="path"]["path"="crossing"]["crossing"="uncontrolled"]({{bbox}});out body;>;out skel qt;

testing

way/1205470144

=> All good

way presets/highway/path/crossing/_unmarked.json

highway=path
path=crossing
crossing=unmarked

Overpass turbo query: https://overpass-turbo.eu/?R&Q=[out:json][timeout:25];way["highway"="path"]["path"="crossing"]["crossing"="unmarked"]({{bbox}});out body;>;out skel qt;

testing

way/38771134

=> All good

addTags => All good

    "addTags": {
        "highway": "footway",
        "footway": "crossing",
        "crossing": "unmarked",
        "crossing:markings": "no"
    },

Follow ups

Show possible follow up task

Helper to see usage stats (per country) in combination of tags:

tordans added 3 commits April 24, 2024 17:16
@tordans
crossing/_marked: remove terms
02eb4d0 
Terms are not used for unsearchable preset so we can remove them.
@tordans
cycleway/crossing: add "oneway" field to unmarked in bicycle_foot
41a9443 
This streamlines the presets and makes it easer to review and use them.
@tordans
cycleway/crossing: add "tactile_paving" to traffic_signal
89a1671 
This streamlines the crossings presets
@tordans tordans force-pushed the crossing-refactoring branch from d5cdc9a to 3ad887b Compare April 24, 2024 18:03
@github-actions GitHub Actions
Copy link

🍱 You can preview the tagging presets of this pull request here.

@tordans tordans force-pushed the crossing-refactoring branch 2 times, most recently from b24e74a to 8db5421 Compare April 24, 2024 19:52
tordans added 3 commits April 25, 2024 04:57
@tordans
crossing: Introduce @templates/crossing/defaults
ca2b2e5 
This way we have the same fields in all crossing presets:
- "crossing"
- "tactile_paving"
- "crossing/island"

This change the order of things slightly for some footway, cycleway crossing where `surface` is now a bit lower, but that should not be a problem.
@tordans
crossing: Introduce @templates/crossing/geometry_line
e47e4d6 
This streamlines the fields on all line geometry crossings.
- "oneway"
- "surface"
- "smoothness"
- "crossing_raised"
- "access"

Those fields are always the last in the list. For traffic signal those specific fields are put above. Which is also the only change for one vertex preset in this commit, to have the "crossing_raised" come after the traffic signal specific fields and so the order is the same across presets.

This will roll out the smoothness field for all crossings; it was previously only present in some. But given the importance of smoothness for accessibility I think that is OK. This commit also moves the surface (and smoothness where present) fields further down the list which reduces the priority a bit.

The biggest change in priority is the oneway-field which had the first position before and now is below the defaults- and markings-field.
@tordans
crossing: Introduce @templates/crossing/traffic_signal
50aeb69 
This extract the three fields to be reused in all traffic_signals presets.
- "button_operated"
- "traffic_signals/sound"
- "traffic_signals/vibration"

Nothing else is changed, this is just an extraction into a template.
@tordans tordans force-pushed the crossing-refactoring branch 2 times, most recently from 1052eb6 to a5af304 Compare April 25, 2024 03:48
tordans
tordans commented Apr 25, 2024
View reviewed changes
package.json
@@ -12,7 +12,7 @@
"license": "ISC",
"main": "build.js",
"scripts": {
"lint": "prettier --check data",
"lint": "prettier --check 'data/**/!(*.md)'",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The alternative to this would be to change the prettier config to only apply the json-stringify on .json files and use regular prettier for other files.

However, our prettier config is still somewhat different from what I am used to and the resulting READM.md formatting is not great, so I think this is the better solution.

 % git diff
diff --git a/.prettierrc b/.prettierrc
index 27505a24..78e7d6df 100644
--- a/.prettierrc
+++ b/.prettierrc
@@ -1,6 +1,13 @@
 {
-    "parser": "json-stringify",
     "proseWrap": "never",
     "bracketSpacing": true,
-    "endOfLine": "lf"
+    "endOfLine": "lf",
+    "overrides": [
+        {
+            "files": "*.json",
+            "options": {
+                "parser": "json-stringify"
+            }
+        }
+    ]
 }

@tordans tordans force-pushed the crossing-refactoring branch 5 times, most recently from 7074531 to b77c741 Compare April 26, 2024 09:35
tordans added 6 commits April 26, 2024 12:00
@tordans
crossing: Introduce @templates/crossing/traffic_signal_more
c624700 
This extract the three moreFields to be reused in all traffic_signals presets.
- "traffic_signals/arrow"
- "traffic_signals/countdown"
- "traffic_signals/minimap"

For unclear reasons the cycleway/crossing/traffic_signals did not have those more fields which are now added to streamline the presets.
@tordans
crossing: remove "oneway" on highway/crossing vertex
99caf27 
I added this in openstreetmap@3e5e99f an I think that was a mistake so lets remove it again.
@tordans
crossing: Add kerb to all vertex crossings
cb4b64e 
The convention is, to tag this on the node _and_/_or_ on the separate `barrier=kerb+kerb=*` node when the path is mapped separately.
It should be part of all crossing vertex presets.
@tordans
crossing: Use the same property order for fields/crossing/markings*
3ab88d0 
All those fields used a different order of properties, which made it hard to compare them.

This commit does not change anything on the fields, it just streamlines the same order of properties across files.
@tordans
crossing: Move the @templates/crossing/markings to first field posi…
cd56a0d 
…tion

Using the preset I find the markings field to be the most important to change. The `@templates/crossing/defaults` is less important for all situation except for `data/presets/highway/crossing.json`. The main reasons for this is, that only on the base `highway/crossing` the field `crossing` is actually visible. For the more precise presets this field is hidden by some automatic part of the system.
@tordans
crossing: Introduce highway=path+path=crossing fields
30a8be9 
All fields are unsearchable (for now) so we can learn how to name properly.

The names are adapted from `presets/highway/cycleway/crossing/bicycle_foot.json`.

The terms are removed because the presets are unsearchable.
@tordans tordans force-pushed the crossing-refactoring branch from 49f782c to 8c52b99 Compare April 26, 2024 10:01
tordans added 5 commits April 26, 2024 13:18
@tordans
crossing: New approach with @templates/crossing/bicycle_relevance a…
75d756f 
…nd `@templates/crossing/defaults`

The field "crossing" is removed from the `/defaults` fields.
- it is only relevant for the geometry line because it is hidden on geometry vertex.
- but on geometry line, we want it to be on the first position of fields
- the `/defaults` fields however should be positioned below the `markings` which are more relevant for specifying the kind of crossing
- the `/defaults` fields now includes `crossing_raised` which was removed from the previous and discontinued `/geomery_line` fields template.

The new `@templates/crossing/bicycle_relevance`
- is used on all highways that have bicycle relevance which are `highway=path|cycleway` and not on `highway=footway`

For all traffic_signal presets, the order of fields is different to give the `/traffic_signal` more prominence.
@tordans
crossing: Fix marking* templates by allowing line geometries
8ca0db5 
The markings templates are not touched by this PR and it does seem to work without this. However, the fields are used on line and point geometries so either the `geometry` field is ignored during build or something else is happening…
@tordans
crossing: New approach with @templates/crossing/bicycle_more, addin…
46ac971 
…g `segregated`

The fields `oneway` and `access` are important for `highway=cycleway|path` crossings but not essential. They are more of a advanced user setup which should be visible when prev filled in but only added by users that read more about it before. They are moved to the `moreFields` for that reason.

The `segregated` is added here for the same reasons and because of it's importance for highway types that likely have bike traffic.
Ping openstreetmap#317

The `surface` and `smoothness` is extracted from the `@template` because it makes more sense to split them up in `fields` and `moreFields`. A templates adds too much abstraction in this case.
@tordans
crossing: Apply flashing_light to all footway=crossing and crossi…
28638cf 
…ng notes

The field `flashing_light` was used on some of those presets. It is now more systematic.

I also kept them on the `traffic_signals` presets because those can have additional `flashing_lights` as well.
@tordans
crossing: Extract tactile_paving from shared @template to node cr…
118ae02 
…ossings

The common practice is to tag this in the `highway=crossing` nodes and on separate `barrier=kerb` nodes but not on the crossing ways. Same as the `kerb` field.
tordans added 7 commits April 26, 2024 13:18
@tordans
crossing: Add readme for crossing presets and fields
2cec131
@tordans
crossing: Add readme docs on the presets/@templates
289a57b
@tordans
Fix npm run lint by ignoring *.md files in data
fa267ab 
Usually prettier can switch automatically to check Markdown and format it. However, this prettier config forces the JSON formatter for all files.
@tordans
Fix "Check file endings" workflow to also allow .md files
8bfaef1 
`npm run build` still works, so I don't think this is an issue.

This also removes the second run of very similar code in the prettier workflow which I think is probably a legacy redundancy that can just be deleted.

x
@tordans
crossing: Re-add crossing to all fields
6c0174e 
This fixes "crossing: New approach with …`@templates/crossing/defaults`".

We need the "crossing" field on vertex/node fields as well to allow to quickly change the preset.

SQ
@tordans
crossing: Add lit to all crossing "way" geometry presets via new `@…
b1ab6e3 
…template/geometry_way_more`

The "lit" value was present on some of the presets before and is common to be applied to all kind of ways.
@tordans
crossing: Fix fields order for highway/crossing
4381d3a
@tordans tordans force-pushed the crossing-refactoring branch from 8c52b99 to 4381d3a Compare April 26, 2024 11:19
@tordans tordans requested a review from tyrasd April 26, 2024 12:01
@tordans
Copy link
Collaborator Author

tordans commented Apr 26, 2024

@tyrasd this is ready for review. The testing part of the PR descriptions has links that make comparing the current and new setup easy.

This is quite the undertaking… but I think it looks much better now!

tordans
tordans commented Apr 28, 2024
View reviewed changes
data/presets/highway/path/_crossing.json
"value": "crossing"
},
"searchable": false,
"name": "Cycle & Foot Crossing"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thinking about this again: Is this really the right name? It is not wrong, to describe a path as Bike+Foot infrastructure. However this wording was inspired by https://github.com/openstreetmap/id-tagging-schema/pull/1201/files#diff-3b8f616654b5d2845f63eb05ce32b27741d3945ad0a71e55033de12a656d8b56R48 which behaves differently from this preset because it also has the addTags part about foot+bicycle=designated.

Since the preset is unsearchable, no one would be confused by this when searching for presets.

And also, I thing that even if people do not add the explicit access keys, routers would still consider the generic path bike + foot infrastructure (which is why some regions prefer this tagging for the bike+foot-case in the first place).

So my opinion is this is OK … or can be changed in follow up PRs when other cases are added or reevaluated.

This was referenced Apr 28, 2024
Draft
Open
tyrasd
tyrasd approved these changes May 22, 2024
View reviewed changes
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

Looks good to me after a first look. I'll will perform a double-check before merging

@tordans tordans mentioned this pull request Jun 23, 2024
Open
This was referenced Jul 2, 2024
Open
Open
Open
@tordans tordans mentioned this pull request Jul 13, 2024
Open
k-yle
k-yle reviewed Sep 3, 2024
View reviewed changes
Copy link
Collaborator

@k-yle k-yle left a comment

Choose a reason for hiding this comment

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

the end result seems much more organised than before, and I like the idea of putting READMEs in subfolders, since we don't allow comments in the presets themselves

data/presets/highway/path/crossing/_traffic_signals.json Outdated Show resolved Hide resolved
data/presets/highway/crossing/_marked.json Show resolved Hide resolved
@tordans @k-yle
Crossings: Improve wording for name
b2ad536 
Co-authored-by: Kyℓe Hensel <k-yle@users.noreply.github.com>
@danieldegroot2 danieldegroot2 mentioned this pull request Oct 15, 2024
Open
@tordans tordans mentioned this pull request Oct 24, 2024
Open
@tyrasd
Copy link
Member

tyrasd commented Nov 13, 2024

Let's get this merged. Sorry for needlessly letting this sit around. 🙇

@tyrasd tyrasd merged commit a090dad into openstreetmap:main Nov 13, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k-yle k-yle k-yle left review comments

@tyrasd tyrasd tyrasd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tordans @tyrasd @k-yle