Modify

Opened 3 years ago

Last modified 10 days ago

#21333 new enhancement

[Patch] Extend SharpAngles test to railways

Reported by: Famlam Owned by: team
Priority: normal Milestone: 24.07
Component: Core validator Version:
Keywords: template_report railway sharpangles Cc:

Description

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

Attachments (6)

21333.patch (3.3 KB ) - added by taylor.smock 3 years ago.
Fairly basic (non-tested) patch to add railways to SharpAngles test.
21333.2.patch (3.4 KB ) - added by taylor.smock 3 years ago.
Blacklist "crossing_box", "loading_ramp", "platform", "roundhouse", "signal_box", "station", "traverser", "wash", "workshop"
21333.3.patch (4.2 KB ) - added by taylor.smock 3 years ago.
Set ignoreRailway/Highway from config values
21333.4.patch (4.2 KB ) - added by taylor.smock 3 years ago.
Fix typo in setting (maxlength -> maxangle)
21333.5.patch (4.6 KB ) - added by taylor.smock 3 years ago.
Move Config reading to startTest
21333.6.patch (5.6 KB ) - added by taylor.smock 3 years ago.
Fix unit tests

Download all attachments as: .zip

Change History (28)

comment:1 by gaben, 3 years ago

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

comment:2 by Famlam, 3 years ago

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

Last edited 3 years ago by Famlam (previous) (diff)

comment:3 by skyper, 3 years ago

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

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

in reply to:  3 comment:4 by Famlam, 3 years ago

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)

comment:5 by taylor.smock, 3 years ago

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.

by taylor.smock, 3 years ago

Attachment: 21333.patch added

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

comment:6 by Famlam, 3 years ago

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)

in reply to:  6 comment:7 by taylor.smock, 3 years ago

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.

comment:8 by Famlam, 3 years ago

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"

by taylor.smock, 3 years ago

Attachment: 21333.2.patch added

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

comment:9 by gaben, 3 years ago

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

Last edited 3 years ago by gaben (previous) (diff)

in reply to:  9 comment:10 by taylor.smock, 3 years ago

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.

Last edited 3 years ago by taylor.smock (previous) (diff)

comment:11 by skyper, 3 years ago

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

in reply to:  11 comment:12 by taylor.smock, 3 years ago

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).

by taylor.smock, 3 years ago

Attachment: 21333.3.patch added

Set ignoreRailway/Highway from config values

comment:13 by skyper, 3 years ago

Summary: Extend SharpAngles test to railways[Patch] Extend SharpAngles test to railways

comment:14 by GerdP, 3 years ago

Owner: changed from team to GerdP

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

by taylor.smock, 3 years ago

Attachment: 21333.4.patch added

Fix typo in setting (maxlength -> maxangle)

comment:15 by GerdP, 3 years ago

  • 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?

in reply to:  15 comment:16 by taylor.smock, 3 years ago

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.

Last edited 3 years ago by taylor.smock (previous) (diff)

by taylor.smock, 3 years ago

Attachment: 21333.5.patch added

Move Config reading to startTest

by taylor.smock, 3 years ago

Attachment: 21333.6.patch added

Fix unit tests

comment:17 by gaben, 3 years ago

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

comment:18 by skyper, 3 years ago

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

comment:19 by GerdP, 6 months ago

Owner: changed from GerdP to team

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.

comment:20 by taylor.smock, 6 months ago

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

comment:21 by gaben, 6 weeks ago

Milestone: 24.06

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

comment:22 by stoecker, 10 days ago

Milestone: 24.0624.07

Milestone closed.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from team to the specified user.
Next status will be 'needinfo'. The owner will be changed from team to Famlam.
as duplicate The resolution will be set to duplicate. Next status will be 'closed'. The specified ticket will be cross-referenced with this ticket.
The owner will be changed from team to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.