Modify

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#18863 closed defect (fixed)

split ways: false positive warnings about missing members

Reported by: skyper Owned by: GerdP
Priority: normal Milestone: 20.05
Component: Core Version: latest
Keywords: template_report split way incomplete member warning Cc: JeroenHoek, GerdP

Description

Follow up of #18596:
JeroenHoek in comment 93:

Replying to skyper comment 92:

Found a false positive: see attached example and split at node id:4518025255. There are at least the privious and next members present if ordered. Could it be that there is a problem when splitting the first way of a relation when above members are only stops and platforms ?

I've had a look, and it looks like relation CB 2 has its ways in the wrong order, causing this bug to pop up. It is correct that the split way operation cannot be sure of the direction, but it shouldn't offer to download in this case, because the neighbouring members are already known.

So two bugs really:

  • Bus route CB 2 here is broken
  • The split-way command should not offer to download anything when the neighbouring members are known, but not connected

I even found a smaller example file.

What steps will reproduce the problem?

  1. load attached file
  2. split ways at node (id:3872529672)

What is the expected result?

No warning as all needed members are downloaded

What happens instead?

A warning asking to download additional incomplete members

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

The relations might be out of order but then JOSM cannot solve this problem and be silent.
For role backward/forward no additional members are needed as the proper order can be determined looking at the direction of the way to split.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-03-04 21:44:53 +0100 (Wed, 04 Mar 2020)
Revision:16038
Build-Date:2020-03-05 02:30:56
URL:https://josm.openstreetmap.de/svn/trunk

Attachments (8)

josm_18596_false_positive_small.osm.bz2 (1.9 KB ) - added by skyper 4 years ago.
small example
Burkheimer.png (12.3 KB ) - added by JeroenHoek 4 years ago.
josm_18596_false_positive.osm.bz2 (148.0 KB ) - added by JeroenHoek 4 years ago.
josm_18863_false_positive_small2.osm.bz2 (12.2 KB ) - added by skyper 4 years ago.
new smaller example
0001-Split-way-don-t-download-members-if-unneeded.patch (198.1 KB ) - added by JeroenHoek 4 years ago.
Fix
18863-detect-broken.patch (3.3 KB ) - added by GerdP 4 years ago.
josm_18596_false_positive_inside.osm.bz2 (1.9 KB ) - added by GerdP 4 years ago.
modified example where split ways first and last node are inside the download area
18863-detect-broken.2.patch (2.3 KB ) - added by GerdP 4 years ago.
only perform additional check if first and last node of split-way are inside download area

Download all attachments as: .zip

Change History (48)

by skyper, 4 years ago

small example

comment:1 by skyper, 4 years ago

Summary: split ways: false positives warnings about missing memberssplit ways: false positive warnings about missing members

comment:2 by JeroenHoek, 4 years ago

The split-way command wants to download missing members for the second relation, where there is a way without neighbouring members (see Burkheimer.png). That looks correct (even though the download doesn't help much for this broken relation).

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

by JeroenHoek, 4 years ago

Attachment: Burkheimer.png added

by JeroenHoek, 4 years ago

comment:3 by JeroenHoek, 4 years ago

The other test case (josm_18596_false_positive.osm.bz2​) does cause an unnecessary request to download. I am using that one a regression test.

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

comment:4 by JeroenHoek, 4 years ago

I think I've fixed the dialog being triggered unnecessarily in josm_18596_false_positive.osm.bz2.

comment:5 by JeroenHoek, 4 years ago

Summary: split ways: false positive warnings about missing members[PATCH] split ways: false positive warnings about missing members

in reply to:  2 comment:6 by skyper, 4 years ago

Replying to JeroenHoek:

The split-way command wants to download missing members for the second relation, where there is a way without neighbouring members (see Burkheimer.png). That looks correct (even though the download doesn't help much for this broken relation).

Damm, my bbox is inches too small but moving the node just at the boarder of the bbox in the SE inside the download area does not change anything. The problem here is that we are at the start/end of the line. You screenshot is incorrect as there is one more downloaded way present. Anyway, the small example might be a boarder case.

by skyper, 4 years ago

new smaller example

comment:7 by skyper, 4 years ago

Found a valid small example. Again, nothing JOSM can do as downloading more members won't help.

Split at node id:5332134803

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

comment:8 by JeroenHoek, 4 years ago

You screenshot is incorrect

That would be a bug in JOSM. Not likely in this case.

Found a valid small example​

Split node id?

in reply to:  8 comment:9 by skyper, 4 years ago

Replying to JeroenHoek:

You screenshot is incorrect

That would be a bug in JOSM. Not likely in this case.

Sorry, screenshot is ok but too small as there are more downloaded member present.

Found a valid small example​

Split node id?

Sorry, added the node id to the comment.

in reply to:  7 ; comment:10 by JeroenHoek, 4 years ago

Replying to skyper:

Found a valid small example. Again, nothing JOSM can do as downloading more members won't help.

Split at node id:5332134803

It is downloading extra member for relation 7240.2. This is correct, because it only has one member downloaded in the sample. The next member is not a way, but the previous one is, so it gets downloaded. This is by design. The extra member doesn't help here, because the relation is broken, but the split way command can't know that until it downloads the missing member.

in reply to:  10 comment:11 by skyper, 4 years ago

Replying to JeroenHoek:

Replying to skyper:

Found a valid small example. Again, nothing JOSM can do as downloading more members won't help.

Split at node id:5332134803

It is downloading extra member for relation 7240.2. This is correct, because it only has one member downloaded in the sample. The next member is not a way, but the previous one is, so it gets downloaded. This is by design. The extra member doesn't help here, because the relation is broken, but the split way command can't know that until it downloads the missing member.

You mean 7240.1, as all ways in the example are member of 7240.2.
For 7240.1 the message is wrong as both end nodes (id:30229386 and id:27545233) of the way (id:213219507) to split are completely downloaded. There will be no additional ways. This time the relation is not only unordered but broken.

comment:12 by JeroenHoek, 4 years ago

You mean 7240.1

No, I mean 7240.2. 7240.1 is not causing a download with or without this patch applied. 7240.1 is already weird in terms of ordering, so the split way won't be able to fix anything there in any case.

josm_18863_false_positive_small2.osm.bz2​ doesn't seem to add anything new to ticket; this patch already contains josm_18596_false_positive.osm.bz2​ as regression test case, which demonstrates the same problem.

comment:13 by skyper, 4 years ago

Cc: GerdP added

Any reason but time that this patch was not applied, yet?

comment:14 by GerdP, 4 years ago

Your discussion about 7240.1 and 7240.2 seemed to be ongoing. I waited for a confirmation from you that Jeroen is right.

comment:15 by GerdP, 4 years ago

In 16200/josm:

see #18863 split ways: false positive warnings about missing members
Solution for the problem case given in the unit test looks OK.
Ticket mentiones two problems, so I am not sure if both are fixed now.

comment:16 by GerdP, 4 years ago

Milestone: 20.03
Owner: changed from team to skyper
Status: newneedinfo

in reply to:  14 comment:17 by skyper, 4 years ago

Replying to GerdP:

Your discussion about 7240.1 and 7240.2 seemed to be ongoing. I waited for a confirmation from you that Jeroen is right.

Thanks for applying. I will test tomorrow.

in reply to:  15 comment:18 by skyper, 4 years ago

Owner: changed from skyper to team
Status: needinfonew
Summary: [PATCH] split ways: false positive warnings about missing memberssplit ways: false positive warnings about missing members

Real world is tricky, especially, with unordered and broken relations. In future, I hope the situation gets better and this is a step forward. Thanks.

Replying to GerdP:

Ticket mentions two problems, so I am not sure if both are fixed now.

First one fixed. Second one, see below.

Replying to GerdP:

Your discussion about 7240.1 and 7240.2 seemed to be ongoing. I waited for a confirmation from you that Jeroen is right.

Not sure, there is a right and wrong, but different view points:

  • The current approach is looking at the members of the relation and their neighbors in the member list.
  • I look at the objects (ways) in current data and if all connected ways, at the end nodes, are already downloaded.

In josm_18863_false_positive_small2.osm.bz2 all relations are broken with maybe one exception: 7240.2, a ptv1 with variants in both directions all in one relation. To make sure, all needed data is available in the example, I did download all nodes of the way to split. Have a look at the minimal bboxes.

Sadly, josm_18596_false_positive_small.osm.bz2 was incomplete, so I have to reckeck similar situations with a unordered route ending at the way to split where both end nodes were downloaded with parents.

So, I still think, there should be no warning but this will only work if you have a look at the end nodes and their parent ways. In my point of view, it should be first looked at the end nodes and later the order in member list comes into play. I am not sure, if this check can be included in the current approach, though, something similar is needed (#17898), atm, to handle situations with not yet known parents of ways outside download area, until #18959 is fixed.

comment:19 by GerdP, 4 years ago

If the parents of the split node are all known there should be no need to look at the member list, else the incomplete ways of the relation might also end at the given node, right?

in reply to:  19 comment:20 by skyper, 4 years ago

Replying to GerdP:

If the parents of the split node are all known there should be no need to look at the member list, else the incomplete ways of the relation might also end at the given node, right?

The parents of the end nodes of the way to split. This should work for most cases. Special treatment need:

  • loops not for download
  • dual-way routes with role forward/backward not for download
  • ptv1 is most tricky where even middle nodes might get important. Not sure if 7240.2 is valid where the two variants come together in a middle node.

You still need to look at the member list for the right position and if the new way needs to be added multiple times but usually not in order to decide if downloading more members is needed.

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

comment:21 by Don-vip, 4 years ago

Status of this ticket?

comment:22 by skyper, 4 years ago

The method to determine if ways are needed to download uses the member list neighbors but should use the neighbors in data, e.g. connected ways at end nodes of the way to split.

Still an issue see comment 18 and followups. I summarized the situation by updating the description of #17898

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

comment:23 by Don-vip, 4 years ago

Resolution: fixed
Status: newclosed

Thanks. Then we can close this one.

in reply to:  23 comment:24 by skyper, 4 years ago

Replying to Don-vip:

Thanks. Then we can close this one.

I understand that #18596 was closed but this one is not fixed at all. The examples are valid and there is no other ticket about the issue. Should I open another ticket again and link to the examples from #18596 and this ticket?

comment:25 by Don-vip, 4 years ago

Resolution: fixed
Status: closedreopened

I thought the remaining points were described in #17898 as per your last comment.

Last edited 4 years ago by Don-vip (previous) (diff)

comment:26 by skyper, 4 years ago

Sorry for my miscommunication. I kept #17898 as summary and enhancement but referencing this ticket as one remaining problem.
Thanks to JeroenHoek, the start was made and at least for downloaded route relations this is one last minor issue opening a dialog and downloading one unneeded way under special circumstances (route is already broken, minimal downloaded area bboxes etc.). I'd say overall a major progress for route relations.
Why not simply move this ticket to next milestone?

comment:27 by GerdP, 4 years ago

Milestone: 20.0320.04
Owner: changed from team to GerdP
Status: reopenednew

comment:28 by GerdP, 4 years ago

@Dirk: Didn't want to change the status to new. How can I revert that?

in reply to:  28 comment:29 by stoecker, 4 years ago

Replying to GerdP:

@Dirk: Didn't want to change the status to new. How can I revert that?

Without tricks: No way. Simply set it to assigned when you care for this ticket, otherwise leave it as new :-)

comment:30 by GerdP, 4 years ago

Status: newassigned

by GerdP, 4 years ago

Attachment: 18863-detect-broken.patch added

comment:31 by GerdP, 4 years ago

The patch detects the case that a complete way connected with the split-way shows that the relation order is broken. This check is only performed when the direct neighbours according to the relation are both incomplete.
@Skyper: While writing this I think the additional check should only be done when the first/last node of the split-way is not outside the download area. Else the incomplete ways could still be connected to the split-way, right?

by GerdP, 4 years ago

modified example where split ways first and last node are inside the download area

by GerdP, 4 years ago

Attachment: 18863-detect-broken.2.patch added

only perform additional check if first and last node of split-way are inside download area

in reply to:  31 ; comment:32 by skyper, 4 years ago

Replying to GerdP:

The patch detects the case that a complete way connected with the split-way shows that the relation order is broken. This check is only performed when the direct neighbours according to the relation are both incomplete.
@Skyper: While writing this I think the additional check should only be done when the first/last node of the split-way is not outside the download area. Else the incomplete ways could still be connected to the split-way, right?

Yes, that is why I would look at the end nodes from the beginning on. Once you have downloaded both there is nothing more to download. Only one end node is not enough for all cases, though, I would not mind if both are downloaded. Much better than from member list, see below.
At least one child node should be downloaded already just to determine that all parent relations are known but that is #17898.

Another disadvantage of downloading neighbors from relation`s member list is that the downloaded object could be everywhere on the globe and not nearby, thus creating a big/huge bbox.

in reply to:  32 ; comment:33 by GerdP, 4 years ago

Replying to skyper:

Yes, that is why I would look at the end nodes from the beginning on. Once you have downloaded both there is nothing more to download. Only one end node is not enough for all cases, though, I would not mind if both are downloaded. Much better than from member list, see below.
At least one child node should be downloaded already just to determine that all parent relations are known but that is #17898.

Both end nodes are already downloaded, else the split-way would not be complete and thus it would not be displayed.

Another disadvantage of downloading neighbors from relation`s member list is that the downloaded object could be everywhere on the globe and not nearby, thus creating a big/huge bbox.

The bbox is not changed when one or two more member are downloaded.

Your answer is quite confusing. It would make sense if we would download small areas around the end nodes, but that doesn't happen.

in reply to:  33 ; comment:34 by skyper, 4 years ago

Replying to GerdP:

Replying to skyper:

Yes, that is why I would look at the end nodes from the beginning on. Once you have downloaded both there is nothing more to download. Only one end node is not enough for all cases, though, I would not mind if both are downloaded. Much better than from member list, see below.
At least one child node should be downloaded already just to determine that all parent relations are known but that is #17898.

Both end nodes are already downloaded, else the split-way would not be complete and thus it would not be displayed.

Sorry, I meant downloaded with parents, e.g. within downloaded area.

Another disadvantage of downloading neighbors from relation`s member list is that the downloaded object could be everywhere on the globe and not nearby, thus creating a big/huge bbox.

The bbox is not changed when one or two more member are downloaded.

Probably not the correct wording but some boxes change.
Just test it with zoom to data. If the member is somewhere else the data area turns huge. Validator will run tests on the downloaded object and if you modify something the bbox of the changeset might get huge.

Your answer is quite confusing. It would make sense if we would download small areas around the end nodes, but that doesn't happen.

Exactly. That would make life much easier.

in reply to:  34 ; comment:35 by GerdP, 4 years ago

Your answer is quite confusing. It would make sense if we would download small areas around the end nodes, but that doesn't happen.

Exactly. That would make life much easier.

I might also download much more data than just the neeeded way member(s), at least all ways connected to the end node and also other relations, so I don't think that the current approach (with my patch) is wrong.

in reply to:  35 comment:36 by skyper, 4 years ago

Replying to GerdP:

Your answer is quite confusing. It would make sense if we would download small areas around the end nodes, but that doesn't happen.

Exactly. That would make life much easier.

I might also download much more data than just the needed way member(s), at least all ways connected to the end node and also other relations, so I don't think that the current approach (with my patch) is wrong.

Good point. Makes sense. Thanks for sharing. So only one split-way child node needs to be within downloaded area for #17898 to make sure all parent relations are known.

Version 1, edited 4 years ago by skyper (previous) (next) (diff)

comment:37 by skyper, 4 years ago

I apologize for being quite ignorant and looking at the problem from one perspective, only. Additionally, I made some confusion. Well, let's get this straight.

  • Looking at the member list neighbors is the way to go. Jeroen's approach is right, sorry for criticizing.
  • My problem is with unordered relations and both child end nodes of the split-way inside downloaded area.
  1. check if all parents are known, see #17898
  2. check if more information can be gathered by downloading
    • check if both end nodes are within downloaded area
    • check if both neighbors in member list are complete
  3. dialog with option to download both neighbors from member list (download only ways without parents)

Looking at only one neighbor does only work with ordered relations and for dual(multi)-way route relations without roles. Better always download both which does not harm much.

comment:38 by GerdP, 4 years ago

Resolution: fixed
Status: assignedclosed

In 16242/josm:

fix #18863: split ways: false positive warnings about missing members

  • apply 18863-detect-broken.2.patch to detect the case that a complete way connected with the split-way shows that the relation order is broken. This check is only performed when the direct neighbours according to the relation are both incomplete anf if first and last node of split-way are inside download area.

comment:39 by GerdP, 4 years ago

In 16302/josm:

see #18863:split ways: false positive warnings about missing members
Correct and simplify additional check introduced with r16242 : order of "!" and "(" was wrong
There is no need to look at other members when the parents of the end nodes of the split-way are known.

comment:40 by Klumbumbus, 4 years ago

Milestone: 20.0420.05

Milestone renamed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain GerdP.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.