[PATCH] mapcss - extended functionality for instruction text: ...

[Gubaer]

This patch still supports the semantics of the instruction text: ... as specified on the ​Wiki and as currently implemented by JOSM:

• text: auto - indicates that the text to be rendered is supplied by one of the name tags
• text: a_tag_name - indicates that the text to be rendered is supplied by the tag "a_tag_name"

In addition the patch addes the following functionality:

• text: text("a static text") - render a static text
• text: tag("a_tag_name") - render the value of a tag
• text: parent_tag("a_tag_name") - render the value of a tag of one of the parent relations
• text: eval(....) - any eval'ed expression

As soon as the patch is applied I'm going to update the JOSMs MapCSS documentation.

[Gubaer]

patch

[anonymous]

"auto" does not work (try Potlatch2 style)

[bastiK]

this was me, damn trac logs me out...

[Gubaer]

Strange, in my environment it does, see the attached screenshot

The supplied test data is loaded. There is only one node with a name=... tag. Since with potlatch style only the label of this node is drawn, this looks OK.

[Gubaer]

Shows applied potlatch style. Works with auto style.

[bastiK]

In combination with an icon (try place=suburb)

[bastiK]

It seems, your patch no longer uses TagLookupCompositionStrategy for text: "name"; but treats it somewhat like an eval expression.

This is a little unfortunate because it breaks the flyweight concept for StyleCache.java. Two primitive styles now have a different static text and as a consequence can no longer share a common style object. You can investigate this, by selecting 2 entries in the selection dialog, then right click > inspect. It will say "The 2 selected Objects have identical style caches" (or the converse) in the 2nd tab.

I have to admit, that the problem is more on a theoretical / conceptual level, but there are also measurable consequences. When I load a 30 MB osm file (urban area), with the following style, your patch increases memory use by 2 MB. This is about 5% of JOSM's total memory use.

node {
    symbol-shape: square;
    text: "name";
    font-size: 20;
}

way {
    width: 3;
    color: lime;
    text: "name";
    text-position: line;
    font-color: lightyellow;
    font-size: 23;
}

[Gubaer]

Updated patch

[Gubaer]

Updated patch

[Gubaer]

Updated test data

[Gubaer]

Please ignore mapcss-patch.txt - Uploaded the wrong file.

[Gubaer]

This version of the patch again uses TagLookupCompositionStrategy. If style autors use the instruction text: a_tag_name this helps to save memory. If, however, they use the more verbose form text: tag("a_tag_name") or the new expression text: parent_tag("a_tag_name")) memory is still "wasted" on style objects.

My impression is, that the MapCSS implementation currently has to many conversions between various forms of expression descriptors.

• the parsers finds an expression in an Instruction, i.e. tag("a_tag_name")
• for this we store another kind of "expression" in the Cascade, an instance of TagKeyReference or an IconReference
• when we create the text style, we convert this in yet another kind of "expression placeholder", a TagLookupCompositionStrategy

If Expression and its subclasses had a hashCode/equals implementation, they could directly be stored in Cascades and in text styles and we would not have to convert between various forms of "expressions".

In combination with an icon (try place=suburb)

This should work now, see the updated test data.

[bastiK]

Replying to Gubaer:

This version of the patch again uses TagLookupCompositionStrategy. If style autors use the instruction text: a_tag_name this helps to save memory. If, however, they use the more verbose form text: tag("a_tag_name") or the new expression text: parent_tag("a_tag_name")) memory is still "wasted" on style objects.

My impression is, that the MapCSS implementation currently has to many conversions between various forms of expression descriptors.

Can you elaborate?

• the parsers finds an expression in an Instruction, i.e. tag("a_tag_name")
• for this we store another kind of "expression" in the Cascade, an instance of TagKeyReference or an IconReference

I don't consider IconReference an expression. It is a hack to remember where a certain image url comes from, so we can search paths relative to the mapcss file. I'm not too happy with it, but don't see an alternative.

• when we create the text style, we convert this in yet another kind of "expression placeholder", a TagLookupCompositionStrategy

The style contains the data as much processed as possible. E.g. BasicStroke and Font objects are already constructed and ready to be used for paining.

If Expression and its subclasses had a hashCode/equals implementation, they could directly be stored in Cascades and in text styles and we would not have to convert between various forms of "expressions".

Expressions have to be evaluated in a certain context, how would that work if you save the Expressions unprocessed in the Cascade?

*::* {
  myWidth: 3;
}
way[highway] {
  width: prop("myWidth")+1;
}
*::* {
  myWidth: 5;
}
...

With the eval expressions, we have a (very limited) imperative language with dynamic typing. You can write (for whatever reason)

  dummy: "name";
  width: length(prop("dummy"));

but

  text: "name";
  width: length(prop("text"));

will have unexpected result, because the internal data structure is exposed to the user (TagKeyReference.toString()). This is the same for icon-image.

[Gubaer]

Can you elaborate?

No. It was just an "impression", not well though throug. In addition, my memory misled me. I though, that we have an Environemnt in drawNodeText when labelCompositionStrategy is executed, but we haven't.

I don't consider IconReference an expression. It is a hack to remember where a certain image url comes from,
so we can search paths relative to the mapcss file. I'm not too happy with it, but don't see an alternative.

Yes, I saw that. I'm aware, it isn't an expression, but I had the impression it was something "close" to an expression. And it came handy to me. As you already saw in the patch, I followed the concept. I bascially mimicked it by adding TagKeyReference.

How is memory consumption with your 30MB test file? I didn't measure myself, but I guess that memory has dropped to what it was before the patch.

[bastiK]

In [4007/josm]:

see #6150 mapcss - extended functionality for instruction text: ... (patch by Gubaer)

[bastiK]

• left out text() function in expression. We already have one NOP operator (eval), why add another alias?
• the parent_tag() function should work like this:

  relation[type=route][route=bicycle][network][name] > way {
     text: eval(concat(parent_tag("network"), " - ", parent_tag("name")));
  }
  

  I.e. do not pick the name of a random parent, but the one that was chosen in the parent selector chain. I checked your code in, but please do not document the current provisional behaviour.

You can close if you consider this done.

[Gubaer]

I.e. do not pick the name of a random parent, but the one that was chosen in the parent selector chain.

Agree, this is important.

I don't close this ticket yet, I'll just update its subject.

[Gubaer]

The new patch should improve the parent_tag(...) function. After all selectors of a rule have been applied and before the instructions are executed, the matching referrer objects are remembered in the environement. parent_tag(...) now has access to these objects when it executes.

[bastiK]

Replying to Gubaer:

The new patch should improve the parent_tag(...) function. After all selectors of a rule have been applied and before the instructions are executed, the matching referrer objects are remembered in the environement. parent_tag(...) now has access to these objects when it executes.

Why do you save all matching referrers and not just the first one? I'm not saying it is wrong per se, but like to understand your reasoning.
Keep in mind that the following is not yet implemented

relation[...] > way > node {}

but it will surely come. Then you'd have a more complicated structure and I'm concerned about consistency: E.g. you could refer to parent_tag() in one instruction and to grandparent_tag() in the next instruction but it isn't clear that 'grandparent' is parent of 'parent'.
In the current setup (with only two selectors), it would be theoretically possible, that two instructions in the same declaration block refer to different parents, which is strange.

Also, I wouldn't loop over the referrers (and match them against Selectors) twice, the environment can be changed as a side effect of Selector.applies(). (Then this method should probably be renamed.)

This is not only for efficiency, but in future extension I could imagine some form of instruction inside a selector. It would be wrong to execute such an instructions twice.

In Expression.parent_tag() you restrict the parents to relation class, this is probably not necessary.

[Gubaer]

Why do you save all matching referrers and not just the first one?

I was thinking ahead. What if in the future somebody needs another feature, like parent_count() which would allow for expressions like eval(cond(parent_count() == 1, "One parent", "More than one parent"))? I know one shouldn't development software for imaginary requirements, but I couldn't resist ...

Keep in mind that the following is not yet implemented

Yes, I realized that and if it was introduced, it would have a large impact on the current MapCSS implementation, both on the grammar and classes like DescendentSelector. I'm sure it will come and I know about people who are eagerly waiting for it, but it was better to stick to what we have now instead of anticipating something which we don't know yet, how it will actually look like.

Also, I wouldn't loop over the referrers (and match them against Selectors) twice,

Do you mean, once in getMatchingReferrers(..) and once in parent_tag(...) ? Then, I don't see the problem, because they are only once matched against Selectors: in getMatchingReferrers(...).

In Expression.parent_tag() you restrict the parents to relation class, this is probably not necessary.

Agree. If I remove this, the following style rule makes sense too:

way > node[my_type=special] {
    text: eval(parent_tag("my_special_tag_on_a_way"))
}

[Gubaer]

Updated patch

[Gubaer]

Updated style sheet with test styles

[Gubaer]

Updated test data

[bastiK]

ideas for improvement, based on the patch by Gubaer

[bastiK]

Added a patch that demonstrates my ideas, because this is faster than words. :)
Does that make sense to you? (It is untested btw.)

Replying to Gubaer:

Why do you save all matching referrers and not just the first one?

I was thinking ahead. What if in the future somebody needs another feature, like parent_count() which would allow for expressions like eval(cond(parent_count() == 1, "One parent", "More than one parent"))? I know one shouldn't development software for imaginary requirements, but I couldn't resist ...

OK, why not - but still parent_tag() can refer to different primitives within the same expression. This can be fixed by only looking at the first entry from env.matchingReferrers in parent_tag() method.

Keep in mind that the following is not yet implemented

Yes, I realized that and if it was introduced, it would have a large impact on the current MapCSS implementation, both on the grammar and classes like DescendentSelector. I'm sure it will come and I know about people who are eagerly waiting for it, but it was better to stick to what we have now instead of anticipating something which we don't know yet, how it will actually look like.

Also, I wouldn't loop over the referrers (and match them against Selectors) twice,

Do you mean, once in getMatchingReferrers(..) and once in parent_tag(...) ? Then, I don't see the problem, because they are only once matched against Selectors: in getMatchingReferrers(...).

No, getMatchingReferrers() and Selector.applies(). (see patch)

[Gubaer]

Does that make sense to you? (It is untested btw.)

Yes. It looks fine. In addition I run it against my set of standard tests and they look OK, too.

[bastiK]

In [4011/josm]:

applied #6150 - mapcss - improve parent_tag (based on patch by Gubaer)

[bastiK]

Is the test data for the repository?

[Gubaer]

Do you mean whether it should be checked in, too? No, I think this isn't necessary. I only created it in order to demonstrate what the patch is supposed to do and in order to have some kind of smoke test for various versions of the patch.