Modify

Opened 8 years ago

Closed 8 years ago

#8934 closed defect (fixed)

[patch] Text-halo-color and text-halo-radius Properties are not applied within area selectors

Reported by: skorasaurus Owned by: team
Priority: normal Milestone:
Component: Core mappaint Version: latest
Keywords: mapcss Cc:

Description

I have been attempting to style the names of a few features using an eval selector.

For example, I have 10 or so features that include small_area : true; as follows:

area[power=generator]:closed {
    color: black;
    width: 2;
    fill-color: #444444;
    fill-opacity: 0.6;
    small_area : true;}

area[eval(prop(small_area))]:closed {
    text-color: black;
    font-size: 100;
    text: name;
    text-halo-color: white;
    text-halo-radius: 20;
    text-position: center;
}

text-halo-color and text-halo-radius are not applied to my selectors although font-size does.
I have attached a simple test case that shows this bug.

Attachments (7)

testing-bugreport.mapcss (968 bytes) - added by skorasaurus 8 years ago.
TEST CASE
8934-1-HaloedTextInAreas.patch (1.6 KB) - added by AlfonZ 8 years ago.
8934-2-HaloedTexts.patch (3.3 KB) - added by AlfonZ 8 years ago.
8934-3a-DisplayTextRefactor.patch (5.3 KB) - added by AlfonZ 8 years ago.
Patch to be applied after patch#2. Moved halo drawing from drawArea, drawBoxText and drawTextOnPath to displayText.
8934-3b-HaloedTextsRefactored.patch (4.7 KB) - added by AlfonZ 8 years ago.
Combined patch#2 and patch#3. Moved halo drawing from drawArea, drawBoxText and drawTextOnPath to displayText. Doing that, areas now have haloed texts, disabled nodes' haloed texts use inactiveColor and disabled ways' texts use inactiveColor.
8934-4a-String2GlyphVector.patch (3.7 KB) - added by AlfonZ 8 years ago.
Patch to be applied after patch#3. Replace multiple selections between String and GlyphVector by converting String to GlyphVector beforehand.
8934-4b-HaloedTextsRefactored2.patch (4.7 KB) - added by AlfonZ 8 years ago.
Combined patch#2, patch#3 and patch#4. Moved halo drawing from drawArea, drawBoxText and drawTextOnPath to displayTexts. Doing that, areas now have haloed texts, disabled nodes' haloed texts use inactiveColor and disabled ways' texts use inactiveColor. Avoid multiple selections between String and GlyphVector by converting String to GlyphVector beforehand (used by nodes and areas).

Download all attachments as: .zip

Change History (15)

Changed 8 years ago by skorasaurus

Attachment: testing-bugreport.mapcss added

TEST CASE

Changed 8 years ago by AlfonZ

Changed 8 years ago by AlfonZ

Attachment: 8934-2-HaloedTexts.patch added

comment:1 Changed 8 years ago by AlfonZ

Summary: Text-halo-color and text-halo-radius Properties are not applied within Eval selectors[patch] Text-halo-color and text-halo-radius Properties are not applied within area selectors

In 8934-1-HaloedTextInAreas.patch the halo drawing code is based on one from drawBoxText().

I noticed that if there's halo set, it gets drawn even when the element (node/way/area) is disabled (filtered out or layer is inactive).
8934-2-HaloedTexts.patch in addition disables halo drawing and uses inactiveColor for texts (even for non-haloed ones) of disabled elements.

comment:2 Changed 8 years ago by bastiK

Hi AlfonZ, the second patch gets rejected when the first is applied. Can you please combine both in one?

As the halo drawing code is now at 3 places, it might be a good idea to put it in a method...

Last edited 8 years ago by bastiK (previous) (diff)

comment:3 Changed 8 years ago by AlfonZ

The second patch is already combined.
I'll look at refactoring tomorrow.

Changed 8 years ago by AlfonZ

Patch to be applied after patch#2.
Moved halo drawing from drawArea, drawBoxText and drawTextOnPath to displayText.

Changed 8 years ago by AlfonZ

Combined patch#2 and patch#3.
Moved halo drawing from drawArea, drawBoxText and drawTextOnPath to displayText.
Doing that, areas now have haloed texts, disabled nodes' haloed texts use inactiveColor and disabled ways' texts use inactiveColor.

Changed 8 years ago by AlfonZ

Patch to be applied after patch#3.
Replace multiple selections between String and GlyphVector by converting String to GlyphVector beforehand.

Changed 8 years ago by AlfonZ

Combined patch#2, patch#3 and patch#4.
Moved halo drawing from drawArea, drawBoxText and drawTextOnPath to displayTexts.
Doing that, areas now have haloed texts, disabled nodes' haloed texts use inactiveColor and disabled ways' texts use inactiveColor.
Avoid multiple selections between String and GlyphVector by converting String to GlyphVector beforehand (used by nodes and areas).

comment:4 Changed 8 years ago by AlfonZ

Moved halo drawing from drawArea, drawBoxText and drawTextOnPath to displayText - patch#3.

It works, but doesn't look that nice. drawTextOnPath's usage complicates it a little bit, because it places text along a path character by character, so doesn't use drawString.

I was thinking about adding extra displayText(String,...) that would convert String to GlyphVector and handing that to displayText(GlyphVector,...) which would do the drawing - patch#4.
It seems to work, at the expense of replacing drawString by drawGlyphVector. Now it might be even possible to remove surrounding Font defaultFont = g.getFont(); ... g.setFont(defaultFont);.

Last edited 8 years ago by AlfonZ (previous) (diff)

comment:5 Changed 8 years ago by bastiK

Looks clear and tidy with patch 4, but I fear it has an impact on the performance when drawString is replaced by drawGlyphVector. What I would do is have both String s and GlyphVector gv as arguments of a single displayText(...) method and then do some checks
if (s != null) { drawString } else { drawGlyphVector }, if (gv == null) { make gv from s; drawGlyphVector } and so on. Not very elegant, maybe you find something better. :)

comment:6 in reply to:  5 ; Changed 8 years ago by AlfonZ

Replying to bastiK:

What I would do is have both String s and GlyphVector gv as arguments of a single displayText(...) method and then do some checks
if (s != null) { drawString } else { drawGlyphVector }, if (gv == null) { make gv from s; drawGlyphVector } and so on. Not very elegant, maybe you find something better. :)

Do you mean something like in patch#3?

Looks clear and tidy with patch 4, but I fear it has an impact on the performance when drawString is replaced by drawGlyphVector.

I've done some performance testing, using timer in StyledMapRenderer.render. Dataset consisted of (200 nonhaloed node texts, 200 haloed node texts, 200 nonhaloed line texts, 200 haloed line texts, 200 nonhaloed area texts, 200 haloed area texts).

  • without patch
    • normal
      PAINTING TOOK 3063 [PHASE1 took 16] (at scale 11102.581343440308)
      PAINTING TOOK 3015 [PHASE1 took 15] (at scale 11102.581343440308)
    • elements disabled (filtered out or layer inactive)
      PAINTING TOOK 3000 [PHASE1 took 15] (at scale 11102.581343440308)
      PAINTING TOOK 2906 [PHASE1 took 15] (at scale 11102.557091305049)
  • patch#3 (unified halo drawing, use drawString)
    • normal
      PAINTING TOOK 3985 [PHASE1 took 16] (at scale 11343.941807424417)
      PAINTING TOOK 3953 [PHASE1 took 15] (at scale 11343.941807424417)
    • elements disabled
      PAINTING TOOK 1453 [PHASE1 took 16] (at scale 11343.92767355973)
      PAINTING TOOK 1406 [PHASE1 took 0] (at scale 11343.919695825709)
  • patch#4 (unified halo drawing, use drawGlyphVector)
    • normal
      PAINTING TOOK 4031 [PHASE1 took 16] (at scale 10985.71206614267)
      PAINTING TOOK 3922 [PHASE1 took 15] (at scale 10985.71206614267)
    • elements disabled
      PAINTING TOOK 1453 [PHASE1 took 16] (at scale 10985.71206614267)
      PAINTING TOOK 1421 [PHASE1 took 16] (at scale 10985.712066142663)

My conclusion:
Increased times in normal are due to newly drawn halos on areas' texts.
Decreased times in disabled are due to disabled halo drawing in disabled node texts and line texts.
Replacing drawString by drawGlyphVector doesn't impact performance.

comment:7 in reply to:  6 Changed 8 years ago by bastiK

Replying to AlfonZ:

Replying to bastiK:

What I would do is have both String s and GlyphVector gv as arguments of a single displayText(...) method and then do some checks
if (s != null) { drawString } else { drawGlyphVector }, if (gv == null) { make gv from s; drawGlyphVector } and so on. Not very elegant, maybe you find something better. :)

Do you mean something like in patch#3?

Yes, exactly.

Looks clear and tidy with patch 4, but I fear it has an impact on the performance when drawString is replaced by drawGlyphVector.

I've done some performance testing, using timer in StyledMapRenderer.render. Dataset consisted of (200 nonhaloed node texts, 200 haloed node texts, 200 nonhaloed line texts, 200 haloed line texts, 200 nonhaloed area texts, 200 haloed area texts).

  • without patch
    • normal
      PAINTING TOOK 3063 [PHASE1 took 16] (at scale 11102.581343440308)
      PAINTING TOOK 3015 [PHASE1 took 15] (at scale 11102.581343440308)
    • elements disabled (filtered out or layer inactive)
      PAINTING TOOK 3000 [PHASE1 took 15] (at scale 11102.581343440308)
      PAINTING TOOK 2906 [PHASE1 took 15] (at scale 11102.557091305049)
  • patch#3 (unified halo drawing, use drawString)
    • normal
      PAINTING TOOK 3985 [PHASE1 took 16] (at scale 11343.941807424417)
      PAINTING TOOK 3953 [PHASE1 took 15] (at scale 11343.941807424417)
    • elements disabled
      PAINTING TOOK 1453 [PHASE1 took 16] (at scale 11343.92767355973)
      PAINTING TOOK 1406 [PHASE1 took 0] (at scale 11343.919695825709)
  • patch#4 (unified halo drawing, use drawGlyphVector)
    • normal
      PAINTING TOOK 4031 [PHASE1 took 16] (at scale 10985.71206614267)
      PAINTING TOOK 3922 [PHASE1 took 15] (at scale 10985.71206614267)
    • elements disabled
      PAINTING TOOK 1453 [PHASE1 took 16] (at scale 10985.71206614267)
      PAINTING TOOK 1421 [PHASE1 took 16] (at scale 10985.712066142663)

My conclusion:
Increased times in normal are due to newly drawn halos on areas' texts.
Decreased times in disabled are due to disabled halo drawing in disabled node texts and line texts.
Replacing drawString by drawGlyphVector doesn't impact performance.

I think you are right that there is no performance difference in drawString and createGlyphVector; drawGlyphVector.

However, there is one problem I wasn't aware of until now. From the javadoc of createGlyphVector:

This means that this method is not useful for some scripts, such as Arabic, Hebrew, Thai, and Indic, that require reordering, shaping, or ligature substitution.

So for example Arabic text is written in reverse order and without ligatures. While the problem is currently visible for text with halo and text along a line, the default JOSM style does not use these features, so it is possible to work without being affected by this bug.

Therefore I think we should stick to patch3b for the moment. (Will be applied when stabilization is over.)

comment:8 Changed 8 years ago by bastiK

Resolution: fixed
Status: newclosed

In 6122/josm:

fixed #8934 - Text-halo-color and text-halo-radius Properties are not applied within area selectors (patch by AlfonZ)

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.