#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?
- load attached file
- 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)
Change History (48)
by , 5 years ago
Attachment: | josm_18596_false_positive_small.osm.bz2 added |
---|
comment:1 by , 5 years ago
Summary: | split ways: false positives warnings about missing members → split ways: false positive warnings about missing members |
---|
follow-up: 6 comment:2 by , 5 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).
by , 5 years ago
Attachment: | Burkheimer.png added |
---|
by , 5 years ago
Attachment: | josm_18596_false_positive.osm.bz2 added |
---|
comment:3 by , 5 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.
comment:4 by , 5 years ago
I think I've fixed the dialog being triggered unnecessarily in josm_18596_false_positive.osm.bz2
.
comment:5 by , 5 years ago
Summary: | split ways: false positive warnings about missing members → [PATCH] split ways: false positive warnings about missing members |
---|
comment:6 by , 5 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.
follow-up: 10 comment:7 by , 5 years ago
follow-up: 9 comment:8 by , 5 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?
comment:9 by , 5 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.
follow-up: 11 comment:10 by , 5 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.
comment:11 by , 5 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 , 5 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.
follow-up: 17 comment:14 by , 5 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:16 by , 5 years ago
Milestone: | → 20.03 |
---|---|
Owner: | changed from | to
Status: | new → needinfo |
comment:17 by , 5 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.
comment:18 by , 5 years ago
Owner: | changed from | to
---|---|
Status: | needinfo → new |
Summary: | [PATCH] split ways: false positive warnings about missing members → split 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.
follow-up: 20 comment:19 by , 5 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?
comment:20 by , 5 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:
loopsnot for downloaddual-way routes with rolenot for downloadforward/backward
- 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.
comment:22 by , 5 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
follow-up: 24 comment:23 by , 5 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Thanks. Then we can close this one.
comment:24 by , 5 years ago
comment:25 by , 5 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
I thought the remaining points were described in #17898 as per your last comment.
comment:26 by , 5 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 , 5 years ago
Milestone: | 20.03 → 20.04 |
---|---|
Owner: | changed from | to
Status: | reopened → new |
follow-up: 29 comment:28 by , 5 years ago
@Dirk: Didn't want to change the status to new. How can I revert that?
comment:29 by , 5 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 , 5 years ago
Status: | new → assigned |
---|
by , 5 years ago
Attachment: | 18863-detect-broken.patch added |
---|
follow-up: 32 comment:31 by , 5 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 , 5 years ago
Attachment: | josm_18596_false_positive_inside.osm.bz2 added |
---|
modified example where split ways first and last node are inside the download area
by , 5 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
follow-up: 33 comment:32 by , 5 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.
follow-up: 34 comment:33 by , 5 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.
follow-up: 35 comment:34 by , 5 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.
follow-up: 36 comment:35 by , 5 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.
comment:36 by , 5 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.
Did update #17898.
comment:37 by , 5 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.
- check if all parents are known, see #17898
- 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
- 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.
small example