Modify

Opened 9 years ago

Closed 9 years ago

#6107 closed defect (fixed)

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

Reported by: anonymous Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: Cc:

Description

  • 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

Attachments (4)

mapcss-patch.txt (42.6 KB) - added by anonymous 9 years ago.
mapcss-patch-2.txt (42.3 KB) - added by anonymous 9 years ago.
Replaces former mapcss-patch.txt
mapcss-patch-3.txt (47.4 KB) - added by anonymous 9 years ago.
Replaces former mapcss-patch-2.txt
2lines-extended.patch (3.9 KB) - added by bastiK 9 years ago.

Download all attachments as: .zip

Change History (24)

Changed 9 years ago by anonymous

Attachment: mapcss-patch.txt added

comment:1 in reply to:  description ; Changed 9 years ago by 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.

comment:2 Changed 9 years ago by 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

comment:3 in reply to:  2 Changed 9 years ago by 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".


comment:4 in reply to:  1 ; Changed 9 years ago by 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?

Changed 9 years ago by anonymous

Attachment: mapcss-patch-2.txt added

Replaces former mapcss-patch.txt

comment:5 Changed 9 years ago by 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.

comment:6 in reply to:  4 ; Changed 9 years ago by 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).

comment:7 in reply to:  6 Changed 9 years ago by 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.

Changed 9 years ago by anonymous

Attachment: mapcss-patch-3.txt added

Replaces former mapcss-patch-2.txt

comment:8 Changed 9 years ago by 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.


comment:9 Changed 9 years ago by bastiK

In [3987/josm]:

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

comment:10 Changed 9 years ago by bastiK

In [3988/josm]:

added tests (patch by anonymous, see #6107)

comment:11 Changed 9 years ago by bastiK

In [3989/josm]:

forgot data (see #6107)

comment:12 Changed 9 years ago by 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.

comment:13 Changed 9 years ago by 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.

comment:14 Changed 9 years ago by bastiK

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

comment:15 Changed 9 years ago by 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.

comment:16 in reply to:  14 Changed 9 years ago by 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?

comment:17 in reply to:  14 Changed 9 years ago by 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?

comment:18 in reply to:  15 Changed 9 years ago by 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. :)

Changed 9 years ago by bastiK

Attachment: 2lines-extended.patch added

comment:19 Changed 9 years ago by 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)

comment:20 Changed 9 years ago by bastiK

Resolution: fixed
Status: newclosed

In [3992/josm]:

applied #6107 - MapCSS enhancement (patch by anonymous)

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.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.