Modify

Opened 9 months ago

Closed 8 months ago

Last modified 7 months 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 9 months ago.
small example
Burkheimer.png (12.3 KB) - added by JeroenHoek 9 months ago.
josm_18596_false_positive.osm.bz2 (148.0 KB) - added by JeroenHoek 9 months ago.
josm_18863_false_positive_small2.osm.bz2 (12.2 KB) - added by skyper 9 months ago.
new smaller example
0001-Split-way-don-t-download-members-if-unneeded.patch (198.1 KB) - added by JeroenHoek 9 months ago.
Fix
18863-detect-broken.patch (3.3 KB) - added by GerdP 8 months ago.
josm_18596_false_positive_inside.osm.bz2 (1.9 KB) - added by GerdP 8 months 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 8 months 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)

Changed 9 months ago by skyper

small example

comment:1 Changed 9 months ago by skyper

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

comment:2 Changed 9 months ago by 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).

Last edited 9 months ago by JeroenHoek (previous) (diff)

Changed 9 months ago by JeroenHoek

Attachment: Burkheimer.png added

Changed 9 months ago by JeroenHoek

comment:3 Changed 9 months ago by JeroenHoek

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 9 months ago by JeroenHoek (previous) (diff)

comment:4 Changed 9 months ago by JeroenHoek

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

comment:5 Changed 9 months ago by JeroenHoek

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

comment:6 in reply to:  2 Changed 9 months ago by skyper

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.

Changed 9 months ago by skyper

new smaller example

comment:7 Changed 9 months ago by skyper

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

Split at node id:5332134803

Last edited 9 months ago by skyper (previous) (diff)

comment:8 Changed 9 months ago by JeroenHoek

You screenshot is incorrect

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

Found a valid small example​

Split node id?

comment:9 in reply to:  8 Changed 9 months ago by skyper

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.

comment:10 in reply to:  7 ; Changed 9 months ago by 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.

comment:11 in reply to:  10 Changed 9 months ago by skyper

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 Changed 9 months ago by JeroenHoek

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.

Changed 9 months ago by JeroenHoek

Fix

comment:13 Changed 8 months ago by skyper

Cc: GerdP added

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

comment:14 Changed 8 months ago by GerdP

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 Changed 8 months ago by GerdP

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 Changed 8 months ago by GerdP

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

comment:17 in reply to:  14 Changed 8 months ago by skyper

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.

comment:18 in reply to:  15 Changed 8 months ago by skyper

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 Changed 8 months ago by 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?

comment:20 in reply to:  19 Changed 8 months ago by skyper

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 8 months ago by skyper (previous) (diff)

comment:21 Changed 8 months ago by Don-vip

Status of this ticket?

comment:22 Changed 8 months ago by skyper

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 8 months ago by skyper (previous) (diff)

comment:23 Changed 8 months ago by Don-vip

Resolution: fixed
Status: newclosed

Thanks. Then we can close this one.

comment:24 in reply to:  23 Changed 8 months ago by skyper

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 Changed 8 months ago by Don-vip

Resolution: fixed
Status: closedreopened

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

Last edited 8 months ago by Don-vip (previous) (diff)

comment:26 Changed 8 months ago by skyper

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 Changed 8 months ago by GerdP

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

comment:28 Changed 8 months ago by GerdP

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

comment:29 in reply to:  28 Changed 8 months ago by stoecker

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 Changed 8 months ago by GerdP

Status: newassigned

Changed 8 months ago by GerdP

Attachment: 18863-detect-broken.patch added

comment:31 Changed 8 months ago by 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?

Changed 8 months ago by GerdP

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

Changed 8 months ago by GerdP

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

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

comment:32 in reply to:  31 ; Changed 8 months ago by skyper

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.

comment:33 in reply to:  32 ; Changed 8 months ago by 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.

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.

comment:34 in reply to:  33 ; Changed 8 months ago by skyper

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.

comment:35 in reply to:  34 ; Changed 8 months ago by 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 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.

comment:36 in reply to:  35 Changed 8 months ago by skyper

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.

Did update #17898.

Last edited 8 months ago by skyper (previous) (diff)

comment:37 Changed 8 months ago by skyper

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 Changed 8 months ago by GerdP

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 Changed 8 months ago by GerdP

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 Changed 7 months ago by Klumbumbus

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.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.