[WIP PATCH] Catch impossible angles in highways

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

[taylor.smock]

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

[GerdP]

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

[taylor.smock]

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

[taylor.smock]

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

[GerdP]

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

[taylor.smock]

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

[taylor.smock]

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.

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

[GerdP]

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

[taylor.smock]

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

[taylor.smock]

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.

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

[taylor.smock]

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.

[taylor.smock]

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.

[taylor.smock]

Add missing @since xxx tag

[taylor.smock]

FIx ant pmd checkstyle issues

[GerdP]

• clear severityBreakPoints before adding new values

[GerdP]

remove debug code

[GerdP]

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.

[taylor.smock]

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

[GerdP]

Looks good to me. Do you plan more changes?

[taylor.smock]

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

[GerdP]

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.

[taylor.smock]

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

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

[taylor.smock]

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.

[taylor.smock]

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.

[GerdP]

In 15405/josm:

fix #18127: Catch impossible angles in highways

Patch 18127.12 by taylor.smock

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

[GerdP]

In 15406/josm:

see #18127: correct @since

[taylor.smock]

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.

[taylor.smock]

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.

[GerdP]

In 15412/josm:

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

[Klumbumbus]

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

[taylor.smock]

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

[taylor.smock]

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.

[GerdP]

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

[GerdP]

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