[Patch] Extend SharpAngles test to railways

[Famlam]

What steps will reproduce the problem? ¶

1. In ​https://www.openstreetmap.org/changeset/111286965 I fixed a (light_rail) railway that went in a very sharp zig-zag pattern (forth-back-forth). JOSM validator didn't complain about it

What is the expected result? ¶

Just like a highway=* having very sharp angles, complain about the same for railways

What happens instead? ¶

Not detected

Please provide any additional information below. Attach a screenshot if possible. ¶

As sharp angles tests are already implemented for highway=*s, I guess it should be fairly simple to also make it trigger for railway=* https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/data/validation/tests/SharpAngles.java
Probably only line 64
return (way.hasKey("highway") && !way.hasTag("area", "yes") && !way.hasKey("via_ferrata_scale") && !ignoreHighways.contains(way.get("highway")));
should be changed to include way.hasKey("railway")

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2021-09-03 03:12:33 +0200 (Fri, 03 Sep 2021)
Build-Date:2021-09-03 01:31:19
Revision:18193
Relative:URL: ^/trunk

Identification: JOSM/1.5 (18193 nl) Windows 10 64-Bit
OS Build number: Windows 10 Home 2009 (19043)
Memory Usage: 1608 MB / 1820 MB (389 MB allocated, but free)
Java version: 1.8.0_301-b09, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920×1080 (scaling 1.00×1.00)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252
Locale info: nl_NL
Numbers with default locale: 1234567890 -> 1234567890
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (35640)
+ SimplifyArea (35640)
+ imagery_offset_db (35640)
+ pt_assistant (1ff2e15)
+ reverter (35732)
+ tageditor (35640)
+ turnlanes-tagging (288)
+ undelete (35640)
+ utilsplugin2 (35792)

Tagging presets:
+ http://mijndev.openstreetmap.nl/~allroads/JOSM/Presets/NL-Fiets.zip

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/PublicTransport&zip=1
+ %UserProfile%\Documents\tijdelijke bestanden\josm-eigen.mappaint.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Styles/Sidewalks&zip=1

Validator rules:
+ %UserProfile%\Documents\tijdelijke bestanden\josm-eigen.validator.mapcss

Last errors/warnings:
- 00007.420 E: Lokaliseren van afbeelding 'bus.png' mislukt
- 02121.277 E: Communicatie met OSM-server mislukt - null
- 02122.628 W: {Way id=448357441 version=2 MVT> nodes=[{Node id=1520058668 version=2 V lat=52.0842253,lon=5.176727}, {Node id=1520058849 version=3 V lat=52.0844512,lon=5.1767098}]} not found in HistoryDataSet

[gaben]

If we want to be precise, the way.hasKey("railway") is not enough as it can contain lifecycle prefix, like disused:railway=*.

[Famlam]

Yeah, but that's lacking in the majority of validator tests (and also [lifecycle]:highway in this test) ;). Probably needs some general fix like way.hasKeyLifecycle(...) or so. Let's keep this one on topic

[skyper]

Are there any railway=* on open ways which need to be excluded?

Using [railway] or [/^(.*:)?railway$/] does not make much difference, I guess.

[Famlam]

Replying to skyper:

Are there any railway=* on open ways which need to be excluded?

Only closed ways I think could be false positives. (A railway=platform open way shouldn't have sharp kinks either, but follow the tracks)

[taylor.smock]

Speaking as the original author, it is feasible. Like @skyper just said, we would want to have a list of railways that should be excluded, and I'm not familiar enough with railway mapping to know what should be excluded.

There are a few other places where we would have to change language (Check for sharp angles on roads -> Check for sharp angles on man made transportation ways or something like that).

I'd also probably add a method called addIgnoredRailway to match the addIgnoredHighway method.

[taylor.smock]

Fairly basic (non-tested) patch to add railways to SharpAngles test.

[Famlam]

Like @skyper just said, we would want to have a list of railways that should be excluded, and I'm not familiar enough with railway mapping to know what should be excluded.

Ah, I now see that closed ways aren't excluded. In that case, I would add !way.isClosed() to line 71, i.e.
|| (way.hasKey("railway") && !way.isClosed() && !ignoreRailway.contains(way.get("railway"))));
(As there's various small building-like structures under railway=*, and railway=station, both of which could hypothetically contain sharp angles if someone literally draws the exact contours. In that case, the array at line 40 can stay empty to my knowledge)

[taylor.smock]

Replying to Famlam:

Ah, I now see that closed ways aren't excluded. In that case, I would add !way.isClosed() to line 71, i.e.
|| (way.hasKey("railway") && !way.isClosed() && !ignoreRailway.contains(way.get("railway"))));
(As there's various small building-like structures under railway=*, and railway=station, both of which could hypothetically contain sharp angles if someone literally draws the exact contours. In that case, the array at line 40 can stay empty to my knowledge)

Are you certain about !way.isClosed()? Are there any real-world cases where there might be something like a roundabout for trains? Or even just a really large loop.

I can just blacklist platform, station, turntable (maybe?), roundhouse, traverser, and wash.

[Famlam]

Hmmm, yeah, true. Some miniature railways will probably be closed, unsplit loops, and perhaps railways without relations might be glued together too. I guess they're rare, but cannot guarantee they don't exist ;)

I agree that it might be good to leave it on for turntable too, as that'll always be a circle and a large deviation from a circle is better to fix, even though it's not an actual rail that you map, but the circumference of it.

Ok, in that case I would whitelist:
"crossing_box", "loading_ramp", "platform", "roundhouse", "signal_box", "station", "traverser", "wash", "workshop"

[taylor.smock]

Blacklist "crossing_box", "loading_ramp", "platform", "roundhouse", "signal_box", "station", "traverser", "wash", "workshop"

[gaben]

What's the purpose of the addIgnoredRailway() method? Future plugin something? I couldn't find usage of the addIgnoredHighway() neither (I cloned core only).

[taylor.smock]

Replying to gaben:

What's the purpose of the addIgnoredRailway() method? Future plugin something? I couldn't find usage of the addIgnoredHighway() neither (I cloned core only).

IIRC, I think I added addIgnoredHighway for plugins, and I just added addIgnoredRailway to be consistent.
I don't believe addIgnoredHighway is actually used in plugins. I do use SharpAngles in my MapWithAI plugin (for a validation test -- I need to move this to the dedicated validations testing plugin which I created).

EDIT: If you want to check if something is used in a plugin, fork ​https://gitlab.com/smocktaylor/josm/-/tree/gitlab-ci/ , remove the appropriate methods, and run the plugin-binary-compatibility-generate-ci job.

[skyper]

Maybe, the ignore lists could be user customizable as advanced preferences? Same is true for maxAngle and maxLength.

[taylor.smock]

Replying to skyper:

Maybe, the ignore lists could be user customizable as advanced preferences? Same is true for maxAngle and maxLength.

That is easy enough. I'll probably just set addIgnoredHighway as @Deprecated, just in case.

maxAngle and maxLength just use it at initialization. They can be set in code (I do this with maxLength for a street address order test in MapWithAI).

[taylor.smock]

Set ignoreRailway/Highway from config values

[GerdP]

Small typo: 21333.3.patch uses the same preference key validator.sharpangles.maxlength for length and angle.

[taylor.smock]

Fix typo in setting (maxlength -> maxangle)

[GerdP]

• The evaluation of preferences should happen in method startTest(), else a restart is needed after changes of them.
• maybe change setMaxLength() and setMaxAngle() to protected? IIGTR they are only useful in unit tests?

[taylor.smock]

Replying to GerdP:

• The evaluation of preferences should happen in method startTest(), else a restart is needed after changes of them.
• maybe change setMaxLength() and setMaxAngle() to protected? IIGTR they are only useful in unit tests?

See ​MapWithAI StreetAddressOrder for a use of setMaxLength. But I can move the preferences into startTest (I hadn't gotten around to checking the code where I use the SharpAngle test to ensure things didn't break, looks like it won't).

EDIT: Hopefully attachment:21333.6.patch​ applies cleanly -- my test directly is polluted with work for #21139.

[taylor.smock]

Move Config reading to startTest

[taylor.smock]

Fix unit tests

[gaben]

After changes came alive, please document the new advanced preference in Help/Preferences/Advanced.

[skyper]

Validator preferences are documented under Help/Preferences/Validator with the test description. Yet, again it is a wiki.

[GerdP]

Sorry, can't remember why I changed the ticket so that I am responsible. Hope that's not the reason why this is still open.

[taylor.smock]

Well, I kind of forgot about it. I'll take a look at my code again and (probably) change some things.

[gaben]

Seems to be a good candidate for the second next release.

[stoecker]

Milestone closed.

[taylor.smock]

Ticket retargeted after milestone closed

[taylor.smock]

In 19162/josm:

Fix #21333: Extend SharpAngles test to railways