Lines cannot be combined when they share an overlapping segment

[stoecker]

Current SVN:15542

I know this is an invalid construct in OSM, but not in other use cases. JOSM should properly join this ways and ignore the fact that they overlap.

[Don-vip]

I don't see how you could join these ways "properly" without resulting in an invalid OSM geometry. Thus I would close this ticket as wontfix.

[GerdP]

I agree.

[stoecker]

Overlapping lines aren't an invalid geometry. It's only something which in OSM database is unwanted. But JOSM is no longer an OSM only editor and BTW joining such lines worked without problem in the past where simply the end of lines has been used for deciding what to join.

There is simply no reason to fail for such a case. And as said in #18368 we don't fail always. It is no feature when you only reject it sometimes, but simply a bug.

My personal usecase: I need to extract a geometry as a polygon which has a hole. The target software does not understand multipolygons, so an overlapping segment is required.

To reject unwanted things in OSM we have the validator. The editor functionality is not the right place for this.

[stoecker]

Replying to Don-vip:

I don't see how you could join these ways "properly" without resulting in an invalid OSM geometry.

Simply connecting them at the end. The same result you can get by drawing a new way along the points (or use the follow way function). These tools do not reject creating a line of that form.

[GerdP]

In fact it is quite difficult to draw a way with overlapping segments, so that would be an argument to allow the combining with overlaps.

[stoecker]

Replying to GerdP:

In fact it is quite difficult to draw a way with overlapping segments, so that would be an argument to allow the combining with overlaps.

Hmm. You're right. I have to use merge or other node joining to get this result. Since when does drawing an way point onto an existing way stops the drawing completely without a modifier to prevent this behaviour? That's fine as default, but there must be a way to prevent this.

[GerdP]

I am not sure what to do with this case (and also the ones in #18386)
The current code in CombineWayAction is able to combine closed ways and allows to create "invalid" OSM ways, like in attached sample combine-buildings.osm as well as correct ways as in sample combine-highways.osm
I think I've now also learned why it splits and reassembles the ways, this is needed for the rightmost example in combine-highways.osm where the ring doesn't start/end at the node where the lower way connects.
In other words, it will combine ways as long as that have no overlapping segments, and it might silently remove overlapping segments.
I think it should

• not add or remove any overlaps (combination must have same length as the uncombined ways)
• also allow to combine ways when there end nodes are connected
• warn if the combined ways is self-intersecting or when it has overlapping segments

Maybe it shold also allow to combine a bunch of ways "as much as possible"? Think of a complex multipolygon with lots of inner rings. If user selects all ways of that MP and presses C the result would be the closed rings, each a single way.

[stoecker]

I think it should

• not add or remove any overlaps (combination must have same length as the uncombined ways)

Yes.

• also allow to combine ways when there end nodes are connected

Yes. I'd accept that this is a special case for two ways - i.e. that the multi-way join does not handle all cases, so user is forced to step down to selecting two ways only. You anyway need to do that when you join many ways as often you select something which cannot be joined anyway (e.g. a closed outer nearby you did overlook).

• warn if the combined ways is self-intersecting or when it has overlapping segments

Only as notice, not as warning dialog a user has to click.

Maybe it shold also allow to combine a bunch of ways "as much as possible"? Think of a complex multipolygon with lots of inner rings. If user selects all ways of that MP and presses C the result would be the closed rings, each a single way.

I think that is probably not a good idea. It would make work easier, but currently users are forced to have a lock at what they do when joining fails. I think that's a good thing. Semi-automatic error handling will probably lead to hidden issues.

[GerdP]

OK. I'll work on a patch for this.

[GerdP]

The patch implements this:

• 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 (try the samples in combine-highways.osm). With the result from Multipolygon.joinWays() it sometimes silently reverted ways. See sample silent-revert.osm, select the short way first and than the other and press C. The short way is reverted. 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.

[stoecker]

Will you add this ticket stuff and the examples from the related tickets to the test cases?

[GerdP]

I'll try.

[GerdP]

todo: check that ways are not reverted.

[GerdP]

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

[GerdP]

In 15576/josm:

see #18367: Improve unit test coverage so that NodeGraph.getMostFrequentVisitedNodesFirst() is called