Opened 8 years ago
Closed 5 years 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?
- 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)
Change History (41)
by , 8 years ago
| Attachment: | overlapping multipolygon.osm added |
|---|
comment:1 by , 8 years ago
by , 8 years ago
| Attachment: | 13165_alpha.patch added |
|---|
comment:2 by , 8 years ago
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....
by , 8 years ago
| Attachment: | overlapping_multipolygon2.osm added |
|---|
by , 8 years ago
| Attachment: | 13165_v0.patch added |
|---|
comment:3 by , 8 years ago
| 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 by , 8 years ago
| Milestone: | → 16.09 |
|---|
comment:5 by , 8 years ago
| Milestone: | 16.09 → 16.10 |
|---|
by , 8 years ago
| Attachment: | 13165_v1.patch added |
|---|
comment:7 by , 8 years ago
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 by , 8 years ago
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:10 by , 8 years ago
| Milestone: | 16.12 → 17.01 |
|---|
comment:11 by , 8 years ago
| Milestone: | 17.01 → 17.02 |
|---|
comment:12 by , 8 years ago
| Milestone: | 17.02 → 17.03 |
|---|
comment:13 by , 8 years ago
| Milestone: | 17.03 → 17.04 |
|---|
comment:14 by , 8 years ago
| Milestone: | 17.04 → 17.05 |
|---|
comment:15 by , 8 years ago
| Summary: | [Patch] Validator did not warn about overlapping multipolygons → [Patch needs work] Validator did not warn about overlapping multipolygons |
|---|
comment:16 by , 8 years ago
| Milestone: | 17.05 |
|---|
comment:17 by , 6 years ago
| Keywords: | multipolygon added |
|---|
comment:19 by , 6 years ago
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.
by , 6 years ago
| Attachment: | 13165_v2.patch added |
|---|
comment:20 by , 6 years ago
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.
by , 5 years ago
| Attachment: | 13165_v3.patch added |
|---|
comment:21 by , 5 years ago
| Cc: | added |
|---|
like v2, but with performance improvements for MPs.
@Klumbumbus: Please can you try this patch reg. highlighting of overlapping areas?
comment:22 by , 5 years ago
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 by , 5 years ago
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.
by , 5 years ago
| Attachment: | 13165_v4.patch added |
|---|
comment:24 by , 5 years ago
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...
by , 5 years ago
| Attachment: | 13165_v5.patch added |
|---|
comment:25 by , 5 years ago
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:27 by , 5 years ago
| Milestone: | → 20.03 |
|---|
follow-up: 31 comment:30 by , 5 years ago
| Resolution: | fixed |
|---|---|
| Status: | closed → reopened |
comment:32 by , 5 years ago
| Resolution: | → fixed |
|---|---|
| Status: | reopened → closed |
No idea. Probably accidential.
I'll have a look at this.