Modify

Opened 10 years ago

Closed 4 years ago

#9599 closed defect (fixed)

[Patch] "Join Overlapping Areas" creates a new way when joining 2 buildings

Reported by: didier2020 Owned by: team
Priority: normal Milestone: 20.02
Component: Core Version: tested
Keywords: join area Cc: hjart

Description (last modified by Don-vip)

for this exemple, "Shift J" (Join overlapping areas) create a new ways (id=0)
why not

using "R" (Inverser le chemins) on one of the 2 ways => "Shift J" modify one ways and delete the other
it's seems better ?

Attachments (5)

ex5.osm (2.5 KB ) - added by didier2020 10 years ago.
exemple fusion
join_area_comands.png (76.6 KB ) - added by Balaitous 10 years ago.
Undo/Redo history when join area
perplexing.osm (2.0 KB ) - added by GerdP 4 years ago.
I think the results of this action are more or less unpredictable for non-trivial cases, and are also sometimes perplexing with trivial cases. With the attached example nothing is changed (that's correct) but one way is marked as modified and would be uploaded.
9599.patch (5.1 KB ) - added by GerdP 4 years ago.
join-mp-ways.osm (3.4 KB ) - added by GerdP 4 years ago.
special case with multipolygon: select all ways and join areas

Download all attachments as: .zip

Change History (29)

by didier2020, 10 years ago

Attachment: ex5.osm added

exemple fusion

comment:1 by Don-vip, 10 years ago

Priority: minornormal

comment:2 by Don-vip, 10 years ago

Description: modified (diff)
Summary: "Maj J" create a new way"Join Overlapping Areas" creates a new way when joining 2 buildings

comment:3 by Don-vip, 10 years ago

Milestone: 14.02

comment:4 by Don-vip, 10 years ago

Milestone: 14.0214.03

did not have enough time to work on this one.

comment:5 by Don-vip, 10 years ago

Milestone: 14.0314.04

Sorry, still not enough time this month.

comment:6 by Balaitous, 10 years ago

Seems to be complex. JoinArea algorithm first split way into small piece, then clears unnecessary ways, join others.
According to splitting algorithm, initial way became the first piece, and could (or not) be deleted !
If it is not deleted, it will be used by the join algorithm.
Changing way direction, change the order of broken pieces of way, so initial way could be deleted in the first case, and kept in the other.

Somehow, the algorithm use the current dataset as a working space, leaving in undo/redo history many temporary way.

in reply to:  6 comment:7 by skyper, 10 years ago

Replying to Balaitous:

Seems to be complex. JoinArea algorithm first split way into small piece, then clears unnecessary ways, join others.
According to splitting algorithm, initial way became the first piece, and could (or not) be deleted !
If it is not deleted, it will be used by the join algorithm.
Changing way direction, change the order of broken pieces of way, so initial way could be deleted in the first case, and kept in the other.

How about temporally storing the lowest id ? In fact undo/redo needs to keep it somewhere anyway.

by Balaitous, 10 years ago

Attachment: join_area_comands.png added

Undo/Redo history when join area

comment:8 by Balaitous, 10 years ago

I have put an exemple :

As you can see many way were created, and then deleted. Where are the 2 initials way ?
This is what I call using the dataset as a working space.

Undo/Redo history when join area

I think it should be better to split the algorithm into two parts :

  • A geometry part, witch operate without any knowledge of dataset -> separate of JoinAreaAction, reusable for other purpose;
  • A dataset part, check of user selection, tag management and update of geometry -> the aim of JoinAreaAction.
Last edited 10 years ago by skyper (previous) (diff)

in reply to:  8 comment:9 by skyper, 10 years ago

I think it should be better to split the algorithm into two parts :

  • A geometry part, witch operate without any knowledge of dataset -> separate of JoinAreaAction, reusable for other purpose;
  • A dataset part, check of user selection, tag management and update of geometry -> the aim of JoinAreaAction.

+1

comment:10 by Don-vip, 10 years ago

Milestone: 14.0414.05

Feel free to improve the algorithms :) But starting from next week :)

comment:11 by simon04, 10 years ago

I do not consider the number of temporary ways created critical. When joining a way existing on the server (having a "real" id) with another way, the id of the existing way is persisted.

Code clean-ups, especially when bugs are removed or features are added, are very welcome. However, one has to make one's choice whether it's worth the effort in this case. ;-)

comment:12 by Don-vip, 10 years ago

Milestone: 14.0514.06

comment:13 by simon04, 10 years ago

Milestone: 14.06

I'd remove this nice-to-have algorithm-code-cleanup from the release planning …

comment:14 by simon04, 8 years ago

Ticket #12923 has been marked as a duplicate of this ticket.

comment:15 by simon04, 8 years ago

Cc: hjart added

comment:16 by hjart, 8 years ago

Uh, 3 years since it was first reported? Sorry for maybe being a bit too noisy, but I just feel like letting you know that I'd appreciate if the id of the older building/polygon was kept.

comment:17 by skyper, 4 years ago

Keywords: join area added

Still reproducible:

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-02-03 22:02:21 +0100 (Mon, 03 Feb 2020)
Revision:15813
Build-Date:2020-02-04 02:30:59
URL:https://josm.openstreetmap.de/svn/trunk

by GerdP, 4 years ago

Attachment: perplexing.osm added

I think the results of this action are more or less unpredictable for non-trivial cases, and are also sometimes perplexing with trivial cases. With the attached example nothing is changed (that's correct) but one way is marked as modified and would be uploaded.

by GerdP, 4 years ago

Attachment: 9599.patch added

comment:18 by GerdP, 4 years ago

Milestone: 20.02
Summary: "Join Overlapping Areas" creates a new way when joining 2 buildings[Patch] "Join Overlapping Areas" creates a new way when joining 2 buildings

With the patch, the oldest way should be kept for each created polygon. Quite a lot of new code is needed for that.
Part of the problem is that CombineWayAction selects the first old way and not the oldest. I don't dare to change that.

I'll add unit tests and maybe I can also fix the problem shown in perplexing.osm.

comment:19 by GerdP, 4 years ago

Resolution: fixed
Status: newclosed

In 15883/josm:

fix #9599: Join Overlapping Areas" creates a new way when joining 2 buildings
Two different problems are solved with this:

  • when inner loops are removed it was possible that the old way was removed and only a new part remained
  • when ways are joined with CombineWaysAction, the first old way was kept, not the oldest one

comment:21 by GerdP, 4 years ago

Resolution: fixed
Status: closedreopened

There is still a problem when inner ways of multipolygons are involved.

comment:22 by GerdP, 4 years ago

Resolution: fixed
Status: reopenedclosed

In 15886/josm:

fix #9599: Join Overlapping Areas" creates a new way when joining 2 buildings

  • now also takes care when inner ways of multipolygons are selected together with outer ways

by GerdP, 4 years ago

Attachment: join-mp-ways.osm added

special case with multipolygon: select all ways and join areas

comment:23 by GerdP, 4 years ago

Resolution: fixed
Status: closedreopened

There is a regression: When I select the inner ways of join-mp-ways.osm and join them I see a popup asking me to confirm the removal of a member. Ths should not appear.

comment:24 by GerdP, 4 years ago

Resolution: fixed
Status: reopenedclosed

In 15887/josm:

fix #9599: Join Overlapping Areas" creates a new way when joining 2 buildings
Have to call keepOlder() earlier and keepOlder() must return the original way, not the copy, as the copy will not be part of the dataset.

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.