Modify

Opened 3 years ago

Closed 10 months ago

#18217 closed enhancement (fixed)

[PATCH] Complain about area=yes on major roads (like highway=primary area=yes)

Reported by: mkoniecz Owned by: team
Priority: normal Milestone: 21.07
Component: Core validator Version:
Keywords: template_report area highway Cc: Klumbumbus

Description

What steps will reproduce the problem?

  1. create way, tag it with highway=primary area=yes noname=yes
  2. run validator

What is the expected result?

Validator complains, maybe mentions that area:highway should be used for that

What happens instead?

Nothing

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

http://overpass-turbo.eu/s/N0p finds 25k cases

I spotted problem during generating a custom map and encountering highway=unclassified area=yes where highway=service area=yes was a correct tagging.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2019-10-08 23:08:20 +0200 (Tue, 08 Oct 2019)
Build-Date:2019-10-08 21:10:57
Revision:15445
Relative:URL: ^/trunk

Identification: JOSM/1.5 (15445 en) Linux Ubuntu 16.04.6 LTS
Memory Usage: 405 MB / 869 MB (152 MB allocated, but free)
Java version: 1.8.0_201-b09, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: :0.0 1366x768
Maximum Screen Size: 1366x768
libcommons-logging-java: libcommons-logging-java:all-1.2-1+build1
fonts-noto: fonts-noto:-
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (34977)
+ PicLayer (35104)
+ apache-commons (35092)
+ buildings_tools (35171)
+ continuosDownload (82)
+ ejml (35122)
+ geotools (35169)
+ imagery_offset_db (34908)
+ jts (35122)
+ log4j (34908)
+ measurement (35051)
+ reverter (35084)
+ todo (30306)

Validator rules:
+ ${HOME}/Documents/install_moje/OSM software/josm/data/validator/deprecated.mapcss
+ ${HOME}/Documents/install_moje/OSM software/josm/data/validator/unnecessary.mapcss
+ ${HOME}/Documents/install_moje/OSM software/josm/data/validator/combinations.mapcss

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.
- W: Failed to add ${HOME}/Documents/install_moje/OSM software/josm/data/validator/deprecated.mapcss to tag checker
- W: java.nio.file.NoSuchFileException: ${HOME}/Documents/install_moje/OSM software/josm/data/validator/deprecated.mapcss
- W: Failed to add ${HOME}/Documents/install_moje/OSM software/josm/data/validator/unnecessary.mapcss to tag checker
- W: java.nio.file.NoSuchFileException: ${HOME}/Documents/install_moje/OSM software/josm/data/validator/unnecessary.mapcss
- W: Failed to add ${HOME}/Documents/install_moje/OSM software/josm/data/validator/combinations.mapcss to tag checker
- W: java.nio.file.NoSuchFileException: ${HOME}/Documents/install_moje/OSM software/josm/data/validator/combinations.mapcss

Attachments (6)

18217.patch (2.4 KB) - added by Gabe Reichenberger 12 months ago.
major highways with area=yes
18217.2.patch (2.4 KB) - added by Gabe Reichenberger 12 months ago.
KEEP This has a very small update from the previous 18217.patch switching contains to equals in string comparison.
18217_updated.patch (819 bytes) - added by Gabe 12 months ago.
mapcss regarding area=yes with major highways
josm_18217.patch (1.4 KB) - added by skyper 12 months ago.
patch
josm_18217_v2.patch (1.3 KB) - added by skyper 12 months ago.
patch version 2: shorter class and correct grammar in message
josm_18217_v3.patch (1.1 KB) - added by skyper 11 months ago.
version 3 without additional class

Download all attachments as: .zip

Change History (38)

comment:1 Changed 3 years ago by Klumbumbus

with modifiable search area --> http://overpass-turbo.eu/s/N0y

comment:2 Changed 3 years ago by Don-vip

Keywords: area highway added

Changed 12 months ago by Gabe Reichenberger

Attachment: 18217.patch added

major highways with area=yes

comment:3 Changed 12 months ago by Gabe Reichenberger

Added way validation test with the following rules:

1) Way has area=yes tag.
2) Way has highway tag >= tertiary (including links).

If both are true user gets warning that "area=yes" should not be used on major highways (tertiary or above).

comment:4 Changed 12 months ago by anonymous

Summary: Complain about area=yes on major roads (like highway=primary area=yes)[PATCH] Complain about area=yes on major roads (like highway=primary area=yes)

Changed 12 months ago by Gabe Reichenberger

Attachment: 18217.2.patch added

KEEP This has a very small update from the previous 18217.patch switching contains to equals in string comparison.

comment:5 Changed 12 months ago by GerdP

Some remarks:

  • Do you know the hasTag() mehthod?
  • Why don't you use Set.contains()?
  • I suggest tr("Found area=yes on major highway.") for the message to avoid the confusing upper case Area.

comment:6 Changed 12 months ago by GerdP

BTW: The linked overpass query asks also for unclassified and residential. Without those the current count is just 145 major highways with area=yes.

comment:7 Changed 12 months ago by GerdP

Hmm,sorry, I see no reason why this test needs java code instead of mapcss in either highways.mapcss or combinations.mapcss

comment:8 Changed 12 months ago by skyper

Mmh, why not use mapcss instead of java? This should work, does it not?

area:closed2[highway =~ /^(motorway|motorway_link|trunk|trunk_link|primary|primary_link|secondary|secondary_link|tertiary|tertiary_link|residential|unclassified)$/][area=yes][highway] {
  throwError: tr("{0} together with {1} is invalid", "{3.tag}", "{4.tag}");
  group: tr("suspicious tag combination");
  suggestAlternative: "area:highway=*";
  assertMatch:   "area highway=trunk area=yes";
  assertNoMatch: "area highway=service area=yes";
  assertNoMatch: "way highway=trunk";
}
Last edited 12 months ago by skyper (previous) (diff)

comment:9 in reply to:  5 Changed 12 months ago by anonymous

Replying to GerdP:

Some remarks:

  • Do you know the hasTag() mehthod?
  • Why don't you use Set.contains()?
  • I suggest tr("Found area=yes on major highway.") for the message to avoid the confusing upper case Area.

1) The hasTag and hasKey method have similar functionality.
2) For each element of the major highway tag set, an individual features’ highway tag must be an exact match.
3) I can adjust the message.

comment:10 in reply to:  6 Changed 12 months ago by anonymous

Replying to GerdP:

BTW: The linked overpass query asks also for unclassified and residential. Without those the current count is just 145 major highways with area=yes.

145 globally? Talked with some expert mappers and apparently there are some cases where highway=<anything under tertiary> can be used in conjunction with area=yes? If this isn’t true I can definitely update.

comment:11 in reply to:  8 ; Changed 12 months ago by anonymous

Replying to skyper:

Mmh, why not use mapcss instead of java? This should work, does it not?

area:closed2[highway =~ /^(motorway|motorway_link|trunk|trunk_link|primary|primary_link|secondary|secondary_link|tertiary|tertiary_link|residential|unclassified)$/][area=yes][highway] {
  throwError: tr("{0} together with {1} is invalid", "{3.tag}", "{4.tag}");
  group: tr("suspicious tag combination");
  suggestAlternative: "area:highway=*";
  assertMatch:   "area highway=trunk area=yes";
  assertNoMatch: "area highway=service area=yes";
  assertNoMatch: "way highway=trunk";
}

I’m not familiar with mapcss but if that works then sure, add it! I’ll be focusing more on adding via Java.

comment:12 Changed 12 months ago by GerdP

Replying to anonym:

1) The hasTag and hasKey method have similar functionality.

No, hasTag("area", "yes") is simpler and easier to read than way.hasKey("area") && way.get("area").equals("yes").
Besides that sonarlint will complain about your code, it wants "yes".equals(way.get("area")) which also allows to omit the hasKey() check.

2) For each element of the major highway tag set, an individual features’ highway tag must be an exact match.

I would use something like this

if (hasTag("area", "yes") && MAJOR_HIGHWAYS.contains(way.get("highway"))) ...

3) I can adjust the message.

I think there is no need to adjust something in the patch, we always prefer to use mapcss rules where possible.

comment:13 in reply to:  12 Changed 12 months ago by anonymous

Replying to GerdP:

Replying to anonym:

1) The hasTag and hasKey method have similar functionality.

No, hasTag("area", "yes") is simpler and easier to read than way.hasKey("area") && way.get("area").equals("yes").
Besides that sonarlint will complain about your code, it wants "yes".equals(way.get("area")) which also allows to omit the hasKey() check.

2) For each element of the major highway tag set, an individual features’ highway tag must be an exact match.

I would use something like this

if (hasTag("area", "yes") && MAJOR_HIGHWAYS.contains(way.get("highway"))) ...

3) I can adjust the message.

I think there is no need to adjust something in the patch, we always prefer to use mapcss rules where possible.

Oh that hasKey function. Forgot about that. Definitely better.

Cool, if that mapcss addition functions as intended ignore my Java

comment:14 in reply to:  11 ; Changed 12 months ago by skyper

Replying to anonymous:

145 globally? Talked with some expert mappers and apparently there are some cases where highway=<anything under tertiary> can be used in conjunction with area=yes? If this isn’t true I can definitely update.

Please, share their thoughts and do you have an valid example?

Personally, I like to get completely rid of highway=* plus area=yes but that will take time. One major problem is the lack of support of area:highway=* in many software, e.g. #20102.

Replying to anonymous:

I’m not familiar with mapcss but if that works then sure, add it! I’ll be focusing more on adding via Java.

It is untested, written in a hurry and probably needs little polish, but should be enough for a proof of concept.

comment:15 in reply to:  14 Changed 12 months ago by anonymous

Replying to skyper:

Replying to anonymous:

145 globally? Talked with some expert mappers and apparently there are some cases where highway=<anything under tertiary> can be used in conjunction with area=yes? If this isn’t true I can definitely update.

Please, share their thoughts and do you have an valid example?

Personally, I like to get completely rid of highway=* plus area=yes but that will take time. One major problem is the lack of support of area:highway=* in many software, e.g. #20102.

Replying to anonymous:

I’m not familiar with mapcss but if that works then sure, add it! I’ll be focusing more on adding via Java.

It is untested, written in a hurry and probably needs little polish, but should be enough for a proof of concept.

According to https://wiki.openstreetmap.org/wiki/Key:area looks like highway=pedestrian is the only highway tag that has a clear way of being used so looks like we can adjust the validation for everything except highway=pedestrian

I can test shortly as I learn mapcss real quick. :)

comment:16 Changed 12 months ago by GerdP

I think highway=residential + area=yes is / was used often to map objects for which we now have place=square.

comment:17 Changed 12 months ago by mkoniecz

looks like highway=pedestrian is the only highway tag that has a clear way of being used

highway=service highway=track highway=footway highway=path can also be combined with area=yes in a valid tagging describing actual features

Big part of highway=service + area=yes is incorrect tagging for the renderer, but not everything

Last edited 12 months ago by mkoniecz (previous) (diff)

comment:18 in reply to:  16 ; Changed 12 months ago by Gabe

Replying to GerdP:

I think highway=residential + area=yes is / was used often to map objects for which we now have place=square.

Can you please point me to the destination where I can apply the mapcss?

comment:19 in reply to:  18 Changed 12 months ago by anonymous

Replying to Gabe:

Replying to GerdP:

I think highway=residential + area=yes is / was used often to map objects for which we now have place=square.

Can you please point me to the destination where I can apply the mapcss?

found it, nvm!

comment:20 Changed 12 months ago by skyper

As already mentioned, this test could be added to trunk/resources/data/validator/combinations.mapcss or trunk/resources/data/validator/highway.mapcss. With the later a local class on top of the file for the highway values could be added.

For testing you can always place the rules in a local file and load it via Tag checker rules tab in the validator preferences.

Changed 12 months ago by Gabe

Attachment: 18217_updated.patch added

mapcss regarding area=yes with major highways

comment:21 Changed 12 months ago by Gabe

Patch above seems to be working as intended. Only suggestion is to have ONLY the way's highway tag in the error message instead of all the invalid tags? What do you think? Show all invalid tags with area=yes or just the scenario specific tag?

comment:22 Changed 12 months ago by skyper

Your patch so far only checks all ways. It misses MPs and I think we only should check closed ways (area:closed2).
Regarding the message, I would say either the specific tag or none at all which should group all objects.

comment:23 Changed 12 months ago by Gabe

I have tried area:closed2 and am not seeing any warnings for closed areas. I loaded part of the Washington state boundary and edited the relation tags adding highway=primary and area=yes. No warnings after that.

Changed 12 months ago by skyper

Attachment: josm_18217.patch added

patch

comment:24 Changed 12 months ago by skyper

Ah, sorry, I was wrong. We need to split it into two lines as MPs do not need area=yes. Find attached patch.

*[highway =~ /^(motorway|motorway_link|trunk|trunk_link|primary|primary_link|secondary|secondary_link|tertiary|tertiary_link|residential|unclassified)$/] {
set common_road;
}
way:closed.common_road[area=yes],
relation.common_road[type=multipolygon] {
  throwError: tr("Area with {0} above {1} are invalid", "highway=*", "highway=service");
  group: tr("suspicious tag combination");
  suggestAlternative: "area:highway=*";
  assertMatch:   "way highway=trunk area=yes";
  assertMatch:   "relation highway=trunk type=multipolygon";
  assertNoMatch: "way highway=service area=yes";
  assertNoMatch: "way highway=trunk";
}

comment:25 in reply to:  24 ; Changed 12 months ago by Gabe Reichenberger <v-garei@…>

Replying to skyper:

Ah, sorry, I was wrong. We need to split it into two lines as MPs do not need area=yes. Find attached patch.

*[highway =~ /^(motorway|motorway_link|trunk|trunk_link|primary|primary_link|secondary|secondary_link|tertiary|tertiary_link|residential|unclassified)$/] {
set common_road;
}
way:closed.common_road[area=yes],
relation.common_road[type=multipolygon] {
  throwError: tr("Area with {0} above {1} are invalid", "highway=*", "highway=service");
  group: tr("suspicious tag combination");
  suggestAlternative: "area:highway=*";
  assertMatch:   "way highway=trunk area=yes";
  assertMatch:   "relation highway=trunk type=multipolygon";
  assertNoMatch: "way highway=service area=yes";
  assertNoMatch: "way highway=trunk";
}

I see I see, according to the documentation I would have thought area:closed2 would have worked. So the patch is tested? Thanks for looking into this.

Changed 12 months ago by skyper

Attachment: josm_18217_v2.patch added

patch version 2: shorter class and correct grammar in message

comment:26 in reply to:  25 ; Changed 12 months ago by skyper

josm_18217_v2.patch uses correct grammar in message and a shorter regex in the class.

Replying to Gabe Reichenberger <v-garei@…>:

I see I see, according to the documentation I would have thought area:closed2 would have worked.

Some functions only work with styles and not with validator test. This might be a bug, though. New ticket?

So the patch is tested?

Yes, my patches are usually tested, at least with some virtual test objects. BTW, the search dialog can be used with mapcss, e.g. for testing. Additionally there is the option to use "normal" JOSM Search in mapcss.

Thanks for looking into this.

I was not sure if you would have liked to solve it by yourself, so please, apologize if I stole the task.

comment:27 in reply to:  26 Changed 12 months ago by Gabe Reichenberger <v-garei@…>

Replying to skyper:

josm_18217_v2.patch uses correct grammar in message and a shorter regex in the class.

Replying to Gabe Reichenberger <v-garei@…>:

I see I see, according to the documentation I would have thought area:closed2 would have worked.

Some functions only work with styles and not with validator test. This might be a bug, though. New ticket?

So the patch is tested?

Yes, my patches are usually tested, at least with some virtual test objects. BTW, the search dialog can be used with mapcss, e.g. for testing. Additionally there is the option to use "normal" JOSM Search in mapcss.

Thanks for looking into this.

I was not sure if you would have liked to solve it by yourself, so please, apologize if I stole the task.

Not a problem, just glad a solution was found! I will be working on a few more validations so I will get more chances.

comment:28 Changed 11 months ago by skyper

Cc: Klumbumbus added

Ping

comment:29 Changed 11 months ago by skyper

For performance reasons, it might be better to work without defining the class first and taking comment:17 on #20902 and below into account we are probably better off with a general warning about relations with area=*.

Let's see where #20902 takes us.

Changed 11 months ago by skyper

Attachment: josm_18217_v3.patch added

version 3 without additional class

comment:30 Changed 11 months ago by skyper

Milestone: 21.06

Damn, I should read my own patches more carefully as I always left out area=* for relations.

Find patch version 3 without the extra class.
Should be ready for commit.

comment:31 Changed 11 months ago by Don-vip

Milestone: 21.0621.07

comment:32 Changed 10 months ago by Don-vip

Resolution: fixed
Status: newclosed

In 18104/josm:

fix #18217 - Complain about area=yes on major roads (like highway=primary area=yes) (patch by skyper)

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.