Modify

Opened 7 years ago

Closed 4 years ago

#14528 closed defect (irreproducible)

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

Reported by: eladner Owned by: team
Priority: major Milestone:
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 (10)

Screenshot from 2017-03-16 17-49-19.png (25.1 KB ) - added by eladner 7 years ago.
before the Join Overlapping. two separate ways, non intersecting
Screenshot from 2017-03-16 17-50-49.png (40.1 KB ) - added by eladner 7 years ago.
after joining. the selected way in the screen shot is marked as the inner way, the other is the outer.
join-areas-rewrite.patch (53.5 KB ) - added by michael2402 7 years ago.
before-shift-J.png (423.1 KB ) - added by alexkemp 7 years ago.
screen-shot of JOSM-11787 before pressing <Shift>-J
shift-J-1(resolve-conflicts).png (401.5 KB ) - added by alexkemp 7 years ago.
screen-shot of JOSM-11787 resolution-conflict dialog
shift-J-2(resolve-conflicts).png (401.0 KB ) - added by alexkemp 7 years ago.
screen-shot of JOSM-11787 resolution-conflict dialog
after-shift-J.png (425.7 KB ) - added by alexkemp 7 years ago.
screen-shot of JOSM-11787 end result of pressing <Shift-J>
14528-rework-join-areas.patch (100.1 KB ) - added by michael2402 7 years ago.
Join.png (29.8 KB ) - added by eladner 7 years ago.
Join operation failure for 360 wraparound of an inner object.
josm-joinAreasAction-minimal-set-of-boundaries.patch (7.7 KB ) - added by cmuelle8 6 years ago.
intermediary patch for the problem observed by klumbumbus, though the problem solving follows a very general approach, so might fix some other cases as well; may serve as a better bridge until the new style joinAreas code from Michael is ready and might also give some more time to thoroughly test the new code; fixes ticket:10511 test-cases

Download all attachments as: .zip

Change History (101)

by eladner, 7 years ago

before the Join Overlapping. two separate ways, non intersecting

by eladner, 7 years ago

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

comment:1 by eladner, 7 years ago

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

comment:2 by Klumbumbus, 7 years ago

Cc: michael2402 added
Keywords: regression added
Milestone: 17.03

comment:3 by Don-vip, 7 years ago

Priority: normalmajor

comment:4 by mdk, 7 years ago

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 by mdk, 7 years ago

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 by michael2402, 7 years ago

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 by eladner, 7 years ago

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 by bastiK, 7 years ago

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 by Klumbumbus, 7 years ago

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

comment:10 by alexkemp, 7 years ago

Problem continues in 11747.

comment:11 by michael2402, 7 years ago

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.

in reply to:  11 ; comment:12 by Klumbumbus, 7 years ago

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 by Don-vip, 7 years ago

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 7 years ago by Don-vip (previous) (diff)

comment:14 by Don-vip, 7 years ago

In 11749/josm:

see #14528 - checkstyle

comment:15 by Don-vip, 7 years ago

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

in reply to:  12 ; comment:16 by michael2402, 7 years ago

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.

in reply to:  16 ; comment:17 by Don-vip, 7 years ago

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}; }.

in reply to:  17 ; comment:18 by Don-vip, 7 years ago

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 by Don-vip, 7 years ago

In 11753/josm:

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

in reply to:  19 comment:20 by Don-vip, 7 years ago

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 by Don-vip, 7 years ago

The code is difficult to understand, we have:

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

comment:22 by Don-vip, 7 years ago

In 11755/josm:

see #14528 - typos

in reply to:  18 comment:23 by michael2402, 7 years ago

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.

in reply to:  12 comment:24 by bastiK, 7 years ago

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 by Don-vip, 7 years ago

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

comment:26 by michael2402, 7 years ago

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.

by michael2402, 7 years ago

Attachment: join-areas-rewrite.patch added

comment:27 by michael2402, 7 years ago

Just wanted to give you some progress on this one:

The current implementation is a bit hacky, with hacks to work around those hacks. This made if relatively long and hard to maintain. It also uses other actions to do the things it does (e.g. split/join ways of multipolygons). This is very confusing to the user, since there are many prompts for member conflicts but the user does not really know where they are and what he/she is resolving.

So I decided to do a full rewrite of this. The merge algorithm now uses

The old algorithm is still used in fixTouchingPolygons / findBoundaryPolygons / joinAreas. They are public, but I did not find any uses so far except for the internal in RightAndLefthandTraffic

If this is fixed, we can remove the old algorithm.

For Multipolygons, I decided to explicitly ask the user if they should be joined. When joining them, they are handled just like a normal area and the new outline is computed. There is no tag magic going on - we require that the tags of the multipolygon and the area it should be merged with are equal.
The history of the old multipolygon is kept. When merging two of them, the bigger (more members) one is used.

There are still some things to do:

  • Tags need to match exactly for now. Adding the merge dialog is no big deal.
  • Allow to abort if user is asked if he/she wants to work with relations
  • Testing. I am currently adding some more JoinAreaTest cases.
  • There is still a bug somewhere when merging 3 or more ways - not all intersections are used then.
  • Merge 2 or more intersections at the same spot (e.g. a shared line segment is intersected by an other one)
  • We can preserve way histories by re-using the old ways, like we do with the relation.
  • If we have two touching multipolygons, I don't split the outer way. We can do this, but I'd rather only do it if the way is only referenced by multipolygons and has no tags associated with it. We don't want to make ugly multipolygons more ugly.

But some old bugs were fixed as well:

  • Tags on inner ways are preserved
  • Only one command is committed - so there are no half-destroyed areas if the algorithm fails half way and no more rollback hacks are needed.

comment:28 by bastiK, 7 years ago

What are your plans with respect to the upcoming release?

comment:29 by alexkemp, 7 years ago

Problem is worse in 11784. Pressing Shift-J to join 2 x Houses to make an L-shaped house produces an Unexpected Exception. The continued coding mess created by trying to 'fix' a non-problem (this routine works without issue in JOSM-Stable) is embarrassing. Please revert to JOSM-11639 coding & finish this in your own time.

comment:30 by michael2402, 7 years ago

Yes, I'd like to revert it for the next stable.

comment:31 by bastiK, 7 years ago

alexkemp: No need to get rude, as you can see in #10511, there are issues with the code in the stable version.

comment:32 by michael2402, 7 years ago

Both versions have their issues.

The most concering issue to me is how the current code (all versions) handles Multipolygons. They easily get completely destroyed.

The new algorithm will have all neccessary features, but I don't like to apply it untested.

I just have to get a clean trunk to get the revert committed, should be done tomorrow ;-).

comment:33 by alexkemp, 7 years ago

@bastiK: Pointing out that something does not work is NOT rude. Please try to grow up.

@michael2402: I'm most happy to test the latest version. My mapping work is almost entirely adding houses, and I use Shift-J on most every occasion. That use is very simple, so is unlikely to test edge- or complicated-situations.

in reply to:  33 comment:34 by stoecker, 7 years ago

Replying to alexkemp:

@bastiK: Pointing out that something does not work is NOT rude. Please try to grow up.

Alex. This is not the first time you are annoying. This is a bug tracker and not your favourite forum. Stop telling us what we should do and control your language. You are welcome to give feedback and suggestions, but JOSM development does NOT follow your personal preferences or problems. An individual user is not our focus and will never be.

in reply to:  33 ; comment:35 by michael2402, 7 years ago

Replying to alexkemp:

@michael2402: My mapping work is almost entirely adding houses, and I use Shift-J on most every occasion.

This is good as well. We will see if there are any corner cases (rounding issues in geometry, ...).

If you see a new exception dialog, feel free to report it (best with a note on where it happened) - this helps us tracking down bugs. JOSM has a pretty complex codebase, with all those plugins, and some ~10 year old algorithms that were patched to account for X, so there may be several cases where we missed something or where that patch broke something else.

in reply to:  32 ; comment:36 by stoecker, 7 years ago

Replying to michael2402:

The new algorithm will have all neccessary features, but I don't like to apply it untested.

I just have to get a clean trunk to get the revert committed, should be done tomorrow ;-).

What time is required to test the algorithm? Our release cycle is not set in stone. I prefer improvements over reverts in development of JOSM.

P.S. I'd also prefer to get rid of the remaining gsoc-core issues in core (and also plugins), which I hope you will fix soon.

in reply to:  35 comment:37 by alexkemp, 7 years ago

Replying to michael2402:

If you see a new exception dialog, feel free to report it (best with a note on where it happened)

I did not want to pollute these comments with what would have been a vast report, filled with stack-trace, etc., etc..

The last exception occurred immediately after pressing Shift-J. It is very difficult to say any more on that without a large technical report.

comment:38 by Don-vip, 7 years ago

Take the time you need Michael, no pressure :) I have temporarily pushed the release date a few days, but it can be postponed again if needed. There is no major issue in the current tested version, so we could even skip the 17.03 release completely if we need a few more weeks.

in reply to:  38 comment:39 by Klumbumbus, 7 years ago

Replying to Don-vip:

There is no major issue in the current tested version, so we could even skip the 17.03 release completely if we need a few more weeks.

Well, there is #14470, which would be nice to have in the stable version soon. I have read several comments in the forum like "... and bing has gone again".

in reply to:  36 comment:40 by michael2402, 7 years ago

Replying to stoecker:

Replying to michael2402:

The new algorithm will have all neccessary features, but I don't like to apply it untested.

I just have to get a clean trunk to get the revert committed, should be done tomorrow ;-).

What time is required to test the algorithm? Our release cycle is not set in stone. I prefer improvements over reverts in development of JOSM.

It should be ready for testing next week. Since it is a complete re-implementation, I don't know how many issues we will find. In my tests, the geometry part behaves relatively robust. I'm a bit concerned that multipolygon handling/creation is to conservative at the moment (creating new ways instead of splitting old ones as long as it is not absolutely sure that the old one is only for the current geometry). We have to test this on real-world examples and see if we need to issue a warning in those cases. I'm not a fan of warnings, since they confuse new users and are only ignored by the old ones.

P.S. I'd also prefer to get rid of the remaining gsoc-core issues in core (and also plugins), which I hope you will fix soon.

Yes, I already worked on some of that. I spend some time writing a patch for the GPX-Part to make it based on listeners and to get rid of the last layer in core using the isChanged flag. I am cleaning up drawing code as well, using the new MapViewPath which simplifies things like line iterations or Way->Line conversions. And then I attempted to fix this simple issue here ;-).

I have not looked into plugins much (other than verifying if the SVN ones use the given API).

comment:41 by bastiK, 7 years ago

To get a predictable testing period for my own pet project, I would prefer either release roughly as planned or skip 17.03 entirely.

by alexkemp, 7 years ago

Attachment: before-shift-J.png added

screen-shot of JOSM-11787 before pressing <Shift>-J

by alexkemp, 7 years ago

screen-shot of JOSM-11787 resolution-conflict dialog

comment:42 by alexkemp, 7 years ago

JOSM-11787: No exception now after pressing <shift>-J (thank you), and the results are no longer a mess, but those results are still wrong. 4 x screen-shots are attached:-

  1. before: a dormer-garage extension due to be added to a semi-detached house (2 x overlapping houses)
  2. <Shift>-J 1: conflict resolution dialog before
  3. <Shift>-J 2: conflict resolution dialog immediately before press <OK>
  4. after: result (whoops)

by alexkemp, 7 years ago

screen-shot of JOSM-11787 resolution-conflict dialog

by alexkemp, 7 years ago

Attachment: after-shift-J.png added

screen-shot of JOSM-11787 end result of pressing <Shift-J>

comment:43 by bastiK, 7 years ago

While moving forward is preferable, we shouldn't treat a rollback as taboo, or else we end up in endless non-releasable state as has happened before and caused a lot of tension. The occasional revert is the price to pay for linear development model rather than main branch and development branches. [11729] would not have been merged into a release branch as after short testing it becomes apparent that significantly more work is needed (nothing wrong with that).

In this particular case, I don't really care, but we should make up our minds if #14470 is serious enough to demand a swift release (in which case a rollback is inevitable), or we postpone the next release to end of April, which seems to match the time that is required to finish the patch and have a comfortable testing window.

comment:44 by michael2402, 7 years ago

A revert of the algorithm should be pretty simple - you only need to revert one file. It would give us more time for test feedback.

comment:45 by Don-vip, 7 years ago

I'm ok to revert and release quickly, if the 8 failing unit tests are fixed.

comment:46 by bastiK, 7 years ago

I'm on it.

comment:47 by Don-vip, 7 years ago

Great thank you :)

comment:48 by alexkemp, 7 years ago

Specifics reported with JOSM-11787 in comment:42 continues exactly with JOSM-11796.

in reply to:  43 comment:49 by stoecker, 7 years ago

Replying to bastiK:

While moving forward is preferable, we shouldn't treat a rollback as taboo

That's clear. But here in this situation it looks to me like revert or not revert both are suboptimal.

It seems decided then to revert, release and continue after the release?

I'd also like to have my multipolygon validator warning->error stuff online before there are no more left in the database to error about :-) Currently it is down from 240000 to 160000.

comment:50 by michael2402, 7 years ago

The old one was more usable in simple scenarios (needs of the many and so on...) ;-) The new e.g. has problems when an outer line segment is used by two areas. So I'd say revert.

I'll have a look at it and the remaining issues for the release this evening.

in reply to:  46 comment:51 by michael2402, 7 years ago

Replying to bastiK:

I'm on it.

Do you want to revert or should I revert?

comment:52 by bastiK, 7 years ago

Please go ahead.

comment:53 by michael2402, 7 years ago

In 11822/josm:

See #14528: Revert join areas algorithm to old version for 17.03 release.

comment:54 by michael2402, 7 years ago

Milestone: 17.0317.04

comment:55 by alexkemp, 7 years ago

Can confirm that JOSM-11826 works fine (limited testing on L-shaped houses & Shift-J). As #10511 is 3 years old & was fixed a month ago, that hopefully means, other reports to one side, that this issue is completed. Hooray! and well done. Do call on me if you need someone to be annoying, again.

comment:56 by eladner, 7 years ago

revert tested and works as before. in most test cases, it works fine, but for super extreme nonsense tests, it will get confused about inner and outer areas and create a multipolygon with inner areas that were in fact covered by other polygons.

Probably works for 99% of test cases, though.

comment:57 by michael2402, 7 years ago

I completely reworked the way join areas works.

Instead of the old special multipolygon handling I use a common "area". This makes the code a lot easier.

Still some minor issues:

  • Unused nodes are not removed.
  • When joining a building=yes with a landuse=farmland, the user is not even warned. This is due to the way tag conflicts are handled (copied from old code, we might want to remove the autofix here. But I have not looked so deep into the tag resolution code)
Last edited 7 years ago by michael2402 (previous) (diff)

by michael2402, 7 years ago

in reply to:  57 ; comment:58 by stoecker, 7 years ago

Replying to michael2402:

I completely reworked the way join areas works.

Instead of the old special multipolygon handling I use a common "area". This makes the code a lot easier.

Still some minor issues:

  • Unused nodes are not removed.
  • When joining a building=yes with a landuse=farmland, the user is not even warned. This is due to the way tag conflicts are handled (copied from old code, we might want to remove the autofix here. But I have not looked so deep into the tag resolution code)

For area joining the tag resolution dialog should be called in any case where new tags are coming from more than one involved object (stripping the uninteresting tags like source beforehand). That would also handle the case you mention.

BTW: What happens e.g. with "barrier" - will it stay on the ring instead of moving to a multipolygon?

in reply to:  58 ; comment:59 by michael2402, 7 years ago

Replying to stoecker:

Replying to michael2402:

I completely reworked the way join areas works.

Instead of the old special multipolygon handling I use a common "area". This makes the code a lot easier.

Still some minor issues:

  • Unused nodes are not removed.
  • When joining a building=yes with a landuse=farmland, the user is not even warned. This is due to the way tag conflicts are handled (copied from old code, we might want to remove the autofix here. But I have not looked so deep into the tag resolution code)

For area joining the tag resolution dialog should be called in any case where new tags are coming from more than one involved object (stripping the uninteresting tags like source beforehand). That would also handle the case you mention.

I'll have to look into it on how to implement this the best way - I don't want to re-invent the wheel there

BTW: What happens e.g. with "barrier" - will it stay on the ring instead of moving to a multipolygon?

No, it will be moved. Handling this is even more problematic if we e.g. have a barrier ring that is merged with an other way. What should happen then?
We can't handle all tags. Especially since joining existing areas is an expert action that normal mappers rarely use. I think that we should use a common policy (e.g. move all tags to the multipolygon) and hope that the user knows what he is merging.

comment:60 by stoecker, 7 years ago

For "Update multipolygon" we care about "barrier". There should be no different handling here. Merging ways with conflicting tags should also be prevented. In case there is trouble the user should be told about it and he can fix the issues (not necessarily in a dialog, but e.g. by hand). Afterwards he can call the function again. Silently ignoring such things will produce a lot of trouble.

in reply to:  59 comment:61 by stoecker, 7 years ago

Replying to michael2402:

For area joining the tag resolution dialog should be called in any case where new tags are coming from more than one involved object (stripping the uninteresting tags like source beforehand). That would also handle the case you mention.

I'll have to look into it on how to implement this the best way - I don't want to re-invent the wheel there

Update Multipolygon action already uses logic for that, but see #14651 as well.

in reply to:  60 comment:62 by michael2402, 7 years ago

Replying to stoecker:

For "Update multipolygon" we care about "barrier". There should be no different handling here.

Thanks for the hint. I have to look into this.

Merging ways with conflicting tags should also be prevented. In case there is trouble the user should be told about it and he can fix the issues (not necessarily in a dialog, but e.g. by hand). Afterwards he can call the function again. Silently ignoring such things will produce a lot of trouble.

Currently, I use the merge tags dialog separately: First create the new area, then call the merge dialog with the old primitives as source and the new one as target. But there is still a bug in my use of the merge dialog that prevents the tags from being added to the multipolygon. I need to have a closer look at the multipolygon functions for this. I don't want to add the tags to the outer ways first, because it would require me to change the outer ways. They can still be part of other multipolygons, so this may create conflicts.

comment:63 by stoecker, 7 years ago

P.S. Keep in mind, that old-style multipolygons will vanish very soon (probably gone with next release). An area style on the outer way applies then always to the way and no longer to the polygon. That should make the code easier.

comment:64 by michael2402, 7 years ago

Yes, I don't even try to handle the old-style multipolygons.

It's the new ones that might cause problems: You can e.g. split a way and use that part of the way as a multipolygon outline. That way, you might even have the same tag on the outline and on the relation (think of road / parking, both have smoothness). This is valid and needs to be handled.

comment:65 by eladner, 7 years ago

If I recall, common area tags were pushed up to the multipolygon and others were left in place. Trying to handle every weird combination of tags people try to merge would be a huge undertaking. I'd think anybody advanced enough to use multipolygons would also be advanced enough to check things out after merging.

comment:66 by Don-vip, 7 years ago

Milestone: 17.0417.05

comment:67 by Don-vip, 7 years ago

Milestone: 17.0517.06

comment:68 by Don-vip, 7 years ago

Milestone: 17.0617.07

comment:69 by Don-vip, 7 years ago

Milestone: 17.0717.08

comment:70 by Don-vip, 7 years ago

Milestone: 17.0817.09

comment:71 by anonymous, 7 years ago

Note.. recent testing on version 12695, most join operations succeed on a geometry level except for large numbers of overlapping polygons. In some instances, polygons that completely overlap will create multipolygons with erroneous "inner" ways that should have been covered by other selected polygons.

steps to recreate... Create a simple square. Around the edges with complete 360 coverage, overlap 6-10 duplicates of the square, ensure that no internal space is not covered by the square. Shift-J will leave a hole in the middle. Undo and delete one of the surrounding squares so that the outer squares no longer wrap all the way around the inner square. The join will succeed at this point. (visual attached - Join.png)

by eladner, 7 years ago

Attachment: Join.png added

Join operation failure for 360 wraparound of an inner object.

in reply to:  71 comment:72 by eladner, 7 years ago

Regarding comment 71 - anonymous:
Sorry.. that was my comment above. Forgot to log in.

comment:73 by Klumbumbus, 7 years ago

Join operation failure for 360 wraparound of an inner object.

The "oprhaned points" are there because they are tagged with something. So they don't get lost at the join operation. If you remove the tags from the nodes, they should be removed during the join operation.

comment:74 by eladner, 7 years ago

Yep. Ignore the orphaned points comment. I mistakenly tagged them with "building=yes" so they CORRECTLY didn't get purged during the join.

comment:75 by Don-vip, 6 years ago

Milestone: 17.0917.10

comment:76 by Don-vip, 6 years ago

Milestone: 17.1017.11

comment:77 by Don-vip, 6 years ago

Milestone: 17.1117.12

by cmuelle8, 6 years ago

intermediary patch for the problem observed by klumbumbus, though the problem solving follows a very general approach, so might fix some other cases as well; may serve as a better bridge until the new style joinAreas code from Michael is ready and might also give some more time to thoroughly test the new code; fixes ticket:10511 test-cases

in reply to:  77 ; comment:78 by Klumbumbus, 6 years ago

Replying to cmuelle8:

the problem observed by klumbumbus

To what exactly are you referring?

in reply to:  78 comment:79 by cmuelle8, 6 years ago

Replying to Klumbumbus:

Replying to cmuelle8:

the problem observed by klumbumbus

To what exactly are you referring?

to comment:73 - sorry, seems to have been reported by eladner first,
but you were the one adding the picture to your comment. but then,
to be nitpicking, i did not say you observed it first ;-p

comment:80 by cmuelle8, 6 years ago

@Klumbumbus: As told, please do not take the patch as an offering to an overall solution to the problems with the joinAreas action. It solves some problems while leaving others untouched.

E.g. merging a single outer way with an existing MP (inner + outer) is solved in a suboptimal way. This is due to the overall -old- design, namely taking all ways from existing relations and adding them back in after the operation.

If you create a MP with one outer (1) and one inner way (2) and draw a closed way (3) intersecting both, then select all three ways (without the relation in the selection), the operation will create a new MP with a correct result. But the user is also asked to 'fix' the old MP by querying him to keep/remove (meaningless, because altered!) ways, which most of the time results in a broken relation:

  • ways queried about in the keep/remove dialogs do not exist in their previous form
  • user cannot decide meaningfully what to keep or remove (i.e. 'blind flight')
  • the result of readding ways to the relations they have been taken out of is, from a user perspective, unpredictable
  • this appears to be a problem with as little as one multipolygon in the input (it may be worse with larger input)
  • the desired result (in the simple case depicted below) would be to rather update the existing multipolygon instead of creating a new one
+----------1+
|           |
|   +---2+  |
|   |  +-+--+---3+
|   |  | |  |    |
|   +--+-+  |    |
|      |    |    |
+------+----+    |
       |         |
       +---------+

I think the old code could do better on this, if it would not try to remove the ways from the existing relations (unless maybe all members take part in the join op). But i've not played around enough with this part yet to make a proof to my point on this. So for now this is a problem that just remains in effect which I refered to with the word "untouched" above. But the "360 overlap around center" and the test-cases in ticket:10511 should be solved.

I've not examined the new code in the patch that is in reverted state currently. So this might have been solved already there, or not.

Last edited 6 years ago by cmuelle8 (previous) (diff)

comment:81 by Don-vip, 6 years ago

Milestone: 17.1218.01

comment:82 by Don-vip, 6 years ago

Milestone: 18.0118.02

comment:83 by Don-vip, 6 years ago

Milestone: 18.0218.03

comment:84 by Don-vip, 6 years ago

Milestone: 18.0318.04

comment:85 by Don-vip, 6 years ago

Michael, do you plan to look at this? (with #10511 also)

comment:86 by michael2402, 6 years ago

I tried to solve it but failed. Currently I have no idea on how to solve it in an universal way that works in all cases.

comment:87 by Don-vip, 6 years ago

Milestone: 18.04

OK, thanks.

comment:88 by eladner, 6 years ago

What was wrong with the code prior to [11729] ? I don't recall there being any major issues with joining wacky sets of ways prior to that. If anything, it had fewer issues than there are with the current code.

The status quo doesn't bother me, however. I've learned to work around it.

comment:89 by GerdP, 4 years ago

Can this ticket be closed? I cannot reproduce any of the mentioned problems.

comment:90 by eladner, 4 years ago

I cannot reproduce it anymore, either. Close!

comment:91 by GerdP, 4 years ago

Resolution: irreproducible
Status: newclosed

OK, thanks for the feedback.

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.