Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iD damages relations when splitting ways #8578

Open
robhubi opened this issue Jul 13, 2021 · 21 comments
Open

iD damages relations when splitting ways #8578

robhubi opened this issue Jul 13, 2021 · 21 comments
Labels
bug-release-blocker An important bug - let's get this fixed in the next release!

Comments

@robhubi
Copy link

robhubi commented Jul 13, 2021

My System

Windows 10, 64 Bit
Firefox 89.0.2, 64 Bit
MS Edge 91.0.864.67, 64 Bit
iD 2.19.6

Description

Splitting ways that are members of relations destroys the order of the members. See also Order matters.

Steps to Reproduce

Starting point
The Rabenfeldstraße is, among other things, part of the Relation Weingarten-Tour. It has 58 members:
iDbR_01

Change
The "Rabenfeldstraße" is split with the scissors:
iDbR_02

Actual Result

As expected, the relation Weingarten-Tour has 59 members afterwards, but the order is destroyed:
iDbR_03

The effect is not always reproducible. On the "Panorama Tour (Eibiswald)" it no longer occurred the third time I wanted to take screenshots. With the "Weingarten Tour" I took the screenshots on the first repetition.
Of course, I did not save the changes.

Expected Result

Unchanged order of members as done by JOSM.

Other, possibly relevant issues

#4978
#4876
#4861

PLEASE

Do not allow splitting of ways that are part of a relation as long as the issue is unresolved.

@hungerburg
Copy link

Did you try with release version 2.20? Reading #8612 makes it look like iD started to validate and therefore fetch route relations in their entirety, which might be promising?

On the expected result: Of course order will be changed, there is a new member in town! Then, behaviour does not depend on how JOSM performs, the error is an error, regardless of that.

But most of all: How is order destroyed? Does only the new member end up in the wrong place? Or are members completely shuffled?

@robhubi
Copy link
Author

robhubi commented Aug 27, 2021

Version 2.20.1 shows the error in the same way.

If the way "3" is divided, the relation list "... 2, 3, 4 ..." should turn into the sequence "... 2, 3a, 3b, 4 ...". This is what is meant by constant order.
JOSM is mentioned because it could provide a blueprint for problem solving.

The issue documented above shows the effect of the error. Before the way splitting, the first 5 members of the relation are completely different from the first 5 members after the way splitting.

@hungerburg
Copy link

Recently an iD user did split several streets around my home-base, afflicting lots of PSV route relations. The editor actually did a very good job at this. The change to the relation was minimal, order was perfectly retained. Shouldn't both PSV and cycle routes be similar?

@robhubi
Copy link
Author

robhubi commented Oct 6, 2021

Interesting. Unfortunately, hiking and cycling routes are often affected by this issue.
Which boundary conditions lead to this behaviour can probably only be judged by the software developer.

@hungerburg
Copy link

You can do some black-box-testing. iD is only supported by volunteers (at the moment.)

@1ec5
Copy link
Collaborator

1ec5 commented Oct 7, 2021

The Rabenfeldstraße is, among other things, part of the Relation Weingarten-Tour.

This route relation forms a loop with a short spur at the southwestern corner. Most of the reported cases of incorrect relation member ordering so far have involved routes that double back on themselves in a similar manner. As you point out, recreational routes are particularly prone to this issue because, unfortunately, it’s common for spurs and excursions to be added to the main route relation. By contrast, public transport routes (and increasingly road routes) rely on superrelations or route master relations to associate linear routes with each other.

There’s quite a bit of relevant technical discussion about the issue in #4876 and #4589. The relevant code is here:

// We're adding a member that must stay paired with an existing member.
// (This feature is used by `actionSplit`)
//
// This is tricky because the members may exist multiple times in the
// member list, and with different A-B/B-A ordering and different roles.
// (e.g. a bus route that loops out and back - #4589).
//
// Replace the existing member with a temporary way,
// so that `osmJoinWays` can treat the pair like a single way.

@robhubi
Copy link
Author

robhubi commented Oct 7, 2021

Many thanks for this hint! The explanation for the relative frequency of issues in the Recreational Routes seems very plausible to me.
The SW excerpt shows me that the developers have considered many things. Now I also see that splitting ways works in principle also for relations members. However, it sometimes fails for unknown (random?) reasons. Examples:

Test Panorama Tour (Eibiswald) on 12 July 2021.
Test repeated 5x within one hour (test case see issue above):

  • 1st test: order damaged
  • 2nd test: order damaged
  • 3rd test: order damaged
  • 4th test: order ok
  • 5th test: order ok

Even in later tests, the order was always maintained.

Test Weingarten Tour on 12 July 2012
The test was carried out 2 times in a row, the order was damaged each time.

Test Weingarten Tour, today
The test was carried out 4 times in a row:

  • 1st test: order ok
  • 2nd test: order damaged
  • 3rd test: order ok
  • 4th test: order ok

Changed order after the 2nd test:
WGT2erT

In principle, keeping order seems to work, but sometimes (by random?) it goes wrong.

@tordans
Copy link
Collaborator

tordans commented Oct 8, 2021

In principle, keeping order seems to work, but sometimes (by random?) it goes wrong.

I think I remember that it has to do with which parts of the relation have been downloaded at the time of the test. Is this something that might have changed in your test cases?

@robhubi
Copy link
Author

robhubi commented Oct 8, 2021

Yes, I paid attention to which parts were downloaded.

  • Test case 2: Before splitting members manually NOT downloaded - order damaged
  • Test case 3: Before splitting members manually NOT downloaded - order ok

There seems to be no direct correlation with the issue.

@hungerburg
Copy link

Test case 2 was in July, three months before case 3 and an iD release in between; see my post above, iD changed downloading touching stuff; which then did not change anything though?

@robhubi
Copy link
Author

robhubi commented Oct 8, 2021

Sorry for the confusion. In this post

Yes, I paid attention to which parts were downloaded.

* Test case 2: Before splitting members manually NOT downloaded - order damaged

* Test case 3: Before splitting members manually NOT downloaded - order ok

There seems to be no direct correlation with the issue.

Test cases related to the last test "Test Weingarten Tour, today" of 7 Oct. 21. iD version was 2.20.1, tests were completed within 1 hour.

@1ec5
Copy link
Collaborator

1ec5 commented Oct 9, 2021

By the way, you may find it more convenient to test iD without uploading your changes to the live OSM server: click the “Download osmChange file” link at the bottom of the upload screen, then manually inspect the file’s list of relation member IDs to see if the order is correct. One side of the split will have a negative value, and the other should be adjacent to it in the list.

@robhubi
Copy link
Author

robhubi commented Feb 8, 2022

Addendum for completeness
This issue was the subject of a discussion on OSMF-Talk.
A discussion about 23 mails ensued. A short summary can be found here.

@hungerburg
Copy link

To bring here the information the OP supplied in the local forum: The case "Weingartentour" seems to be fixed with the new release, only the case "Panorama Tour" remains wanting - I guess that is this one https://www.openstreetmap.org/relation/3132692 ?

The Panorama route looks quite simple, one complete round, no spurs. Searching for differences - it goes thtough a roundabout, that is mapped in much detail - https://www.openstreetmap.org/relation/3132692#map=19/46.68934/15.24868 - Tough to sort, quite close to the Rabenfeldstraße https://www.openstreetmap.org/way/962203250 too.

PS: What is completeness meant to say?

@natfoot
Copy link

natfoot commented Apr 28, 2022

I use ID mostly for my edits and if I edit and an existing multipolygon (...Node Line Node Line Node Line Node Line Node...) and remove the center node in the multi polygon there is now a hole in the multiploygon. Save. When I save I get no errors that the multipolygon is now (broken). No warning no errors are being displayed.

Video evidence to the above description: https://youtu.be/0-9AMyahcGc

To recreate this
select a large recently unchanged multipolygon.
create three new nodes in a line
cut middle one
delete it.
Save the data.

@hungerburg
Copy link

@natfoot I guess, what you describe is worth an issue of its own. This here is only about sorting the order of relation members, not about them providing closed ways.

BTW: your example matches the label bug much more than this one.

@arminus
Copy link

arminus commented Sep 18, 2022

Just stumbled into this issue with https://www.openstreetmap.org/changeset/126323215 by splitting a simple way - fixed the damage in JOSM afterwards. Didn't happen on the split of another way, so this looks like the new ordering is just determined by a coin toss and not by considering the original order ;-)

Never had this problem when splitting ways in JOSM, just saying...

@1ec5
Copy link
Collaborator

1ec5 commented Sep 18, 2022

Just stumbled into this issue with https://www.openstreetmap.org/changeset/126323215 by splitting a simple way

This is related to the roundabout case discussed above in that this route relation is nonlinear: it includes all of this roadway but also this pathway that intersects it midway. If this roadway had been split and the route were continuous, I suspect the split you performed would’ve worked without any problems. In fact, if the relation had been poorly sorted beforehand, iD would’ve fixed the sorting. The reason JOSM doesn’t have this problem is that it doesn’t automatically sort relation members in the same manner.

/ref #8578 (comment)

@arminus
Copy link

arminus commented Sep 19, 2022

Just stumbled into this issue with https://www.openstreetmap.org/changeset/126323215 by splitting a simple way

This is related to the roundabout case discussed above in that this route relation is nonlinear: it includes all of this roadway but also this pathway that intersects it midway. If this roadway had been split and the route were continuous,

that split broke 3 MTB relations, I can't be sure they were ok before the split, but all 3 of them already broken/non-continuous? I don't know... And the fix in JOSM was to shuffle up a couple of ways "left" of the split, that intersection you pointed out had nothing to do with it.

Be that as it may - I know from my own experience that relation sorting is tricky. I was unsuccessfully trying to do my own resort implementation for my via ferrata and cross country skiing trails map site elevation profiles for a long time and eventually gave up. But that was only for a data consumer. If a resorting approach is known to break properly sorted relations (even only in edge cases) in the actual data, I'd make that an option (with a possibility to visually verify) and not a default.

I frequently observe broken relations for the ferrata and cross country skiing trail relations I keep an eye on, that iD behavior could be an explanation.

@1ec5
Copy link
Collaborator

1ec5 commented Sep 20, 2022

And the fix in JOSM was to shuffle up a couple of ways "left" of the split, that intersection you pointed out had nothing to do with it.

The sorting code operates on the whole relation without assuming that any members were previously sorted, so it’s possible for a loop or backtrack to affect members far away. MTB Prinzenweg-Rundtour and MTB Alpbachtal Rundtour both have a loop (a very large one) along with a “tail”, so splitting anything along either relation would definitely trigger #4876. I can’t explain MTB Almtour though – I was under the impression that this issue doesn’t affect routes that only consist of a loop. Maybe it affects them too, maybe nondeterministically as #8578 (comment) discovered.

If a resorting approach is known to break properly sorted relations (even only in edge cases) in the actual data, I'd make that an option (with a possibility to visually verify) and not a default.

At a minimum, disabling the automatic resorting by default would require a more robust UI for ordering relations manually. But I think it would probably uncover a lot of other issues that would be even more commonplace than this loop issue. If I remember correctly, automatic resorting was implemented in response to a lot of feedback that iD was mangling member order of perfectly linear relations, so those latent issues would need to be addressed at the same time.

As far as I can tell, the loop/discontinuity issue raised here so far doesn’t appear to be a fundamentally intractable shortcoming of the automatic resorting approach. Rather, it’s an edge case that wasn’t addressed originally, essentially a to-do item as I mentioned in #8578 (comment). Unfortunately, it has been tripping up certain kinds of route relations more frequently than originally expected.

It is possible, though not particularly convenient, to visually verify the order before saving: with the relation selected, hover the cursor over each member in the list. As you hover over each member, iD highlights it on the map. Verify that the members are highlighted in the order that you expect. You can drag members around to fix the order.

@tyrasd tyrasd added bug A bug - let's fix this! bug-release-blocker An important bug - let's get this fixed in the next release! and removed bug A bug - let's fix this! labels Mar 30, 2023
@Mahabarata
Copy link

See also the issue #8415

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-release-blocker An important bug - let's get this fixed in the next release!
Projects
None yet
Development

No branches or pull requests

8 participants