Modify

Opened 14 years ago

Closed 13 years ago

#5179 closed defect (fixed)

[PATCH] Join overlapping areas produces bad results

Reported by: bilbo Owned by: team
Priority: normal Milestone:
Component: Core Version: latest
Keywords: Cc:

Description

I noticed the "Join overlapping areas" tool sometimes produces bad results, often on more complex ways - results which are not a union of the two areas that should be joined.

See attached example. Select both ways in it and press Shift-J to join the areas. Result is one way, which is not the sum of the areas.

Attachments (26)

failed_merge.osm (3.1 KB ) - added by bilbo 14 years ago.
Example that demonstrates the bug
bug-join.osm (16.5 KB ) - added by bilbo 14 years ago.
Another example where the algorithm misbehaves, though the results are not as bad as in the first one
failed_merge-2.osm (1.2 KB ) - added by extropy 14 years ago.
Anoder example, two simple rectangles.
osm-join-areas.patch (14.2 KB ) - added by extropy 14 years ago.
New implementation of findInnerWays, fixes reported problems.
join-areas-adjacent.osm (1.1 KB ) - added by bastiK 14 years ago.
Shows problem of the first version of the patch
osm-join-areas-2.patch (14.2 KB ) - added by extropy 14 years ago.
Improved patch.
join-areas-example 2-3.osm (2.9 KB ) - added by anonymous 14 years ago.
osm-join-areas-3.patch (19.4 KB ) - added by extropy 14 years ago.
Improved patch.
join-areas-example 4.osm (1.2 KB ) - added by bastiK 14 years ago.
next example
2.osm (1.3 KB ) - added by bastiK 14 years ago.
example 4:2 - produces self overlapping way
osm-join-areas-4.patch (24.7 KB ) - added by extropy 14 years ago.
osm-join-areas-5.patch (25.0 KB ) - added by extropy 14 years ago.
Improved patch - preserves path orientation when possible.
join-areas-example 5-6.osm (8.8 KB ) - added by bastiK 14 years ago.
join-areas-example7.osm (48.0 KB ) - added by bastiK 14 years ago.
osm-join-areas-6.patch (29.1 KB ) - added by extropy 14 years ago.
Fixes example 5 and 7
t5179.osm (1.2 KB ) - added by Ldp 14 years ago.
Example to show the popup and "8" shape after join
osm-join-areas-6.1.patch (1.9 KB ) - added by extropy 14 years ago.
Fixed nothing changed popup.
join-areas-example invalid shapes.osm (18.6 KB ) - added by bastiK 14 years ago.
Then I have a view invalid shapes for you to consider. :)
5179-NPE-relation-way.osm (1.4 KB ) - added by Ldp 14 years ago.
osm-join-areas-6.2.patch (74.7 KB ) - added by extropy 14 years ago.
New shiny implementation, handles all cases up to date.
osm-join-areas-6.3.patch (221.6 KB ) - added by extropy 14 years ago.
Improved existing multipolygon handling, added tests to data_nodist
osm-join-areas-6.4.patch (226.4 KB ) - added by extropy 14 years ago.
Fixed duplicate nodes when joining.
osm-join-areas-6.5.patch (233.0 KB ) - added by extropy 14 years ago.
Split touching inner polygons.
Screenshot-join-sel.png (58.3 KB ) - added by bastiK 14 years ago.
after join, one way is still selected
osm-join-areas-6.6.patch (232.8 KB ) - added by extropy 14 years ago.
Outer ways also get split when only touching.
osm-join-areas-6.6.1.patch (4.0 KB ) - added by extropy 13 years ago.
Separate multipolygon relations for each resulting multipolygon.

Download all attachments as: .zip

Change History (101)

by bilbo, 14 years ago

Attachment: failed_merge.osm added

Example that demonstrates the bug

by bilbo, 14 years ago

Attachment: bug-join.osm added

Another example where the algorithm misbehaves, though the results are not as bad as in the first one

comment:1 by bilbo, 14 years ago

In the second example there is one overlapping way, where I can select it and then press Shift-J to create a multipolygon frmo it, however in this case it also does not work well, though the results are not as bad as in the first example.

comment:2 by bastiK, 14 years ago

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

comment:3 by bastiK, 14 years ago

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

by extropy, 14 years ago

Attachment: failed_merge-2.osm added

Anoder example, two simple rectangles.

by extropy, 14 years ago

Attachment: osm-join-areas.patch added

New implementation of findInnerWays, fixes reported problems.

comment:4 by extropy, 14 years ago

Made a path that fixes all the problems.

The patch completely rewrites findInnerWays function.
The new implementation properly traverses around the polygon rather than just tests for inside/outside.

Did not know where to put geometric utility functions, so feel free to move them out to some other file.

comment:5 by stoecker, 14 years ago

Could you please update and check the join areas example file in data_nodist as well, so all cases are covered and you're sure nothing breaks?

in reply to:  4 comment:6 by bastiK, 14 years ago

Replying to extropy:

Made a path that fixes all the problems.
The patch completely rewrites findInnerWays function.
The new implementation properly traverses around the polygon rather than just tests for inside/outside.

Yes, Weiler–Atherton is much better than the current strange method.

Did not know where to put geometric utility functions, so feel free to move them out to some other file.


There are some issues when the areas share nodes. I'll add the simplest case.

by bastiK, 14 years ago

Attachment: join-areas-adjacent.osm added

Shows problem of the first version of the patch

comment:7 by anonymous, 14 years ago

Thanks for the testing :)

Fixed a silly off by one error, now works for the adjacent areas too.

by extropy, 14 years ago

Attachment: osm-join-areas-2.patch added

Improved patch.

by anonymous, 14 years ago

Attachment: join-areas-example 2-3.osm added

comment:8 by bastiK, 14 years ago

Found more pathological examples, that should be addressed by a new and shiny algorithm. ;)

(anonymous was me)

comment:9 by extropy, 14 years ago

Keep them coming.

What is the expected behavior of example 3?

A multipolygon or the inner part removed?

in reply to:  9 comment:10 by bastiK, 14 years ago

Replying to extropy:

Keep them coming.

What is the expected behavior of example 3?

A multipolygon or the inner part removed?

I'd say, just the outer way. But I'm not sure.

comment:11 by extropy, 14 years ago

Okay, here is the next patch.

by extropy, 14 years ago

Attachment: osm-join-areas-3.patch added

Improved patch.

by bastiK, 14 years ago

Attachment: join-areas-example 4.osm added

next example

comment:12 by bastiK, 14 years ago

Why isn't there an error? It fails silently.

comment:13 by extropy, 14 years ago

example 4 works fine for me.

What result does it produce for you?

comment:14 by bastiK, 14 years ago

Sorry, you are right - example 4 works fine.

by bastiK, 14 years ago

Attachment: 2.osm added

example 4:2 - produces self overlapping way

comment:15 by bastiK, 14 years ago

Also, is there a reason to ask if the way should be reversed?

in reply to:  15 comment:16 by extropy, 14 years ago

Replying to bastiK:

Also, is there a reason to ask if the way should be reversed?

I guess no, as the orientation of resulting (outer) way is arbitrary anyway.

Give me an example and i'll try to the prompt.

comment:17 by extropy, 14 years ago

remove the prompt.

comment:18 by bastiK, 14 years ago

2.osm should show the promt

comment:19 by stoecker, 14 years ago

As you do this anyway: The ways should not be joined or reversed in case they have not been joined before (e.g. if ways are streets or the like).

In these cases multiple ways should be added which represent the outlines when joined.

Logically the whole system should work with areas which already use this system. :-)

The current join areas command does handle such cases totally incorrect.

by extropy, 14 years ago

Attachment: osm-join-areas-4.patch added

comment:20 by extropy, 14 years ago

Fixed the ordering of outer segments - removes the prompt and self-overlapping ways.
Had to make some changes in CombineWayAction, it used HashSets that made the order random and bad stuff happened.

There may still be similar problems with inner parts of multipolygons as they use the old code. I don't have any more brain power left today to pick that code apart.

in reply to:  19 comment:21 by extropy, 14 years ago

Replying to stoecker:

As you do this anyway: The ways should not be joined or reversed in case they have not been joined before (e.g. if ways are streets or the like).

Join overlapping areas only works for areas (buildings, parking spaces, landuse,...). Non-area ways like streets are not target audience for this tool.

In these cases multiple ways should be added which represent the outlines when joined.

Logically the whole system should work with areas which already use this system. :-)

The current join areas command does handle such cases totally incorrect.

I do not quite understand what you are saying here. Maybe make some examples?

in reply to:  20 comment:22 by bastiK, 14 years ago

Replying to extropy:

Fixed the ordering of outer segments - removes the prompt and self-overlapping ways.
Had to make some changes in CombineWayAction, it used HashSets that made the order random and bad stuff happened.

There may still be similar problems with inner parts of multipolygons as they use the old code. I don't have any more brain power left today to pick that code apart.

OK, I think it's in pretty good shape, already. :)

comment:23 by bastiK, 14 years ago

As multipolygons are the area data type in osm (and a cosed way is only a simplification for the most common case) this tool should be able to join 2 multipolygons as well as 1 multiploygon and 1 closed way.

In this context, some multipolgons happen to have a highway as part of their outer way ring. This highway should probably be neither reversed, nor combined with other ways. (But can be split.)

comment:24 by bastiK, 14 years ago

Summary: Join overlapping areas produces bad results[PATCH] Join overlapping areas produces bad results

in reply to:  23 ; comment:25 by extropy, 14 years ago

Replying to bastiK:

As multipolygons are the area data type in osm (and a cosed way is only a simplification for the most common case) this tool should be able to join 2 multipolygons as well as 1 multiploygon and 1 closed way.

In this context, some multipolgons happen to have a highway as part of their outer way ring. This highway should probably be neither reversed, nor combined with other ways. (But can be split.)

Okay, this makes sense. But it's quite bigger project to join arbitrary multipolygons. Let me think on it.

by extropy, 14 years ago

Attachment: osm-join-areas-5.patch added

Improved patch - preserves path orientation when possible.

in reply to:  25 ; comment:26 by anonymous, 14 years ago

Replying to extropy:

Replying to bastiK:

As multipolygons are the area data type in osm (and a cosed way is only a simplification for the most common case) this tool should be able to join 2 multipolygons as well as 1 multiploygon and 1 closed way.

In this context, some multipolgons happen to have a highway as part of their outer way ring. This highway should probably be neither reversed, nor combined with other ways. (But can be split.)

Okay, this makes sense. But it's quite bigger project to join arbitrary multipolygons. Let me think on it.

Sure, it is more a general idea for every one, just thought that would be nice. ;)

Might be rather complicated: for outer rings it is a union and for inner inner rings a cut, what if they overlap, ...

comment:27 by bastiK, 14 years ago

anonymous <- me

in reply to:  26 comment:28 by extropy, 14 years ago

Replying to anonymous:

Replying to extropy:

Replying to bastiK:

As multipolygons are the area data type in osm (and a cosed way is only a simplification for the most common case) this tool should be able to join 2 multipolygons as well as 1 multiploygon and 1 closed way.

In this context, some multipolgons happen to have a highway as part of their outer way ring. This highway should probably be neither reversed, nor combined with other ways. (But can be split.)

Okay, this makes sense. But it's quite bigger project to join arbitrary multipolygons. Let me think on it.

Sure, it is more a general idea for every one, just thought that would be nice. ;)

Might be rather complicated: for outer rings it is a union and for inner inner rings a cut, what if they overlap, ...

The logic is quite simple - you split the overlapping polygons into basic regions, each region having zero or more polygons that fill it. If zero - it's a hole, if nonzero then filled. Then glue the filled regions back together.

The trick is getting the code right.

by bastiK, 14 years ago

Attachment: join-areas-example 5-6.osm added

comment:29 by bastiK, 14 years ago

Hi, I added 2 more examples, the latter takes quite some time to process (and the result is not right).

by bastiK, 14 years ago

Attachment: join-areas-example7.osm added

by extropy, 14 years ago

Attachment: osm-join-areas-6.patch added

Fixes example 5 and 7

comment:30 by extropy, 14 years ago

Fixed 5 and 7. To fix 6 more rewrite is needed.

This changeset is getting uncomfortably large for my first patch.

Maybe we can commit the changes as is (provided the code looks OK) and then I'll try to make it work with multipolygons/more than 2 polygons/example 6.

comment:31 by bastiK, 14 years ago

(In [3445]) see #5179 (patch by viesturs) - Join overlapping areas produces bad results

comment:32 by bastiK, 14 years ago

Did a little white space formatting - hope you don't mind.

Do you know the JTS Topology Suite? If you run JTSTestBuilder.java, it shows a nice GUI where you can play with it. Maybe one can copy a view tricks from there. ;)

comment:33 by Ldp, 14 years ago

(Issue 1)
With r3445, every join will produce a popup "No intersection found. Nothing was changed." Hitting OK will allow you to continue, and the areas are actually joined.

(Issue 2)
Merging the top left and bottom right areas (only touching in a single shared node) in the t5179.osm example will also produce that popup, but the result is an '8' shaped way.

by Ldp, 14 years ago

Attachment: t5179.osm added

Example to show the popup and "8" shape after join

by extropy, 14 years ago

Attachment: osm-join-areas-6.1.patch added

Fixed nothing changed popup.

comment:34 by extropy, 14 years ago

Finally got to a PC and some time.

Fixed "nothing changed" popup.

I think the 8 shaped way is expected behavior. Is it not? The old implementation did the same thing.

comment:35 by bastiK, 14 years ago

(In [3449]) see #5179 (patch by viesturs) - Fixed "nothing changed" popup (Join overlapping areas produces bad results)

in reply to:  34 comment:36 by bastiK, 14 years ago

Replying to extropy:

I think the 8 shaped way is expected behavior. Is it not?

Not sure, I asked it on talk:
http://lists.openstreetmap.org/pipermail/talk/2010-August/053216.html

comment:37 by Ldp, 14 years ago

I know it used to do this before as well. With the current work on this, I saw an opportunity to get that 8 shaped way issue fixed as well.

I say fixed, as that's my personal take on this: areas touching each other in just 1 single shared node should not be considered candidates for joining. I consider those to be 2 separate areas.

I could point out what OGC Simple Features thinks of self-intersecting ways, but as that's not entirely relevant to OSM, I won't. :-) (it is relevant to postgis based users of OSM data, in some situations)

in reply to:  37 ; comment:38 by extropy, 14 years ago

Replying to Ldp:

I know it used to do this before as well. With the current work on this, I saw an opportunity to get that 8 shaped way issue fixed as well.

Okay, let's fix it.

I say fixed, as that's my personal take on this: areas touching each other in just 1 single shared node should not be considered candidates for joining. I consider those to be 2 separate areas.

I could point out what OGC Simple Features thinks of self-intersecting ways, but as that's not entirely relevant to OSM, I won't. :-) (it is relevant to postgis based users of OSM data, in some situations)

I don't know OGC but have some common sense.

I propose this behavior:

  • Areas overlap: join them
  • Areas touching. Do not join them.
  • Areas with common segment. Join them.

If areas have multiple places where they touch/overlap, treat each case independently.
This results in:

  • Touching in multiple places - do not join.
  • Overlapping in one place, touching in other - join the overlapping part, leave the touching one as is.
  • Overlapping in two places - join at both places, creating a multipolygon.

Sounds OK?

in reply to:  38 ; comment:39 by bastiK, 14 years ago

Replying to extropy:

Replying to Ldp:

I know it used to do this before as well. With the current work on this, I saw an opportunity to get that 8 shaped way issue fixed as well.

Okay, let's fix it.

I say fixed, as that's my personal take on this: areas touching each other in just 1 single shared node should not be considered candidates for joining. I consider those to be 2 separate areas.

I could point out what OGC Simple Features thinks of self-intersecting ways, but as that's not entirely relevant to OSM, I won't. :-) (it is relevant to postgis based users of OSM data, in some situations)

I don't know OGC but have some common sense.

That's good to hear. ;)

I propose this behavior:

  • Areas overlap: join them
  • Areas touching. Do not join them.
  • Areas with common segment. Join them.

If areas have multiple places where they touch/overlap, treat each case independently.
This results in:

  • Touching in multiple places - do not join.
  • Overlapping in one place, touching in other - join the overlapping part, leave the touching one as is.
  • Overlapping in two places - join at both places, creating a multipolygon.

Sounds OK?

Yes, sounds good.

What about fixing a single closed way with broken geometry? (E.g. self-overlapping segments, duplicate nodes except first and last.) Are you going to address this too or is it a different issue?

comment:40 by bastiK, 14 years ago

Summary: [PATCH] Join overlapping areas produces bad resultsJoin overlapping areas produces bad results

in reply to:  39 comment:41 by extropy, 14 years ago

What about fixing a single closed way with broken geometry? (E.g. self-overlapping segments, duplicate nodes except first and last.) Are you going to address this too or is it a different issue?

Geometry cleanup is a prerequisite to sane joining. The current code already does some very crude cleanup.
I'll try my best!

by bastiK, 14 years ago

Then I have a view invalid shapes for you to consider. :)

comment:42 by anonymous, 14 years ago

Hi, the last example (and example 6) take ages on my computer. I tried 2 shapes of comparable complexity in the jts library, and it took only a split second to compute the union of shapes. So maybe some optimization is still possible...

comment:43 by extropy, 14 years ago

Yeah, tons of optimization to do, but first, let's get the behavior right,

by Ldp, 14 years ago

Attachment: 5179-NPE-relation-way.osm added

comment:44 by Ldp, 14 years ago

When joining two areas, where one has the tags on a relation and the other on a way, and they differ, you get the popup where you can resolve the tagging conflicts. If you cancel that, you get an NPE.

The example file has two ways: 1 untagged which is an outer member in a multipolygon relation tagged a=1, and 1 tagged as a=2. Join them, and press cancel on the "Conflicts when combining ways" dialog. (Pressing OK works ok)

Build-Date: 2010-08-23 10:42:00
Revision: 3456
Is-Local-Build: true

Identification: JOSM/1.5 (3456 SVN en)
Memory Usage: 32 MB / 989 MB (10 MB allocated, but free)
Java version: 1.6.0_20, Sun Microsystems Inc., Java HotSpot(TM) Client VM
Operating system: Windows XP
Dataset consistency test: No problems found

Plugin: AddrInterpolation (21710)
Plugin: ghost (2)
Plugin: multipoly (22171)
Plugin: terracer (22169)
Plugin: validator (22688)
Plugin: wmsplugin (22735)

java.lang.NullPointerException

at org.openstreetmap.josm.actions.JoinAreasAction.fixMultipolygons(JoinAreasAction.java:963)
at org.openstreetmap.josm.actions.JoinAreasAction.joinAreas(JoinAreasAction.java:284)
at org.openstreetmap.josm.actions.JoinAreasAction.actionPerformed(JoinAreasAction.java:216)
at javax.swing.SwingUtilities.notifyAction(Unknown Source)
at javax.swing.JComponent.processKeyBinding(Unknown Source)
at javax.swing.KeyboardManager.fireBinding(Unknown Source)
at javax.swing.KeyboardManager.fireKeyboardAction(Unknown Source)
at javax.swing.JComponent.processKeyBindingsForAllComponents(Unknown Source)
at javax.swing.JComponent.processKeyBindings(Unknown Source)
at javax.swing.JComponent.processKeyEvent(Unknown Source)
at java.awt.Component.processEvent(Unknown Source)
at java.awt.Container.processEvent(Unknown Source)
at java.awt.Component.dispatchEventImpl(Unknown Source)
at java.awt.Container.dispatchEventImpl(Unknown Source)
at java.awt.Component.dispatchEvent(Unknown Source)
at java.awt.KeyboardFocusManager.redispatchEvent(Unknown Source)
at java.awt.DefaultKeyboardFocusManager.dispatchKeyEvent(Unknown Source)
at java.awt.DefaultKeyboardFocusManager.preDispatchKeyEvent(Unknown Source)
at java.awt.DefaultKeyboardFocusManager.typeAheadAssertions(Unknown Source)
at java.awt.DefaultKeyboardFocusManager.dispatchEvent(Unknown Source)
at java.awt.Component.dispatchEventImpl(Unknown Source)
at java.awt.Container.dispatchEventImpl(Unknown Source)
at java.awt.Window.dispatchEventImpl(Unknown Source)
at java.awt.Component.dispatchEvent(Unknown Source)
at java.awt.EventQueue.dispatchEvent(Unknown Source)
at java.awt.EventDispatchThread.pumpOneEventForFilters(Unknown Source)
at java.awt.EventDispatchThread.pumpEventsForFilter(Unknown Source)
at java.awt.EventDispatchThread.pumpEventsForHierarchy(Unknown Source)
at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
at java.awt.EventDispatchThread.run(Unknown Source)

comment:45 by extropy, 14 years ago

Working on second version, stay tuned.

by extropy, 14 years ago

Attachment: osm-join-areas-6.2.patch added

New shiny implementation, handles all cases up to date.

comment:46 by extropy, 14 years ago

Finally completed the improvements.

In this patch:

  • works on more than 2 polygons
  • fixes broken geometry
  • resolves tag conflicts before actual join
  • splits into separate polygons when only one shared node.

One more thing to finalize: when a polygon is self-overlapping, OSM displays that the overlapping parts are holes. The current join implementation also makes holes there. Is it correct?

comment:47 by stoecker, 14 years ago

Could you please update the join areas example file in data_nodist, so when we change code again, we can verify all cases are still working.

comment:48 by anonymous, 14 years ago

On it.

It appears there are several multipolygon relation tests in there that still don't work properly. Some more work to do.

comment:49 by extropy, 14 years ago

Resolution: fixed
Status: newclosed

anonymous = me

comment:50 by extropy, 14 years ago

Resolution: fixed
Status: closedreopened

comment:51 by bastiK, 14 years ago

Is it supposed to join 2 multipolygons or multipolygon & way? This did not quite work for me.

There is a class Multipolygon.java that can identify inner and outer rings.

by extropy, 14 years ago

Attachment: osm-join-areas-6.3.patch added

Improved existing multipolygon handling, added tests to data_nodist

in reply to:  51 comment:52 by extropy, 14 years ago

Replying to bastiK:

Is it supposed to join 2 multipolygons or multipolygon & way? This did not quite work for me.

There is a class Multipolygon.java that can identify inner and outer rings.

With the latest patch joining two multipolygons also work, but you have to select all both inner and outer ways, otherwise they will be treated as simple polygons.

You cannot join a multipolygon and unclosed way right now. And I frankly don't understand the use case.

comment:53 by bastiK, 14 years ago

Why should the inner and outer ways be selected if I could simply select the multipolygon relation itself?

Example: have a complex forest modeled as multipolygon and you like to extend it

  • by a separate patch of forest
  • by a small area touching one or more forest outer ways

In the first case, the user could simply add it as outer way to the multipolygon, in the second case by joining the forest outer ways with the new ring. However, in both cases, "Join Areas" (as I imagine it) should have the same result.

It says "Join (overlapping) Areas": In osm, an area is either a multipolygon or (for simple geometries) a closed way. The tool should be able to handle both the general and the simplified case without problems!

comment:54 by bastiK, 14 years ago

(In [3504]) reworked reverseWay and combineWay such that it can be used by other actions without hack (see #5179 - Join overlapping areas rewrite)

comment:55 by bastiK, 14 years ago

The whole makeCommitsOneAction stuff is an ugly hack and should not be there. I did a little rework for reverseWay & combineWay; does that help?

in reply to:  53 comment:56 by bastiK, 14 years ago

Replying to bastiK:

Why should the inner and outer ways be selected if I could simply select the multipolygon relation itself?

Example: have a complex forest modeled as multipolygon and you like to extend it

  • by a separate patch of forest
  • by a small area touching one or more forest outer ways

In the first case, the user could simply add it as outer way to the multipolygon, in the second case by joining the forest outer ways with the new ring. However, in both cases, "Join Areas" (as I imagine it) should have the same result.

It says "Join (overlapping) Areas": In osm, an area is either a multipolygon or (for simple geometries) a closed way. The tool should be able to handle both the general and the simplified case without problems!

This is just a suggestion, if you like to keep it the way it is now - no problem.

in reply to:  53 comment:57 by anonymous, 14 years ago

Replying to bastiK:

Why should the inner and outer ways be selected if I could simply select the multipolygon relation itself?

Good idea, we can add this at some point.

The current implementation especially ignores multipolygon relations when only one path of the relation is selected, because the behavior is different. (The relations themselves are preserved.)

For example, if you join a polygon with hole with another polygon, the result is union of filled areas (intersection of the holes).

But if you join the hole part of multipolygon with other polygon, the result is union of the holes.

in reply to:  55 comment:58 by extropy, 14 years ago

Replying to bastiK:

The whole makeCommitsOneAction stuff is an ugly hack and should not be there. I did a little rework for reverseWay & combineWay; does that help?

I just used what was already there.
I don't fully understand how the undo/redo stack works.
What should be the correct way to fix this "hack"?

comment:59 by bastiK, 14 years ago

The correct way to do that: create a number of Command objects, put them together in a SequenceCommand and send that package to Main.main.undoRedo.add(...). That's all, no manipulation of the undo/redo buffer should be done while generating the command.

Yes, it was like this before (it comes from a plugin), but I thought, you might be pretty familiar with the code by now. :)

comment:60 by Ldp, 14 years ago

Resolution: fixed
Status: reopenedclosed

I should mention that for the people who rely on Join Areas, to stay on r3503 until it's patched to handle r3504, or you'll screw up your objects.

comment:61 by bastiK, 14 years ago

Resolution: fixed
Status: closedreopened
Summary: Join overlapping areas produces bad results[PATCH] Join overlapping areas produces bad results

comment:62 by bastiK, 14 years ago

(In [3510]) fix rework [3504] (see #5179)

comment:63 by bastiK, 14 years ago

didn't mean to break anything...

comment:64 by bastiK, 14 years ago

Sorry it took so long to get back, I wanted wait for the 3522 release before including the new code.

Now I tested a little and found some minor issues:

  • In the test file line 7 column 1 it produces duplicate nodes.
  • line 8 column 1: The inner way that is created, looks odd. I think each closed way should have the following properties: (1) No duplicate nodes except first and last. (2) No self intersection of any kind.
  • l. 9 /c. 2 (tricky islands): Under certain circumstances, after Joining, the outer or one of the inner ways stays selected. It happens randomly and I couldn't find a way to reproduce reliably.
  • l. 10 /c. 2 (multipolygon): I don't see what is wrong with that multipolygon. (Execpt that both the outer way and the relation are tagged natural=water, where one would suffice.) When you do Join Action, it creates an irregular and complex polygon.

Otherwise I think it is quite good, already.

by extropy, 14 years ago

Attachment: osm-join-areas-6.4.patch added

Fixed duplicate nodes when joining.

comment:65 by extropy, 14 years ago

Hi!

Fixed L7C1.

L9C2 is funny. What is more funny that different ways is selected each time.

L8C1 - working on a fix.

About L10C2. It's a bit tricky to define the correct behavior. See l7c3, that is very similar to L10C2. Also see http://josm.openstreetmap.de/ticket/5179#comment:39

by extropy, 14 years ago

Attachment: osm-join-areas-6.5.patch added

Split touching inner polygons.

by bastiK, 14 years ago

Attachment: Screenshot-join-sel.png added

after join, one way is still selected

comment:66 by bastiK, 14 years ago

the screenshot demonstrates, that a way remains selected directly after join operation

comment:67 by bastiK, 14 years ago

Remaining things:

  • l7c3 / l10c2 still not optimal, i guess it's not so easy ;)
  • l3c2 - should be multipolygon with 2 inner ways
  • when i select the 2 multipolygons on the bottom, it says "please select at least one closed way..."
  • bottom: for specific selection order the the result is wrong: 1. left inner way 2. left outer way 3. right outer way 4. right inner way ---> it produces multiple multipolygons


For 99% of cases it works quite well, i think. Unfortunately there is a variety of odd examples and you can be sure that power user get to the limits. :)

I'd prefer if it was a little more stable, but i would also commit the current state of the patch.

Sebastian

by extropy, 14 years ago

Attachment: osm-join-areas-6.6.patch added

Outer ways also get split when only touching.

in reply to:  67 comment:68 by extropy, 14 years ago

Hi!

Replying to bastiK:

Remaining things:

  • l7c3 / l10c2 still not optimal, i guess it's not so easy ;)

This was on purpose. I was thinking that they should be part of outer polygon. Fixed.

  • l3c2 - should be multipolygon with 2 inner ways

Same as above.

  • when i select the 2 multipolygons on the bottom, it says "please select at least one closed way..."

Could not reproduce.

  • bottom: for specific selection order the the result is wrong: 1. left inner way 2. left outer way 3. right outer way 4. right inner way ---> it produces multiple multipolygons

Fixed.


For 99% of cases it works quite well, i think. Unfortunately there is a variety of odd examples and you can be sure that power user get to the limits. :)

I'd prefer if it was a little more stable, but i would also commit the current state of the patch.

Sebastian

Also I made a patch for the extrude tool. Can you take a look at #5427 ?

comment:69 by bastiK, 13 years ago

(In [3554]) see #5179 (patch by viesturs) - improve join areas

comment:70 by bastiK, 13 years ago

Hi, the tests seem to work now, only l7c1 is strange. Please try to stick to JOSM coding style, even if you don't like it :)

e.g.

for␣(Integer i␣:␣list)␣{
    ...
}

(no newline instead of last space)

Cheers, Sebastian

in reply to:  70 comment:71 by anonymous, 13 years ago

Replying to bastiK:

Hi, the tests seem to work now, only l7c1 is strange.

There is a bug in there (wrong multipolygon). I will fix it.

Please try to stick to JOSM coding style, even if you don't like it :)

e.g.

for␣(Integer i␣:␣list)␣{
    ...
}

(no newline instead of last space)

I'm trying, I'm trying.

Cheers, Sebastian

comment:72 by extropy, 13 years ago

Resolution: fixed
Status: reopenedclosed

anonymous <- me

by extropy, 13 years ago

Attachment: osm-join-areas-6.6.1.patch added

Separate multipolygon relations for each resulting multipolygon.

comment:73 by stoecker, 13 years ago

What is this last attachment? Adding an attachment to a fixed bug is usually useless.

comment:74 by extropy, 13 years ago

Resolution: fixed
Status: closedreopened

I did not mean to close the bug, it was an accident.

The last attachment fixes the bug mentioned in comment 70.

It seems it's not been committed.

Please commit the osm-join-areas-6.6.1.patch

comment:75 by bastiK, 13 years ago

Resolution: fixed
Status: reopenedclosed

In [3595/josm]:

applied #5179 (patch by bilbo) - Join overlapping areas / Separate multipolygon relations for each resulting multipolygon

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.