[PATCH] Selected ways of multipolygons drawn before other ways

[Daeron]

When selecting a way that is one of the ways forming the outer ring of a multipolygon, the whole outer ring (i.e. all the different ways that have outer role in the multipolygon relation) is highlighted. Also, if selecting other than the lowest id, also the lowest id-way is selected, but the node count is shown as the total of the all and not the correct amount.

This bug was introduced in r2309.

[Daeron]

It seems that the lowest id member can be selected fine, it highlights only the selected way. But selecting any other outer-way highlights the whole outer ring. The phantom way seems to appear when ctrl-clicking the selected way (other than the lowest id), or adding anything else to the selection. Also the lowest id way cannot be added to the selection.

The phantom way also gets added to the selection if the inside of the polygon is clicked away from any way.

[hansendc]

Could you give me an example of this? Perhaps coordinates in OSM of one of these? I'm not really sure what a multipolygon is. This may also be fixed by the patch I just posted to #3780.

[Daeron]

Replying to hansendc:

Could you give me an example of this? Perhaps coordinates in OSM of one of these? I'm not really sure what a multipolygon is. This may also be fixed by the patch I just posted to #3780.

There's one lake using multiple outer-ways at ​http://api.openstreetmap.org/api/0.6/map?bbox=23.7277,60.1572,24.0958,60.31335

The Multipolygon "Lohjanjärvi" is the one, only the first outer-member can be selected normally, if any of the other three are selected, the whole outer ring of the lake is highlighted.

I also tested the patch for the other bug, it didn't help at all with this bug.

You can also test this bug with the multipolygon.osm -file that's in the data directory of josm's svn-tree.

[hansendc]

I don't think this is the fault of the selection code. MapPaintVisitor::joinWays() has the following:

if(n != null)
{

w = new Way(w);
w.setNodes(n);
if (selected) {

data.addSelected(Collections.singleton(w),false /* don't notify listeners */);

} else {

data.clearSelection(w);

}

}

That addSelected() was recently changed from setSelected(). I didn't do that:

​http://josm.openstreetmap.de/changeset/2282/trunk/src/org/openstreetmap/josm/data/osm/visitor/MapPaintVisitor.java

It's possible that there's some bug when the selected item is not in the data set, but I'm not sure. What is the behavior supposed to be here, anyway? joinWays() is a horrid little piece of code, and I'm not groking it very well. It seems a wee bit insane that we're changing the selection inside of the *drawing* code.

So, I'm still really unclear on this bug. It really seems to me from the code that the intended behavior is to select an entire polygon when you select any one of its ways, and this is implemented with a temporary way that isn't part of the data set. I'm not sure how you are supposed to go about getting at the individual ways. That isn't clear from reading the code.

[Daeron]

Replying to hansendc:

It's possible that there's some bug when the selected item is not in the data set, but I'm not sure. What is the behavior supposed to be here, anyway? joinWays() is a horrid little piece of code, and I'm not groking it very well. It seems a wee bit insane that we're changing the selection inside of the *drawing* code.

So, I'm still really unclear on this bug. It really seems to me from the code that the intended behavior is to select an entire polygon when you select any one of its ways, and this is implemented with a temporary way that isn't part of the data set. I'm not sure how you are supposed to go about getting at the individual ways. That isn't clear from reading the code.

The old behavior was to highlight the background of the area that the outer ways represent, and of those ways, only highlight the selected one. That way you can see where the way starts and ends, but also see that it is part of a multipolygon.

[Daeron]

Replying to hansendc:

That addSelected() was recently changed from setSelected(). I didn't do that:

​http://josm.openstreetmap.de/changeset/2282/trunk/src/org/openstreetmap/josm/data/osm/visitor/MapPaintVisitor.java

This bug only appeared when your selection code was committed. In the previous revision (r2308) it worked just fine.

[hansendc]

Replying to Daeron:

Replying to hansendc:

That addSelected() was recently changed from setSelected(). I didn't do that:

​http://josm.openstreetmap.de/changeset/2282/trunk/src/org/openstreetmap/josm/data/osm/visitor/MapPaintVisitor.java

This bug only appeared when your selection code was committed. In the previous revision (r2308) it worked just fine.

Daeron, thanks for the excellent bug descriptions and patience while I fixed this. It made it a lot easier.

The MapPaintVisitor code is a bit, urm, obtuse. My new selection code made the mistake of actually returning all of the selected objects, despite if they were in the data set or not. It trusted that users would not select objects that were not in the data set in the first place. MapPaintVisitor broke that assumption with its "joined" ways. This patch keeps track of the selection state of the outer polygon outside of the objects themselves, instead.

[Daeron]

Replying to hansendc:

Daeron, thanks for the excellent bug descriptions and patience while I fixed this. It made it a lot easier.

The MapPaintVisitor code is a bit, urm, obtuse. My new selection code made the mistake of actually returning all of the selected objects, despite if they were in the data set or not. It trusted that users would not select objects that were not in the data set in the first place. MapPaintVisitor broke that assumption with its "joined" ways. This patch keeps track of the selection state of the outer polygon outside of the objects themselves, instead.

While your patch fixed this bug, it introduced another: the drawing of the more complex multipolygons is now broken.

In the multipolygon.osm the example 6 in each of the four first tests are broken, the fifth test is broken and in sixth test the example two is broken. Those were the obvious errors I could spot, but there might be more.

[Daeron]

It also seems, that the patch made the selection of the outer ring appear behind other objects, as shown in the attached picture.

[anonymous]

Oh, sweet Jesus, who's code is this? I swear I'm going to convert it to C and submit it to next year's OCCC.

Anyway, here's another go. It's a bit more extreme than the last try. It actually does completely away with the synthetic joined ways. Instead, we just generate the PolyData directly.

This seems to behave itself and I've compared it with the 2308 build. They seem identical in appearance.

[stoecker]

The original code was made by me. Plainly the code joins the individual way elements to create closed rings (there may be multiple closed or unclosed inner and outer rings).

I really don't know what the selection stuff has to do there. I think Karl tried to fix the issue that the red selection of outer polygons was behind other elements and introduced that strange reselection.

I did not really check the code, but found it a bit strange as well.

[Gubaer]

(In [2350]) applied #3781: patch by hansendc: Selecting a way that is part outer ring of multipolygon is broken

[stoecker]

That patch again reintroduces the bug, that the selected way is not drawn correctly. For overlapping ways there must be a solution, which stores the selected way parts and draws them at the end of whole drawing so they don't get overdrawn. The problem is that redrawing outer or inner ways is also no option, as this may produce wrong style. Best would probably to get the select list at beginning of drawing and add temporary ways with correct style to this list replacing the real way. Then the final drawing step to draw selected ways could be done correctly.

[jttt]

What happens is visitAll is this:

• relations are drawn (including selected ways), mappaintDrawnCode is set to current value
• ways are drawn, mappaintDrawnCode is not set
• selected ways are drawn, but only when mappaintDrawnCode is different from current value - selected relation member will not be drawn again

The overlapping problem can be fixed by removing mappaintDrawnCode check at line 1564. I will later commit a fix that also removes unnecessary painting of selected ways or relation members...

[stoecker]

As I already told above removing the "already drawn" check will make situation worse, as the ways will be drawn with wrong style.

[hansendc]

Return the right PolyData, and consult pd.selected in the right spot.

[hansendc]

This will help, but it doesn't fix the over selection issue.

[hansendc]

Here's some work to make the MapPaintVisitor code more readable. This gets rid of a few class-global variables and breaks up some functions.

[Gubaer]

Good move and brave! When I first got in touch with this "OCCC candidate" I also started to bring it in shape but then I gave up.

[hansendc]

This is everything in the last patch, plus a fix to the draw order of selected ways which were part of a multipolygon, which I think was the original bug.

Honestly, I think we need to get rid of a bunch of this paintid stuff. It makes us be lazy about keeping track of when things are painted. But, it gets really difficult to go track down when things actually *were* painted. I think we should work toward having the polygon drawing not draw any of the ways at all. Have it strictly draw the fill polygons themselves. It can set hints for how the ways should be drawn, but it shouldn't actually draw them.

[Gubaer]

Hmm, doesn't apply against r2351. Could you repost a patch?

[hansendc]

I've repulled SVN and reapplied.

[Gubaer]

(In [2354]) applied #3781: patch by hansendc: Selected ways of multipolygons drawn before other ways

[stoecker]

The paintid Is mainly a workaround to prevent redrawing. When we have proper datastructures which allow to reduce the number of elements to check against (i.e. fast coordinate based data access), then this ID stuff can be removed completely.