Modify

Opened 2 years ago

Closed 9 months ago

Last modified 5 months ago

#16707 closed enhancement (fixed)

Warn about overlapping connected buildings

Reported by: naoliv Owned by: GerdP
Priority: normal Milestone: 20.03
Component: Core validator Version:
Keywords: building overlap Cc: GerdP, michael2402, Klumbumbus

Description

It seems that for cases like this (also attached the example) JOSM should warn about the overlapping buildings.

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

We can only see an informational level message about "Overlapping Areas", but not any kind of warning about the overlapping buildings.

JOSM:

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2018-09-02 22:28:43 +0200 (Sun, 02 Sep 2018)
Revision:14220
Build-Date:2018-09-03 01:32:21
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (14220 en) Linux Debian GNU/Linux testing (buster)
Memory Usage: 819 MB / 7168 MB (329 MB allocated, but free)
Java version: 10.0.2+13-Debian-1, Oracle Corporation, OpenJDK 64-Bit Server VM
Screen: :0.0 1600x900, :0.1 1280x1024
Maximum Screen Size: 1600x1024
Java package: openjdk-10-jre:amd64-10.0.2+13-1
Java ATK Wrapper package: libatk-wrapper-java:all-0.33.3-21
VM arguments: [--add-modules=java.activation,java.se.ee, -Dawt.useSystemAAFontSettings=gasp]
Program arguments: [--language=en]
Dataset consistency test: No problems found

Attachments (3)

example.osm (1.3 KB) - added by naoliv 2 years ago.
16707.patch (1.3 KB) - added by GerdP 9 months ago.
16707.2.patch (4.3 KB) - added by GerdP 9 months ago.

Download all attachments as: .zip

Change History (26)

Changed 2 years ago by naoliv

Attachment: example.osm added

comment:1 Changed 2 years ago by Don-vip

Keywords: building overlap added
Milestone: 18.09
Type: enhancementdefect

comment:2 Changed 2 years ago by Don-vip

Cc: GerdP michael2402 added
Milestone: 18.09
Type: defectenhancement

Not so easy as I thought. CrossingWays should probably be reworked to detect this, but it could quickly result in a major performance loss. Do we have a volunteer for the task? :)

comment:3 Changed 10 months ago by GerdP

We can use the routines from MultipolygonTest for this. Something like
collect all building polygons (not multipolygons) in collection buildings and call

  MultipolygonTest mptest = new MultipolygonTest();
  mpTest.makeFromWays(buildings);
  for (TestError err mpTest.getErrors()) {
    ...
  }

Of course we have to filter warnings about touching rings.
We could also examine the resulting relation to check if any inners were created.

comment:4 Changed 10 months ago by GerdP

Cc: Klumbumbus added

We have a test in geometry.mapcss

area:closed:areaStyle  area:closed:areaStyle {
  throwOther: tr("Overlapping Areas");
}

which finds this kind of problem.
We could add rules like this to produce a warning for some of the objects which are tested in CrossingWays and remove them from CrossingWays or I might add a new java test OverlappingAreas which would perform those tests.
What would be the preferred solution?

comment:5 Changed 10 months ago by Klumbumbus

Java test can highlight the affected way segments, which is a big advantage over the mapcss tests.

comment:6 Changed 10 months ago by GerdP

Milestone: 20.03
Owner: changed from team to GerdP

With buildings the highlighting is probably not so important, but with large natural polygons it can help. I've started to code today and first tests look promising. Some fine tuning is needed reg. layer tag, but performance should be no problem.

comment:7 Changed 9 months ago by GerdP

Seems I forgot what I wrote in ticket:17011#comment:21 and later :(
I think I failed because I didn't like the changes needed to implement better highlighting.

comment:8 Changed 9 months ago by GerdP

In 15938/josm:

see #16707: improve highlighting of overlapping areas and "zoom to error"

  • store the overlapping area with the TestError
  • return also the intersection area in Geometry.polygonIntersectionResult()

Changed 9 months ago by GerdP

Attachment: 16707.patch added

comment:9 Changed 9 months ago by GerdP

The attached patch adds a test in geometry.mapcss and disables the corresponding test in the java code.
Please review the mapcss rule. I don't think we need the entrance part here but it also exists in the above rule for "building inside building", so I copied it.
@Klumbumbus: I think we should also replace these spatial tests in CrossingWays:
"Crossing building/residential area"
"Crossing residential areas"
"Crossing residential area/way"

comment:10 Changed 9 months ago by Klumbumbus

The entrance part seems to be obsolete meanwhile. When it was added in 2014 (r6611) building=entrance had still ~135.000 uses. However mapped as area was always strange I guess (~500 at this time).
On the other hand the entrance part makes sense as building=entrance is deprecated in general. So it avoids throwing the "building inside building" warning for a case where we have the "is deprecated" warning anyway. Then again, we have more deprecated building values in deprecated.mapcss which could be added too...

comment:11 Changed 9 months ago by GerdP

Problem with test "Crossing building/residential area":
If we replace that with the spatial test the "good" part of the building is highlighted :(

Changed 9 months ago by GerdP

Attachment: 16707.2.patch added

comment:12 Changed 9 months ago by GerdP

Please review 16707.2.patch:

  • use spatial test in geometry.mapcss instead of CrossingWays.java to find overlapping buildings or overlapping residential areas.
  • "Crossing buildings" is replaced by "Overlapping buildings"
  • "Crossing residential areas" is replaced by "Overlapping identical landuses"

If I got that right the message "Crossing residential area/way" is never actually produced because closed areaStyle ways with e.g. amenity=parking are ignored in CrossingWays.

comment:13 Changed 9 months ago by GerdP

I forgot about the tag level=*
In CrossingWays we ignore all objects with different level tag. Should we do the same in the CrossingFinder?
Up to now CrossingFinder evaluates the layer tag only. I think this is OK, see #18293.

Last edited 9 months ago by GerdP (previous) (diff)

comment:14 Changed 9 months ago by GerdP

Resolution: fixed
Status: newclosed

In 15961/josm:

fix #16707: Warn about overlapping connected buildings

  • add rule in geometry.mapcss to check overlapping buildings
  • change rule which checks overlapping identical landuses to also check landuse=residential
  • disable the corresponding tests in CrossingWays

effects for the user:

  • Message "Crossing buildings" is replaced by "Overlapping buildings"
  • Message "Crossing residential areas" is replaced by generic "Overlapping Identical Landuses"
  • different highlighting of the overlapping area

comment:15 Changed 9 months ago by skyper

please, include building:part=* in building test. Thanks

comment:16 Changed 9 months ago by GerdP

@Klumbumbus: Do you know how?

comment:17 Changed 5 months ago by Klumbumbus

In 16779/josm:

see #16707 - Warn about overlapping building:part too

comment:18 Changed 5 months ago by Klumbumbus

Thanks to someone trying to add spam to this ticket I saw this forgotten rule :D

comment:19 Changed 5 months ago by Klumbumbus

Hmm. Thinking about it again r16779 likely produces too much false positives. building:part usually is not used with layer=* so overlaps exist. More opinions?

comment:20 Changed 5 months ago by GerdP

That's why I wrote comment:16
I was also unsure if building:part=* is allowed to overlap building=*

comment:21 in reply to:  20 ; Changed 5 months ago by Klumbumbus

Replying to GerdP:

if building:part=* is allowed to overlap building=*

I would say no. This line should be fine:

area[building][building!~/no|entrance/]  area[building:part][building:part!~/no|entrance/],

comment:22 in reply to:  21 Changed 5 months ago by skyper

So we need to look at building:levels and building:min_level to find overlapping building:parts. Still would add a sharing crossing node if the parts are attached, e.g. on top of each other without space in between.

Replying to Klumbumbus:

Replying to GerdP:

if building:part=* is allowed to overlap building=*

I would say no. This line should be fine:

area[building][building!~/no|entrance/]  area[building:part][building:part!~/no|entrance/],

+1, but I have not looked at real data, yet, and where is a complex example on the wiki?

Last edited 5 months ago by skyper (previous) (diff)

comment:23 Changed 5 months ago by Klumbumbus

Let's continue in #19544.

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.