Modify

Opened 2 years ago

Closed 2 years ago

#14202 closed defect (fixed)

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

Reported by: mdk Owned by: team
Priority: major Milestone: 17.01
Component: Core validator Version: latest
Keywords: template_report Cc: bastiK, GerdP, Klumbumbus

Description

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

Attachments (3)

invalidArea.osm (1.2 KB) - added by mdk 2 years ago.
invalidArea2.osm (1.2 KB) - added by mdk 2 years ago.
14402.patch (8.1 KB) - added by GerdP 2 years ago.

Download all attachments as: .zip

Change History (17)

Changed 2 years ago by mdk

Attachment: invalidArea.osm added

comment:1 Changed 2 years ago by mdk

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

comment:2 Changed 2 years ago by Don-vip

Cc: bastiK GerdP Klumbumbus added
Milestone: 17.01

Regression of #13086

comment:3 Changed 2 years ago by 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.

Changed 2 years ago by mdk

Attachment: invalidArea2.osm added

comment:4 Changed 2 years ago by 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

comment:5 Changed 2 years ago by 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.

comment:6 Changed 2 years ago by 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.

comment:7 Changed 2 years ago by 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.

comment:8 Changed 2 years ago by 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).

comment:9 Changed 2 years ago by 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"

comment:10 Changed 2 years ago by 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.

comment:11 Changed 2 years ago by GerdP

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

Changed 2 years ago by GerdP

Attachment: 14402.patch added

comment:12 Changed 2 years ago by 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.

comment:13 Changed 2 years ago by GerdP

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

comment:14 Changed 2 years ago by Don-vip

Resolution: fixed
Status: newclosed

In 11441/josm:

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

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.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.