make target highlighting not feel sluggish

[bastiK]

With the option "Highlight target ways and nodes" disabled, you still get highlighted cross-hair nodes.

[akks]

Do you mean centers of edges in select mode or something else?

[akks]

In 5075/josm:

see #7503 - disable highlighting middle-edge-nodes (and re-renders) together with other targets highlighting

[akks]

Please check highlighting is not broken :)

Now it does not re-render on mouse moves when "Highlight target ways and nodes" is disabled. (and shows no red X, only cursor changes).

[bastiK]

Replying to akks:

Do you mean centers of edges in select mode or something else?

Yes, also known as virtual nodes.

[akks]

Then it seems to be fixed... :)

One more optimization is possible, but hard to implement: when highlighting is enabled, redraw is done for every mouse move near way or node, even if highlighted primitive does not change (I tried to remember primitive under cursor and check if it is new, but then virtual node highlighting becomes buggy)

[bastiK]

Replying to akks:

Then it seems to be fixed... :)

One more optimization is possible, but hard to implement: when highlighting is enabled, redraw is done for every mouse move near way or node, even if highlighted primitive does not change (I tried to remember primitive under cursor and check if it is new, but then virtual node highlighting becomes buggy)

Would be good to improve this as well: I've turned off highlighting because of low performance and others seem to have problems too.

[akks]

We would better leave it to xeen, author of highlighting code. Maybe he has some extra ideas how to improve highlighting performance.

[xeen]

akks suggestion sounds reasonable, I’ve attached a test patch with this implemented for mouse move. Redraws now should only occur when the highlighted primitives change. For some reason I didn’t have to do anything for virtual nodes, they simply work. I’m not sure if moving the cursor above a virtual node triggers repaints, but I assume so. Is there an easy way to print a message for every repaint, regardless which code path triggered it?

In mouseDragged, when moving primitives, removeHighlighting is called everytime. It does not trigger a repaint for every move though, so that seems to be acceptable. I haven’t looked into the highlighting in draw mode, but I suspect it might benefit from a similar approach.

Due to JOSM numerous options correct highlighting is very fragile (and there are bugs open where it’s impossible to correctly highlight, but can’t remember the conditions). So please try the attached patch if it breaks anything obvious I missed and if it actually works on your systems.

[bastiK]

Replying to xeen:

Is there an easy way to print a message for every repaint, regardless which code path triggered it?

MapView.paint?

[xeen]

MapView.paint?

so… uhhh… well.

In any case, it appears w/ above patch I only get redraws for highlighting changes, even for virtual nodes. I’ll commit the patch when I’m in the mood :)

[akks]

I tested it, it works and looks correct :)

[xeen]

In 5093/josm:

only repaint when the to-be-highlighted primitives change.

The patch only affects the select mode when not dragging. While this should
reduce CPU usage with target highlighting enabled, it’s doubtful it will have
visible effect on those systems where target highlighting feels sluggish. See
#7503 for more ideas to improve the situation.

[xeen]

This patch brings the only-repaint-on-highlight-change to the draw action. Please ignore eclipse enforcing the coding style. The patch is only useful if the rubber band helper line is not being drawn; otherwise the repaints are required to paint the helper line. Thus it would only reduce CPU usage in a helper-off target-on scenario (but not improve performance, see above). I doubt this is a common case. I’ll look into painting the helper line onto its own component, so no complete redraws are required.

[xeen]

Hm. It appears there’s already MapViewPaintable, which does exactly what I wanted to do – in other words the helper line is already redrawn on its own; thus above patch should reduce the CPU usage while drawing (if the highlights do not change).

However, I noticed that lastClipBounds.contains(g.getClipBounds()) is always false in certain situations for me. The bounds are as follows:

lastClipBounds: java.awt.Rectangle[x=0,y=0,width=828,height=800]
g.getClipBounds(): java.awt.Rectangle[x=0,y=800,width=828,height=172]

This causes canUseBuffer to be set to false, thus issuing a layer repaint. If I shrink JOSM’s window height by some pixels (less than 800) the y=800 offset vanishes and I get these values:

lastClipBounds: java.awt.Rectangle[x=0,y=0,width=828,height=708]
g.getClipBounds(): java.awt.Rectangle[x=0,y=0,width=828,height=708]

which are obviously fine.

The Graphics2D g doesn’t seem to be modified anywhere between the canUseBuffer= line and where lastClipBounds is updated, so I’m a bit baffled where the y=800 offset comes from.

Any hints on that?

In any case I’ll check the patch some more and then commit it, because with the issue above fixed the rubber band helper line should feel fast(er), even with target highlighting activated.

[xeen]

In 5098/josm:

Reduce repaints required when in draw mode. This should improve performance a bit with target highlighting activated. See #7503

[xeen]

In 5099/josm:

flip default value for wayIsFinished variable when entering drawMode

By default no draw is active, i.e. the helper line needs not to be
drawn. wayIsFinished should reflect this, because otherwise super-
fluous repaints are issued. See #7503

[xeen]

In 5101/josm:

reduce repaints for DeleteAction with target highlighting enabled (see #7503)

[xeen]

I cannot reproduce the bug I experienced in comment:14. Unless I experience it again I’ll assume this was due to cosmic rays hitting my computer.

The checked in patches reduce the layer repaints by only calling for redraw if the highlighted primitives change. Also, only removed/added primitives are being updated with setHighlighted. As long as DataSet isn’t overriden, this only causes the count highlightUpdateCount to be increased, so this isn’t any performance gain on its own. I don’t know if it has any repercussions further "up" apart from repaint, but it’s the correct thing to do anyway.

Apart from the rubber band helper line feeling more snappy in certain cases, all these patches didn’t improve perceived performance. If the highlighting needs to change, then there is little else to do than a repaint. Moving highlighting to a map view paintable would be a possibility. However given that MVPs can only draw on top special measures would have to be taken to achieve a similar "outline highlight effect". Also, it would duplicate a lot of the normal drawing code, so it doesn’t seem to appealing.

A more general solution would be to track what part of the existing buffer needs to repaint and only repaint that. For example if a node is highlighted/remove/moved… only a small area about that node would have to be redrawn. I don’t know the paint code, so maybe the effort to implement outweighs the benefit. On high zoom levels only a few primitives have to be drawn due to QuadBuckets, right?

Highlighting could also be sped up by simplifying the rules used to render it. Although the improvements will probably be very small since it currently is one transparent stroke for each node/way segment. Only when use-real-widths is activated, more than stroke will be required, depending on the width of the object.

[xeen]

If single way segments are to be highlighted, then this is currently done regardless if the segments are actually visible or not. I only know of DeleteAction using this feature and there it only ever highlights a single way segment – the one under the cursor. Unless there are other users of way segment highlighting, the below patch would have no impact in a best-case scenario.

Index: src/org/openstreetmap/josm/data/osm/visitor/paint/MapPainter.java
===================================================================
--- src/org/openstreetmap/josm/data/osm/visitor/paint/MapPainter.java   (revision 5101)
+++ src/org/openstreetmap/josm/data/osm/visitor/paint/MapPainter.java   (working copy)
@@ -156,8 +156,10 @@
 
                 Point p1 = nc.getPoint(ws.getFirstNode());
                 Point p2 = nc.getPoint(ws.getSecondNode());
-                highlightSegs.moveTo(p1.x, p1.y);
-                highlightSegs.lineTo(p2.x, p2.y);
+                if(isSegmentVisible(p1, p2)) {
+                    highlightSegs.moveTo(p1.x, p1.y);
+                    highlightSegs.lineTo(p2.x, p2.y);
+                }
             }
 
             drawPathHighlight(highlightSegs, line);

[rickmastfan67]

I just want to comment on this as it's appeared to make a few things impossible to do without several extra annoying steps.

Let's say that there's a T-intersection that was recently turned into a +-intersection. So, you go to draw the new road extension from that former T. Well, that's no longer possible because of some of the tweaks that were done in this ticket.

Steps to reproduce:

1. Load the attached "ticket7503bug.osm" file (will be attached after this post)
2. Select the "primary" highway as well as the intersection node for all three ways.
3. Hit the "A" button to go into drawing mode.
4. Left click on the intersection node in attempt to continue to draw the primary highway.

What happens:
A totally new way is draw that isn't connected to the primary highway, which forces you to then merge the ways together. It gets more annoying when the original primary highway has a relation as that screen also pops up because of the combining of the 2 ways.

What should happen:
The primary highway should be continued.

I mean, if I wanted to start an entirely new way from that intersection instead of continuing the original way, I would have just gone into the drawing mode and left clicked on the intersection node to start drawing. This is very annoying IMO.

Repository Root: http://josm.openstreetmap.de/svn
Build-Date: 2012-03-19 02:32:22
Last Changed Author: simon04
Revision: 5105
Repository UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
URL: http://josm.openstreetmap.de/svn/trunk
Last Changed Date: 2012-03-18 21:33:23 +0100 (Sun, 18 Mar 2012)
Last Changed Rev: 5105

Identification: JOSM/1.5 (5105 en)
Memory Usage: 122 MB / 2730 MB (83 MB allocated, but free)
Java version: 1.6.0_31, Sun Microsystems Inc., Java HotSpot(TM) 64-Bit Server VM
Operating system: Windows 7

Plugin: OpeningHoursEditor (27852)
Plugin: buildings_tools (27984)
Plugin: licensechange (27964)
Plugin: mapdust (27884)
Plugin: measurement (27957)
Plugin: openstreetbugs (27852)
Plugin: reverter (27865)
Plugin: turnrestrictions (27891)
Plugin: undelete (27852)
Plugin: utilsplugin2 (28045)

[xeen]

Try with the patch in #7524. With it applied, you don’t have to execute step 4 to continue drawing, because it already is when entering drawing mode. As far as I can tell, this is how it was before I broke drawing.

[rickmastfan67]

I can't build custom JOSM's on my end (believe me I've tried, but could never get eclipse to work correctly), otherwise I would test it. :(

So, I've just reverted back to [5097] for now which was the last nightly build that worked.

[xeen]

Try ​http://mathphys.fsk.uni-heidelberg.de/~stefan/josm-p7524.jar

[rickmastfan67]

Just tested the build you posted and that looks like it has fixed the partially broken draw feature. ;)

[xeen]

I committed the patch. Next latest-builds should work again and have less repaints.

[xeen]

Another idea which might improve the situation: Reduce the amount of repaints required when moving the mouse. This avoid "in the middle" repaints at the cost of higher response times. I don’t notice the additional delay but feel the improvement when covering long distances. The delay might be more noticable on low end systems. Can you try?

[bastiK]

Not sure, Java already has some kind of sophisticated system to queue and merge repaint requests.

[xeen]

Thanks for the hint. I read up on the documentation and you are right. Java automatically merges paint requests and I doubt I believe adding another layer of merging won’t actually improve anything.

I have another idea which partly works. When repainting due to highlighting changes it’s possible to further restrict the area in need of repainting. With the former patches in place it’s relatively easy to track which primitives’ highlighting changed and therefore need repainting. This especially improves perf when zoomed out (highlighting a node would require a 5x5 pixel repaint or so instead of the whole screen).

The problem is: this will cut off text labels in certain situations. The reason for this is that text may expand indefinitely and thus span the whole screen if it is long enough. When repainting the smaller rectangle, that text gets cut off because only primitives that intersect the small rectangle get repainted and overlapping text is not counted as intersection. This is not a bug in the intersection code since it’s perfectly reasonable that off-screen nodes don’t print text into the map view.

One solution for this is to always start the small rectangle on the left, regardless of what actually needs repainting. So, highlighting a node on the right would result in a large horizontal repaint area. The vertical axis shouldn’t be a problem since text can’t break; enlarging the small rectangle by some magic number should be sufficient. This is what I’ll likely implement once I find the time and rule out other bugs.

Another idea is to give text a maximum width before cutting it off. I would argue that any text that doesn’t fit the default width of the add/edit properties window may be omitted. This is another bug though and should be discussed first anyway.

I don’t know the MapPaint CSS code, so I’ll appreciate any hints if my “start from the left” plan is doomed to fail.

[stoecker]

Usually such issues are done by doubled buffer. I.e. have a text only buffer and copy its contents after repaint has been done.

[stoecker]

Be aware that in the past somebody already tried to implement partial repainting and failed to get it done :-)

[bastiK]

Replying to xeen:

Thanks for the hint. I read up on the documentation and you are right. Java automatically merges paint requests and I doubt I believe adding another layer of merging won’t actually improve anything.

I have another idea which partly works. When repainting due to highlighting changes it’s possible to further restrict the area in need of repainting. With the former patches in place it’s relatively easy to track which primitives’ highlighting changed and therefore need repainting. This especially improves perf when zoomed out (highlighting a node would require a 5x5 pixel repaint or so instead of the whole screen).

The problem is: this will cut off text labels in certain situations. The reason for this is that text may expand indefinitely and thus span the whole screen if it is long enough. When repainting the smaller rectangle, that text gets cut off because only primitives that intersect the small rectangle get repainted and overlapping text is not counted as intersection. This is not a bug in the intersection code since it’s perfectly reasonable that off-screen nodes don’t print text into the map view.

One solution for this is to always start the small rectangle on the left, regardless of what actually needs repainting. So, highlighting a node on the right would result in a large horizontal repaint area. The vertical axis shouldn’t be a problem since text can’t break; enlarging the small rectangle by some magic number should be sufficient. This is what I’ll likely implement once I find the time and rule out other bugs.

Another idea is to give text a maximum width before cutting it off. I would argue that any text that doesn’t fit the default width of the add/edit properties window may be omitted. This is another bug though and should be discussed first anyway.

At least you can restrict it to a horizontal stripe that spans the whole screen from left to right.

I don’t know the MapPaint CSS code, so I’ll appreciate any hints if my “start from the left” plan is doomed to fail.

Restricting the repaint area will definitely speed up the painting a lot. I've tried this some time ago, but had problem getting the boxes right. I'm wondering if it makes sense to assign a bbox to each map element (icon, box, line, text - what is called ElemStyle in the code). Then put them all in a QuadBuckets. This would make it possible to determine precisely the set of elements that have to be redrawn in order to refresh a given box. On the downside it increases memory usage and it seems to be more or less redundant information in 99% of the cases.

[bastiK]

Replying to stoecker:

Usually such issues are done by doubled buffer. I.e. have a text only buffer and copy its contents after repaint has been done.

Not sure if this works. I tried to have one buffer for each (JOSM-)Layer and this definitely did not work. It was quite slow, also, a full screen bitmap can be pretty large in memory.

[xeen]

Another problem is that primitives have "zero-width" but their drawn representation doesn’t. When only redrawing a small rectangle it’s thus possible to get nodes partially hidden for example, if the node isn’t in the BBox but their yellow rectangle representation is. You can get around this by enlarging the query-rectangle, but not the redraw one. The expansion would have to be max_obj_width/2 (or _height/2)…

Even if that is solved by either determining the correct amount to expand or implementing intersection on the drawn representation instead of the osm data there’s another problem: one-way arrows and dashed lines are somehow depend on their visible portion. Thus the dashes and arrows will move when repainting only partly leaving render-artifacts.

So there’s definitely a lot to do to make repainting extracts possible.

[bastiK]

Replying to xeen:

Another problem is that primitives have "zero-width" but their drawn representation doesn’t. When only redrawing a small rectangle it’s thus possible to get nodes partially hidden for example, if the node isn’t in the BBox but their yellow rectangle representation is. You can get around this by enlarging the query-rectangle, but not the redraw one. The expansion would have to be max_obj_width/2 (or _height/2)…

Even if that is solved by either determining the correct amount to expand or implementing intersection on the drawn representation instead of the osm data there’s another problem: one-way arrows and dashed lines are somehow depend on their visible portion. Thus the dashes and arrows will move when repainting only partly leaving render-artifacts.

This can be fixed by setting correct value for ​dash_phase. (Put way length till the start point of drawing)

So there’s definitely a lot to do to make repainting extracts possible.

True.

[akks]

In 6058/josm:

see #7503: allow to disable highlighting of members/primitives (for slow machines)
(using old Draw target highlight option in display preferences)