Modify

Opened 4 months ago

Closed 4 months ago

Last modified 4 months 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 4 months ago.
combine-buildings.osm (1.7 KB) - added by GerdP 4 months ago.
combine-highways.osm (3.3 KB) - added by GerdP 4 months ago.
18367.patch (4.8 KB) - added by GerdP 4 months ago.
silent-revert.osm (889 bytes) - added by GerdP 4 months ago.
18367-v2.patch (9.4 KB) - added by GerdP 4 months ago.

Download all attachments as: .zip

Change History (22)

Changed 4 months ago by stoecker

Attachment: nocombine.osm added

comment:1 Changed 4 months ago by 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.

comment:2 Changed 4 months ago by GerdP

I agree.

comment:3 Changed 4 months ago by 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.

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

Version 1, edited 4 months ago by stoecker (previous) (next) (diff)

comment:4 in reply to:  1 Changed 4 months ago by 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.

comment:5 Changed 4 months ago by 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.

comment:6 in reply to:  5 Changed 4 months ago by 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.

Changed 4 months ago by GerdP

Attachment: combine-buildings.osm added

Changed 4 months ago by GerdP

Attachment: combine-highways.osm added

comment:7 Changed 4 months ago by 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.

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

comment:8 in reply to:  7 Changed 4 months ago by 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.

comment:9 Changed 4 months ago by GerdP

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

Changed 4 months ago by GerdP

Attachment: 18367.patch added

Changed 4 months ago by GerdP

Attachment: silent-revert.osm added

comment:10 Changed 4 months ago by 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.

comment:11 Changed 4 months ago by stoecker

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

comment:12 Changed 4 months ago by GerdP

I'll try.

Changed 4 months ago by GerdP

Attachment: 18367-v2.patch added

comment:13 Changed 4 months ago by GerdP

todo: check that ways are not reverted.

comment:14 Changed 4 months ago by GerdP

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 Changed 4 months ago by GerdP

Milestone: 19.12

comment:16 Changed 4 months ago by GerdP

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.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.