[Patch] Validator didn't complain about a node beeing used more than once in a closed way [regression?]

[mdk]

What steps will reproduce the problem?

1. Open invalidArea.osm
2. Validate

What is the expected result?

Warning or error like way contains node more than once.

What happens instead?

No error, warning or hint.

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

I'm not 100% sure, but I think the validator has detected such errors in the past (but I can't tell you when).
The specialty is, that in a closed way, start end end is the same node. Nevertheless, this is the only allowed duplication of a node. In this case the start node also appears a third time in the middle of the way, which is illegal. But it looks like the Validator ignores ALL duplication of the start node:

  <way id='-30906' action='modify' visible='true'>
    <nd ref='-30981' />
    <nd ref='-30677' />
    <nd ref='-30981' />  <- this is not allowed
    <nd ref='-30651' />
    <nd ref='-30650' />
    <nd ref='-30653' />
    <nd ref='-30652' />
    <nd ref='-30981' />
    <tag k='building' v='yes' />
  </way>

URL:http://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2017-01-03 19:14:48 +0100 (Tue, 03 Jan 2017)
Build-Date:2017-01-04 02:32:31
Revision:11429
Relative:URL: ^/trunk

Identification: JOSM/1.5 (11429 en) Linux Ubuntu 16.10
Memory Usage: 798 MB / 1774 MB (204 MB allocated, but free)
Java version: 1.8.0_111-8u111-b14-2ubuntu0.16.10.2-b14, Oracle Corporation, OpenJDK 64-Bit Server VM
Screen: :0.0 1920x1080
Maximum Screen Size: 1920x1080
Java package: openjdk-8-jre:amd64-8u111-b14-2ubuntu0.16.10.2
VM arguments: [-Djosm.restart=true, -Djosm.dir.name=JOSM-latest, -Djava.net.useSystemProxies=true]
Dataset consistency test: No problems found

Plugins:
+ ColumbusCSV (32885)
+ FastDraw (33004)
+ HouseNumberTaggingTool (32699)
+ OpeningHoursEditor (33004)
+ RoadSigns (33088)
+ SimplifyArea (33004)
+ buildings_tools (33004)
+ contourmerge (1030)
+ imagery-xml-bounds (33004)
+ imagery_offset_db (33004)
+ pbf (33004)
+ poly (33004)
+ public_transport (33088)
+ reltoolbox (33088)
+ reverter (33088)
+ terracer (33088)
+ turnrestrictions (33088)
+ utilsplugin2 (33088)

Tagging presets:
+ https://josm.openstreetmap.de/josmfile?page=Presets/OneClick&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Presets/LaneAttributes&preset&zip=1

Map paint styles:
+ https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1
+ https://josm.openstreetmap.de/josmfile?page=Styles/Maxspeed&style&zip=1

Last errors/warnings:
- W: Cannot start IPv4 remotecontrol https server on port 8112: Keystore was tampered with, or password was incorrect
- W: Cannot start IPv6 remotecontrol https server on port 8112: Keystore was tampered with, or password was incorrect

[Don-vip]

Regression of #13086

[GerdP]

The test case (I call it a spike) is handled like an 8-shape way which we considered to be okay. I'd by happy to change that. The alternative would be to detect spikes (sequences of nodes like a-b-a or a-b-c-b-a) which is not that trivial.
A very special case would be a spike a-b-c-a where b lies exactly on the line described by a and c.

[mdk]

But the worst problem I see is, that the validator check depends on where the way is closed. 'invalidArea2.osm' contains exactly the same nodes and the closed way is also identical execpt of node used as start and end:

<way id='-31451' action='modify' visible='true'>
    <nd ref='-31443' />
    <nd ref='-31449' />
    <nd ref='-31447' />
    <nd ref='-31449' />
    <nd ref='-31441' />
    <nd ref='-31439' />
    <nd ref='-31445' />
    <nd ref='-31443' />
    <tag k='building' v='yes' />
  </way>

But in this case the validator warn about self intersecting way

[GerdP]

I agree both are invalid and I also think we should consider an 8-shape closed way as invalid.
The special cases are caused by the fact that we want to allow P-shaped or b-shaped (unclosed) ways, so the first and last point are treated special.

[mdk]

In my oppinion a way with a loop (no metter if p, b or 8) should follow at least the rule, that the loop must contain at lest 3 nodes. GerdP also want to force the nodes in the loop are not placed exactly on a line - in other words, that the loop area must be greater than 0.

[mdk]

BTW the rule that an area should have a size greater than 0 should also be checked for normal areas.

But I ofthen found closed ways with only two nodes (a-b-a) which never make any sense.

[GerdP]

I see two ways to handle this. Yours is the complex one.
The simple rule would be:
"ways must not contain more than one node twice and that one node has to be the first and/or last"
This would not allow spikes or 8-shapes or something like an "Rod of Asclepius" ​https://en.wikipedia.org/wiki/Rod_of_Asclepius

I agree that a closed way should have an area size greater than 0 (maybe 0.000000001 to handle rounding erros).

[GerdP]

"ways must not contain more than one node twice and that one node has to be the first and/or last"
Sorry, that would still allow 8-shapes and some spikes.
The simple rule should be
"all nodes of a way must appear only once, only the first and/or last node are allowed to appear exactly two times"

[mdk]

This rule would also accept a-b-c-a-d-e-f-d (a barbell) which is a failure again. or as logical operator means, that both sides of the expression can be true. You mean an either ... or.

[GerdP]

I wanted to include closed ways. In your example a and d appear more than once.

[GerdP]

The patch in 14402.patch implements what I tried to describe and adds / changes the unit test.
Sorry for the missleading name, it should have been 14202.patch, but I don't know how to remove/rename an uploaded file.

[Don-vip]

In 11441/josm:

fix #14202 - fix self-intersecting way test (patch by GerdP)