Modify

Opened 3 years ago

Closed 13 months ago

Last modified 12 months ago

#14287 closed defect (fixed)

Not capturing all elements when using ∈

Reported by: naoliv Owned by: team
Priority: normal Milestone: 19.05
Component: Core validator Version:
Keywords: performance regression Cc: simon04, Klumbumbus

Description

Validate the attached example file and see only one warning about amenity inside amenity - amenity=parking inside amenity=parking

Since we have 6 identical nodes, we should see 6 objects with a warning (and not only one).

JOSM:

URL:http://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2017-01-25 02:17:36 +0100 (Wed, 25 Jan 2017)
Build-Date:2017-01-25 02:33:55
Revision:11490
Relative:URL: ^/trunk

Identification: JOSM/1.5 (11490 en) Linux Debian GNU/Linux 9.0 (stretch)
Memory Usage: 271 MB / 10206 MB (111 MB allocated, but free)
Java version: 1.8.0_111-8u111-b14-3-b14, Oracle Corporation, OpenJDK 64-Bit Server VM
Screen: :0.0 1600x900, :0.1 1280x1024
Maximum Screen Size: 1600x1024
Java package: openjdk-8-jre:amd64-8u111-b14-3
Java ATK Wrapper package: libatk-wrapper-java:all-0.33.3-13
VM arguments: [-Dawt.useSystemAAFontSettings=on]
Program arguments: [--language=en]
Dataset consistency test: No problems found

Attachments (4)

example.osm (1.5 KB) - added by naoliv 3 years ago.
14287.patch (5.0 KB) - added by GerdP 13 months ago.
14287.PNG (4.4 KB) - added by GerdP 13 months ago.
14287-v2.patch (5.2 KB) - added by GerdP 12 months ago.

Download all attachments as: .zip

Change History (21)

Changed 3 years ago by naoliv

Attachment: example.osm added

comment:1 Changed 3 years ago by naoliv

The test here is JOSM's node[amenity=parking] ∈ *[amenity=parking] { (...) but doing some local tests, the problem seems to be related with

comment:2 Changed 3 years ago by Don-vip

Milestone: 17.01

comment:3 Changed 3 years ago by Don-vip

In 11491/josm:

see #14287 - add non regression unit test

comment:4 Changed 3 years ago by Don-vip

Cc: simon04 added
Keywords: performance regression added

@Simon: This is a side effect of performance improvements done in r6609, especially this part in Selector.AbstractFinder.visit:

            public void visit(Collection<? extends OsmPrimitive> primitives) {
                for (OsmPrimitive p : primitives) {
                    if (e.child != null) {
                        // abort if first match has been found
                        break;
                    } else if (isPrimitiveUsable(p)) {
                        p.accept(this);
                    }
                }
            }

I have added a unit test to quickly debug the problem. Any hint about how to resolve this issue without a severe performance drawback?

comment:5 Changed 3 years ago by Don-vip

In 11492/josm:

see #14287 - add non regression unit test (forgot test file)

comment:6 Changed 3 years ago by Klumbumbus

Cc: Klumbumbus added

comment:7 Changed 3 years ago by Don-vip

Milestone: 17.0117.02

comment:8 Changed 3 years ago by Don-vip

Milestone: 17.0217.03

comment:9 Changed 3 years ago by Don-vip

Milestone: 17.0317.04

comment:10 Changed 3 years ago by Don-vip

Milestone: 17.04

comment:11 Changed 13 months ago by GerdP

I wonder if we should produce 6 warnings or one warning with 7 objects?

Changed 13 months ago by GerdP

Attachment: 14287.patch added

Changed 13 months ago by GerdP

Attachment: 14287.PNG added

comment:12 Changed 13 months ago by GerdP

It would be a bit more work to produce multiple warnings for each. With the attached patch you get

comment:13 Changed 13 months ago by GerdP

Resolution: fixed
Status: newclosed

In 15064/josm:

fix #12627,#14287,#14289,#17695

  • let CrossingFinder and ContainsFinder find all objects instead of stopping at first match
  • if objects are selected, make sure that ContainsFinder is called for enclosing objects which are not in the selection
  • enable corresponding unit tests

Draw back: MapCSSTagChecker is a bit slower.

comment:14 Changed 13 months ago by Klumbumbus

Milestone: 19.05

comment:15 Changed 12 months ago by GerdP

Replying to GerdP:

  • if objects are selected, make sure that ContainsFinder is called for enclosing objects which are not in the selection

Unfortunately my change only works in some special cases where also the right selector is a possible match for the selected element. The "amenity=parking inside amenity=parking" is one of them as the right and left selectors both match for a node with amenity=parking.
The following will not work:

*[building]  area[natural=water] {
  throwWarning: tr("Building inside Water Area");
}

when the building is selected. I try to find a solution for that as well.

Changed 12 months ago by GerdP

Attachment: 14287-v2.patch added

comment:16 Changed 12 months ago by GerdP

The patch improves results when Test.partialSelection is true. This happens when either elements are selected or when the test is performed before uploading data.
Problem: In some cases it requires seconds to performs the additional tests, for example if someone selects many objects and executes the validator.
I am working on a better solution which rebuilds the index so that only those tests which are needed to find "enclosing" objects are executed...

comment:17 Changed 12 months ago by GerdP

In 15103/josm:

see #14287: improve results when Test.partialSelection is true. This happens when either elements are selected or when the test is performed before uploading data.
In a final step the surrounding objects of the tested elements are also tested with a reduced set of rules which will find problems like "amenity inside amenity".

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.