[Patch] MapCSS enhancement + small refactoring + one bug fix

[anonymous]

• fixes a typo
• introduces new LabelCompositionStrategy (and refactors related methods in MapPainter)
• introduces new MapCSS directive 'text-from-tag'
• contributes some javadoc
• fixes part of the currently broken unit test environment and contributes two new tests

[bastiK]

-        if (icon == null && symbol == null && !allowOnlyText)
-            return null;

There is a performance penalty here. (I measured 20-50% increase in style generation time.) This may be worth it, but I'd like to look into it some more.

[bastiK]

Regarding 'text-from-tag':

If it was my decision, I'd do it the same way, but in the "specification draft" there are different conventions:
​http://wiki.openstreetmap.org/wiki/MapCSS/0.2

[anonymous]

Replying to bastiK:

Regarding 'text-from-tag':

If it was my decision, I'd do it the same way, but in the "specification draft" there are different conventions:
​http://wiki.openstreetmap.org/wiki/MapCSS/0.2

You're right. It's only now, that I realize that text: is already meant to have a tag name as value. I'll fix that and submit a new patch.

The specification doesn't mention the key value pair text: auto but I guess that it should be left unchanged. If text:auto is used, JOSM selects the first available from a configurable list of "name tags".

[anonymous]

Replying to bastiK:

-        if (icon == null && symbol == null && !allowOnlyText)
-            return null;

There is a performance penalty here. (I measured 20-50% increase in style generation time.) This may be worth it, but I'd like to look into it some more.

What do you mean with "style generation time" ? It's probably not part of the rendering loop, is it? So, it's less critical performance wise?

[anonymous]

Replaces former mapcss-patch.txt

[anonymous]

Now implements the text: property as described in ​http://wiki.openstreetmap.org/wiki/MapCSS/0.2. Still supports text:auto, although not specified (yet?). See the second patch.

[bastiK]

Replying to anonymous:

Replying to bastiK:

-        if (icon == null && symbol == null && !allowOnlyText)
-            return null;

There is a performance penalty here. (I measured 20-50% increase in style generation time.) This may be worth it, but I'd like to look into it some more.

What do you mean with "style generation time" ? It's probably not part of the rendering loop, is it? So, it's less critical performance wise?

If you do benchmark like this:

Index: src/org/openstreetmap/josm/data/osm/visitor/paint/MapPaintVisitor.java
===================================================================
--- src/org/openstreetmap/josm/data/osm/visitor/paint/MapPaintVisitor.java	(revision 3981)
+++ src/org/openstreetmap/josm/data/osm/visitor/paint/MapPaintVisitor.java	(working copy)
@@ -94,7 +94,7 @@
     };
 
     public void visitAll(final DataSet data, boolean virtual, Bounds bounds) {
-        //long start = System.currentTimeMillis();
+        long start = System.currentTimeMillis();
         BBox bbox = new BBox(bounds);
         this.data = data;
 
@@ -197,6 +197,8 @@
         scDisabledRestrictions.drawAll();
         scDisabledRestrictions = null;
 
+        long phase1 = System.currentTimeMillis();
+
         scNormalAreas.drawAll();
         scSelectedAreas.drawAll();
         scNormalLines.drawAll();
@@ -209,7 +211,8 @@
         scSelectedNodes.drawAll();
 
         painter.drawVirtualNodes(data.searchWays(bbox));
-        //System.err.println("PAINTING TOOK "+(System.currentTimeMillis() - start)+ " (at scale "+circum+")");
+        long now = System.currentTimeMillis();
+        System.err.println(String.format("PAINTING TOOK %d [PHASE1 took %d] (at scale %s)", now - start, phase1 - start, circum));
     }
 
     public void setGraphics(Graphics2D g) {

it's the time reported as "phase1". For the first time, this takes about 50% of the full painting time and then drops to ~5% for subsequent painting (because the generated styles are cached). So for normal mapping it doesn't matter that much, but if you open a large data file in JOSM (e.g. for imports) it can be significant (e.g wait 2 min or 3 min before something is displayed).

[anonymous]

Replying to bastiK:

Replying to anonymous:

Replying to bastiK:

-        if (icon == null && symbol == null && !allowOnlyText)
-            return null;

There is a performance penalty here. (I measured 20-50% increase in style generation time.) This may be worth it, but I'd like to look into it some more.

What do you mean with "style generation time" ? It's probably not part of the rendering loop, is it? So, it's less critical performance wise?

If you do benchmark like this:

I've measured rendering time for a reasonably big and densely mapped area. My findings are, that

• in the first iteration of the rendering loop, phase1 always takes ~60% of the time, regardless of whether the two lines above are present or not.
• by removing the lines, the time for the first interation of the rendering loop is increased by ~30%
• one third (10%) of this increase is due to PaintColors.TEXT.get() in

TextElement te = TextElement.create(c, PaintColors.TEXT.get());

• another third (~ 15%) of this increase is due to Font font = ElemStyle.getFont(c); within TextElement.create(...)

These two statements aren't efficient enough to be part of the rendering loop.

[anonymous]

Replaces former mapcss-patch-2.txt

[anonymous]

This version of the patch provides the same features as before and fixes the performance issues. According to my measurements the rendering loop is even slightly faster with the patch than without the patch.

[bastiK]

In [3987/josm]:

mapcss: improvements in text label selection, performance and typo (patch by anonymous, see #6107)

[bastiK]

In [3988/josm]:

added tests (patch by anonymous, see #6107)

[bastiK]

In [3989/josm]:

forgot data (see #6107)

[bastiK]

Omitted the removal of the 2 lines for now, so we can have easier comparison.

Replying to anonymous:

I've measured rendering time for a reasonably big and densely mapped area. My findings are, that

• in the first iteration of the rendering loop, phase1 always takes ~60% of the time, regardless of whether the two lines above are present or not.

In theory, the increase is 100% due to phase1 as nothing has changed with phase2. (Unless you have loaded a style that makes use of the new features.)

• by removing the lines, the time for the first interation of the rendering loop is increased by ~30%
• one third (10%) of this increase is due to PaintColors.TEXT.get() in

TextElement te = TextElement.create(c, PaintColors.TEXT.get());

• another third (~ 15%) of this increase is due to Font font = ElemStyle.getFont(c); within TextElement.create(...)

These two statements aren't efficient enough to be part of the rendering loop.

This sounds as if the code is executed for each paint request. But it is executed only once for each primitive (also when the painting cache is cleared, globally, or per object). I changed the comments in the code because this might be misleading.

[anonymous]

Something went wrong when the patch was applied in [3988].

The content of the groovy test cases is included twice in the file,
see ​http://josm.openstreetmap.de/browser/josm/trunk/test/unit/org/openstreetmap/josm/gui/mappaint/AllMappaintTests.groovy

Please fix in the repository or let me know if you need another patch.

[bastiK]

Fixed. (for some reason the patch was rejected for JOSMFixture.java)

[anonymous]

Omitted the removal of the 2 lines for now, so we can have easier comparison.

This is a pity because the patch is basically useless as long as these two lines are there :-(

Although the patch fixes a typo and contributes some cosmetical changes, it's main goal is to get the MapCSS property text: aTagName to work. Unfortunatelly, it doesn't work unless these two lines are removed. I'd say it's safe to remove them, because there isn't a negative impact on performance. Regardless of whether I comment out these two lines or not, I measure the same timing values in the visitAll(...) method when the big loop prepares the style elements for rendering.

[anonymous]

Replying to bastiK:

Fixed. (for some reason the patch was rejected for JOSMFixture.java)

Thanks. Unfortunatelly, [3989] didn't work either. Could you please apply this one again?

[anonymous]

Replying to bastiK:

Fixed. (for some reason the patch was rejected for JOSMFixture.java)

Three groovy files have been patched in [3988], one is now fixed, the other two are still broken (the contain the same source twice). Could you please reapply the patch?

[bastiK]

Replying to anonymous:

Omitted the removal of the 2 lines for now, so we can have easier comparison.

This is a pity because the patch is basically useless as long as these two lines are there :-(

Although the patch fixes a typo and contributes some cosmetical changes, it's main goal is to get the MapCSS property text: aTagName to work. Unfortunatelly, it doesn't work unless these two lines are removed. I'd say it's safe to remove them, because there isn't a negative impact on performance. Regardless of whether I comment out these two lines or not, I measure the same timing values in the visitAll(...) method when the big loop prepares the style elements for rendering.

I know, this split is simply to separate the different parts of the patch, so the changes are more transparent. Just give me a few more minutes. :)

[bastiK]

Normally nodes without own style are annotated (AUTO_LABEL_COMPOSITION_STRATEGY) but nodes with style are not. The reason is, that in the latter case you can write it explicitly in the style definition. It is more natural to write text: auto; when you want annotation instead of writing text: none; to suppress it. I updated the 2-lines patch to account for that.

Updated benchmark:

pre-patch:

PAINTING TOOK 12177 [PHASE1 took 6880] (at scale 1652.3183265553841)
PAINTING TOOK 12248 [PHASE1 took 6922] (at scale 1652.3183265553841)

current (r3990):

PAINTING TOOK 11383 [PHASE1 took 5949] (at scale 1652.3183265553841)
PAINTING TOOK 11293 [PHASE1 took 5946] (at scale 1652.3183265553841)

with 2lines-extended.patch applied:

PAINTING TOOK 12676 [PHASE1 took 6659] (at scale 1652.3183265553841)
PAINTING TOOK 12041 [PHASE1 took 6652] (at scale 1652.3183265553841)

[bastiK]

In [3992/josm]:

applied #6107 - MapCSS enhancement (patch by anonymous)