[PATCH] Add cycleway:*:buffer to Highways/Ways/Cycle Lane/Track preset

[huntertur]

Hello JOSM maintainer(s),

I have created a patch to add support for cycleway:*:buffer attributes in the Highways/Ways/Cycle Lane/Track preset. This patch is based on revision 19227. Following the precedent of cycleway:*:lane, I have added checkboxes for both, left, and right.

I believe this is important enough to add for the following reasons:

• With the exception of cycleway:left:buffer, the buffer keys have usage rates that are ~50%-65% of the lane keys: ​https://taginfo.openstreetmap.org/keys
• The buffer tags are consumed by at least one other project, PeopleForBikes Bicycle Network Analysis, which uses this key as part of a decision matrix to determine whether a stretch of roadway is low stress or high stress for cycling.

While people sometimes use a numeric value for these buffer tags, I decided it would be more appropriate to just have a checkbox in the preset, since usage rates for specific measurements of the width of bike lane buffers look to be extremely low compared to the boolean yes/no: ​https://taginfo.openstreetmap.org/keys/cycleway%3Aboth%3Abuffer#values

This is my first time contributing to JOSM; please let me know if I am doing something incorrectly :)

[huntertur]

Patch to add the buffer attributes

[huntertur]

Screenshot of the proposed changes to the preset

[anonymous]

It seems the yes/no values aren't documented: ​Key:cycleway:buffer

[huntertur]

Thanks; I've now edited the wiki article to mention them, as well as the significant difference in usage between cycleway:both:buffer (much more popular) and cycleway:buffer (much less popular).

[skyper]

Replying to huntertur:

Thanks; I've now edited the wiki article to mention them, as well as the significant difference in usage between cycleway:both:buffer (much more popular) and cycleway:buffer (much less popular)

according to ​taginfo it is the opposite:

5463 cycleway:buffer
93   cycleway:buffer:right
32   cycleway:buffer:left
18   cycleway:buffer:both

[huntertur]

I'm seeing the following:

5463 for cycleway:buffer
20444 for cycleway:both:buffer

40765 cycleway:right:buffer
5265 cycleway:left:buffer

[skyper]

Oh, sorry, I always forget that both/left/right are tied to cycleway.

[Famlam]

My main thought is whether it's clear to a non-expert user (who just tries to fill all the fields) what's meant with Buffered (side). Maybe slightly more descriptive, like Space between cars and bicycle lane (side) (and hope people won't tick it for tracks -> adding a validator warning against this)

[huntertur]

I can look at changing the verbiage and adding the validator warning. I'll attach an updated patch once this is done.

[huntertur]

cycleway:buffer patch, 2024-09-21 version

[huntertur]

I added a new version as cycleway_buffer_2.patch, with changed verbiage and added validator warning based on your suggestion.

[Famlam]

Hi, shouldn't the validator warn about everything except *=lane?

• For track, it's the default that it's buffered so tagging this is useless
• For shared_lane and share_busway it's by definition not buffered so tagging this is useless too

On the other hand, for separate (in your current patch) it could warn for any cycleway:* key on that side of the road, not limited to buffered.
(p.s. note that there's also combi's possible like cycleway:both=* + cycleway:right:buffered=* which aren't covered yet)

[huntertur]

cycleway:buffer support, 2024-09-22 version

[huntertur]

These are good suggestions. I have improved the validation rules in the newly-attached cycleway_buffer_3.patch.

[huntertur]

(Additionally: how do I run only the assertions from the combinations.mapcss file without running the entire test suite? ant test unfortunately takes a while to complete on my current hardware.)

[skyper]

I usually only work with local copies of the files and start JOSM from a terminal/console with separate preferences for testing using -Djosm.dir.name= or -Djosm.home, see Customization (all operating systems) and Command line options. Then I load the local files and deactivate all presets, validator rules and styles which I do not need. Local rules and styles should automatically reload if they are modified but presets need to be reloaded manually. Only icons are cached and not reloaded if modified.

[huntertur]

Thank you! I'll keep that in mind if more iterations of the patch are needed. Thanks for the guidance, all.

[stoecker]

If you're happy with the patch please say so. I'm not deep enough in the topic to have an own opinion.

[huntertur]

Replying to stoecker:

If you're happy with the patch please say so. I'm not deep enough in the topic to have an own opinion.

I would prefer one more review of the validator changes to ensure they follow the way things are supposed to be done in the JOSM codebase. If that additional set of eyes says it looks good, I will be happy with the patch, and ready to have it be accepted.

[stoecker]

Replying to huntertur:

Replying to stoecker:

If you're happy with the patch please say so. I'm not deep enough in the topic to have an own opinion.

I would prefer one more review of the validator changes to ensure they follow the way things are supposed to be done in the JOSM codebase. If that additional set of eyes says it looks good, I will be happy with the patch, and ready to have it be accepted.

Ah sorry, I mainly meant the others. I assumed you're happy with your own work ;-)

[Famlam]

Hi, I went over it quickly, but these are my main points:

line 782-790

• I'm afraid that my comment on patch v2 was a little bit misunderstood on one part, because I didn't mean to warn about any combination of highway=cycleway + cycleway=* (consider for instance highway=cycleway together with cycleway=link or cycleway=crossing, which is fine). I only meant any combination of cycleway=separate + cycleway:*=* is unlikely. However, maybe I was a bit offtopic there, apologies. Summary: I would revert the change in these lines of the patch (of combinations.mapcss), because this will cause false positives, for example with highway=cycleway + cycleway=crossing.

Asides from that, the majority of the patch looks good to me. Some minor feedback:

line 1072-1091: [/^cycleway(:right|:left|:both|):buffer$/] (and the like)

• You can skip the $ I think, so it'll also find ​these rare keys when encountered. Not a must, just a nice to have.
• throwWarning: tr("{0} together with {1}", "{1.tag}", "{2.tag}"); the first placeholder should be {0.tag} (otherwise it'll display =lane instead of the actual value)

line 1093-1121: /^cycleway(:right|:both):.*$/ (and the like)

• This requires the regex parser to check the full string (only to search for "any character, any number of occurances, till it finds the end" at the end). Simply making it /^cycleway(:right|:both):./ (and the like) already allows the regex search to stop earlier while finding exactly the same. I suspect that'll be more efficient (please correct me if I'm wrong)
• Some of the asserts fail, a.o. assertMatch: "way cycleway=separate cycleway:surface=asphalt"; and assertMatch: "way cycleway:both=separate cycleway:buffer=no";
• Having e.g. cycleway:right=separate + cycleway:right:buffer=yes now triggers two warnings, the one from the rule above and this one

line 1123-1146: way[cycleway:right:buffer][!cycleway:right][!cycleway:both][!cycleway], (and the like)

• Isn't this more generic, not limited to *:buffer? E.g. is there any case where one can have cycleway:right:* without cycleway:right|cycleway:both|cycleway? (Although a more generic rule would require the fixAdd to be removed)
• It could be that cycleway=lane was removed, but the *:buffer forgotten, so I'd use suggestAlternative: "{0.tag} + {1.key}=*"; instead of fixAdd and let the user check what's actually correct.

Please feel free to disagree on any point :)

---

Regarding the tests, my personal approach for mapcss rules is to just download the file locally and add it via preferences -> validator -> rules. If you have the advanced setting validator.check_assert_local_rules set to true, you can see in the console log or the status report (the latter doesn't refresh unless you close & reopen it) if any rule failed. After that I just delete the mapcss file again :). But there's many approaches as Skyper also mentioned one that works equally fine.

[huntertur]

cycleway:buffer support, 2024-09-22 version 2

[huntertur]

Thank you for the validator.check_assert_local_rules suggestion; I mistakenly thought those assertions had been getting run as part of ant test. I've now edited the developers guide on the wiki to mention this. This also masked the fact that I had some broken tests leftover from when I had been iterating on how to write the validation rules.

I implemented most of your suggestions in cycleway_buffer_4.patch, except for parts of those in the lines 1123-1146 section. I genericized the cycleway:*:* checks, except for the plain cycleway:* case, because it doesn't look like I can negate a regex in MapCSS, which prevents me from having it ignore cycleway:left=* or cycleway:right=* while matching everything else. Please correct me if I am wrong!

[Famlam]

Thanks for updating the patch!

---

Changing [cycleway!=lane] into [cycleway!=lane][cycleway!=separate] (and similar for the other 3) requires you to also update the capture number in the warning thrown: {2.tag} now becomes {3.tag}. Or you can move both the !=-selectors to the end (and make it {1.tag}) or you can make use of [cycleway!~/^(lane|separate)$/]. Fully up to you (IMO) which of the three you prefer.

---

way[/^cycleway:both:/][!cycleway:both][!cycleway][!cycleway:left],
way[/^cycleway:both:/][!cycleway:both][!cycleway][!cycleway:right],

This will result in a duplicate warning for cycleway:both:something without other cycleway/cycleway:[side] keys. You can just delete the second one and add [!cycleway:right] to the first one. (The same comment applies to the two lines underneath: way[cycleway:buffer][!cycleway][!cycleway:both][!cycleway:left],+way[cycleway:buffer][!cycleway][!cycleway:both][!cycleway:right] )

---

because it doesn't look like I can negate a regex in MapCSS, which prevents me from having it ignore cycleway:left=* or cycleway:right=* while matching everything else. Please correct me if I am wrong!

I'm happy to inform you that you're wrong ;)
You can use the ?! negative lookahead operator, e.g. this works flawless:

way[/^cycleway(?!:right|:left|:both):/] {
  throwWarning: "rule for demonstration only, do not add to patch";
  assertMatch: "way cycleway:something=yes";
  assertMatch: "way cycleway:something:something=yes";
  assertNoMatch: "way cycleway:both=yes";
  assertNoMatch: "way cycleway:both:something=yes";
  assertNoMatch: "way cycleway:both:something:something=yes";
}

Whether you want to go for more complicated regexes I'll leave up to you :)

[huntertur]

Thank you so much for your patience during this; your replies have helped me better understand the workings of the data validator. I believe I have everything cleaned up now in cycleway_buffer_5.patch. After this, I could possibly volunteer some time to either put the information you shared in the developers guide, or make it more visible if it is already there; hopefully, then, other new contributors won't stumble with it as many times as I have.

[taylor.smock]

Ticket retargeted after milestone closed

[taylor.smock]

Is everyone who reviewed the patch happy with the current status?

[Famlam]

Sorry I had forgotten after my holiday. Thanks for the ping!

I just ran a test in a few countries and I seem to have forgotten about the combination of highway=[something narrow] + cycleway:surface=* (and similar) which StreetComplete adds for paths (and other non-car-road highways) with segregated=yes.

So I suspect line 1116 should be modified to have [segregated!=yes] to prevent triggering on for instance:
way highway=path segregated=yes cycleway:surface=needles footway:surface=paving_stones
(The other option would be to explicitly ban all narrow highways like bridleway/footway/path)

[huntertur]

Thank you for the re-review. I didn't even know that was a valid combination! I've made the change to line 1116 and added your test case to the newly-attached cycleway_buffer_6.patch.

[Famlam]

Unfortunately I found another case. Apparently one can also use ​`cycleway:lanes` or it's directional variants if cycleway itself isn't sufficient. (The wiki was only created today, but the tags seem to be in use for a bit longer)

Example: ​https://www.openstreetmap.org/way/742066021
This could be fixed by changing [/^cycleway(?!:right|:left|:both):/] to [/^cycleway(?!:right|:left|:both|:lanes):/] in line 1116, and
assertNoMatch: "way highway=primary oneway=yes bicycle:lanes=no|designated|yes cycleway:lanes=|lane|no";

After that, no more comments from me :)
(But it would be nice if someone else could also have a look ;) )

[huntertur]

Thanks, Famlam. I've now added your suggested check!

[Famlam]

Thanks! From my side there is no more feedback
(Just hoping for that second pair of eyes in case I missed something; bicycle mapping varies a bit throughout the world and I look through the "Dutch" glasses)

[taylor.smock]

Ticket retargeted after milestone closed

[huntertur]

Is everything OK with the current version of the patch?

[stoecker]

In 19257/josm:

fix #23932 - patch by huntertur - Add cycleway:*:buffer to Highways/Ways/Cycle Lane/Track preset