Multiple warnings for same problem

[GerdP]

What steps will reproduce the problem?

1. Create two closed ways tagged natural=water which overlap
2. Run validator

What is the expected result?

One warning showing the two ways

What happens instead?

5 Warnings and 2 informations

Warnings (5)
- Crossing ways (1)
- Overlapping Identical Natural Areas (2)
- Overlapping Water Areas (2)
Other (2)
- Overlapping Areas (2)

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

The first warning is produced by validation.tests.CrossingWays, the others by MapCSSTagChecker. We should reduce the noise. I think the test in CrossingWays is better because it shows better where the overlap is.

Build-Date:2018-11-20 16:04:40
Revision:14436
Is-Local-Build:true

Identification: JOSM/1.5 (14436 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Home 1803 (17134)
Memory Usage: 617 MB / 3641 MB (354 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080
Maximum Screen Size: 1920x1080
VM arguments: [-agentlib:jdwp=transport=dt_socket,suspend=y,address=localhost:63406, -ea, -Dfile.encoding=UTF-8]
Program arguments: [--debug]
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (34535)
+ apache-commons (34506)
+ buildings_tools (34724)
+ download_along (34503)
+ ejml (34389)
+ geotools (34513)
+ importvec (34520)
+ jaxb (34506)
+ jts (34524)
+ merge-overlap (34664)
+ o5m (34405)
+ opendata (34698)
+ pbf (34576)
+ poly (34546)
+ reverter (34552)
+ undelete (34568)
+ utilsplugin2 (34506)

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/PublicTransport&zip=1

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.
- W: Region [TMS_BLOCK_v2] Resetting cache

[GerdP]

I tried to find a place to fix this but without success so far.

• It would be possible to finally check the list of errors for duplicates but that is quite complex would slow down processing for a small improvement as these case are rare.
• It might be possible to avoid the duplicate tests in geometry.mapcss so that only the Overlapping Water Areas (2) message is produced but we would still see the message twice for the two possible orders of the overlapping ways.
• If the CrossingWay test finds the same problem it gives the more detailed information by hiliting only the crossing way segments.

Anyway, as a side effect I found a nice optimization: r14973

[Klumbumbus]

Does the mapcss test find overlappings which the java test doesn't find? If no we could just remove the mapcss test and if yes maybe you could integrate the missing cases in the java test and then delete the mapcss tests?

[GerdP]

The java test only find overlaps where the ways are crossing without a shared node.

[GerdP]

My understanding is that the mapcss tests are the preferred solution in JOSM. If that is not the case I can try to code a specialized test for overlapping areas with the following goals:

• find overlaps for closed ways and multipolygons
• hilite the involved way segements to improve "zoom to problem"
• suppress duplicate errors for the same combination of objects

The problem is that I see no way to use such a function in the mapcss rules, as it would not work with the selectors.

[Klumbumbus]

Replying to GerdP:

My understanding is that the mapcss tests are the preferred solution in JOSM.

Yes as long as the test is possible with mapcss. mapcss has some limitations, among others:

• not possible to highlight way segments
• usually you can select only one object at a time (and have limited access to its parent or child object tags) but you cannot select or compare two different objects

For overlapping ways tests I prefer if the validator highlights the way segments as sometimes you have two very long ways which share a long distance together with lots of nodes and the overlapping point is hard to find without segment exact highlighting/zooming. This would mean a Java test or mapcss needs to be revamped.

[GerdP]

See also #17422

[GerdP]

problem cases for overlapping areas

[GerdP]

OK, I started to code a new OverlappingAreas test.
I found one more reason to do that : the current mapcss tests don't find overlaps where one area is inside the other (wood inside wood)

I think we need a basic Java test that finds those overlaps and a comfortable way to configure the combinations which should produce errors.
For example, two buildings sharing one or more segments are perfectly correct while two natural=wood areas should probably produce an info.

Maybe I find a way to use that test with mapcss rules.

[Klumbumbus]

Replying to GerdP:

two buildings sharing one or more segments are perfectly correct while two natural=wood areas should probably produce an info.

No. There are various of reasons that areas with the same "main key" touch each other. We finally removed this warning in #13818. Please don't bring it back.

Touching areas are fine, overlapping usually not.

[GerdP]

Proof of concept regarding hilite, also finds intersections of multipolygons with ways or multipolygons. Only checks identical natural or landuse areas for now.

[GerdP]

Suppress duplicate messages from MapCSSTagChecker where only the order of primitives is different

[GerdP]

distinguish between overlapping buildings and "Building inside building"

[GerdP]

@Klumbumbus:
I've improved the patch to find some more overlaping objects and reduce false positives.
For now, all new results are prefixed with OA: so that it is easier to compare with the existing tests.
In a final version I would remove the corresponding tests using ⧉ from geometry.mapcss and some others from CrossingWays.java.

Major differences to the existing tests are:

• In opposite to the mapcss tests multipolygons are also handled (slower) but duplicate tests are avoided (faster)
• In opposite to CrossingWays this test finds all overlaps (a inside or enclosing b, a crossing b with shared nodes), and it is slower.

I don't see a way to use the test with mapcss and I don't want to invent something very similar, so I've dropped the idea of a config file for now.

[GerdP]

• check layer tag

[GerdP]

• check layer tag as in CrossingWays
• don't treat natural=coastline as water area for now, this requires more thinking
• don't check multipolygon relations against its members (improves performance)

Reg. natural=coastline:
The mapcss rules use a trick by treating closed ways with natural=coastline as water area while they are in fact islands.
So, the mapcss rules don't find overlaps where the coastline is made of multiple ways. The CrossingWays test might find them when they don't share nodes, else they are not detected.
I could try to compute the sea area but this is error prone, so I think I'll only collect the unclosed natural=coastline ways which can be combined to closed rings (the larger islands).

[GerdP]

I am unsure what to do when elements are selected. The current behaviour in trunk version is a bit unpredictable: If you select the two larger polygons in attached 17011-sel.osm and run validator you'll get two warnings about "Overlapping Identical Natural Areas" although the selected elements don't overlap. If I got that right the mapcss test checks each selected way against all ways in the dataset and stops when an overlap is found.
So, it will not find multiple overlaps for the same area and it finds overlaps for elements not in the selection.

The new test would only find intersections between selected elements. I plan to change that so that selected elements are tested against all area elements.

[GerdP]

v4 implements

• check each selected element against all elements in same bbox
• status monitor update as in MapCSSTagChecker

@Klumbusbus: I wonder why the implementation for mapcss ⧉ (CrossingFinder) considers the layer tag while the ∈ (ContainsFinder) doesn't. The latter looks wrong to me. Is it intended?

[Klumbumbus]

Replying to GerdP:

Is it intended?

I don't know.

[GerdP]

Replying to Klumbumbus:

Replying to GerdP:

Is it intended?

I don't know.

My current thinking is that an object is not inside another object if they have different layers. If I got that right the rule

/* Building inside building (spatial test) */
*[building][building!~/no|entrance/][any(tag("layer"),"0") = any(parent_tag("layer"),"0")] ∈
*[building][building!~/no|entrance/] {
  throwWarning: tr("Building inside building");
}

implements this but other rules using the ∈ operator ignore it.
I also still have to make up my mind regarding handling of incomplete multipolygons, so still a lot to do...

[GerdP]

building inside building samples

[GerdP]

@Klumbumbus:
Please run validator on attached bib.osm. The upper right mp creates two warnings with trunk version, I wonder if the "building inside building" is intended here? Found this case in an older OSM extract.
JOSM behaves a bit strange here because both the relation and the outer way have the building=yes tag.
If I remove the tag from the multipolygon I get one error "Multipolygon relation should be tagged with area tags and not the outer way" instead of the two warnings (good), if instead I remove the tag from the outer way both warnings disappear (also good).
For the unchanged object I think we should see no warning "Building inside building" but one error "Area style on outer way" or maybe
"Area style repeated on outer way".
What do you think?

[GerdP]

With v5 the code also finds many intersections with incomplete Multipolygons. It does NOT yet find those intersections with incomplete MP where a way overlaps with shared points.
It also changes the code in MultipolygonTest to produce the "Area style repeated on outer way" error message mentioned in comment:15
When we remove the corresponding mapcss tests the overall performance is quite good.

[GerdP]

I wonder why coastline appears in these rules:

/* Overlapping areas (spatial test) */
area[natural =~ /^(water|wetland|coastline)$/], area[landuse=reservoir] {
  set water_area;
}

/* area:closed:areaStyle.water_area ⧉ area:closed:areaStyle.water_area -- does not work for now -- see ticket#10215 */
area:closed:areaStyle[natural =~ /^(water|wetland|coastline)$/] ⧉ area:closed:areaStyle.water_area,
area:closed:areaStyle[landuse=reservoir]                        ⧉ area:closed:areaStyle.water_area {
  throwWarning: tr("Overlapping Water Areas");
}

It doesn't seem to work with the default map style. I found no test case where "Overlapping Water Areas" is produced for a closed way with natural=coastline. I don't understand the use of areaStyle here.

[Klumbumbus]

Replying to GerdP:

For the unchanged object I think we should see no warning "Building inside building" but one error "Area style on outer way" or maybe
"Area style repeated on outer way".
What do you think?

Yes, the "Building inside building" warning is not needed in this case.

[Klumbumbus]

Replying to GerdP:

I wonder why coastline appears in these rules:

Probably it should find cases where e.g. a lake crosses the coastline and peaks into the ocean.

It doesn't seem to work with the default map style. I found no test case where "Overlapping Water Areas" is produced for a closed way with natural=coastline.

Me neither. It works with the internal Potlatch2 style though. Maybe it was tested with this style only in #10120.

I don't understand the use of areaStyle here.

I too don't understand atm why it was added. Coastline has no areastyle in the default style and I'm not aware of valid cases of natural=water|wetland or landuse=reservoir as non-areas.

[GerdP]

Example: Download w247650788,w247650836. A natural=wetland crossing a natural=coastline which describes an island.
With Potlatch2 style The warning is: "Overlapping Water Areas" which is vey misleading as the island is not a water area.
With default style I see no warning.
The CrossingWays test is also silent as it ignores natural=wetland:

    static boolean isCoastline(OsmPrimitive w) {
        return w.hasTag("natural", "water", "coastline") || w.hasTag("landuse", "reservoir");
    }

I am not sure but I think it is a common way to map natural=wetland areas overlapping/crossing other areas. Probably because there is no clear outline.

My conclusion so far:

• wetland and coastline should be removed from the mapcss rules
• areaStyle should be removed
• waterway=riverbank might be added (also to CrossingWays)

So I'll try to implement a replacement for this:

/* Overlapping areas (spatial test) */
area[natural=water], area[landuse=reservoir], area[waterway=riverbank] {
  set water_area;
}

/* area:closed:areaStyle.water_area ⧉ area:closed:areaStyle.water_area -- does not work for now -- see ticket#10215 */
area:closed[natural=water] ⧉ area:closed.water_area,
area:closed[landuse=reservoir]                        ⧉ area:closed.water_area,
area:closed[waterway=riverbank]                       ⧉ area:closed.water_area  {
  throwWarning: tr("Overlapping Water Areas");
}

BTW: I also wonder why it is not "Overlapping water areas" or "Overlapping identical landuses" (Only 1st word capitalized)

[GerdP]

I still try to make up my mind reg. "mapcss rules" vs. "java code" reg. spatial tests:
Quite a lot of work was invested to replace class OverlappingAreas by mapcss rules, so I really hesitate to revert this work again.
I've compiled a list of problems with the curent implementation of the selectors for ∈ (contains)and ⧉ (crossing area):

• multipolygons (MP) are either ignored (crossing) or not fully supported (contains doesn't find MP inside MP).
• if nothing is selected some tests are performed twice, e.g. "Overlapping Identical Landuses" will test A against B and B against A. The test is complex, so this is performance relevant and it also produces duplicate messages.
• It would be good to skip the tests "Overlapping Identical Natural Area" and "Overlapping Areas" when the same combination of two natural=water areas was already tested in the "Overlapping Water Areas" test. This is again releavant regarding performance and duplicated messages.
• The implementation for "zoom to problem" is poor as it "forgets" the results of the intersection test instead of storing it with the TestError instance(hilite)
• If objects are selected the results are a bit unpredictable. Example: When I have a node building=yes inside a way with building=yes I get a correct warning "Building inside building" when nothing is selected. I also get this warning when I select the way, but not when I select only the node. I did not yet find out why.
• #10215 is still open

I'd prefer to improve the java code for "mapcss rules", else we would have two maintain both solutions or remove the support for spatial tests in mapcss.
I think I can solve the MP problems and maybe also the next three issues. I hope the "builing inside building" problem is rather small and I have an idea for #10215

[GerdP]

I've started to improve the code for the mapcss tests: r15064, r15068:15069
Next on my todo list for the coming week:

• implement better highlighting for CrossingFinder (store information about crossing elements in Environment)
• implement support for mulipolygons in CrossingFinder using similar code as for ContainsFinder
• maybe implement a cache for intersection results (is probably needed for complex MP)

@Klumbumbus:
With r15069 we still have this result for the given two overlapping natural=water areas:

Warnings (3)
- Crossing ways (1)
- Overlapping Identical Natural Areas (1)
- Overlapping Water Areas (1)
Other (1)
- Overlapping Areas (1)

Once the CrossingFinder fully works we could change the CrossingWays test to ignore areas but maybe we need a new kind of flag in the rules. If we have two rules like those for "Overlapping Water Areas" and "Overlapping Identical Natural Areas" JOSM doesn't know that it can skip the latter if the preceding rule matched (of course it cannot skip the latter if it sets a new flag)
Simpler alternative: Given two objects which match both ⧉ rules, should we simply assume that the "throwWarning" clause of the latter rule(s) can be ignored? This would require that those rules are ordered by severity.
What do you think?

[qeef]

Replying to GerdP:

• If objects are selected the results are a bit unpredictable. Example: When I have a node building=yes inside a way with building=yes I get a correct warning "Building inside building" when nothing is selected. I also get this warning when I select the way, but not when I select only the node. I did not yet find out why.

Because ∈ is implemented in the opposite way it should, i.e. if x∈Y then Y is selected. You already found out https://josm.openstreetmap.de/ticket/10391.

I'd prefer to improve the java code for "mapcss rules", else we would have two maintain both solutions or remove the support for spatial tests in mapcss.

I also prefer improving java code for mapcss rules.

[GerdP]

In 15129/josm:

see #17011: Ignore relations without members in DuplicateRelation test

Don't produce "Relations with same members" for relations without members. They will be flagged as empty relations.