Modify

Opened 8 weeks ago

Closed 7 weeks ago

Last modified 6 weeks ago

#15203 closed defect (fixed)

Wrong "Area style way is not closed" message for highway=motorway_junction

Reported by: GerdP Owned by: team
Priority: normal Milestone: 17.08
Component: Core validator Version:
Keywords: template_report Cc: bastiK, Klumbumbus

Description

What steps will reproduce the problem?

  1. Draw a way (unclosed) and tag it highway=motorway_junction
  2. Run Validator

What is the expected result?

A warning that the tag is meant for nodes

What happens instead?

A warning that "Area style way is not closed"

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

I failed to find the code that produces this message.

URL:http://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2017-07-31 01:02:55 +0200 (Mon, 31 Jul 2017)
Build-Date:2017-07-31 01:53:17
Revision:12545
Relative:URL: ^/trunk

Identification: JOSM/1.5 (12545 en) Windows 10 64-Bit
OS Build number: Windows 10 Home 1703 (15063)
Memory Usage: 1377 MB / 5461 MB (1148 MB allocated, but free)
Java version: 1.8.0_121-b13, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080
Maximum Screen Size: 1920x1080
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (33185)
+ apache-commons (32994)
+ buildings_tools (33004)
+ ejml (32680)
+ geotools (33380)
+ jts (32699)
+ o5m (33243)
+ opendata (33438)
+ pbf (33241)
+ poly (33004)
+ reverter (33088)
+ utilsplugin2 (33328)

Last errors/warnings:
- W: Cannot start IPv4 remotecontrol https server on port 8112: Keystore was tampered with, or password was incorrect
- W: Cannot start IPv6 remotecontrol https server on port 8112: Keystore was tampered with, or password was incorrect

Attachments (1)

taghistory_highway.png (92.0 KB) - added by Klumbumbus 7 weeks ago.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 8 weeks ago by Klumbumbus

Can't reproduce with r12691. Please make sure that there are no other tags on the way which may produce the warning. Also please add the parts tagging presets, mappaint styles and validator rules of your status report.

comment:2 Changed 8 weeks ago by GerdP

Okay, please make sure that the new way is not selected before running the validator.
Means, I should have added "press Esc" as step 2)

comment:3 Changed 8 weeks ago by GerdP

Forget the "Press Esc" stuff.
I've also tried with a fresh 12695 and a renamed C:\Users\Gerd\AppData\Roaming\JOSM
The result is a bit different:
1) 1st execution of validator just opens the Window "Prüfergebnisse" (empty)
2) 2nd execution shows the wrong warning

comment:4 Changed 8 weeks ago by Klumbumbus

Hm, strange. I can reproduce but only with fresh preferences.

comment:5 Changed 8 weeks ago by GerdP

OK, I was now able to debug this. The error is added in MultipolygonTest.visit(Way w)
because ElemStyles.hasOnlyAreaOrTextStyleElements(w) returns true. I don't understand the code
in this method. Why does it test if (!(s instanceof AreaElement || s instanceof TextElement)) .
What has TextElement to do with this?

comment:6 Changed 8 weeks ago by Don-vip

Cc: bastiK added

@bastiK: we don't understand r11799 ?
By the way the javadoc was not updated ;)

comment:7 Changed 8 weeks ago by bastiK

Cc: Klumbumbus added

What does this line in our main style represent?

way|z17-[highway^=motorway][setting("highway_labels")],

If it can be replaced by

way|z17-[highway=motorway][setting("highway_labels")],
way|z17-[highway=motorway_link][setting("highway_labels")],

then this would be more efficient (and cover up the bug introduced with r11799).

comment:8 Changed 8 weeks ago by Klumbumbus

Yes, can be replaced. (Fixing the real bug would be good too so we don't run into it again accidentally as this this connection is not obvious.)

comment:9 Changed 8 weeks ago by bastiK

Resolution: fixed
Status: newclosed

In 12700/josm:

fixed #15203 - Wrong "Area style way is not closed" message for highway=motorway_junction

comment:10 Changed 8 weeks ago by bastiK

In 12701/josm:

see #15203 - avoid = operator in default style

improves efficiency; makes sure, it is not applied to unwanted tags (like motorway_junction)

comment:11 Changed 8 weeks ago by Klumbumbus

In 12702/josm:

see #15203 - typo

comment:12 Changed 8 weeks ago by Don-vip

Milestone: 17.08

comment:13 Changed 7 weeks ago by GerdP

Resolution: fixed
Status: closedreopened

The wrong warning is gone, but the wanted warning is not produced.

Last edited 7 weeks ago by GerdP (previous) (diff)

comment:14 Changed 7 weeks ago by bastiK

Milestone: 17.08
Type: defectenhancement

comment:15 Changed 7 weeks ago by bastiK

Resolution: fixed
Status: reopenedclosed

In 12708/josm:

fixed #15203 - add validator warning for highway=motorway_junction used on a way

comment:16 Changed 7 weeks ago by bastiK

Milestone: 17.08

comment:17 Changed 7 weeks ago by GerdP

There are a few more highway values which should only be used on nodes:
give_way
milestone
mini_roundabout
stop
street_lamp
traffic_signals
turning_loop
turning_circle

comment:18 Changed 7 weeks ago by bastiK

Resolution: fixed
Status: closedreopened

Klumbumbus, do you like to check this and make the commit?

comment:19 Changed 7 weeks ago by Klumbumbus

For performance reasons I don't like checks for extremly rare cases. Current taginfo numbers for these tags on ways:

3 - motorway_junction
0 - give_way
23 - milestone
20 - mini_roundabout
0 - stop
0 - street_lamp
31 - traffic_signals
55 - turning_loop
48 - turning_circle

Judging only from these numbers I would say for none of these tags a check is useful.

Looking at taghistory:

I would say a check for turning_circle is useful as this one rises much faster


Last edited 7 weeks ago by Klumbumbus (previous) (diff)

Changed 7 weeks ago by Klumbumbus

Attachment: taghistory_highway.png added

comment:20 Changed 7 weeks ago by GerdP

I've recently corrected many mistagged ways with highway=* key. What I don't understand is
that JOSM warns when I use highway=road while it completely ignores many typical typos.

comment:21 in reply to:  19 Changed 7 weeks ago by Don-vip

Replying to Klumbumbus:

Looking at taghistory:

what the hell happened end of 2015?

comment:22 Changed 7 weeks ago by GerdP

That was probably also my work. I stopped that in march 2016 and started again a few days ago.

comment:23 Changed 7 weeks ago by Don-vip

Ah it's only ways, didn't read the legend correctly, sorry.

comment:24 Changed 7 weeks ago by naoliv

Gerd, maybe you can use a local rule for this?

way[highway =~ /^(give_way|milestone|mini_roundabout|motorway_junction|stop|street_lamp|traffic_signals|turning_circle|turning_loop)$/],
relation[highway =~ /^(give_way|milestone|mini_roundabout|motorway_junction|stop|street_lamp|traffic_signals|turning_circle|turning_loop)$/] {
        throwError: tr("{0} should only be used on nodes", concat("highway=", tag(highway)));
}

Or even create a rule at https://josm.openstreetmap.de/wiki/Rules with "deeper" QA rules? (which could include this and everything else that won't be included in JOSM by default)

comment:25 Changed 7 weeks ago by GerdP

Well, I don't need those rules for me. Anyhow, if you don't like the idea I can live with that.

comment:26 Changed 7 weeks ago by bastiK

Milestone: 17.08

comment:27 in reply to:  22 Changed 7 weeks ago by Klumbumbus

Milestone: 17.08
Resolution: fixed
Status: reopenedclosed
Type: enhancementdefect

Replying to GerdP:

That was probably also my work. I stopped that in march 2016 and started again a few days ago.

If this is a regular clean up work for some users then we should add the warnings to reduce this. I'll add the warnings in the next milestone.

The main intention of this ticket (false positive warning) was fixed.

comment:28 in reply to:  20 ; Changed 7 weeks ago by Klumbumbus

Replying to GerdP:

What I don't understand is that JOSM warns when I use highway=road while it completely ignores many typical typos.

Well, all not perfect, but it helps to detect typos. Can you give examples of typos, which are not detected?

comment:29 Changed 6 weeks ago by Klumbumbus

In 12775/josm:

see #15203 - warn about some highway tags used on ways instead of nodes

comment:30 in reply to:  29 Changed 6 weeks ago by GerdP

Replying to Klumbumbus:

In 12775/josm:

see #15203 - warn about some highway tags used on ways instead of nodes

Thanks, I've somehow missed comment 28, will do some testing tomorrow.

comment:31 in reply to:  28 Changed 6 weeks ago by GerdP

Replying to Klumbumbus:

Replying to GerdP:

What I don't understand is that JOSM warns when I use highway=road while it completely ignores many typical typos.

Well, all not perfect, but it helps to detect typos. Can you give examples of typos, which are not detected?

OK, I found out that typical typos like highway=unclassfied (missing i) are detected as "Presets do not contain property value" with Severity.OTHER while the correct highway=road gives "Unspecific highway type" with Severity.WARNING
because highway.mapcc contains this rule:
way[highway=road] {

throwWarning: tr("Unspecific highway type");
assertMatch: "way highway=road";
assertNoMatch: "way highway=residential";

}

I think it should be the other way round, means, if the highway key is used with an unknown value it should give a warning while
highway=road should result in an Severity.OTHER message.

If I got that right, the file words.cfg is only used for tag keys, typos like higway instead of highway also give a warning with
a useful message like "Misspelled property key - Key 'higway' looks like 'highway'. ".
The content of words.cfg really looks a bit out-aged.

Maybe we can add a similar file for well known typos in often used keys, so that
highway=unclassfied gives a warning like "Misspelled property value - Value 'unclassfied' looks like 'unclassified'. ".

comment:32 Changed 6 weeks ago by Klumbumbus

highway=road is fine as Severity.WARNING as highway=road should be fixed to another value.

The reason that "Presets do not contain property value" is Severity.OTHER is that this is a pretty general test. There are a lot of tags, which are fine but not in the presets (e.g. because they are less used, see also DevelopersGuide/DefaultPresets). So this test is just an information, that an tag is not in the presets and may be "wrong" but not for sure. The ignore list helps a bit to clean up the results of this test. However the test is still not appropriate as Severity.WARNING

comment:33 Changed 6 weeks ago by GerdP

Okay, I agree that we should not simply change the severity levels. What do you think about the new file for typical typos?
I think I'll try to implement that.

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.