Modify

Opened 4 years ago

Last modified 3 years ago

#18240 new enhancement

Force/propose/automatically download of incomplete multipolygon relations for validation

Reported by: Geimas5 Owned by: team
Priority: normal Milestone:
Component: Core validator Version: latest
Keywords: Cc:

Description

In the latest version of JOSM, there are some validation rules for multipolygon relations, however, these rules are only run when the relation has been completely downloaded. This can result in uploads in which the user unintentionally breaks these multipolygons due to preventable problems such as them not being closed. Even for users who are aware of the need to manually download incomplete multipolygons, which is far from everyone, it can be easy to not notice having touched an incomplete multipolygon in areas with a high multipolygon density.

What I am suggesting is for the implementation of some way to either force the user to download all incomplete multipolygons, or at the least, make the user aware of having touched incomplete multipolygons and providing a method for the user to download all of the incomplete relations they have touched.

Attachments (2)

Sample.osm (1.1 MB ) - added by Geimas5 4 years ago.
18240.patch (7.2 KB ) - added by GerdP 4 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 by GerdP, 4 years ago

The same problem occurs with other incomplete relations. On the other hand, you should see a popup message when you modify a relation in a way that can corrupt it. So, it is likely that mapper simply ignores the warning.
If you don't see such a popup please let us know how to reproduce the problem.

Last edited 4 years ago by GerdP (previous) (diff)

comment:2 by Geimas5, 4 years ago

I could not remember having seen any such popup, I have therefore tried a few different scenarios.

Deleting a way which is part of a relation
I do get a message about the way being part of a relation, but not anything about the validations not working because not all members of the relation has been downloaded.

Switching outer/inner
No popup/validation error until after having downloaded all members of the relation.

Adding a random not-connected member to a relation
No popup/validation error until after having downloaded all members of the relation.

Creating a self-intersecting relation by moving a node
I do get a validation error if the self-intersection is only involving one way. But not when I use two ways. The validations are of course working when the the entire relation has been downloaded.

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

Replying to Geimas5:

I could not remember having seen any such popup, I have therefore tried a few different scenarios.

Deleting a way which is part of a relation
I do get a message about the way being part of a relation, but not anything about the validations not working because not all members of the relation has been downloaded.

This is more or less always the case. Incomplete data is typically ignored in validation tests.

Switching outer/inner
No popup/validation error until after having downloaded all members of the relation.

I think this is not OK. It should produce a popup.

Adding a random not-connected member to a relation
No popup/validation error until after having downloaded all members of the relation.

Again, this should produce the popup.

Creating a self-intersecting relation by moving a node
I do get a validation error if the self-intersection is only involving one way. But not when I use two ways. The validations are of course working when the the entire relation has been downloaded.

Hmm, that is a rather special case. The current implementation of MultipolygonTest doesn't check complex self-intersections when the relation is incomplete. Simple self-intersections or crossing ways are found in another check. Please attach a sample file.

comment:4 by GerdP, 4 years ago

Component: CoreCore validator

by Geimas5, 4 years ago

Attachment: Sample.osm added

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

Replying to GerdP:
I have added a Sample for the last scenario.

comment:6 by GerdP, 4 years ago

OK, I see. It seems I was wrong, the ways are not tested in the CrossingWays test.

by GerdP, 4 years ago

Attachment: 18240.patch added

comment:7 by GerdP, 4 years ago

Owner: changed from team to GerdP
Status: newassigned
Summary: Force/propose/automatically download of incomplete multipolygon relations for validation[Patch]Force/propose/automatically download of incomplete multipolygon relations for validation

A 1st patch to address these issues.

  • Warn if incomplete multipolygon relation (MP) was modified.
  • Perform check for crossing ways for all complete ways of the MP

Open issues:

  • most of the mentioned issues seem to depend on the way how you work with relations
    1. it seems that changes made in the "Edit Relation" popup are always considered to be wanted, therefore no warnigs are produced when you do something like adding members or changing roles. I think that makes sense. Maybe it would be a good idea to perform a complete relation check when you press OK to close the edit dialog.
    2. If you try to delete way 668064148 which is a member of tow relations you'll see the popup that asks for confirmation.
    3. If you select way 345099033 and right click on relation 9295910 you see an entry "add selection to 1 relation". If you click this you see a warning popup in the lower left corner
  • the status modified is not set for the relation in your sample file. Although the geometry of the MP was changed only the moved node has the status modified. This corresponds to the OSM changeset logic: Neither the parent way nor the relation would be uploaded. Same problem occurs when you move the node of a normal way like 345099033 so that the way is self intersecting. This change doesn't set the modifiedstatus for the way and thus it is not verified on upload.

Maybe we should introduce a new status moved. Usage: If a node is moved it would have the moved
flag set. The flag would be used when you run validator on a selection of objects or when validator is executed because you started an upload.
The flag would be used to check all ways that refer to the moved node as well as the relations which refer to those ways.

comment:8 by GerdP, 4 years ago

Summary: [Patch]Force/propose/automatically download of incomplete multipolygon relations for validation[Patch][RFC]Force/propose/automatically download of incomplete multipolygon relations for validation

@team I am busy with project mkgmap, so I'll work on this again next month. The patch is rather simple, so maybe I should commit it before 19.10 is released?

comment:9 by GerdP, 4 years ago

In 15510/josm:

see #18240 : apply 18240.patch

  • Warn if incomplete multipolygon relation (MP) was modified.
  • Perform check for crossing ways for all complete ways of the MP

comment:10 by Don-vip, 4 years ago

Can you please write more useful commit messages? Reading the changelog I have no idea what the commit is about. You can edit SVN messages after commit on the JOSM SVN repository.

comment:11 by GerdP, 4 years ago

Done

comment:12 by Don-vip, 4 years ago

Thanks! :)

in reply to:  7 comment:13 by GerdP, 4 years ago

Replying to GerdP:

Maybe we should introduce a new status moved. Usage: If a node is moved it would have the moved
flag set. The flag would be used when you run validator on a selection of objects or when validator is executed because you started an upload.
The flag would be used to check all ways that refer to the moved node as well as the relations which refer to those ways.

Thought again about this. The flag would not help much for the various tests about geometry (crossing ways, overlapping ways or areas, amenity=parking inside amenity=parking etc). Most of those tests require more input to calculate reasonable results and - to me - it seems impossible to predict which objects are needed.
The only solution that I see is to perform a full (and maybe time consuming) validation on upload and filter the results so that only those errors (or warnings) are shown which refer to modified objects or parents of the modified objects.
I am not happy with this idea.
I think the basic idea of the validation tests is to warn when something is wrong. The user should NOT deduce that his change is correct just because JOSM wasn't able to find something wrong. No idea how to communicate that :(

comment:14 by skyper, 4 years ago

Summary: [Patch][RFC]Force/propose/automatically download of incomplete multipolygon relations for validationForce/propose/automatically download of incomplete multipolygon relations for validation

comment:15 by GerdP, 3 years ago

Owner: changed from GerdP to team
Status: assignednew

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 Geimas5.
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.