Modify

Opened 10 months ago

Closed 10 months ago

Last modified 7 months ago

#18455 closed enhancement (fixed)

Warn about main tag with incorrect object type (steps)

Reported by: skyper Owned by: Don-vip
Priority: normal Milestone: 20.01
Component: Core validator Version:
Keywords: template_report object type presets Cc:

Description

What steps will reproduce the problem?

  1. have a closed way tagged with highway=steps (254486816)
  2. run validator

What is the expected result?

A warning about an incorrect/unusual object type (closed way).

What happens instead?

No warning

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

The object types are set correctly in presets as I get a warning using it and I have to manually add highway=steps.

In general, it would be nice if you get a warning if presets do not allow the tag on the object type.
Thanks

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2019-12-21 15:53:55 +0100 (Sat, 21 Dec 2019)
Revision:15607
Build-Date:2019-12-22 02:30:55
URL:https://josm.openstreetmap.de/svn/trunk

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.
- W: Region [TMS_BLOCK_v2] Resetting cache

Attachments (2)

josm_18455_wording.patch (1.1 KB) - added by skyper 10 months ago.
wording patch
18455-performance.patch (3.1 KB) - added by GerdP 10 months ago.
POC: filter TaggingPreset.data to avoid tests which never match. Reduces runtime from ~10 secs to ~2 secs with my test data.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 10 months ago by Don-vip

Keywords: presets added
Milestone: 20.01
Owner: changed from team to Don-vip
Status: newassigned

comment:2 Changed 10 months ago by Don-vip

Resolution: fixed
Status: assignedclosed

In 15640/josm:

fix #18455 - detect objects not matching their presets object type (info level)

comment:3 Changed 10 months ago by GerdP

I think the error message "Wrong presets types" is missleading. Seems to say that the preset is wrong?

comment:4 Changed 10 months ago by Don-vip

Feel free to improve the error message.

comment:5 Changed 10 months ago by GerdP

What do you think about "Type not in preset" ?

comment:6 Changed 10 months ago by Don-vip

Yep, ok :)

comment:7 Changed 10 months ago by GerdP

In 15642/josm:

see #18455: Improve error group string "Wrong presets types" -> "Type not in preset"

comment:8 in reply to:  5 Changed 10 months ago by skyper

Replying to GerdP:

What do you think about "Type not in preset" ?

How about "Object type not in preset" and "Object type {0} is not supported by tagging preset: {1}" ?

comment:9 Changed 10 months ago by GerdP

Yes, better.

Changed 10 months ago by skyper

Attachment: josm_18455_wording.patch added

wording patch

comment:10 Changed 10 months ago by skyper

Resolution: fixed
Status: closedreopened

added patch for better wording.

comment:11 Changed 10 months ago by GerdP

In 15645/josm:

fix #18455: Improve error messages again
Patch by skyper, modified formatting

comment:12 Changed 10 months ago by GerdP

Resolution: fixed
Status: reopenedclosed

comment:13 Changed 10 months ago by Klumbumbus

Nice test! Thanks.

comment:14 Changed 10 months ago by skyper

Resolution: fixed
Status: closedreopened

Thank too,

found a little problem with relations. (1970390)

  • Object type relation is not supported by tagging preset: General attributes (oneway) (1)
  • Object type relation is not supported by tagging preset: River (1)

comment:15 Changed 10 months ago by skyper

Found one more one a building mapped as mulipolygon:

  • Object type multipolygon is not supported by tagging preset: Simple 3D buildings outline (1)

Maybe do not check all relations.

comment:16 in reply to:  15 Changed 10 months ago by skyper

Replying to skyper:

Found one more one a building mapped as mulipolygon:

  • Object type multipolygon is not supported by tagging preset: Simple 3D buildings outline (1)

Forget this one. Its from an additional preset.

comment:17 Changed 10 months ago by Don-vip

Resolution: fixed
Status: reopenedclosed

In 15667/josm:

fix #18455 - smarter error detection, should lead to less false positives

comment:18 Changed 10 months ago by GerdP

It seems this additional test requires a lot of CPU power. TagChecker takes much longer compared to r15628.
According to Java Mission Control the "hot" methods are

Stack Trace	                                                                        Sample Count	Percentage(%)
org.openstreetmap.josm.gui.tagging.presets.TaggingPresetItem.matches(Iterable, Map)	121	        9,967
org.openstreetmap.josm.gui.tagging.presets.items.KeyedItem.matches(Map) 	        70	        5,766

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

Replying to GerdP:

It seems this additional test requires a lot of CPU power.

Yes, that was also one good reason to make it an INFO level. Do you see a possible optimization?

comment:20 Changed 10 months ago by GerdP

The test is executed even if info level is not selected. A simple improvement would be to check this first.

comment:21 Changed 10 months ago by Don-vip

You're right, I thought I checked it and forgot to code it.

comment:22 Changed 10 months ago by Don-vip

In 15682/josm:

see #18455 - don't perform INFO level tests in TagChecker if not enabled

comment:23 Changed 10 months ago by GerdP

Maybe TaggingPresetItem.matches(Iterable<? extends TaggingPresetItem> data, Map<String, String> tags)
could be improved. Current loop iterates over all items. This is needed since the check item.matches(tags) may return false for the last item. If the list of items could be sorted so that we can first check if a mandantory tag is missing we might save some iterations, esp. those for link, space etc..

comment:24 Changed 10 months ago by Don-vip

In 15683/josm:

see #18455 - fix unit tests

Changed 10 months ago by GerdP

Attachment: 18455-performance.patch added

POC: filter TaggingPreset.data to avoid tests which never match. Reduces runtime from ~10 secs to ~2 secs with my test data.

comment:25 Changed 10 months ago by GerdP

This is the careful version which only adds the logic in TagChecker. I'd prefer to use the index in TaggingPresets.
Would probably also improve reaction time when mouse moves over dialogs.

comment:26 Changed 9 months ago by GerdP

@Vincent: I tried to code this approach in TaggingPresets but the performance improvement is much smaller compared to the solution in 18455-performance.patch as we would still iterate over many presets which never match.
Should I commit the patch?

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

Replying to GerdP:

Should I commit the patch?

Looks fine, go ahead.

comment:28 Changed 9 months ago by GerdP

In 15788/josm:

see #18455: improve performance

  • filter TaggingPreset.data to avoid tests which never match. This reduces significantly the number of iterations.

comment:29 Changed 7 months ago by Don-vip

In 16154/josm:

see #17533 - see #18455 - partial revert of r15640, causing untagged nodes to be skipped entirely

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Don-vip.
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.