Modify

Opened 4 years ago

Last modified 8 months ago

#20473 new defect

Multipolygon repeating the tag of an outer way is not flagged

Reported by: GerdP Owned by: team
Priority: normal Milestone:
Component: Core validator Version:
Keywords: template_report multipolygon Cc:

Description

What steps will reproduce the problem?

  1. Load attched file and run validator

What is the expected result?

A warning that both the multipolygon and the outer ring have a tag leisure=water_park

What happens instead?

Only an info message
"With the currently used mappaint style the style for outer way mismatches the area style"

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

There are probably two different cases: If there are inner rings within the outer this is an obvious error, but if there is no inner it requires expert knowledge to find out what is wrong.

Build-Date:2021-02-09 15:57:34
Revision:17488
Is-Local-Build:true

Identification: JOSM/1.5 (17488 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Home 2004 (19041)
Memory Usage: 793 MB / 1820 MB (675 MB allocated, but free)
Java version: 1.8.0_272-b10, AdoptOpenJDK, OpenJDK 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920×1080 (scaling 1.00×1.00)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→32×32, 32×32→32×32
VM arguments: [-agentlib:jdwp=transport=dt_socket,suspend=y,address=localhost:63940, -ea, -Djosm.homexxx=c:\temp\josmxxx, -javaagent:D:\eclipse-java-2020-09\eclipse\configuration\org.eclipse.osgi\577\0\.cp\lib\javaagent-shaded.jar, -Dfile.encoding=UTF-8]
Program arguments: [--debug]
Dataset consistency test: No problems found

Plugins:
+ MakeParallel (v1.1.0)
+ buildings_tools (35669)
+ o5m (35640)
+ pbf (35650)
+ poly (35640)
+ reltoolbox (35640)
+ reverter (35688)
+ undelete (35640)
+ utilsplugin2 (35691)

Attachments (2)

20473-sample.osm (9.6 KB ) - added by GerdP 4 years ago.
20473.patch (6.5 KB ) - added by GerdP 4 years ago.

Download all attachments as: .zip

Change History (18)

by GerdP, 4 years ago

Attachment: 20473-sample.osm added

comment:1 by skyper, 4 years ago

Component: CoreCore validator
Keywords: multipolygon added

comment:2 by GerdP, 4 years ago

I started to work on this and found lots of simiar problems like same ref or name on outer way(s) and the relation.
I am not yet sure if the message should contain a fixed string like "Outer way has same tag as multipolygon" or if it should also contain the tag(s). If the latter, should we produce one warning for each equal tag?
In many cases the multipolygons also produce other warnings like "Multipolygon tags - tag describing the area might be missing", see e.g. r2944039 (and lots of similar cases in that area). No idea if it makes sense to create even more warnings?

in reply to:  2 comment:3 by skyper, 4 years ago

Replying to GerdP:

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

There are probably two different cases: If there are inner rings within the outer this is an obvious error, but if there is no inner it requires expert knowledge to find out what is wrong.

+1, first difference to check.

Additionally, a difference between "primary" and "secondary" tags could be made and I am not sure what to do with "ignorable tags". We do not need a warning for automatically discarded tags and tags like note=*, description=* and FIXME=* could get a different treatment.

Replying to GerdP:

I am not yet sure if the message should contain a fixed string like "Outer way has same tag as multipolygon" or if it should also contain the tag(s). If the latter, should we produce one warning for each equal tag?

I'd go with a fixed string and a warning for each MP and its members, as selecting the objects is enough to find the tags all have in common.

In many cases the multipolygons also produce other warnings like "Multipolygon tags - tag describing the area might be missing", see e.g. r2944039 (and lots of similar cases in that area). No idea if it makes sense to create even more warnings?

I am not sure, if we will be able to hide all other warnings and I am not sure if it is the best choice in all cases.

comment:4 by GerdP, 4 years ago

+1, first difference to check.

Not sure what you mean. Please elaborate.

Additionally, a difference between "primary" and "secondary" tags could be made

My first approach is to ignore the uninteresting tags and treat all others equal. I have no idea how to separate further.

I'd go with a fixed string and a warning for each MP and its members

So, only one warning per relation? Or one for each pair of MP and member? Or one for each tag? I agree that it is easy enough to find the tag when the error is double clicked so that the involved objects are highlighted.

I am not sure, if we will be able to hide all other warnings and I am not sure if it is the best choice in all cases.

I don't want to hide the existing messages for this new one, I think about suppressing the new warning if other similar messages are already produced.

in reply to:  4 comment:5 by skyper, 4 years ago

Replying to GerdP:

Not sure what you mean. Please elaborate.

It makes a difference if the MP has members with only role outer or also with inner. Thinking about it again, as long as we do not want to offer an autofix, it makes no difference unless you plan to have different warning levels.

Additionally, a difference between "primary" and "secondary" tags could be made

My first approach is to ignore the uninteresting tags and treat all others equal. I have no idea how to separate further.

I was thinking about the list for the "areas" tags, in order to have different warning levels, though, that could be a future enhancement.

I'd go with a fixed string and a warning for each MP and its members

So, only one warning per relation? Or one for each pair of MP and member? Or one for each tag? I agree that it is easy enough to find the tag when the error is double clicked so that the involved objects are highlighted.

I thought about only one per relation but that does not work out nice if you have different tags on the members. The longer I think about it, the more puzzled I am. I thought selecting and having a look at the tags in common is easy but what happens if tags in question were modified/deleted in between? So I guess I am back to having the tag in the warning and have one warning per relation for each tag.

One thing I noticed, I am not using the lookup button very often in my workflow. Maybe, that would help in this case.

I am not sure, if we will be able to hide all other warnings and I am not sure if it is the best choice in all cases.

I don't want to hide the existing messages for this new one, I think about suppressing the new warning if other similar messages are already produced.

I am not feeling well, if a warning with low priority can suppress a warning on higher levels.

comment:6 by GerdP, 4 years ago

I see no way to implement an autofix.
I guess we have to limit the test to check only a specific list of tags like landuse, natural etc, else this will produce lots of rather useless messages, e.g. when a building MP has the same addr:* tags as the outer way or other descriptive tags like image=*, operator=*

comment:7 by GerdP, 4 years ago

In #19136 I've implemented a method TagChecker.hasAcceptedPrimaryTagForMultipolygon() which contains probably the most complete list of primary tags for MP, but the message just caclulates a boolean. It doesn't return the tag / tag key.

The more samples that I look at the fewer warnings look good to me. So, the patch only warns for those tags, not for name or ref or similar.

comment:8 by skyper, 4 years ago

I often find tags added to the outer way instead to the MP like surface=*, level=* or even duplicating most of the tags. I was hoping to find these problems to take a look at but it seems complicated.

comment:9 by GerdP, 4 years ago

I can add another list of tag keys as a preference to say: test these as well

comment:10 by skyper, 4 years ago

Ok, another list in preference. I rather need an ignore (black) list than a white list, I guess.

comment:11 by GerdP, 4 years ago

You mean: Compare all tags by default and exclude via blacklist?

comment:12 by skyper, 4 years ago

Yes exactly.

by GerdP, 4 years ago

Attachment: 20473.patch added

comment:13 by GerdP, 4 years ago

The patch implements the check to find when a relation has the same tag as one or more outer ways

  • group "Multipolygon outer way repeats major tag of relation" in warnings
  • group "Multipolygon outer way repeats tag of relation" in information for those that don't match the first
  • The tag is shown with "Same tag:{0}={1}"
  • preference validator.TagChecker.ignore-keys-outer-mp-same-tag can be used to list tag keys which should be ignored by this test

Not sure yet about the wording, it suggests that the tag on the outer way is wrong but in my example the tag has to be removed from the MP.

comment:14 by GerdP, 4 years ago

By the way: We have the mapcss test

/* see ticket #7639 -- Warn when a node has the same tags as its parent way. */
way >:sameTags node:tagged {
  throwWarning: tr("Nodes duplicating parent way tags");
}

This doesn't show the tag(s) but it only works when all interesting tags are the same.

comment:15 by GerdP, 4 years ago

In 17501/josm:

see #20473: Multipolygon repeating the tag of an outer way is not flagged

  • add new check in TagChecker
  • add new preference validator.TagChecker.ignore-keys-outer-mp-same-tag tgat can be used to list tag keys which should be ignored by this test

comment:16 by GerdP, 8 months ago

This ticket is still open, but I think the problem was fixed?

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from team to the specified user.
Next status will be 'needinfo'. The owner will be changed from team to GerdP.
as duplicate The resolution will be set to duplicate. Next status will be 'closed'. The specified ticket will be cross-referenced with this ticket.
The owner will be changed from team to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.