Modify

Opened 3 months ago

Closed 2 months ago

#18385 closed defect (fixed)

Combine way action may remove parts of the ways

Reported by: GerdP Owned by: GerdP
Priority: normal Milestone: 19.12
Component: Core Version:
Keywords: Cc: Don-vip

Description

Attachments (0)

Change History (11)

comment:1 Changed 3 months ago by GerdP

Owner: changed from team to GerdP
Status: assignednew

comment:2 Changed 3 months ago by GerdP

Resolution: fixed
Status: newclosed

In 15555/josm:

fix #18385: Combine way action may remove parts of the ways

  • Count all edges that are added to the graph, even if they are rejected because they are duplicates.
  • add new method buildSpanningPathNoRemove() which returns null if any duplicate edge was found

and use this method in CombineWayAction

comment:3 Changed 3 months ago by GerdP

In 15556/josm:

see #18385 update @since

comment:4 Changed 3 months ago by GerdP

Cc: Don-vip added

@Vincent: I guess org.openstreetmap.josm.plugins.opendata.core.datasets.WayCombiner should also use this new method?
It is only used in org.openstreetmap.josm.plugins.opendata.modules.fr.paris.datasets.urbanisme.SanisettesHandler

comment:5 Changed 3 months ago by GerdP

Resolution: fixed
Status: closedreopened

The wanted result is a joined way, not the message that the ways cannot be joined. So, we need a datastructure that remembers the complete data.

Last edited 3 months ago by GerdP (previous) (diff)

comment:6 Changed 2 months ago by Don-vip

@Gerd: I have removed this class in [o35244:35245]. Not worth keeping it.

comment:7 Changed 2 months ago by Don-vip

There's a Sonar warning: https://josm.openstreetmap.de/sonar/project/issues?id=josm&issues=AW7MgRMl-XCMeap-x5vR&open=AW7MgRMl-XCMeap-x5vR

It would be safer to return an empty collection than null.

comment:8 Changed 2 months ago by GerdP

I also don't like it, but the older public method buildSpanningPath() does the same and callers expect the null value.

comment:9 Changed 2 months ago by Don-vip

I don't find any plugin calling this method, so it is probably safe to change the behaviour if you want.

comment:10 Changed 2 months ago by GerdP

See also last patch in #18367. I can change the methods to private and let tryJoin() return an empty collection to signal that the ways cannot be joined.

comment:11 Changed 2 months ago by GerdP

Resolution: fixed
Status: reopenedclosed

In 15574/josm:

fix #18367 and #18385: CombineWayAction (C) refuses to combine ways or silently reverses ways
Changes:

  • try first to combine the ways with the method Multipolygon.joinWays() If that method returns a single line string we can use it, else use the result of NodeGraph.buildSpanningPathNoRemove(). Both methods will not add or remove segments
  • if ways are combined execute checks for overlapping segments or self-intersection and show a notification popup right after the command was added to the UndoRedoHandler
  • The code which handles reversed ways needed changes. In the unpatched version it sometimes claims wrongly that ways were reversed, in special cases it sometimes silently reverted ways. The old code did not handle the case properly that a node can appear more than once. I really hope that I got it right now.
  • Fix some sonarlint issues
  • let NodeGraph routines return an ArrayList instead of a LinkedList (improves performance a bit)
  • Add unit tests

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.