
According to osm wiki [1] natural=marsh is deprecated and intended to be substituted by natural=wetland + wetland=*. I think we should adapt default polygons style accordingly (see attached patch). Regards, Carlos [1] http://wiki.openstreetmap.org/wiki/Map_Features#Natural Index: resources/styles/default/polygons =================================================================== --- resources/styles/default/polygons (revisión: 1480) +++ resources/styles/default/polygons (copia de trabajo) @@ -61,6 +61,7 @@ military=range [0x04 resolution 18] natural=glacier [0x4d resolution 16] +natural=wetland [0x51 resolution 20] natural=marsh [0x51 resolution 20] natural=mud [0x51 resolution 20] natural=scrub [0x4f resolution 20]

On Sat, Jan 16, 2010 at 12:25:35PM +0100, Carlos Dávila wrote:
According to osm wiki [1] natural=marsh is deprecated and intended to be substituted by natural=wetland + wetland=*. I think we should adapt default polygons style accordingly (see attached patch). Regards, Carlos
Committed in revision 1481. For what it is worth, I was wondering if it would be faster to execute the style as natural ~ 'marsh\|mud\|wetland' [0x51 resolution 20] but mkgmap did not like this: Error in style: Error: (polygons:64): Invalid operation '' at top level Why not? A regexp match with one hash lookup ought to be faster than three hash lookups and string equality comparisons. Best regards, Marko

On 16/01/10 11:48, Marko Mäkelä wrote:
For what it is worth, I was wondering if it would be faster to execute the style as
natural ~ 'marsh\|mud\|wetland' [0x51 resolution 20]
Why not? A regexp match with one hash lookup ought to be faster than three hash lookups and string equality comparisons.
It doesn't work like that, so we don't do multiple hash lookups and equality comparisons for each rule. We go through the tags and look each one up, based on an index consisting of the tag and value. So all rules that could be matched by say natural=mud are indexed by the string 'natural=mud' So if an element has the tag natural=mud then we find the [0x51 ...] directly without looking at natural=marsh or natural=wetland or doing any other comparison. If you have a regexp then you have to run it for every natural=* tag so it becomes natural=* & natural ~ 'marsh\|mud\|wetland' [0x51 resolution 20] So this involves a hash table lookup and a regexp comparison instead of just a hash table lookup and so it will be slower. ..Steve

Hi Steve,
It doesn't work like that, so we don't do multiple hash lookups and equality comparisons for each rule.
We go through the tags and look each one up, based on an index consisting of the tag and value. So all rules that could be matched by say natural=mud are indexed by the string 'natural=mud'
So if an element has the tag natural=mud then we find the [0x51 ...] directly without looking at natural=marsh or natural=wetland or doing any other comparison.
Thank you for clarifying that. I have some more questions: 1. What if there are multiple actions for natural=mud? Which one(s) will be processed and in which order? 2. How and when are top-level AND expressions evaluated? 3. Why doesn't RuleFileReader.optimiseAndSaveBinaryOp() check for second.isType(EXISTS)? Why doesn't it attempt to swap operands when first.isType(NOT_EXISTS)? I committed a non-functional change to RuleFileReader, making optimiseAndSaveBinaryOp take a BinaryOp parameter. Marko

On 17/01/10 20:06, Marko Mäkelä wrote: Hi Marko
1. What if there are multiple actions for natural=mud? Which one(s) will be processed and in which order?
If there are multiple rules with the first term natural=mud then they are put into a list in the order that they occur in the file. They are then evaluated in order until one matches. Only the conditions after natural=mud are run, eg for the rules: natural=mud & colour=brown [0x50 ...] natural=mud [0x51 ...] you create a list of instructions that does the following: if colour == brown then return [0x50, ...] return [0x51, ...]
2. How and when are top-level AND expressions evaluated?
I think the above example shows how the first AND is evaluated: first you look up the program list via natural=mud and then you evaluate colour=brown. The second and subsequent ANDs are handled normally by the AndOp class.
3. Why doesn't RuleFileReader.optimiseAndSaveBinaryOp() check for second.isType(EXISTS)? Why doesn't it attempt to swap operands when first.isType(NOT_EXISTS)? No particular reason other than I am never tempted to start a rule with !=.
So yes you could rewrite (eg): a!=b & c=* as c=* & a!=b that would be valid. ..Steve

Hi Steve,
If there are multiple rules with the first term natural=mud then they are put into a list in the order that they occur in the file. They are then evaluated in order until one matches. Only the conditions after natural=mud are run, eg for the rules:
natural=mud & colour=brown [0x50 ...] natural=mud [0x51 ...]
you create a list of instructions that does the following: if colour == brown then return [0x50, ...] return [0x51, ...]
I see. I guess that only the first matching [] rule will be run and that any {set} or {add} rules will not affect further tests.
3. Why doesn't RuleFileReader.optimiseAndSaveBinaryOp() check for second.isType(EXISTS)? Why doesn't it attempt to swap operands when first.isType(NOT_EXISTS)? No particular reason other than I am never tempted to start a rule with !=.
So yes you could rewrite (eg): a!=b & c=* as c=* & a!=b that would be valid.
OK, I will fix that in the parser, because it makes the language cleaner in my eyes (regular, symmetric, orthogonal, for want of a better word). The same would apply to second.isType(OR), I suppose. One last question. The attached patch does not affect runtime much (similar amount of difference in execution time can be observed between repeated runs), but it affects the map generation: -rw-r--r-- 1 marko marko 45474816 18.1. 09:00 gmapsupp.img -rw-r--r-- 1 marko marko 45475840 16.1. 22:17 gmapsupp-nop.img -rw-r--r-- 1 marko marko 45475840 18.1. 09:13 gmapsupp.img The first gmapsupp.img was generated with the patch applied, the latter two without. As far as I understood your explanation, these would be executed the same, under the highway=* key. (This would not change even if I added the above-mentioned tweakings for second.isType(), because the first.isType(EXISTS) would be checked before second.isType() in any case.) Do you have any idea why the surface ~ '...\|...' would omit some definitions that are included by the surface=... | surface=...? Best regards, Marko

On 18/01/10 07:47, Marko Mäkelä wrote: Hi Marko
I see. I guess that only the first matching [] rule will be run and that any {set} or {add} rules will not affect further tests.
There is a 'continue' keyword that allows further matches to take place. The set and add actions are expected to affect further matches (as in people expect it to happen, as it is meant to work as-if the rules were applied in order), but as you will realise this is difficult to implement as you have to throw away optimisation if you detect that it could happen. The style branch implements that a bit better.
The same would apply to second.isType(OR), I suppose.
What kind of rule would that work for?
Do you have any idea why the surface ~ '...\|...' would omit some definitions that are included by the surface=... | surface=...?
The backslashes should not be there. Running the following works fine with all backslashes removed (in RuleFileReaderTest). @Test public void testRegex() { RuleSet rs = makeRuleSet("highway=* & (surface ~ 'cobblestone|compacted" + "|dirt|earth|grass(_paver)?|gravel|grit|ground" + "|mud|pebblestone|sand|unpaved') " + "[0x42]"); Way el = new Way(1); el.addTag("highway", "primary"); el.addTag("surface", "grass"); GType type = rs.resolveType(el); assertNotNull("regex matches surface=grass", type); } ..Steve

Hi Steve,
I see. I guess that only the first matching [] rule will be run and that any {set} or {add} rules will not affect further tests.
There is a 'continue' keyword that allows further matches to take place.
The 'continue' keyword is for the [] action, right? And the further matches could be terminated by another [] action that would generate another map element, right?
The set and add actions are expected to affect further matches (as in people expect it to happen, as it is meant to work as-if the rules were applied in order), but as you will realise this is difficult to implement as you have to throw away optimisation if you detect that it could happen.
The style branch implements that a bit better.
I did not realise that the style branch has not been merged to trunk yet. Are you planning to do that soon? In which way does the implementation in the style branch differ from trunk?
The same would apply to second.isType(OR), I suppose.
What kind of rule would that work for?
The same type of rules as the first.isType(OR), but with the roles of first and second swapped. I am pulling the example from the source code comment. This is a bad example, because to my understanding, it would not be handled by the isType(OR) but the earlier isType(EXISTS): // (a=b|a=c) & d=f => (a=b&d=f) | (a=c&d=f) => solved The symmetric case would be // d=f & (a=b|a=c) which would now be translated into the same. (Would it be beneficial to check for isType(OR) within isType(AND) before the isType(EQUALS) and isType(EXISTS)?)
The backslashes should not be there. Running the following works fine with all backslashes removed (in RuleFileReaderTest).
You are right, I get the same size gmapsupp.img after removing the \ before |(). Uh oh, It seems that there must be a bug in the default style then. The regexps for smoothness and sac_scale are wrong. But I thought that I successfully tested the patch (setting mkgmap:unpaved=1) with a road whose smoothness was tagged bad. Marko
participants (3)
-
Carlos Dávila
-
Marko Mäkelä
-
Steve Ratcliffe