Modify

Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#16188 closed enhancement (fixed)

[PATCH] Add support for crossing of residential areas with something.

Reported by: marxin Owned by: team
Priority: normal Milestone: 18.04
Component: Core validator Version: latest
Keywords: Klumbumbus Cc: Klumbumbus

Description

The patch makes detection of area more generic and adds support for residential areas.

Attachments (1)

0001-Add-support-for-crossing-of-residential-areas-with-s.patch (7.4 KB) - added by marxin 10 months ago.
V3 of the patch

Download all attachments as: .zip

Change History (17)

comment:1 Changed 10 months ago by Don-vip

Component: CoreCore validator
Keywords: Klumbumbus added

Thanks for the patch!
I see some issues to fix before merging:

  • the removal of mapcss rule means there are several use cases no longer detected (for example, two crossing industrial landuses). It should rather skip the residential value.
  • the translations using string concatenation (tr("Crossing " + types[0] + "s")) won't work. The text to translate is statically extracted from source code. Full strings must be used.

comment:2 in reply to:  1 ; Changed 10 months ago by marxin

Replying to Don-vip:

Thanks for the patch!
I see some issues to fix before merging:

  • the removal of mapcss rule means there are several use cases no longer detected (for example, two crossing industrial landuses). It should rather skip the residential value.

Can you please help me how to exclude landuse=residential in the mapcss. I'm not much familiar with that.

  • the translations using string concatenation (tr("Crossing " + types[0] + "s")) won't work. The text to translate is statically extracted from source code. Full strings must be used.

Oh, you are right. I'm sending updated patch, but it doesn't scale much as one needs to explicitly handle all N*(N-1) possible values.

comment:3 in reply to:  2 ; Changed 10 months ago by Don-vip

Cc: Klumbumbus added

Replying to marxin:

Can you please help me how to exclude landuse=residential in the mapcss. I'm not much familiar with that.

Probably something like that:

#mapcss
area:closed:areaStyle[landuse!=residential][tag("landuse") = parent_tag("landuse")] ⧉ area:closed:areaStyle[landuse!=residential]

MapCSS expert in copy :)

comment:4 in reply to:  3 ; Changed 10 months ago by Klumbumbus

OK, now that you added me in keywords and cc you got my attention ;)

Replying to Don-vip:

Probably something like that:

area:closed:areaStyle[landuse!=residential][tag("landuse") = parent_tag("landuse")]  area:closed:areaStyle[landuse!=residential]

Yes, simply add [landuse!=residential] to exclude this tag, but I think you forgot/overwrote the last [landuse] at the end of the selector:

area:closed:areaStyle[landuse!=residential][tag("landuse") = parent_tag("landuse")]  area:closed:areaStyle[landuse!=residential][landuse]

comment:5 in reply to:  4 ; Changed 10 months ago by Don-vip

Replying to Klumbumbus:

OK, now that you added me in keywords and cc you got my attention ;)

Haha sorry I often do the mistake.

I think you forgot/overwrote the last [landuse] at the end of the selector:

This was intentional. [landuse!=residential] implies we have a landuse value, different from residential, right?

comment:6 in reply to:  5 Changed 10 months ago by Klumbumbus

Replying to Don-vip:

[landuse!=residential] implies we have a landuse value, different from residential, right?

No, e.g. way[landuse!=residential]{} matches a way tagged only with natural=wood and an untagged way too

Last edited 10 months ago by Klumbumbus (previous) (diff)

comment:7 Changed 10 months ago by marxin

So can you please provide a final version of the mapcss change.
I'm bit confused which selector is the right one?

comment:8 Changed 10 months ago by Klumbumbus

If I understand the changed Java test right, then the mapcss selector should change to:

area:closed:areaStyle[landuse!=residential][tag("landuse") = parent_tag("landuse")]  area:closed:areaStyle[landuse!=residential][landuse]

Changed 10 months ago by marxin

V3 of the patch

comment:9 in reply to:  8 ; Changed 10 months ago by marxin

Replying to Klumbumbus:

If I understand the changed Java test right, then the mapcss selector should change to:

area:closed:areaStyle[landuse!=residential][tag("landuse") = parent_tag("landuse")]  area:closed:areaStyle[landuse!=residential][landuse]

Thanks for that. I verified it works for e.g. industrial landuses. Due to that I found one issue in my patch:

p.get("landuse") == "residential"

is different from

p.hasKey("landuse", "residential")

I'm attaching new version of the patch.

Last edited 10 months ago by Don-vip (previous) (diff)

comment:10 in reply to:  9 Changed 10 months ago by Don-vip

Replying to marxin:

p.get("landuse") == "residential"

is different from

p.hasKey("landuse", "residential")

Yes the good call is p.hasTag("landuse", "residential").
p.hasKey("landuse", "residential") means p has landuse=* or residential=*

Version 0, edited 10 months ago by Don-vip (next)

comment:11 Changed 10 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 13671/josm:

fix #16188 - Detect crossing of residential areas (patch by marxin, modified)

comment:12 Changed 10 months ago by Don-vip

Thanks for the patch! I have modified it a little bit to replace the string by an enum. I didn't test it much so I hope I didn't break anything.

comment:13 Changed 10 months ago by Don-vip

In 13678/josm:

see #16188 - rework new test

comment:14 Changed 10 months ago by Don-vip

Milestone: 18.04

comment:15 Changed 10 months ago by Don-vip

With #16264 I agree: we shouldn't raise a warning when a waterway crosses a residential area. This is quite common.

comment:16 Changed 10 months ago by Don-vip

In 13721/josm:

fix #16264, see #16188, see #16189 - reduce validator false positives:

  • increase tolerance of buildings with almost right angle to 1 degree
  • detect crossing residential areas only with themselves and buildings
  • fix crossing railways messages

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.