Modify

Opened 8 days ago

Last modified 3 days ago

#14528 new defect

[Regression] Joining two closed ways results in a mess instead of one merged way

Reported by: eric.ladner@… Owned by: team
Priority: major Milestone: 17.03
Component: Core Version:
Keywords: template_report regression Cc: michael2402

Description

What steps will reproduce the problem?

  1. create two closed overlapping ways.
  2. Merge them with Tools -> Join Overlapping Areas
  3. Observe the chaos left behind.

What is the expected result?

multiple ways should be merged into one

What happens instead?

Any interior areas are not removed as before, but are converted into inner ways and a multipolygon relationship is created where what should be the final outer way is the outer way and the part that should have been discarded is kept as a separate inner way.

Please provide any additional information below. Attach a screenshot if possible.

URL:http://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2017-03-14 19:27:54 +0100 (Tue, 14 Mar 2017)
Build-Date:2017-03-15 02:34:28
Revision:11732
Relative:URL: ^/trunk

Identification: JOSM/1.5 (11732 en) Linux Ubuntu 16.04.2 LTS
Memory Usage: 739 MB / 7282 MB (407 MB allocated, but free)
Java version: 1.8.0_121-b13, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: :0.0 1920x1080
Maximum Screen Size: 1920x1080
Dataset consistency test: No problems found

Plugins:
+ AddrInterpolation (33160)
+ CommandLine (33182)
+ Create_grid_of_ways (32699)
+ FastDraw (33182)
+ FixAddresses (33182)
+ OpeningHoursEditor (33185)
+ PicLayer (33148)
+ SimplifyArea (33004)
+ alignways (33182)
+ apache-commons (32994)
+ buildings_tools (33004)
+ dataimport (33024)
+ download_along (32946)
+ ejml (32680)
+ ext_tools (33004)
+ geotools (33042)
+ gpsblam (33004)
+ importvec (33088)
+ jts (32699)
+ log4j (32699)
+ merge-overlap (33154)
+ opendata (33156)
+ reltoolbox (33088)
+ reverter (33088)
+ splinex (33160)
+ turnrestrictions (33088)
+ utilsplugin2 (33182)
+ waydownloader (33167)

Tagging presets:
+ https://josm.openstreetmap.de/josmfile?page=Presets/OneClick&zip=1

Map paint styles:
- https://josm.openstreetmap.de/josmfile?page=Styles/LessObtrusiveNodes&style&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Fixme&style&zip=1

Last errors/warnings:
- E: org.openstreetmap.josm.plugins.PluginListParseException: Failed to create plugin information from manifest for plugin 'mbtiles.jar'. Cause: org.openstreetmap.josm.plugins.PluginException: An error occurred in plugin mbtiles. Cause: java.io.IOException: invalid manifest format

Attachments (2)

Screenshot from 2017-03-16 17-49-19.png (25.1 KB) - added by eric.ladner@… 8 days ago.
before the Join Overlapping. two separate ways, non intersecting
Screenshot from 2017-03-16 17-50-49.png (40.1 KB) - added by eric.ladner@… 8 days ago.
after joining. the selected way in the screen shot is marked as the inner way, the other is the outer.

Download all attachments as: .zip

Change History (28)

Changed 8 days ago by eric.ladner@…

before the Join Overlapping. two separate ways, non intersecting

Changed 8 days ago by eric.ladner@…

after joining. the selected way in the screen shot is marked as the inner way, the other is the outer.

comment:1 Changed 8 days ago by eric.ladner@…

Looks like [11729] was a significant rework of the way ActionJoinAreas works.

comment:2 Changed 8 days ago by Klumbumbus

Cc: michael2402 added
Keywords: regression added
Milestone: 17.03

comment:3 Changed 8 days ago by Don-vip

Priority: normalmajor

comment:4 Changed 8 days ago by mdk

And it's a regression issue, because this works before (as I tested with r11639).

BTW the actual implementation (r11737) has also an undo problem with this operation:

  1. Draw two overlapping areas as shown in Screenshot from 2017-03-16 17-49-19.png
  2. Select them and join overlapping areas (SHIFT-J)
  3. Undo operation (CTRL-Z)

Undo dosn't work as expected.

comment:5 Changed 8 days ago by mdk

Summary: Joining two closed ways results in a mess instead of one merged way[Regression] Joining two closed ways results in a mess instead of one merged way

comment:6 Changed 8 days ago by michael2402

I'll have to look into it.

Basically what I changed is the algorithm that computed at which side of the line the interior is and used xor for that, the old code used a simple merge. Removing the inner ways should be enough to do the job.

The multipolygon code was always used for merging (CreateMultipolygonAction), this is why multipolygons are created.

comment:7 Changed 8 days ago by eric.ladner@…

Yep. Not faulting the use of multi polygons.. sometimes you have to have the inner if it's required (e.g. joining separate pieces around an inner courtyard).

Thanks for looking at this so quickly.

comment:8 Changed 7 days ago by bastiK

The informal reference file data_nodist/Join_Areas_Tests.osm has been converted to a unit test in [11738]. I've left out the self-intersecting examples as it is debatable what the algorithm should do in these cases. Non-zero winding rule seems more natural to me here (rather than even-odd as is now).

comment:9 Changed 6 days ago by Klumbumbus

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

comment:10 Changed 5 days ago by alexkemp

Problem continues in 11747.

comment:11 Changed 4 days ago by michael2402

I'm still having some problems with this.

There is some code in there that handles multipolygons, but it seems to already have problems in "old" versions - there are tons of merge dialogs. If you click on "keep" in all of them, it sometimes produces multipolygons that have a line as both outer and inner line.

My question now is:
Should we handle multipolygons there? My suggestion would be:

  • If all members of a multipolygon are selected, ask the user if he wants to work on the multipolygon instead.
    • Yes => select all those multipolygons, remove ways from selection.
  • Search all selected areas (closed ways and multipolygons). Warn user if there is any other way/relation selected.
  • Get tags from those areas.
    • Abort if they are not the same (or show a conflict dialog, or warn...)
  • Compute the outline of the union of the areas.
    • Warn if we have several unconnected areas.
  • Now apply:
    • Create the outline ways.
    • If we are left with more than one way, create a multipolygon.
    • Set the tags.

For the objects that need to be created we could re-use the ones that are most similar.

I would like to allow multipolygons to be merged. This allows the user to e.g. connect a meadow-multipolygon with a newly drawn meadow area and might be a easier to use alternative to the multipolygon tools.

comment:12 in reply to:  11 ; Changed 4 days ago by Klumbumbus

Replying to michael2402:

I would like to allow multipolygons to be merged.

This is dangerous. I don't want that users create huge multipolygons.

Many osm users have spend lots of time to repair and shrink broken monster multipolygons.

comment:13 Changed 4 days ago by Don-vip

Big problem with r11748, add a node in a new layer and boom:

java.lang.ExceptionInInitializerError
	at org.openstreetmap.josm.gui.mappaint.styleelement.NodeElement.<clinit>(NodeElement.java:62)
	at org.openstreetmap.josm.gui.mappaint.ElemStyles.generateStyles(ElemStyles.java:383)
	at org.openstreetmap.josm.gui.mappaint.ElemStyles.getImpl(ElemStyles.java:208)
	at org.openstreetmap.josm.gui.mappaint.ElemStyles.getStyleCacheWithRange(ElemStyles.java:124)
	at org.openstreetmap.josm.gui.mappaint.ElemStyles.get(ElemStyles.java:103)
	at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer$ComputeStyleListWorker.add(StyledMapRenderer.java:1567)
	at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer$ComputeStyleListWorker.visit(StyledMapRenderer.java:1548)
	at org.openstreetmap.josm.data.osm.Node.accept(Node.java:231)
	at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer$ComputeStyleListWorker.acceptDrawable(StyledMapRenderer.java:1539)
	at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer$ComputeStyleListWorker.computeDirectly(StyledMapRenderer.java:1526)
	at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer$ComputeStyleListWorker.compute(StyledMapRenderer.java:1507)
	at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer$ComputeStyleListWorker.compute(StyledMapRenderer.java:1)
	at java.util.concurrent.RecursiveTask.exec(RecursiveTask.java:94)
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)
Caused by: java.lang.NullPointerException: labelPositionStrategy
	at java.util.Objects.requireNonNull(Objects.java:228)
	at org.openstreetmap.josm.gui.mappaint.styleelement.TextLabel.<init>(TextLabel.java:104)
	at org.openstreetmap.josm.gui.mappaint.styleelement.TextLabel.create(TextLabel.java:226)
	at org.openstreetmap.josm.gui.mappaint.styleelement.BoxTextElement.create(BoxTextElement.java:206)
	at org.openstreetmap.josm.gui.mappaint.styleelement.BoxTextElement.create(BoxTextElement.java:183)
	at org.openstreetmap.josm.gui.mappaint.styleelement.BoxTextElement.<clinit>(BoxTextElement.java:124)
	... 17 more

then it leads to:

java.lang.NoClassDefFoundError: Could not initialize class org.openstreetmap.josm.gui.mappaint.styleelement.NodeElement
	at org.openstreetmap.josm.gui.mappaint.ElemStyles.generateStyles(ElemStyles.java:383)
	at org.openstreetmap.josm.tools.ImageProvider.getPadded(ImageProvider.java:1398)
	at org.openstreetmap.josm.gui.OsmPrimitivRenderer.renderer(OsmPrimitivRenderer.java:78)
	at org.openstreetmap.josm.gui.OsmPrimitivRenderer.getListCellRendererComponent(OsmPrimitivRenderer.java:48)
	at org.openstreetmap.josm.gui.OsmPrimitivRenderer.getListCellRendererComponent(OsmPrimitivRenderer.java:1)
	at javax.swing.plaf.basic.BasicListUI.updateLayoutState(BasicListUI.java:1361)

Even in wireframe mode JOSM is unusable!

Last edited 4 days ago by Don-vip (previous) (diff)

comment:14 Changed 4 days ago by Don-vip

In 11749/josm:

see #14528 - checkstyle

comment:15 Changed 4 days ago by Don-vip

I don't understand this line:

        Collections.sort(offsetGlyphs, Comparator.comparing(e -> e.offset));

javac is able to compile it but Eclipse gives me a compilation error:

/JOSM/src/org/openstreetmap/josm/gui/mappaint/styleelement/placement/OnLineStrategy.java - line 102
Type mismatch: cannot convert from double to Comparable<? super Comparable<? super U>>

comment:16 in reply to:  12 ; Changed 4 days ago by michael2402

Replying to Klumbumbus:

Replying to michael2402:

I would like to allow multipolygons to be merged.

This is dangerous. I don't want that users create huge multipolygons.

Many osm users have spend lots of time to repair and shrink broken monster multipolygons.

Shouldn't we then completely remove all multipolygon code from JoinAreasAction and only allow it to work on normal circular ways?

Replying to Don-vip:

I don't understand this line:

        Collections.sort(offsetGlyphs, Comparator.comparing(e -> e.offset));

Sort glyphs by offset. The old code iterated though the path n times to compute the position. This allows us to iterate only once - which makes drawing texts on paths with many nodes faster.

I am able to compile that file using eclipse Mars.2 Release (4.5.2). But eclipse gives me a compilation error on Preferences.java for some time now (something with type inference as well).

I could not reproduce that error. And I don't really see how forKeyword could return null.

comment:17 in reply to:  16 ; Changed 4 days ago by Don-vip

Replying to michael2402:

I could not reproduce that error. And I don't really see how forKeyword could return null.

I reproduce it systematically. positionKeyword is null. c.toString() gives me Cascade{ text:Keyword{auto}; }.

comment:18 in reply to:  17 ; Changed 4 days ago by Don-vip

Replying to Don-vip:

I reproduce it systematically. positionKeyword is null. c.toString() gives me Cascade{ text:Keyword{auto}; }.

So c.get(AreaElement.TEXT_POSITION, null, Keyword.class) returns null as it is the default value. This must be changed.

comment:19 Changed 4 days ago by Don-vip

In 11753/josm:

see #14528 - make code compile with Eclipse Neon.2 (4.6.2) Build id: M20161124-1400

comment:20 in reply to:  19 Changed 4 days ago by Don-vip

Replying to Don-vip:

In 11753/josm:

This makes the error go away. I don't understand why, but it's too late for today. Still it could be a risk to specify null as default value used with a constructor that does not accept null as a valid value.

comment:21 Changed 4 days ago by Don-vip

The code is difficult to understand, we have:

  • two interfaces named PositionForAreaStrategy
  • two classes named CompletelyInsideAreaStrategy
  • two classes named OnLineStrategy

comment:22 Changed 4 days ago by Don-vip

In 11755/josm:

see #14528 - typos

comment:23 in reply to:  18 Changed 4 days ago by michael2402

I meant to clean up the code from #10176 ;-). There was a lot of long if/else code in StyledMapRenderer

Replying to Don-vip:

Replying to Don-vip:

I reproduce it systematically. positionKeyword is null. c.toString() gives me Cascade{ text:Keyword{auto}; }.

So c.get(AreaElement.TEXT_POSITION, null, Keyword.class) returns null as it is the default value. This must be changed.

If positionKeyword is null, the default value (OnLineStrategy with an offset of 0) is used.

Replying to Don-vip:

The code is difficult to understand, we have:

  • two interfaces named PositionForAreaStrategy
  • two classes named CompletelyInsideAreaStrategy
  • two classes named OnLineStrategy

Thanks for mentioning that. I removed one of them, since it was not used.

comment:24 in reply to:  12 Changed 4 days ago by bastiK

Replying to Klumbumbus:

Replying to michael2402:

I would like to allow multipolygons to be merged.

This is dangerous. I don't want that users create huge multipolygons.

Many osm users have spend lots of time to repair and shrink broken monster multipolygons.

JOSM is a tool, we don't need to babysit users to that extent. Multipolygons are the designated area data type in OSM, along with closed ways, so it is only natural that the "Join areas"-tool would also join multipolygons. Obviously proper joining of ways is much higher priority.

comment:25 Changed 3 days ago by Don-vip

We can add a warning if two very large polygons are merged. Something like when a high number of nodes is moved.

comment:26 Changed 3 days ago by michael2402

Do we really need this? For merging, you should need to select the relation. We can allow all members to be selected and then prompt the user if the selection should be changed.

This requires a user to either know about relations or to select all multipolygon parts - in which case he/she knows about the problem.

If we only allow areas with the same tags to be merged (or display a merge dialog), the user should see when two areas of different kind are merged. We can also warn if the user attempts to merge polygons that do not overlap. This should prevent most accidents. So I don't see a point in warning the user about merging large areas.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from team to the specified user. Next status will be 'new'.
Next status will be 'needinfo'.The owner will change to eric.ladner@gmail.com
as duplicate The resolution will be set to duplicate. Next status will be 'closed'.The specified ticket will be cross-referenced with this ticket
The owner will be changed from team to anonymous. Next status will be 'assigned'.

Add Comment


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

 
Note: See TracTickets for help on using tickets.