Opened 18 months ago
Last modified 18 months ago
#21333 new enhancement
[Patch] Extend SharpAngles test to railways
Reported by: | Famlam | Owned by: | GerdP |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | Core validator | Version: | |
Keywords: | template_report railway sharpangles | Cc: |
Description
What steps will reproduce the problem?
- 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)
Change History (24)
comment:1 Changed 18 months ago by
comment:2 Changed 18 months ago by
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
comment:3 follow-up: 4 Changed 18 months ago by
Are there any railway=*
on open ways which need to be excluded?
Using [railway]
or [/^(.*:)?railway$/]
does not make much difference, I guess.
comment:4 Changed 18 months ago by
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 Changed 18 months ago by
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.
Changed 18 months ago by
Attachment: | 21333.patch added |
---|
Fairly basic (non-tested) patch to add railways to SharpAngles test.
comment:6 follow-up: 7 Changed 18 months ago by
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)
comment:7 Changed 18 months ago by
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 Changed 18 months ago by
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"
Changed 18 months ago by
Attachment: | 21333.2.patch added |
---|
Blacklist "crossing_box", "loading_ramp", "platform", "roundhouse", "signal_box", "station", "traverser", "wash", "workshop"
comment:9 follow-up: 10 Changed 18 months ago by
What's the purpose of the addIgnoredRailway()
method? Future plugin something? I couldn't find usage of the addIgnoredHighway()
neither (I cloned core only).
comment:10 Changed 18 months ago by
Replying to gaben:
What's the purpose of the
addIgnoredRailway()
method? Future plugin something? I couldn't find usage of theaddIgnoredHighway()
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.
comment:11 follow-up: 12 Changed 18 months ago by
Maybe, the ignore lists could be user customizable as advanced preferences? Same is true for maxAngle and maxLength.
comment:12 Changed 18 months ago by
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).
Changed 18 months ago by
Attachment: | 21333.3.patch added |
---|
Set ignoreRailway/Highway from config values
comment:13 Changed 18 months ago by
Summary: | Extend SharpAngles test to railways → [Patch] Extend SharpAngles test to railways |
---|
comment:14 Changed 18 months ago by
Owner: | changed from team to GerdP |
---|
Small typo: 21333.3.patch uses the same preference key validator.sharpangles.maxlength
for length and angle.
Changed 18 months ago by
Attachment: | 21333.4.patch added |
---|
Fix typo in setting (maxlength
-> maxangle
)
comment:15 follow-up: 16 Changed 18 months ago by
- The evaluation of preferences should happen in method
startTest()
, else a restart is needed after changes of them. - maybe change
setMaxLength()
andsetMaxAngle()
to protected? IIGTR they are only useful in unit tests?
comment:16 Changed 18 months ago by
Replying to GerdP:
- The evaluation of preferences should happen in method
startTest()
, else a restart is needed after changes of them.- maybe change
setMaxLength()
andsetMaxAngle()
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.
comment:17 Changed 18 months ago by
After changes came alive, please document the new advanced preference in Help/Preferences/Advanced.
comment:18 Changed 18 months ago by
Validator preferences are documented under Help/Preferences/Validator with the test description. Yet, again it is a wiki.
If we want to be precise, the
way.hasKey("railway")
is not enough as it can contain lifecycle prefix, likedisused:railway=*
.