Modify

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#18367 closed defect (fixed)

Lines cannot be combined when they share an overlapping segment

Reported by: stoecker Owned by: team
Priority: normal Milestone: 19.12
Component: Core Version:
Keywords: Cc:

Description

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.

Attachments (6)

nocombine.osm (1017 bytes ) - added by stoecker 5 years ago.
combine-buildings.osm (1.7 KB ) - added by GerdP 5 years ago.
combine-highways.osm (3.3 KB ) - added by GerdP 5 years ago.
18367.patch (4.8 KB ) - added by GerdP 5 years ago.
silent-revert.osm (889 bytes ) - added by GerdP 5 years ago.
18367-v2.patch (9.4 KB ) - added by GerdP 5 years ago.

Download all attachments as: .zip

Change History (22)

by stoecker, 5 years ago

Attachment: nocombine.osm added

comment:1 by Don-vip, 5 years ago

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.

comment:2 by GerdP, 5 years ago

I agree.

comment:3 by stoecker, 5 years ago

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.

Last edited 5 years ago by stoecker (previous) (diff)

in reply to:  1 comment:4 by stoecker, 5 years ago

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.

comment:5 by GerdP, 5 years ago

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.

in reply to:  5 comment:6 by stoecker, 5 years ago

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.

by GerdP, 5 years ago

Attachment: combine-buildings.osm added

by GerdP, 5 years ago

Attachment: combine-highways.osm added

comment:7 by GerdP, 5 years ago

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.

Last edited 5 years ago by GerdP (previous) (diff)

in reply to:  7 comment:8 by stoecker, 5 years ago

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.

comment:9 by GerdP, 5 years ago

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

by GerdP, 5 years ago

Attachment: 18367.patch added

by GerdP, 5 years ago

Attachment: silent-revert.osm added

comment:10 by GerdP, 5 years ago

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.

comment:11 by stoecker, 5 years ago

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

comment:12 by GerdP, 5 years ago

I'll try.

by GerdP, 5 years ago

Attachment: 18367-v2.patch added

comment:13 by GerdP, 5 years ago

todo: check that ways are not reverted.

comment:14 by GerdP, 5 years ago

Resolution: fixed
Status: newclosed

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

comment:15 by GerdP, 5 years ago

Milestone: 19.12

comment:16 by GerdP, 5 years ago

In 15576/josm:

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.