[Patch] Add validate rule min_height < height

[hubaishan]

If element has min_height tag so it must have height tag and height > min_height

[skyper]

There is a collision in the use of min_height=* regarding attractions like roller-coasters, see ​OSM-wiki. Barriers could have a min_height=* without height=*, too. Do we get together some white or black list regarding primary tags?

[hubaishan]

So the rule may use building=* or building:part=* as condition

[ivanbranco]

Hi, I posted a issue on the Osmose GitHub and they redirect me here: ​https://github.com/osm-fr/osmose-backend/issues/1842

In addition to this check I would like to propose to flag building:levels<building:min_levels as well.

[skyper]

One question: Are equal values for building:levels and building:min_level ok or not?

[skyper]

patch

[skyper]

Please, find attached patch​ which adds warnings for min_height without height resp. building:min_level without building:levels and in case the min_* tag's value is not lower. All only for areas and building=* plus building:part=*, so far.

I did not test it on real data.

[taylor.smock]

Replying to skyper:

One question: Are equal values for building:levels and building:min_level ok or not?

It looks like not. See ​https://wiki.openstreetmap.org/w/images/3/3a/Min_level_2.svg

In other words, if building:levels === building:min_level, then there is no actual building part there. Maybe just a building:part=roof?

I assume that we are only looking for building and building:part, not not:building or not:building:part. If so, why isn't the regex /^(building|building:part)$/?

[skyper]

patch version 2: regex instead of multiple lines

[skyper]

Replying to taylor.smock:

I assume that we are only looking for building and building:part, not not:building or not:building:part. If so, why isn't the regex /^(building|building:part)$/?

I was not aware that "positive" regex work with placeholders. New patch is two lines shorter now.

[taylor.smock]

Replying to skyper:

I was not aware that "positive" regex work with placeholders. New patch is two lines shorter now.

That isn't exactly what I meant, but it does seem to work. Here is what I was thinking:

- area[min_height                ][!height][/(building|building:part)/], /* #21286 */
+ area[min_height                ][!height][/^(building|building:part)$/], /* #21286 */

The advantage of ^(building|building:part)$ is we don't have to check not:building (and it won't match building:levels). The regexp parser will check the n and go "nope, the first character is not b, this does not match". We might be able to "optimize" the regex a bit, by using ^building(|:part)$, but I don't think that is necessary.
One caveat for the tag(height) <= tag(min_height): this will only work for where the user has mapped the height and min_height using metric units.

[skyper]

Replying to taylor.smock:

Replying to skyper:

I was not aware that "positive" regex work with placeholders. New patch is two lines shorter now.

That isn't exactly what I meant, but it does seem to work. Here is what I was thinking:

- area[min_height                ][!height][/(building|building:part)/], /* #21286 */
+ area[min_height                ][!height][/^(building|building:part)$/], /* #21286 */

The advantage of ^(building|building:part)$ is we don't have to check not:building (and it won't match building:levels). The regexp parser will check the n and go "nope, the first character is not b, this does not match". We might be able to "optimize" the regex a bit, by using ^building(|:part)$, but I don't think that is necessary.

Oh, I see. Now, I better understand why I had problems with regex in the past as I never realized that it describes only substrings. Thought I need [/.*(building|building:part).*/] to work like that.

One caveat for the tag(height) <= tag(min_height): this will only work for where the user has mapped the height and min_height using metric units.

Good spot. Guess I can only compare metric units or even only values in meters. Else I would need to calculate comparable values first which seems to be quite more work. I tried following but it does not work and assertMatch is no help:

area[/^(building|building:part)$/][height =~ /^[0-9]+(\.[0-9]+)?(( )(metre|metres|meter|meters|Metre|Metres|Meter|Meters)|m)?$/][min_height =~ /^[0-9]+(\.[0-9]+)?(( )(metre|metres|meter|meters|Metre|Metres|Meter|Meters)|m)?$/][get(split(" ", tag(height)), 0) <= get(split(" ", tag(min_height)), 0)],
area[/^(building|building:part)$/][building:levels][building:min_level][tag("building:levels") <= tag("building:min_level")] {
  throwWarning: tr("{0} is lower or equal to {1} on {2}", "{1.key}", "{2.key}", "{0.key}");
  group: tr("suspicious tag combination");
  assertMatch:   "area buiding=yes height=10 min_height=\"10 m\"";
  assertNoMatch: "area buiding=yes height=10 min_height=\"9 m\"";
}

[taylor.smock]

Replying to skyper:

Oh, I see. Now, I better understand why I had problems with regex in the past as I never realized that it describes only substrings. Thought I need [/.*(building|building:part).*/] to work like that.

There are various regex pitfalls. You can use ​https://regex101.com/ (select the Java 8 flavor) to test regexes in the future. Big thing: ^ matches the start of a string and $ matches the end of the line, so it is constraining the regex to match either building or building:part and nothing else.

Good spot. Guess I can only compare metric units or even only values in meters. Else I would need to calculate comparable values first which seems to be quite more work. I tried following but it does not work and assertMatch is no help:

There was a ticket about adding support for converting units, but stoecker killed it IIRC. I cannot find that ticket anymore though. :(

EDIT: To clarify, I don't know what we can do besides only work with meters, right now anyway. I would note that I would follow the wiki specification for units. So meters would just be m, feet would be ", yards would be '. So the regex would look like ^[0-9]+(\.[0-9]+)?( m)?$.

[skyper]

patch version 3: use complete strings instead of substrings in regex plus only compare values without unit plus " m" or " ft"

[skyper]

Thanks, got it working, see josm_21286_v3.patch​

[skyper]

I took the chance and have added the common numerical test for min_height in josm_21286_v4.patch​

[skyper]

patch version 4: adds numeric test for min_height

[taylor.smock]

We should be following the ​osmwiki:Map_features/Units specifications. I'll upload your patch, modified, to show what I think it should be.

[taylor.smock]

It looks like we are adding ft in some locations. I'm surprised we haven't heard any complaints from users, since we aren't following the documented specification.

[taylor.smock]

Use ' instead of ft

[Famlam]

@taylor.smock

It looks like we are adding ft in some locations. I'm surprised we haven't heard any complaints from users, since we aren't following the documented specification.
Use ' instead of ft

Regarding the changed feet/inches tests:
The warning:
tr("unusual value of {0}: use abbreviation for unit and space between value and unit", "{0.key}");
is very odd to see for
maxheight=22 ' (note the space between 22 and ', which is actually undesired).

Maybe the message for the feet/inch-tests should be revised? (It's slightly offtopic for this issue, so I can also file a followup report if you prefer)

[taylor.smock]

Replying to Famlam:

Maybe the message for the feet/inch-tests should be revised? (It's slightly offtopic for this issue, so I can also file a followup report if you prefer)

Probably. I don't know what a good, clear, message would be. I don't think unusual value of {0}: use appropriate unit syntax" will cut it.

[Famlam]

How about:
tr("unusual value of {0}: use single/double quote symbols for the imperial unit and no space between value and unit", "{0.key}");
?

[skyper]

Damn, I just thought it would be smart to add the value test for min_height but instead stepped into a honey pot. Now, we should add a rule with autofix for space between value and the inch symbol " and the use of two single quotes instead of a double quote.
@Famlam's proposed message looks fine to me but how to generalize the message of the last rule tr("unusual value of {0}: {1} is default; point is decimal separator; if units, put space then unit", "{0.key}", tr("meters"))

[Famlam]

Now, we should add a rule with autofix for space between value and the inch symbol " and the use of two single quotes instead of a double quote.

Well, the issue itself has been addressed with the current patch. Making the inch-rules perfect could be a separate issue?

@Famlam's proposed message looks fine to me but how to generalize the message of the last rule tr("unusual value of {0}: {1} is default; point is decimal separator; if units, put space then unit", "{0.key}", tr("meters"))

tr("unusual value of {0}: {1} is default; point is decimal separator; if units, use a space before the unit or use single/double quote symbols for imperial units", "{0.key}", tr("meters"))

Bit long, but the other option would probably to split it in two rules.
Or we should ignore imperial in the message for this rule, there's only about 30 min_height values using imperial A'B" worldwide ;)

[taylor.smock]

I'll go ahead and apply a patch that only deals with metric units.

[taylor.smock]

In 18731/josm:

Fix #21286: Add validator rule for min_height < height (patch by skyper, modified)

This additionally fixes an error introduced in r18730.

[stoecker]

Why the tr("meters")? That makes no sense and harder to translate.

[skyper]

Replying to taylor.smock:

I'll go ahead and apply a patch that only deals with metric units.

Now, we get strange results as we only warn about ft with min_height and e.g. not with height and the message for the general warning without fix is still "unusual value of {0}: {1} is default; point is decimal separator; if units, put space then unit", "{0.key}", tr("meters"). We do not fix the comma as decimal separator with ft, as-well.

Replying to stoecker:

Why the tr("meters")? That makes no sense and harder to translate.

It is a copy of the warning for height. I get your point, thought, there are similar uses of placeholder for units in the file. Looks like the file needs some rework besides the problems with the correct unit symbol for foot.

[taylor.smock]

We really need to update the numeric.mapcss file to not recommend the aliases for units (of which ft is one).

What we should probably do is the following:

1. Have alias replacement checks first (with autofixes, so /^(?i)(metres?|meters?)$/ would become m, and so on).
2. Then do the , to . replacements (so /^[0-9]*,[0-9]*( m)?$/).
3. Convert to the default unit (or another arbitrary unit, if there is no default, like with power).

   *[height =~ /^[0-9]*\.[0-9]*( m)?$/] {
       height: get(split(" ", tag("height")), 0);
   }
   *[height =~/^([1-9][0-9]*\'((10|11|[0-9])((\.[0-9]+)?)\")?)$/] {
       // Might be able to reuse regex above via {0.value}
       _tmp: regexp_match("^([1-9][0-9]*\'((10|11|[0-9])((\.[0-9]+)?)\")?)$", tag("height"));
       height: eval((get(prop(_tmp), 1) + 12 * get(prop(_tmp), 2)) * 0.0254);
   }
   

4. Start doing the actual comparisons (like checking for min_height < height using prop and is_prop_set).

Why the tr("meters")?

Do you have a better suggestion? We don't translate meter anywhere in source, and it was (as skyper mentioned) used in a previous warning message.

[stoecker]

In a text like this simply move it inline (replace {1} with meters).

I know that programmers tend to "optimize" such situations, but for i18n sometimes duplication with less complexity is better.

[taylor.smock]

That is totally fair.

I just finished going through the numeric.mapcss unit checks, and realized that fixAdd: concat("{0.key}", "=", get(regexp_match("([0-9.]+) *.+", "{0.key}"), 1), " m"); doesn't work. :(

It looks like it is unsupported, see wikipedia.mapcss:

  /* fixAdd: concat("{0.key}", "=", get(regexp_match("(?i)^([-a-z]+:)(.*)$", tag("{0.key}")),1), trim(replace(URL_decode(get(regexp_match("(?i)^([-a-z]+:)(.+)$", tag("{0.key}")),2)), "_", " "))); */
    /* tag("{0.key}") is not yet supported */

In order to fix this, there are a few interfaces that need to change (mostly to pass a Selector through -- we need the information to get the right key/value/tag).

Do we know if there are any users of our mapcss implementation?

[Famlam]

Correct, the {N.key/tag/value} blocks are unfortunately only replaced by the key/value/tag after all the functions have already been called. I recently gave the following example @Osmose:

the hypothetical fixAdd: concat("x=", replace("{0.value}", "value", "key")); on way a=b would give x=a as output, rather than x=b.

I'd love to see this sequence being changed, so you can actually do things with the functions on the keys/values/tags without hardcoding them.

[taylor.smock]

I was just looking at that. I think I can just add it as a field to the Environment and check and see if it exists. I've already got a special case record for anything with a {i.tag|key|value}, and I just need to figure out where the environment variable is being set for the various code paths.

So it might change soon. I'll have to open a separate ticket for it though. Unless I find a ticket that someone else already opened.

[Famlam]

Unless I find a ticket that someone else already opened.

You can even choose out of two, although they seem to also want to change the output of {0.value} (etc) to no longer provide the regex (for a regex match) but the actual value. That's another step on top of this.
#17669, #22096
Not dedicated for regexp_match nor replace, but I guess it's one and the same issue

[taylor.smock]

I'm going to go ahead and use #17669.

I'm attaching the changes I made to numeric.mapcss to that patch, since those changes requires that the placeholder replacements work.

Unless I find a ticket that someone else already opened.

Well, I guess I already opened a separate ticket for this. I just forgot about it...

[osm@…]

I'm coming here, because Osmose is now enforcing your check that building:min_level must be < building:levels.
That check is wrong, because building:levels only counts non-roof:levels, so a roof-only part of a building has building:min_level == building:levels and if you want to skip the first roof-floor, it's even higher.
The height-checks are fine, because building:height is the total height, including the roof:height, but building:levels does not include roof:levels.

[Nadjita]

If you really want a check, then building:min_level < building:levels+roof:levels

Again, building:levels does not contain roof:levels, so you cannot compare it to height, which includes roof:height.

[Metzor]

I agree with Nadjita.

Without considering roof:levels, there is, for instance, a lot of false-positives for 3D roof modeling. Simple3D documentation advises to avoid overlapping faces as much as possible. For (intentionaly) overlapping roof:shape overlap of lower levels can be avoided by setting building:min_level to building:levels. That's one of the readons why I'd appreciate, if someone implements Nadjita's suggestion.

Fun fact: ​There is buildings with zero building:levels.

[skyper]

Would excluding building=roof and building:part=roof work? Then roofs could be treated in another rule.
I would really appreciate if someone could document the special cases for roofs on the OSM wiki pages.

[Nadjita]

If you are describing a building part, then you use the function. So an office at roof-level is building:part=office, no matter if it's a roof or not. You only use roof, if you don't have anything else to put in there, much like building:part=yes. Can we please just get rid of this nonsensical check? Screwing things up and then wanting to change the wiki to make it right seems like a bad idea…

[gaben]

Could you please add also roof:levels to the ignoretags?