Modify

Opened 11 years ago

Last modified 3 years ago

#8968 new defect

Combining ways: Roles not adjusted when changing direction

Reported by: skyper Owned by: team
Priority: major Milestone:
Component: Core Version: latest
Keywords: template_report, combine, way, role, public-transport-v1 Cc:

Description

What steps will reproduce the problem?

  1. Have a way with role forward/backward and a second connected way with opposite direction.
  2. Combine the ways and have a look at the roles
  3. JOSM combines by changing the direction of one way, depending on the selction order.

What is the expected result?

Ways are combined and if a direction of a way is changed its roles are properly adjusted

What happens instead?

The roles are not adjusted which leads to damaged relations.

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

I did test with one untagged way which direction should always be changed in favour of the one with tags. Adding *=left/right to the tagged way exactly leads to this proper behaviour.

Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2013-08-13 01:35:36
Last Changed Author: Don-vip
Revision: 6143
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2013-08-11 23:47:22 +0200 (Sun, 11 Aug 2013)
Last Changed Rev: 6143

Identification: JOSM/1.5 (6143 en) Linux Debian GNU/Linux 7.1 (wheezy)
Memory Usage: 417 MB / 672 MB (126 MB allocated, but free)
Java version: 1.7.0_25, Oracle Corporation, OpenJDK 64-Bit Server VM
Java package: openjdk-7-jre:amd64-7u25-2.3.10-1~deb7u1
Program arguments: [/media/d95c6885-09a1-48fd-87c5-30922ffbd60f/hbf.osm]
Dataset consistency test: No problems found

Plugin: OpeningHoursEditor (29778)
Plugin: PicLayer (29809)
Plugin: conflation (0.1.6)
Plugin: jts (29613)
Plugin: mirrored_download (29753)
Plugin: notes (0.3)
Plugin: openstreetbugs (29435)
Plugin: reverter (29771)
Plugin: undelete (29555)
Plugin: utilsplugin2 (29771)
Plugin: wikipedia (29778)

Attachments (3)

8968-test1.osm (2.3 KB ) - added by AlfonZ 11 years ago.
dirtyfix.patch (1.0 KB ) - added by akks 10 years ago.
8968-test1_update.osm (5.8 KB ) - added by skyper 7 years ago.
updated test file

Download all attachments as: .zip

Change History (10)

by AlfonZ, 11 years ago

Attachment: 8968-test1.osm added

comment:1 by AlfonZ, 11 years ago

Osm file would be nice to have. I've tried following, but I am not sure, if it's what you meant.

Referring to 8968-test1.osm.
Select BC, then CD, combine. CD is reversed, role of combined way is forward - OK.
Select CD, then BC, combine. CD is reversed, role of combined way is backward - incorrect, should be forward.
Remove sidewalk=right from BC. Select CD, then BC, combine. BC is reversed, role of combined way is backward - OK.

comment:2 by Don-vip, 11 years ago

skyper: is AlfonZ' test file ok ? :)

in reply to:  2 comment:3 by skyper, 11 years ago

Replying to Don-vip:

skyper: is AlfonZ' test file ok ? :)

Yes, only different was that my untagged way had an ID above zero which might have an influence in choosing the final direction.
Plus:

  • combining BD and CD the result is always wrong.

comment:4 by akks, 10 years ago

I tried to investigate this problem.

The problem is incorrect detection of the ways that should be reversed.

 if (!confirmChangeDirectionOfWays()) return null;
       // filter out ways that have no direction-dependent tags
       unreversedWays = ReverseWayTagCorrector.irreversibleWays(unreversedWays);  ////  (!) ONLY TAGS ARE ANALYZED
       reversedWays = ReverseWayTagCorrector.irreversibleWays(reversedWays);      ////  (!) NOT RELATION MEMBERSHIP
       // reverse path if there are more reversed than unreversed ways with direction-dependent tags
       if (reversedWays.size() > unreversedWays.size()) {
           Collections.reverse(path);
           ...
           //SWAP   unreversedWays <-> reversedWays
       }
       //  change tags and memebership info for each 
       for (Way w : reversedWays) {
            ... 
            changePropertyCommands = reverseWayTagCorrector.execute(w, wnew);
       }
       ...

If the way has no "direction-dependent" tag, then it does not appear in reversedWays list and is not even analyzed for relation membership.

Here is the dirty fix - just skipping irreversibleWays calls solve the problem.

by akks, 10 years ago

Attachment: dirtyfix.patch added

comment:5 by akks, 10 years ago

Another place in the code that looks strange:

for (Way w : reversedWays) {
    Way wnew = new Way(w);
    reversedTagWays.add(wnew);
    changePropertyCommands = reverseWayTagCorrector.execute(w, wnew);
}
if ((changePropertyCommands != null) && !changePropertyCommands.isEmpty()) {
    for (Command c : changePropertyCommands) {
        c.executeCommand();
    }
}
}

It seems that the commands are executed only for the last way.
Most of them are to be deleted, so we do not notice it?

comment:6 by simon04, 8 years ago

Keywords: public-transport-v1 added

by skyper, 7 years ago

Attachment: 8968-test1_update.osm added

updated test file

comment:7 by skyper, 7 years ago

The behaviour did change but the bug is more or less still present:

changes:

  • sadly, there is not auto solving of the role which always leads to the tag/membership conflict dialog even though all are solvable.

problem:

  • the final direction of the way is not easy predictable as direction depending tags influence the behaviour.
  • In the tag/membership conflict dialog
    • you have no hint about the final direction.
    • the roles need to be adjusted in prior as one of the two ways did change the direction.

There was way to solve these conflicting membership without looking twice or even three times at the outcome and undoing again.

Did update the test file and added the two expected solutions combining BC <-> CD and BD <-> CD as id:0 should always be reverted
BC <-> BD should produce a conflict but with properly adjusted roles in the dialog.

r11428

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

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