Improve the results of partial validations

[GerdP]

I open this ticket because of a discussion in the german forum:
​https://community.openstreetmap.org/t/selbstuberschneidende-linien-arbeitet-der-datenprufer-bei-josm-sauber/107483

The current status regarding the JOSM validator is quite confusing and unsatisfactory.
We distinguish between

• full test
• partial test
• Test before upload

Problems:

• A full test typically reports so many problems that those caused by my own changes are difficult to find.
• A partial test doesn't always find simple but important geometry errors when e.g. a node of a road is moved so that the road crosses a building or a node of an area way is moved so that the area gets self-crossing. Reason is that the ways are not treated as modified and therefore not passed to the tests.

A lot of code was added to particular tests to select the "correct" objects, e.g. MapCss tests which calculate insideness first calculate the objects which might be concerned.
Still, many important tests don't work well, e.g. CrossingWays or SharpAngles or UnconnectedWays.

I am still not 100% sure how to handle this, but I think almost all users would expect that all parent ways and relations of a node which is moved are passed to all tests, even those which test only the tags of the parent object.
Besides that we should change the tests which need to know all other ways to produce reasonable results to just use all ways and maybe remove unwanted findings.

[GerdP]

I started to work on this, hope to have something presentable in the next week.

[GerdP]

WIP patch to demonstrate what I have in mind.

[GerdP]

The first version of the patch implements this:

• on upload, add parents ways and relations of a changed node to the list of primitives which should be checked to catch case were a way-node was moved and thus the geometry of the way was changed.
• let CrossingWays find a problem if at least one of the crossing ways is in the partial selection
• let DuplicatWays find a problem if at least one of the duplate ways is in the partial selection

Todo:

• I might add a preference to allow to disable this new behaviour.
• I didn't yet think about unit tests.
• Performance is good in normal use cases, but CrossingWays may suffer if e.g. all highways in a large dataset are selected.
• There are probably a few more tests to change, e.g. PowerLines, DuplicateNode
• it contains a bug which keeps the progress monitor from updating. Fixed it just now.

[gaben]

Last time, I heavily modified the PowerLines test, and there is a new validation for waterways (#21881). These can only work properly if the full data is validated. Maybe the "partial" validation definition needs to be adjusted.

Using half of the validations for the sake of partial test is not good in my opinion.

[GerdP]

Using half of the validations for the sake of partial test is not good in my opinion.

I don't understand what you mean here.

[GerdP]

Does anybody understand why AggregatePrimitivesVisitor is used to add the childs of Objects to the list? I see no good reason to do that. When I just split a way that is part of a long route relation I possibly get hundreds of warnings for ways in a distant area if I've downloaded that relation completely.

[GerdP]

skyper told me here: ​https://community.openstreetmap.org/t/josm-prufungen-beim-upload-was-sollte-neben-den-geanderten-objekten-noch-gepruft-werden/107834
The child objects are needed for some mapcss tests.

[GerdP]

major changes to first patch:

• fix bug reg. progress monitor
• move the duplicated code to call AggregatePrimitivesVisitor to ValidationTask, let this method decide what further objects are needed to get a reasonable result
• remove all errors which are not related to the original list of primitives (selection or uploaded objects)

[GerdP]

Hm, shit, the last item "remove all errors which are not related to the original list of primitives (selection or uploaded objects)" doesn't work when the error doesn't contain the expected primitive(s).
Example: When I add a way with bicycle=no to a cycle route relation the full test says correctly
way with bicycle=no is part of a bicycle route relation
but the error contains only a reference to the way, not the relation.

I have to rethink this...

[GerdP]

like 23397-beta.patch, but makes sure that errors coming from MapCss rules contain parent object so that filtering works

[skyper]

Replying to GerdP:

skyper told me here: ​https://community.openstreetmap.org/t/josm-prufungen-beim-upload-was-sollte-neben-den-geanderten-objekten-noch-gepruft-werden/107834
The child objects are needed for some mapcss tests.

Yes, child (>) and parent (<) selectors are problems.

[GerdP]

I've just learned that we already have a preference validator.selectionFilter which is false by default. If enabled, the existing validation tree is filtered to show only those results which relate to the current selection. I guess this has the same problems with TestError instances where the primitives are not set properly.

[GerdP]

I played with this and found an example which tells me that neither this filter nor that in my patch will work well with this example:

Have a waterway that is connected to a bridge. Run validator. That should report the warning
node connects waterway and bridge (1)
The corresponding test in geometry.mapcss:

/* #11127 */
way[railway][bridge] > node,
way[highway][bridge] > node {
  set node_in_bridge;
}
way[waterway] > node.node_in_bridge {
  throwWarning: tr("node connects waterway and bridge");
}

The TestError contains either just the node as primitive (without my patch) or also the waterway but not both.
If you select the highway and run the validator you probably want to see the warning, but you won't.

Besides that the current implementation of the preference validator.selectionFilter is bogus. See #23430.

[SekeRob]

"Have a waterway that is connected to a bridge. Run validator. That should report the warning
node connects waterway and bridge (1)"

A classic case here and probably everywhere else is a fuel station roof below which the pumps are. The paving below these roofs here are mostly paving_stone, sometimes concrete for an environmental reason (and think law too) and the fact the fuels destroy asphalt. Some mappers cut up the service way(s), add the paving type, tag these way sections as covered, maxheight if not forgotten and then connect the ends to the roof at layer 1 regardless if this different paving as often actually is slightly wider than the roof. Same Same but have never seen this flagged by any QA like Osmose and Inspector, water I think to remember one of them does.

The curious case of pre-save validation only checking on current edit set changes, shift+V working unless to clear flags when there are remaining errors/warnings for the current edits, BUT shift+V run when there are no flagged errors yet goes wild, then everything loaded is checked. I delete the validation layer and run the pre-save validator. That one I'd like to get under a shortcut, mayber simething like ctrl+shift+V. This combo does not seem to do anything ATM.

[GerdP]

I delete the validation layer and run the pre-save validator. That one I'd like to get under a shortcut

I think the shortcut for the upload is Shift+Ctrl+Up

[GerdP]

I've removed the additional code for filtering irrelevant errors for now. It might be added later is less important, but has a large potential to hide important errors when tests don't fill the list of primitives with all involved objects. Esp. MapCSSTagChecker would require more (and complex) work to collect the full primitives list.

[GerdP]

23397-also-add-mp.patch also checks if a multipolygon (or boundary relation) is changed when a node is modified.

This is not yet complete. Some tests like UnconnectedNaturalOrLanduse require to see also the nearby nodes to find e.g "Way node near other way" problems.
Edit: UnconnectedNaturalOrLanduse already seems to work.

[GerdP]

I've re-added the filtering. I think this is as close to correct as I can get for now.
Most irrelevant errors are filtered, only those from mapcss tests hich have multiple parents remain.
The bride connected to highway is a typical example.
If you select a route relation for validation this error would not be filtered if one of the way member is involved.
It will not filter messages like way with bicycle=use_sidepath is part of a bicycle route relation.
I think it would be possible to change the code so that all ways which trigger the

set node_in_bridge;

statement are collected so that we could report them when the error is created but that seems to require much more code, so I've simply excluded errors of this kind from filtering, search for incompletePrimitives in the patch.

Open question: Should I add a preference to disable the filtering? If yes, what should be the default? Maybe users expect and want the old behaviour with - in my eyes - too many messages?

[GerdP]

I'll probably commit the last patch tomorrow.

[skyper]

I am a bit rusty in reading patches and probably do not fully understand it but I hope to find some time to test it once it will be committed.
I wonder if the special case for multipolygon relations also covers boundary relations.
Regarding a preference to disable the filtering I think it would be appreciated and the default should be with enabled filtering. Users could still manually disable it if they do not like the new feature.

[gaben]

I also want to test it before commit, but not sure if it will fit in my time until the weekend.

[GerdP]

OK, I'll add code for a preference tomorrow and wait for feedback from gaben. I'll not have much time until 2024-02-05, so maybe this has to wait until after next release.

[GerdP]

add two new preferences

[GerdP]

I've added two preferences:

• The preferences key for the addition of parent objects for modified nodes: validator.partial.add.parents
• The preferences key for the deletion of results which do not belong to selected objects or the parents of modified nodes: validator.partial.removeIrrelevant

I just wonder if I should also add parent relations of modified ways. Probably yes, else we may not find cases where a highway tag was removed from a way that is part of a route relation. I'll check now...
Edit: As of now, there is no test to find non-highway members in route relations, but there are other cases like multipolygon menbers.

[GerdP]

Compared to 23397-beta4.patch this patch

• no longer filters modified or new objects when adding parents. This was meant to improve performance on update but doesn't work when you just select objects and run validator
• also adds parent relations of selected/uploaded ways and relations. This allows to find cases where e.g. a way with landuse=forest is member of a landuse=forest multipolygon

[skyper]

Replying to GerdP:

I just wonder if I should also add parent relations of modified ways. Probably yes, else we may not find cases where a highway tag was removed from a way that is part of a route relation. I'll check now...

Yes, we need it. E.g. adding a bicycle=no to a way which is member of a bicycle route should trigger a warning.

[GerdP]

That already works in tested, probably because the test checks the way and not the relation.

[skyper]

Oh, then I was wrong and there is no problem with the child selector (>) in MapCSS.

[skyper]

New example: Unglue a way which is member of a turn-restriction at the "via" node. With latest I get no warning about the broken relation on upload.

[GerdP]

Yes, that should be fixed with the patch (if not disabled via preferences)

[GerdP]

@gaben: Any results so far?

[GerdP]

In 18960/josm:

fix #23397: Improve the results of partial validations

• pass also parent ways and relations of uploaded/selected objects to the testers, child objects are already added, this allows to find e.g. overlapping polygons problems with tags in members of relations
• let CrossingWays find a problem if at least one of the crossing ways is in the partial selection.
• let DuplicatWays find a problem if at least one of the duplicated ways is in the partial selection
• add code to filter the detected issues so that those issues which are clearly not related to the original list of objects are removed. A few issues from mapcss tests may remain.
• add new preference validator.partial.add.parents to disable the addition of parent objects, default is enabled
• add new preference validator.partial.removeIrrelevant to disable the filtering of irrelevant errors, default is enabled
• duplicated code to call AggregatePrimitivesVisitor was moved to ValidationTask

[GerdP]

I've committed the patch with one additional change in CrossingWays so that it doesn't show errors of nearby but unrelated objects, even if preference validator.partial.removeIrrelevant is set to false.
Hope to get some feedback before next tested is released in case that there are new problems.
I try to add a unit test for the behaviour in CrossingWays.

[GerdP]

In 18961/josm:

see #23397: Improve the results of partial validations

• correct last minute change so that it doesn't use empty collection to filter irrelevant warnings
• correct usage of isPrimitiveUsable() so that CrossingWays.Boundaries works again (old unit test failed)
• add unit tests to check partial validation of DuplicateWay and CrossingWays gives expected results

[GerdP]

In 18962/josm:

see #23397: Improve the results of partial validations

• don't call endTest() in static method CrossingWays.isSelfCrossing(), this fixes the failing unit test AlignInCircleActionTest