Modify

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#6150 closed enhancement (fixed)

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

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

Description

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.

Attachments (9)

mapcss-text-instruction.patch (8.0 KB ) - added by Gubaer 13 years ago.
patch
screenshot.png (145.9 KB ) - added by Gubaer 13 years ago.
Shows applied potlatch style. Works with auto style.
mapcss-patch.txt (42.6 KB ) - added by Gubaer 13 years ago.
Updated patch
mapcss-text-instruction.2.patch (16.1 KB ) - added by Gubaer 13 years ago.
Updated patch
data-set.2.osm (1.7 KB ) - added by Gubaer 13 years ago.
Updated test data
mapcss-parent_tag-improvement.patch (9.9 KB ) - added by Gubaer 13 years ago.
Updated patch
styles.mapcss (1.2 KB ) - added by Gubaer 13 years ago.
Updated style sheet with test styles
data-set.osm (2.1 KB ) - added by Gubaer 13 years ago.
Updated test data
mapcss-parent_tag-bastiK-version.patch (6.6 KB ) - added by bastiK 13 years ago.
ideas for improvement, based on the patch by Gubaer

Download all attachments as: .zip

Change History (29)

by Gubaer, 13 years ago

patch

comment:1 by anonymous, 13 years ago

"auto" does not work (try Potlatch2 style)

comment:2 by bastiK, 13 years ago

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

comment:3 by Gubaer, 13 years ago

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.

by Gubaer, 13 years ago

Attachment: screenshot.png added

Shows applied potlatch style. Works with auto style.

comment:4 by bastiK, 13 years ago

In combination with an icon (try place=suburb)

comment:5 by bastiK, 13 years ago

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;
}

by Gubaer, 13 years ago

Attachment: mapcss-patch.txt added

Updated patch

by Gubaer, 13 years ago

Updated patch

by Gubaer, 13 years ago

Attachment: data-set.2.osm added

Updated test data

comment:6 by Gubaer, 13 years ago

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

comment:7 by Gubaer, 13 years ago

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.

in reply to:  7 ; comment:8 by bastiK, 13 years ago

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.

in reply to:  8 comment:9 by Gubaer, 13 years ago

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.

comment:10 by bastiK, 13 years ago

In [4007/josm]:

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

comment:11 by bastiK, 13 years ago

  • 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.

comment:12 by Gubaer, 13 years ago

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.

comment:13 by Gubaer, 13 years ago

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.

in reply to:  13 comment:14 by bastiK, 13 years ago

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.

comment:15 by Gubaer, 13 years ago

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"))
}

by Gubaer, 13 years ago

Updated patch

by Gubaer, 13 years ago

Attachment: styles.mapcss added

Updated style sheet with test styles

by Gubaer, 13 years ago

Attachment: data-set.osm added

Updated test data

by bastiK, 13 years ago

ideas for improvement, based on the patch by Gubaer

in reply to:  15 comment:16 by bastiK, 13 years ago

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)

comment:17 by Gubaer, 13 years ago

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.


comment:18 by bastiK, 13 years ago

Resolution: fixed
Status: newclosed

In [4011/josm]:

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

comment:19 by bastiK, 13 years ago

Is the test data for the repository?

comment:20 by Gubaer, 13 years ago

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.

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. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.