Modify

Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#3781 closed defect (fixed)

[PATCH] Selected ways of multipolygons drawn before other ways

Reported by: Daeron Owned by: Daeron
Priority: major Milestone:
Component: Core Version: latest
Keywords: Cc: hansendc

Description

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.

Attachments (7)

fix-multipolygon-selection.patch (4.1 KB ) - added by hansendc 14 years ago.
selectionbehind.png (2.0 KB ) - added by Daeron 14 years ago.
fix-multipolygon-selection-1.patch (12.4 KB ) - added by anonymous 14 years ago.
more-multipolygon-work.patch (4.2 KB ) - added by hansendc 14 years ago.
Return the right PolyData, and consult pd.selected in the right spot.
rework-multipolygons-0.patch (22.2 KB ) - added by hansendc 14 years ago.
rework-multipolygons-1.patch (23.4 KB ) - added by hansendc 14 years ago.
multipoly-all-0.patch (13.7 KB ) - added by hansendc 14 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 by Daeron, 14 years ago

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.

comment:2 by stoecker, 14 years ago

Owner: changed from team to hansendc

comment:3 by hansendc, 14 years ago

Owner: changed from hansendc to Daeron
Status: newneedinfo

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.

in reply to:  3 comment:4 by Daeron, 14 years ago

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.

comment:5 by hansendc, 14 years ago

Cc: hansendc added

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.

in reply to:  5 comment:6 by Daeron, 14 years ago

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.

in reply to:  5 ; comment:7 by Daeron, 14 years ago

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.

in reply to:  7 ; comment:8 by hansendc, 14 years ago

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.

by hansendc, 14 years ago

in reply to:  8 comment:9 by Daeron, 14 years ago

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.

comment:10 by Daeron, 14 years ago

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

by Daeron, 14 years ago

Attachment: selectionbehind.png added

comment:11 by anonymous, 14 years ago

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.

by anonymous, 14 years ago

comment:12 by Daeron, 14 years ago

Summary: Selecting a way that is part outer ring of multipolygon is broken[PATCH] Selecting a way that is part outer ring of multipolygon is broken

comment:13 by stoecker, 14 years ago

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.

comment:14 by Gubaer, 14 years ago

Resolution: fixed
Status: needinfoclosed

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

comment:15 by stoecker, 14 years ago

Resolution: fixed
Status: closedreopened
Summary: [PATCH] Selecting a way that is part outer ring of multipolygon is brokenSelected ways of multipolygons drawn before other ways

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.

comment:16 by jttt, 14 years ago

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

comment:17 by stoecker, 14 years ago

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

by hansendc, 14 years ago

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

comment:18 by hansendc, 14 years ago

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

by hansendc, 14 years ago

comment:19 by hansendc, 14 years ago

Summary: Selected ways of multipolygons drawn before other ways[PATCH] Selected ways of multipolygons drawn before other ways

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.

comment:20 by Gubaer, 14 years ago

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.

by hansendc, 14 years ago

comment:21 by hansendc, 14 years ago

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.

comment:22 by Gubaer, 14 years ago

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

by hansendc, 14 years ago

Attachment: multipoly-all-0.patch added

comment:23 by hansendc, 14 years ago

I've repulled SVN and reapplied.

comment:24 by Gubaer, 14 years ago

Resolution: fixed
Status: reopenedclosed

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

comment:25 by stoecker, 14 years ago

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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Daeron.
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.