Modify

Opened 14 months ago

Last modified 10 months ago

#20130 assigned enhancement

Use mapcss rules instead of CrossingWays java code to find overlapping areas

Reported by: GerdP Owned by: GerdP
Priority: normal Milestone:
Component: Core validator Version:
Keywords: crossing Cc: Klumbumbus, simon04

Description (last modified by GerdP)

Today we use the java code in CrossingWays to find e.g. "Crossing building/residential area".
This should better be done using a rule like this

/* Building overlapping landuse residential (spatial test) */
area[building][building!~/no|entrance/]  area[landuse=residential]{
  throwWarning: tr("Overlapping buildings/residential area");
}

Reasoning:

  • it also handles multipolygons (if they are complete)
  • it marks the overlapping area, not the crossing segments in fact this is a contra argument
  • it also handles overlaps where the two areas share nodes

In r17084 CrossingWays also finds crossings between

  • building/waterway=riverbank ("Crossing building/waterway")
  • building/railway=platform ("Crossing building/railway" (false positive?)

It doesn't find other crossing like those between building/natural=water, see #20121

Maybe it is possible to create a single mapcss test to catch different types of areas which shouldn't overlap a building?

I didn't test the impact on performance, I assume it is rather small.

Attachments (2)

20130.patch (4.6 KB) - added by GerdP 13 months ago.
20130.2.patch (10.5 KB) - added by GerdP 13 months ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 14 months ago by GerdP

Description: modified (diff)

comment:2 Changed 14 months ago by Don-vip

Cc: simon04 added
Component: CoreCore validator
Keywords: crossing added

I think we already discussed about it in another ticket and there was a problem, but I don't remember what. I believe CrossingWays is doing something better than the mapCSS rule, maybe the UI reporting of the crossing location, or something like that.

comment:3 Changed 14 months ago by GerdP

In the past, the highlighting of CrossingWays was better, now it's rather the other way around.

comment:4 in reply to:  description Changed 14 months ago by skyper

Replying to GerdP:

Reasoning:

  • it marks the overlapping area, not the crossing segments

Actually with long ways, I prefer the highlighting of the crossing segments. Selecting the object is already possible, so highlighting the whole ways is not an advantage.

comment:5 Changed 14 months ago by GerdP

so highlighting the whole ways is not an advantage.

I don't understand. It is not highlighting the whole way unless the whole area is overlapping. With building the area is rarely large. F6 zooms to the interesting (=overlapping) area.
Please try the proposed rule and compare the highlighting.

Anyway, more important is that it finds all overlaps.

comment:6 Changed 14 months ago by GerdP

No more objections?

comment:7 Changed 14 months ago by GerdP

OK, I'll commit the changes and we'll see what happens...

comment:8 Changed 14 months ago by GerdP

Well, now I remember why this wasn't already done: The marked overlapping area is that inside the residential area, but the problematic part is that which is outside.
So, we need a different selector for this.

Changed 13 months ago by GerdP

Attachment: 20130.patch added

comment:9 Changed 13 months ago by GerdP

Patch implements new selector ⧉o . Works like ⧉ but marks the part of the child that is outside. It also contains this rule:

area:closed:areaStyle[landuse=residential] o area[building][building!~/no|entrance/] {
  throwWarning: tr("Building partly outside of residential area");
}

I found no obvious single unicode character to express this. Please suggest improvements.

comment:10 Changed 13 months ago by GerdP

Milestone: 21.01
Owner: changed from team to GerdP
Status: newassigned

Changed 13 months ago by GerdP

Attachment: 20130.2.patch added

comment:11 Changed 13 months ago by GerdP

20130.2.patch

  • improves the java code so that it doesn't produce errors where the highlighted area is empty. This happened with landuse=* multipolygons where the building just shares a node. (Unfortunately the "overlapping areas" test still marks this, will attach an example)
  • removes the handling of residential areas in CrossingWays
  • finds more landuse areas with the rule
    area:closed:areaStyle[landuse=~ /^(commercial|farmyard|garages|industrial|retail|residential)$/] o area[building][building!~/no|entrance/] {
      throwWarning: tr("Building partly outside of landuse area");
    }
    

If I see no complains or suggestions for improvements I'll commit this tomorrow.

comment:12 Changed 13 months ago by Hb---

Why not stay with the old string "Crossing building/residential area"? All crossing test messages begin with "Crossing".

comment:13 Changed 13 months ago by GerdP

I tried to make clear that the building should be completely inside the landuse area.
I would not mind to use "Crossing building/residential area" but the new test also complains about other types of areas, so we would either need one test for each type or a generic "Crossing building/{0} area" unless you think that "residential" is also a generic word for the listed types. I didn't find a way to extract the value of the parent tag for the {0] placeholder in the message. Any help would be welcomed.
"Crossing building/landuse area, should be inside" might be a better alternative.

comment:14 Changed 13 months ago by Hb---

Similar errors should have similar messages. A bad example of the opposite is the translation of "Crossing railways" to "Bahnkreuzung" in current josm-tested.

Landuse areas may neighbour each other. So peaking of "inside" may be wrong. It is more important that the landuse boundary does not cross the building. Maybe "Crossing building/landuse boundary" would say this.

But "Crossing building/landuse area" is fine, too.

comment:15 Changed 13 months ago by GerdP

But "Crossing building/landuse area" is fine, too.

Would you want to get that message for all landuse areas? If yes we need a 2nd test which would use the normal operator for all landuses not listed in the first test so that the correct part of the building is highlighted. Or at least a list of those landuses which are typically not containing buildings (forest, farmland, reservoir, ...). I'd prefer to warn only about those which typically contain buildings.

comment:16 in reply to:  13 Changed 13 months ago by skyper

Replying to GerdP:

I didn't find a way to extract the value of the parent tag for the {0] placeholder in the message. Any help would be welcomed.

Does it work with an additional [landuse]?

area:closed:areaStyle[landuse=~ /^(commercial|farmyard|garages|industrial|retail|residential)$/][landuse] o area[building][building!~/no|entrance/] {
  throwWarning: tr("Building partly outside of {0} area", "{1.tag}" );
}

comment:17 Changed 13 months ago by GerdP

No, "{1.tag}" refers to the tags of the building way. I'll try to change the logic so that the landuse area is the child.

comment:18 in reply to:  15 Changed 13 months ago by Hb---

Replying to GerdP:

Would you want to get that message for all landuse areas?

No. My scope was limited to the message, not the broadness of the test.

Or at least a list of those landuses which are typically not containing buildings (forest, farmland, reservoir, ...). I'd prefer to warn only about those which typically contain buildings.

I misunderstood the test. It's not a simple Crossing test anymore. Seems that you implement a "Badly placed buildings" test. I would fear to much false positives. A hut in a forest or a stable on a meadow seem to be very common. But a stable crossing the boundary between a forest and a meadow should give a warning.

comment:19 Changed 13 months ago by GerdP

No, my primary goal is to use the mapcss code because it also handles multipolygons. I tried to implement that with the CrossingWays test but that's a lot more complex because the message creation would need a lot of changes.

OTOH comment:17 is also not a good solution. It is quite normal to have a landuse=residential glued to a landuse=commercial and I didn't think about this. If a building crosses both we'll get two different messages for the same problem, but with different highlighted areas.

Working on another solution now...

comment:20 Changed 13 months ago by GerdP

Description: modified (diff)

comment:21 Changed 13 months ago by GerdP

I misunderstood the test. It's not a simple Crossing test anymore

Right, because the problem never was about crossing ways. A building overlapping a landuse area is still wrong when it shares nodes with the landuse boundary.
I don't want to remove the test CrossingWays, I just want a better test to find the problematic areas.

comment:22 Changed 12 months ago by Don-vip

Milestone: 21.0121.02

comment:23 Changed 12 months ago by stoecker

Milestone: 21.0221.03

Milestone renamed

comment:24 Changed 10 months ago by Don-vip

Milestone: 21.03

comment:25 Changed 10 months ago by skyper

@GerdP:
Please, have a look at #20378 and #20680. Crossing and Overlapping test need to watch the child nodes of the ways in order to trigger a warning on upload.

comment:26 Changed 10 months ago by GerdP

Yes, many tests fail whe they are executed with a selection. It's not enough to check the ways which have modified nodes.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as assigned The owner will remain GerdP.
as The resolution will be set.
to The owner will be changed from GerdP to the specified user.
The owner will change to GerdP
as duplicate The resolution will be set to duplicate.The specified ticket will be cross-referenced with this ticket

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.