Inconsistent unit conversation of kerb heights in WheelchairGraphStorageBuilder

[jarinox]

The following method is currently responsible for computing the kerb height in cm given an OSM tag value.

openrouteservice/ors-engine/src/main/java/org/heigit/ors/routing/graphhopper/extensions/storages/builders/WheelchairGraphStorageBuilder.java

Lines 692 to 722 in 27dfb52

	/**
	* Converts a kerb height value to a numerical height (in centimetres). A kerb could be stored as an explicit height or
	* as an indicator as to whether the kerb is lowered or not.
	*
	* @param value The value of the tag
	* @return The presumed height of the kerb in metres
	*/
	private int convertKerbTagValueToCentimetres(String value) {
	int centimetreHeight = -1;
	if (value == null) {
	return -1;
	}
	switch (value) {
	case "yes", KEY_BOTH, "low", "lowered", "dropped", "sloped" -> centimetreHeight = 3;
	case "no", "none", "one", "rolled", "regular" -> centimetreHeight = 15;
	case "at_grade", "flush" -> centimetreHeight = 0;
	default -> {
	double metresHeight = UnitsConverter.convertOSMDistanceTagToMeters(value);
	// If no unit was given in the tag, the value might be in meters or centimeters; we can only guess
	// depending on the value
	if (metresHeight < 0.15) {
	centimetreHeight = (int) (metresHeight * 100);
	} else {
	centimetreHeight = (int) metresHeight;
	}
	}
	}
	return centimetreHeight;
	}

However this method has some conflicting behavior:

• The docstring states, kerb height is returned in meters, the method name convertKerbTagValueToCentimetres says it is returned in centimeters
• The kerb height in meters is correctly computed using the method UnitsConverter.convertOSMDistanceTagToMeters(value). However some weird logic then only multiplies this value by 100, if the metersHeight is less than the arbitray value of 0.15. This is explained by the comment

  // If no unit was given in the tag, the value might be in meters or centimeters; we can only guess
  // depending on the value
  

  The comment however does not make sense. The previously utilized method UnitsConverter.convertOSMDistanceTagToMeters(value) does in fact already check whether a unit suffix (e.g. 0.2 m) is present. The OSM wiki states that the default unit is meters if no unit is explicitly set. This is also the behavior of UnitsConverter.convertOSMDistanceTagToMeters(value). Even if we were to assume different units given the value, this should be implemented in UnitsConverter.convertOSMDistanceTagToMeters(value) where units are read and also the threshold should be chosen more carefully. I don't see, why someone would set a kerb height as small as 0.15 cm in OSM, while a kerb height of 16cm should be pretty reasonable.

During the migration of the WheelchariGraphStorageBuilder, the following new test failed due to this behavior

    void TestUseWorstKerbHeightTag() {
        ReaderWay way = new ReaderWay(1);

        way.setTag("footway", "crossing");
        way.setTag("kerb:height", "0.03");
        addNodeTag(way, "curb", "0.2 m", 1);

        executeParsers(way);

        WheelchairAttributesEncodedValues encValues = new WheelchairAttributesEncodedValues(em);
        WheelchairAttributes attrs = encValues.getAttributes(intsRef);

        assertEquals(20, attrs.getSlopedKerbHeight());
    }

In this test case, UnitsConverter.convertOSMDistanceTagToMeters(value) correctly returns 0.2 for the 0.2 m tag value, however as it is less than 0.15, the current logic assumes it is given in cm already and therefore directly converts it to an integer (to 0) without applying the conversation to cm.