Modify

Opened 4 years ago

Closed 8 months ago

#13165 closed defect (fixed)

[Patch needs work] Validator did not warn about overlapping multipolygons

Reported by: mdk Owned by: team
Priority: normal Milestone: 20.03
Component: Core validator Version: latest
Keywords: template_report, multipolygon Cc: Klumbumbus

Description

What steps will reproduce the problem?

  1. Open attached file and validate.

What is the expected result?

6 warnings about "overlapping identical landuses.
6 other messages about overlapping areas.

What happens instead?

Only two messages of each kind for closed way overlapping closed way.
No warnings about closed way overlapping multipolygon or multipolygon overlapping multipolygon.

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

In general closed ways and multipolygons should always be handled the same way in tests.

URL:http://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2016-07-13 23:20:15 +0200 (Wed, 13 Jul 2016)
Build-Date:2016-07-14 01:31:48
Revision:10531
Relative:URL: ^/trunk

Identification: JOSM/1.5 (10531 en) Linux Ubuntu 15.10
Memory Usage: 622 MB / 876 MB (207 MB allocated, but free)
Java version: 1.8.0_91-8u91-b14-0ubuntu4~15.10.1-b14, Oracle Corporation, OpenJDK Server VM
VM arguments: [-Djosm.restart=true, -Djosm.home=<josm.pref>, -Djava.net.useSystemProxies=true]
Dataset consistency test: No problems found

Plugins:
- ColumbusCSV (32359)
- FastDraw (32639)
- HouseNumberTaggingTool (32584)
- Mapillary (32639)
- OpeningHoursEditor (32583)
- RoadSigns (32584)
- apache-commons (32584)
- apache-http (32584)
- buildings_tools (32639)
- contourmerge (1014)
- imagery-xml-bounds (32489)
- imagery_offset_db (32528)
- pbf (32584)
- poly (32584)
- public_transport (32639)
- reltoolbox (32639)
- reverter (32584)
- terracer (32426)
- turnlanes (32584)
- turnrestrictions (32629)
- utilsplugin2 (32584)

Tagging presets:
- https://josm.openstreetmap.de/josmfile?page=Presets/LaneAttributes&preset&zip=1
- https://josm.openstreetmap.de/josmfile?page=Presets/OneClick&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&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Traffic_signs&zip=1

Last errors/warnings:
- W: Unsaved changes - <html>The relation has been changed.<br><br>Do you want to save your changes?</html>
- W: Unable to remove primitives from TestError [tester=org.openstreetmap.josm.data.validation.tests.CrossingWays$Ways@41069fff, code=601, message=Crossing buildings]. java.lang.UnsupportedOperationException
- W: Unable to remove primitives from TestError [tester=MapCSSTagCheckerAndRule [rule=GroupedMapCSSRule [selectors=[area:closed:areaStyle >LinkSelector{conditions=null} area:closed:areaStyle], declaration=Declaration [instructions=[throwOther: ArrayFunction~tr(class java.lang.String <Overlapping Areas>);], idx=11]]], code=3000, message=Overlapping Areas]. java.lang.UnsupportedOperationException
- W: Unable to remove primitives from TestError [tester=MapCSSTagCheckerAndRule [rule=GroupedMapCSSRule [selectors=[area:closed:areaStyle >LinkSelector{conditions=null} area:closed:areaStyle], declaration=Declaration [instructions=[throwOther: ArrayFunction~tr(class java.lang.String <Overlapping Areas>);], idx=11]]], code=3000, message=Overlapping Areas]. java.lang.UnsupportedOperationException
- W: Unable to remove primitives from TestError [tester=org.openstreetmap.josm.data.validation.tests.CrossingWays$Ways@41069fff, code=601, message=Crossing buildings]. java.lang.UnsupportedOperationException
- W: Unable to remove primitives from TestError [tester=MapCSSTagCheckerAndRule [rule=GroupedMapCSSRule [selectors=[area:closed:areaStyle >LinkSelector{conditions=null} area:closed:areaStyle], declaration=Declaration [instructions=[throwOther: ArrayFunction~tr(class java.lang.String <Overlapping Areas>);], idx=11]]], code=3000, message=Overlapping Areas]. java.lang.UnsupportedOperationException
- W: Unable to remove primitives from TestError [tester=MapCSSTagCheckerAndRule [rule=GroupedMapCSSRule [selectors=[area:closed:areaStyle >LinkSelector{conditions=null} area:closed:areaStyle], declaration=Declaration [instructions=[throwOther: ArrayFunction~tr(class java.lang.String <Overlapping Areas>);], idx=11]]], code=3000, message=Overlapping Areas]. java.lang.UnsupportedOperationException
- W: Unable to remove primitives from TestError [tester=org.openstreetmap.josm.data.validation.tests.CrossingWays$Ways@41069fff, code=601, message=Crossing buildings]. java.lang.UnsupportedOperationException
- W: Unable to remove primitives from TestError [tester=MapCSSTagCheckerAndRule [rule=GroupedMapCSSRule [selectors=[area:closed:areaStyle >LinkSelector{conditions=null} area:closed:areaStyle], declaration=Declaration [instructions=[throwOther: ArrayFunction~tr(class java.lang.String <Overlapping Areas>);], idx=11]]], code=3000, message=Overlapping Areas]. java.lang.UnsupportedOperationException
- W: Unable to remove primitives from TestError [tester=MapCSSTagCheckerAndRule [rule=GroupedMapCSSRule [selectors=[area:closed:areaStyle >LinkSelector{conditions=null} area:closed:areaStyle], declaration=Declaration [instructions=[throwOther: ArrayFunction~tr(class java.lang.String <Overlapping Areas>);], idx=11]]], code=3000, message=Overlapping Areas]. java.lang.UnsupportedOperationException

Attachments (9)

overlapping multipolygon.osm (4.2 KB) - added by mdk 4 years ago.
13165_alpha.patch (6.1 KB) - added by GerdP 4 years ago.
overlapping_multipolygon2.osm (23.3 KB) - added by GerdP 4 years ago.
13165_v0.patch (16.0 KB) - added by GerdP 4 years ago.
13165_v1.patch (14.7 KB) - added by GerdP 4 years ago.
13165_v2.patch (17.9 KB) - added by GerdP 18 months ago.
13165_v3.patch (17.1 KB) - added by GerdP 8 months ago.
13165_v4.patch (8.0 KB) - added by GerdP 8 months ago.
13165_v5.patch (21.2 KB) - added by GerdP 8 months ago.

Download all attachments as: .zip

Change History (41)

Changed 4 years ago by mdk

comment:1 Changed 4 years ago by GerdP

I'll have a look at this.

Changed 4 years ago by GerdP

Attachment: 13165_alpha.patch added

comment:2 Changed 4 years ago by GerdP

Hmm, this is quite complex. The attached patch is a solution for your examples, but it will not work with incomplete multipolygons, all members must be complete. I'll try to find a better solution....

Changed 4 years ago by GerdP

Changed 4 years ago by GerdP

Attachment: 13165_v0.patch added

comment:3 Changed 4 years ago by GerdP

Summary: Validator did not warn about overlapping multipolygons[Patch] Validator did not warn about overlapping multipolygons

OK, v0 still needs complete multipolygons, but I've fixed a few errors and added code to improve the "zoom to error" function. This is very useful when two complex ways or multipolygons overlap. I have no more time for coding from now to middle of October, so maybe someone else continues with this.

comment:4 Changed 4 years ago by Don-vip

Milestone: 16.09

comment:5 Changed 4 years ago by simon04

Milestone: 16.0916.10

comment:6 Changed 4 years ago by simon04

Milestone: 16.1016.11

Milestone renamed

Changed 4 years ago by GerdP

Attachment: 13165_v1.patch added

comment:7 Changed 4 years ago by GerdP

Patch v1 seems to find all problems, but I see a few problems:
1) It will produce duplicate errors if no element is selected or if both overlapping areas are selected.
2) The new code in Selector somehow repeats itself, esp. the two blocks for "check mp-way intersection" and "check way-mp intersection" are probably not both needed if one can make sure that one block is visited with exchanged parms. Looking at it later.
3) I'd prefer to hilite only the way segments which are part of the intersection, but for now the ways forming the rings are used.

comment:8 Changed 4 years ago by GerdP

I've noticed that the current implementation of the checks doesn't allow to find more than one intersection.
This is quite confusing when you select a way which intersects two (or more) other ways and start the validator. I would expect to get two (or more) warnings, one for each other way, but I'll get only one.
Is that intended?

comment:9 Changed 4 years ago by Don-vip

Milestone: 16.1116.12

Milestone renamed

comment:10 Changed 4 years ago by Don-vip

Milestone: 16.1217.01

comment:11 Changed 4 years ago by Don-vip

Milestone: 17.0117.02

comment:12 Changed 4 years ago by Don-vip

Milestone: 17.0217.03

comment:13 Changed 4 years ago by bastiK

Milestone: 17.0317.04

comment:14 Changed 3 years ago by Don-vip

Milestone: 17.0417.05

comment:15 Changed 3 years ago by bastiK

Summary: [Patch] Validator did not warn about overlapping multipolygons[Patch needs work] Validator did not warn about overlapping multipolygons

comment:16 Changed 3 years ago by Don-vip

Milestone: 17.05

comment:17 Changed 2 years ago by Don-vip

Keywords: multipolygon added

comment:18 Changed 2 years ago by StefanB

Is #16884 a duplicate of this ?
I uploaded a test case there.

comment:19 Changed 2 years ago by GerdP

Yes, it probably is. To be honest I don't understand the programming concept of visitors good enough, so my work on the patch came to a halt.

Changed 18 months ago by GerdP

Attachment: 13165_v2.patch added

comment:20 Changed 18 months ago by GerdP

Attached is a new patch that changes CrossingFinder to use also multipolygons when they are complete.
It also implements a better highlighting of the intersecting areas so that "zoom to problem" shows the intersection instead of both (possibly large) polygons.

When complex mp are involved (large lakes with many islands) this test can be slow. I think about a completely different approach:
I might use the code which verifies the geometry of a mp (class MultipolygonTest).
This is much faster. The idea: When I want to find out if a closed way overlaps with the mp I could create a copy of the mp, add the way as a member and check if the geometry is valid.
Problem: This would still require that the mp is complete. Maybe the CrossingWays test should be changed to check incomplete mp so that we catch more cases.

Changed 8 months ago by GerdP

Attachment: 13165_v3.patch added

comment:21 Changed 8 months ago by GerdP

Cc: Klumbumbus added

like v2, but with performance improvements for MPs.
@Klumbumbus: Please can you try this patch reg. highlighting of overlapping areas?

comment:22 Changed 8 months ago by Klumbumbus

13165_v3.patch works for me. I see it highlights the actual overlapping area, not the ways or way segments. That seems to be the best solution.
The special case from #16707 is then still only in "overlapping areas" severity other not like other crossing buildings in severity warning. But that was probably not the goal of this patch anyway.

comment:23 Changed 8 months ago by GerdP

OK, thanks for testing. Of course we have to add tests to geometry.mapcss and remove other code from CrossingWays.java to use it for #16707.
I think I'll remove the code for multipolygons for now. It requires either a lot more memory (v3) or a lot of runtime when processing complex MP and it only works with complete MP and the memory is kept after the validator finishes.
I'll try to find a better solution for that.

Changed 8 months ago by GerdP

Attachment: 13165_v4.patch added

comment:24 Changed 8 months ago by GerdP

13165_v4.patch implements CrossingFinder for complete multipolygons (MP) in a different way.
To reduce memory footprint the calculated MP areas are cached only while MapCSSTagChecker is running. Probably peek memory usage is the same, but this gives control to MapCSSTagChecker whether or not the cache is cleared.
I am probably nitpicking here as most users will not often work with complete and very complex MP like lake Huron or my example for #13289. For the usual MP with < 2000 nodes we should not see a problem.

I'll add some unit tests tomorrow...

Changed 8 months ago by GerdP

Attachment: 13165_v5.patch added

comment:25 Changed 8 months ago by GerdP

v5: No unit tests yet. CrossingFinder now uses two different strategies to find overlapping areas:
1) If both elements are complete, the intersection of the areas is calculated. This finds also cases where the nodes are shared as in #16707 and highlights the area that is overlapping.
2) If one or both elements are not complete (e.g. two multipolygon relations with incomplete members), only the complete ways are checked with the same algorithm used in CrossingWaysand MultipolygonTest. This doesn't find cases as in #16707 but still highlights all intersections.
The patch adds a few fields to Environment to improve run time. My understanding is that we typically have only a few instances of this class, so that should be no problem.

comment:26 Changed 8 months ago by GerdP

Resolution: fixed
Status: newclosed

In 15959/josm:

fix #13165 Validator did not warn about overlapping multipolygons

  • let CrossingFinder accept multipolygons
  • improve highlighting of overlapping areas
  • if incomplete multipolygons are tested the area calculation is used unless open ends are found, else the same algortihm as in CrossingWays is used to hilite the crossing segments. In the later case overlaps with shared nodes are not (yet) found.

comment:27 Changed 8 months ago by GerdP

Milestone: 20.03

comment:28 Changed 8 months ago by GerdP

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

comment:29 Changed 8 months ago by GerdP

In 15995/josm:

see #18802, #13165: Simplify code

comment:30 Changed 8 months ago by GerdP

Resolution: fixed
Status: closedreopened

comment:31 in reply to:  30 Changed 8 months ago by Don-vip

Replying to GerdP:

Why did you reopen this ticket?

comment:32 Changed 8 months ago by GerdP

Resolution: fixed
Status: reopenedclosed

No idea. Probably accidential.

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.