Modify

Opened 5 months ago

Closed 5 months ago

Last modified 3 months ago

#17010 closed enhancement (fixed)

Possible multipolygon duplicate test

Reported by: naoliv Owned by: GerdP
Priority: normal Milestone: 18.11
Component: Core validator Version:
Keywords: Cc:

Description

Validate the attached example (a simple multipolygon with no outer member); we can see this:

https://i.imgur.com/RWJ1sbS.png

We have 4 messages saying the same thing: we are missing an outer member at this relation.

So maybe we have duplicate tests and maybe they could be merged/removed/optimized, to avoid testing the same kind of problem multiple times?

JOSM:

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2018-11-18 18:27:42 +0100 (Sun, 18 Nov 2018)
Revision:14430
Build-Date:2018-11-19 02:32:53
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (14430 en) Linux Debian GNU/Linux unstable (sid)
Memory Usage: 315 MB / 2048 MB (131 MB allocated, but free)
Java version: 11.0.1+13-Debian-2, Oracle Corporation, OpenJDK 64-Bit Server VM
Screen: :0.0 1920x1080
Maximum Screen Size: 1920x1080
Java package: openjdk-11-jre:amd64-11.0.1+13-2
Java ATK Wrapper package: libatk-wrapper-java:all-0.33.3-21
VM arguments: [-Dawt.useSystemAAFontSettings=gasp]
Program arguments: [--language=en]

Plugins:
+ ImportImagePlugin (34576)
+ OpeningHoursEditor (34535)
+ SimplifyArea (34586)
+ apache-commons (34506)
+ buildings_tools (34724)
+ download_along (34503)
+ editgpx (34506)
+ ejml (34389)
+ geojson (87)
+ geotools (34513)
+ jaxb (34506)
+ jogl (1.2.2)
+ jts (34524)
+ log4j (34527)
+ measurement (34529)
+ merge-overlap (34664)
+ opendata (34698)
+ poly (34546)
+ reverter (34552)
+ tageditor (34560)
+ todo (30306)
+ turnlanes-tagging (272)
+ turnrestrictions (34643)
+ undelete (34568)
+ utilsplugin2 (34506)
+ wikipedia (v1.1.1)

Attachments (3)

example.osm (841 bytes) - added by naoliv 5 months ago.
17010-v1.patch (6.5 KB) - added by GerdP 5 months ago.
17010-v2.patch (8.5 KB) - added by GerdP 5 months ago.

Download all attachments as: .zip

Change History (11)

Changed 5 months ago by naoliv

Attachment: example.osm added

comment:1 Changed 5 months ago by GerdP

Owner: changed from team to GerdP

Yes, it would be good to reduce that noise. I think the reason for this is that we also try to make sense of really broken MPs.
The roles outer and inner are more or less only helpers for human consumers. A renderer should not rely on them and doesn't need them, either.

Changed 5 months ago by GerdP

Attachment: 17010-v1.patch added

comment:2 Changed 5 months ago by GerdP

The attached patch implements some improvements:
1) Don't check roles again in RelationChecker when MultipolygonTest is enabled
2) Remove test for "missing outer" in MultipolygonTest
3) Fix test for duplicated Members to check only way members (some boundary MP have the same node with role admin_centre and label)
4) Don't execute simple role tests for complete MP, report only the results found by the geometry checks

While writing this I found a simple case that is still not working well, so this needs more work.
Also the unit tests will fail because they expect all those duplicate messages.

Changed 5 months ago by GerdP

Attachment: 17010-v2.patch added

comment:3 Changed 5 months ago by GerdP

I didn't notice before that the code in class Multipolygon simply ignores ways with unexpected roles, only empty, "inner" and "outer" are processed. With 17010-v2.patch this is now handled.
In a few cases there may still me more messages than needed, for example when the inner way has role outer and the outer way has role inner.
@naoliv: Please try the patch. If you think it is OK I'll modifiy the unit test data in multipolygon.osm.

comment:4 Changed 5 months ago by naoliv

From my user point of view it seems better :-)

comment:5 Changed 5 months ago by GerdP

Thanks for testing.
So I'll try to make the unit tests work. BTW: There is not much change in performance here because these role tests are simple.
If you know a relation that takes too long to test please let me know.

comment:6 Changed 5 months ago by GerdP

Resolution: fixed
Status: newclosed

In 14437/josm:

fix #17010 - Validator creates multiple errors/warnings for same problem

The fix reduces the noise.

  • Don't check roles again in RelationChecker when MultipolygonTest is enabled
  • remove special code that just searches for an outer way
  • correct checkMembersAndRoles, it did not find wrong roles for way members

comment:7 Changed 4 months ago by Don-vip

Component: CoreCore validator

comment:8 Changed 3 months ago by Don-vip

Milestone: 18.11

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain GerdP.
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.