Modify

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#18127 closed enhancement (fixed)

[WIP PATCH] Catch impossible angles in highways

Reported by: taylor.smock Owned by: team
Priority: normal Milestone: 19.10
Component: Core validator Version:
Keywords: angle Cc:

Description (last modified by taylor.smock)

This finds angles in ways (continuous, does not look at intersections) that are less than 45 degrees. It then splits them up depending upon how likely it is to be an actual issue.

The breakpoints (arbitrarily chosen) were 15 degrees and 30 degrees. So <15 degree angle is an error, 15-30 is a warning, and 30-45 is informational.

Its a WIP patch since I haven't written any unit tests yet.

Attachments (15)

18127.patch (6.7 KB ) - added by taylor.smock 5 years ago.
Add check for sharp angles in roads, no unit tests yet
18127.2.patch (7.7 KB ) - added by taylor.smock 5 years ago.
Ignore some special highway cases, find sharp angles at start/end of closed way. Still no tests, and only highlights the shortest WaySegment.
18127.3.patch (11.9 KB ) - added by taylor.smock 5 years ago.
Add tests and fix issue with closed ways where an NPE would occur
18127.4.patch (13.2 KB ) - added by taylor.smock 5 years ago.
Fix an issue where closed ways got the angle for the first node, the last node, and the second to last node (first node == last node) by getting the second node instead of the first node. Also more tests.
zig-zag.osm (4.8 KB ) - added by GerdP 5 years ago.
18127.5.patch (14.0 KB ) - added by taylor.smock 5 years ago.
Set a default maximum length that can be overriden (mostly for tests) and add methods for other classes to add/modify data (static, so it carries over between classes).
18127.6.patch (13.4 KB ) - added by taylor.smock 5 years ago.
Change to primitive data types, remove hashCode and equals, change many static variables to non-static variables along with their set functions, change from highlighting the waysegments to the node.
18127.7.patch (13.5 KB ) - added by taylor.smock 5 years ago.
Add missing @since xxx tag
18127.8.patch (13.5 KB ) - added by taylor.smock 5 years ago.
FIx ant pmd checkstyle issues
18112.9.patch (13.0 KB ) - added by GerdP 5 years ago.
- clear severityBreakPoints before adding new values
18112.10.patch (13.0 KB ) - added by GerdP 5 years ago.
remove debug code
18127.11.patch (12.1 KB ) - added by taylor.smock 5 years ago.
Remove startTest and endTest. The tests now run as the ways are visited.
18127.12.patch (12.0 KB ) - added by taylor.smock 5 years ago.
Drop max severity to Severity.WARNING instead of Severity.ERROR (so angles below 30 degrees get Severity.WARNING). Angles between 30 and 45 degrees are still Severity.OTHER.
18127.13.patch (1.7 KB ) - added by taylor.smock 4 years ago.
Add checks to avoid creating errors when we don't want to check for Other errors, and ensure that the Other or the Warning codes can be ignored.
18127.14.patch (2.6 KB ) - added by taylor.smock 4 years ago.
Ignore ways with via_ferrata_scale, since it is much more likely to be a false positive

Download all attachments as: .zip

Change History (42)

by taylor.smock, 5 years ago

Attachment: 18127.patch added

Add check for sharp angles in roads, no unit tests yet

comment:1 by taylor.smock, 5 years ago

Description: modified (diff)

comment:2 by Don-vip, 5 years ago

Keywords: angle added

comment:3 by GerdP, 5 years ago

Some issues:

  • Only one way segment is highlighted?
  • It doesn't seem to find sharp angles in closed ways when the sharp angle is at the start/end node?
  • pedestrian areas and highway=rest_area might be special cases, see e.g.

https://www.openstreetmap.org/way/152259055
https://www.openstreetmap.org/way/328483801

in reply to:  3 comment:4 by taylor.smock, 5 years ago

Replying to GerdP:

Some issues:

  • Only one way segment is highlighted?

I made the decision to only highlight the shorter segment (some of the examples I've seen have had very long segments, a sharp angle, and then a very short segment). It might be the wrong decision since the user just has to zoom in to both ends of the very long way segment. Maybe highlight the node as well as the way segments? Or add a zoomTo function in TestError?

  • It doesn't seem to find sharp angles in closed ways when the sharp angle is at the start/end node?

I was going to add something for that around L139, but I forgot about it.

  • pedestrian areas and highway=rest_area might be special cases, see e.g.

There is no might about it. They definitely are special cases. I'll probably just ignore them in the test.

https://www.openstreetmap.org/way/152259055
https://www.openstreetmap.org/way/328483801

by taylor.smock, 5 years ago

Attachment: 18127.2.patch added

Ignore some special highway cases, find sharp angles at start/end of closed way. Still no tests, and only highlights the shortest WaySegment.

comment:5 by GerdP, 5 years ago

I get an Exception with this version:
Way https://osm.org/way/10,041,221 caused an error (java.lang.IllegalArgumentException: WaySegment [way=10041221, lowerIndex=9])

by taylor.smock, 5 years ago

Attachment: 18127.3.patch added

Add tests and fix issue with closed ways where an NPE would occur

by taylor.smock, 5 years ago

Attachment: 18127.4.patch added

Fix an issue where closed ways got the angle for the first node, the last node, and the second to last node (first node == last node) by getting the second node instead of the first node. Also more tests.

by GerdP, 5 years ago

Attachment: zig-zag.osm added

comment:6 by GerdP, 5 years ago

Maybe you should only flag those sharp angles where at least one segment is very short. The current implementation flags
https://www.openstreetmap.org/way/9707431
There are in fact two separate ways here, but they are very similar and therefore the mapper just created one OSM way.
I think this is a common method, also often used with flare roads at roundabouts like here:
https://www.openstreetmap.org/way/215014005
No idea if this is correct for the given road, just the first sample that I found. It is likely that a turn restriction is missing here.

There is also a problem when you have a zig-zagging way with two sharp angles in sequence. The code might produce an error AND a warning highlighting the same segment. See attached zig-zag.osm.

comment:7 by GerdP, 5 years ago

I think highway=via_ferrata should also be excluded from this test. Many of them have sharp angles and are probably correct.

by taylor.smock, 5 years ago

Attachment: 18127.5.patch added

Set a default maximum length that can be overriden (mostly for tests) and add methods for other classes to add/modify data (static, so it carries over between classes).

in reply to:  6 comment:8 by taylor.smock, 5 years ago

Replying to GerdP:

Maybe you should only flag those sharp angles where at least one segment is very short. The current implementation flags
https://www.openstreetmap.org/way/9707431

I've set it to 10 meters (but it can be overriden by other classes through a static method)

There are in fact two separate ways here, but they are very similar and therefore the mapper just created one OSM way.
I think this is a common method, also often used with flare roads at roundabouts like here:
https://www.openstreetmap.org/way/215014005
No idea if this is correct for the given road, just the first sample that I found. It is likely that a turn restriction is missing here.

The test will probably still highlight roundabout flares.

There is also a problem when you have a zig-zagging way with two sharp angles in sequence. The code might produce an error AND a warning highlighting the same segment. See attached zig-zag.osm.

I've changed the code to highlight both segments. I could ignore errors that have a waysegment that is already highlighted, but I don't think that would be the right approach.

comment:9 by GerdP, 5 years ago

Maybe it would be better to highlight only the node? My understanding is that this test should find angles which are more or less invisible at normal zoom levels and therefore probably not intended, so if "zoom to" takes you to the node it should be obvious what's wrong.
Besides that it seems to work fine now, but

  • Both Find Bugs and sonarlint are not happy about it, esp. all the modified static fields. Why do you want them to be static?
  • Why do you use e.g. "Double angle" or "Boolean last" instead of "double angle" and "boolean last"?
  • I think you should remove methods equals() and hashCode()
  • Why don't you use way.hasTag("area", "yes")? I see no need to support wrongly spelled tags like area=Yes

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

Replying to GerdP:

Maybe it would be better to highlight only the node? My understanding is that this test should find angles which are more or less invisible at normal zoom levels and therefore probably not intended, so if "zoom to" takes you to the node it should be obvious what's wrong.

That is fair. I've had some issues with zoom and nodes in the past with other tests (I can see the node that is at issue, but I can't autozoom in to the point where the problem is obvious).

Besides that it seems to work fine now, but

  • Both Find Bugs and sonarlint are not happy about it, esp. all the modified static fields. Why do you want them to be static?

I originally wanted them to be static since I didn't anticipate them changing. I should probably remove their static status.

  • Why do you use e.g. "Double angle" or "Boolean last" instead of "double angle" and "boolean last"?

No particular reason. I changed them to the primitive data types.

  • I think you should remove methods equals() and hashCode()

I probably should -- I think one of the Eclipse code linters was suggesting adding equals, and then when equals was added, it was complaining about hashCode. I checked with SonarLint, and it is complaining about that as well.

  • Why don't you use way.hasTag("area", "yes")? I see no need to support wrongly spelled tags like area=Yes

Probably not a bad idea.

by taylor.smock, 5 years ago

Attachment: 18127.6.patch added

Change to primitive data types, remove hashCode and equals, change many static variables to non-static variables along with their set functions, change from highlighting the waysegments to the node.

by taylor.smock, 5 years ago

Attachment: 18127.7.patch added

Add missing @since xxx tag

by taylor.smock, 5 years ago

Attachment: 18127.8.patch added

FIx ant pmd checkstyle issues

by GerdP, 5 years ago

Attachment: 18112.9.patch added
  • clear severityBreakPoints before adding new values

by GerdP, 5 years ago

Attachment: 18112.10.patch added

remove debug code

comment:11 by GerdP, 5 years ago

My suggestion to simplify the code. I'd also prefer to remove the try/catch block and the allWays list and simply call
checkWayForSharpAngles() in visit(), else you should handle the case that a new way has a negative id and getOsmId() returns 0.

by taylor.smock, 5 years ago

Attachment: 18127.11.patch added

Remove startTest and endTest. The tests now run as the ways are visited.

comment:12 by GerdP, 5 years ago

Looks good to me. Do you plan more changes?

comment:13 by taylor.smock, 5 years ago

Absent a major bug, no.

Probably the only changes I might make are changing the severity breakpoints (i.e., instead of 15-30-45 degrees, maybe 10-30-45, but I think the current breakpoints are a good starting point), and adding config options (but I don't think I will unless it is specifically requested).

comment:14 by GerdP, 5 years ago

My suggestions:

  • the message could be "Sharp angle in highway"
  • my gut feeling is that we should only produce one severity warning unless you know an application that struggles with the very sharp angles.
Last edited 5 years ago by GerdP (previous) (diff)

in reply to:  14 ; comment:15 by taylor.smock, 5 years ago

Replying to GerdP:

My suggestions:

  • the message could be "Sharp angle in highway"

I don't mind changing it to Sharp angle in highway.

  • my gut feeling is that we should only produce one severity warning unless you know an application that struggles with the very sharp angles.

Clarification: Do you mean only one of Severity.OTHER, Severity.WARNING, or Severity.ERROR?
I'll check and see if there is an application that has issues with very sharp angles (e.g., gives a routing penalty, drops the road from potential routes, etc.).

in reply to:  15 ; comment:16 by GerdP, 5 years ago

Replying to taylor.smock:

Clarification: Do you mean only one of Severity.OTHER, Severity.WARNING, or Severity.ERROR?
I'll check and see if there is an application that has issues with very sharp angles (e.g., gives a routing penalty, drops the road from potential routes, etc.).

Sorry for the delay, was cycling for a week... Yes, I think we only need Severity.WARNING.

by taylor.smock, 5 years ago

Attachment: 18127.12.patch added

Drop max severity to Severity.WARNING instead of Severity.ERROR (so angles below 30 degrees get Severity.WARNING). Angles between 30 and 45 degrees are still Severity.OTHER.

in reply to:  16 comment:17 by taylor.smock, 5 years ago

Replying to GerdP:

Replying to taylor.smock:

Clarification: Do you mean only one of Severity.OTHER, Severity.WARNING, or Severity.ERROR?
I'll check and see if there is an application that has issues with very sharp angles (e.g., gives a routing penalty, drops the road from potential routes, etc.).

Sorry for the delay, was cycling for a week... Yes, I think we only need Severity.WARNING.

I've dropped Severity.ERROR -- I don't think I want to add angles between 30 and 45 to Severity.WARNING, since there are going to be many false positives in that range. I've already seen some in the 15 to 30 degree range, but they aren't as prevalent (in a valid manner) as the 30-45 degree angles.

EDIT: Don't worry about the delay -- I hope you had a good time cycling.

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

comment:18 by GerdP, 4 years ago

Resolution: fixed
Status: newclosed

In 15405/josm:

fix #18127: Catch impossible angles in highways

Patch 18127.12 by taylor.smock

comment:19 by GerdP, 4 years ago

My thinking was that we should only warn about angles in highway ways below 30, because I thoought that there are many more with angles < 45 and the code would produce lots of TestError instances regardless of the setting in ValidatorPrefHelper.PREF_OTHER.get(), but I think I was overcautions. If this test ever causes performance problems this would be my first approach to improve it.
Another possible issue: Since we use the same code 3800 for both severites the "ignore error" button may not work as expected.

comment:20 by GerdP, 4 years ago

In 15406/josm:

see #18127: correct @since

in reply to:  19 comment:21 by taylor.smock, 4 years ago

Replying to GerdP:

My thinking was that we should only warn about angles in highway ways below 30, because I thoought that there are many more with angles < 45 and the code would produce lots of TestError instances regardless of the setting in ValidatorPrefHelper.PREF_OTHER.get(), but I think I was overcautions. If this test ever causes performance problems this would be my first approach to improve it.
Another possible issue: Since we use the same code 3800 for both severites the "ignore error" button may not work as expected.

Just in case it does become an issue, here is a patch which checks if the severity is Other or if we want to check for Other issues.

by taylor.smock, 4 years ago

Attachment: 18127.13.patch added

Add checks to avoid creating errors when we don't want to check for Other errors, and ensure that the Other or the Warning codes can be ignored.

comment:22 by GerdP, 4 years ago

In 15412/josm:

see #18127 SonarLint: Rename this method name to match the regular expression: 'test[A-Z][a-zA-Z0-9]*$'

comment:23 by Klumbumbus, 4 years ago

Thanks for creating that test. I think ways with osmwiki:Key:via_ferrata_scale should be ignored too.

by taylor.smock, 4 years ago

Attachment: 18127.14.patch added

Ignore ways with via_ferrata_scale, since it is much more likely to be a false positive

in reply to:  23 comment:24 by taylor.smock, 4 years ago

Replying to Klumbumbus:

Thanks for creating that test. I think ways with osmwiki:Key:via_ferrata_scale should be ignored too.

I've uploaded a patch for that. I also split out the logic to check for ways that shouldn't be checked, since I'll probably want to see if I can reuse it for #17528.

comment:25 by Klumbumbus, 4 years ago

Milestone: 19.10
Resolution: fixed
Status: closedreopened

comment:26 by GerdP, 4 years ago

Resolution: fixed
Status: reopenedclosed

In 15448/josm:

fix #18127: 18127.14.patch with some simplifications

  • ignore ways with tag via_ferrata_scale=*
  • only create errors with Severity.OTHER if needed

comment:27 by GerdP, 4 years ago

Ticket #15044 has been marked as a duplicate of this ticket.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


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