School Building heights are being incorrectly parsed/inferred

[piemadd]

Two locations where this can be seen: (many more examples)
McHenry, IL: https://betamap.transitstat.us/#12.07/42.35508/-88.26705/33.6/60
North Brunswick, NJ: https://betamap.transitstat.us/#11.21/40.4781/-74.403/58.6/60

Screenshots

Required information

• Browser: Firefox 121.0.1 on Windows 10 21H2
• Renderer: Maplibre-gl-js 3.6.2
• Tileset version: 20240105.pmtiles from https://maps.protomaps.com/builds/
• Map style (just the layers): https://github.com/transitstatus/transitstatus-map/blob/main/src/assets/mapLayers.json

Code I am unable to test a possible fix (hence why this an issue and not a pull request), but the issue seems to be coming from these lines: https://github.com/protomaps/basemaps/blob/166e1d7b185d9628cb257bf2d50a44473f3bfc26/tiles/src/main/java/com/protomaps/basemap/layers/Buildings.java#L49-L54

When the program tries inferring the height of a building, it uses the building:levels property, but for schools this is used to show what grade levels participate in said building. Using North Brunswick Township High School (osm id 544332434, pmtiles id 35184916421266), we can see the building:levels value is 9,10,11,12, interpreted as 9,101,112. After being multiplied by 3 and getting 2 added, we get the stored height value of 27303338. The solution here seems to be adjusting the code to something like this:

if (height == null) {
  Double levels = parseDoubleOrNull(sf.getString("building:levels"));
  if (levels != null && !sf.getString("building").equals("school")) { //no more funny super tall schools :c
    height = Math.max(levels, 1) * 3 + 2;
  }
}

If you want me to submit a PR, I'm more than happy to, just I'm personally unable to really test this repo due to my lack of hardware on hand. Thanks!

[bdon]

That seems like an improper use of the building:levels tag - would fixing the tags in OSM cover all the cases you've found? Once changed on OSM it should make it into the next daily build.

In any case, we should add a more conservative check that the interpreted height from levels is a reasonable value. Thanks for the detailed report!

[piemadd]

Admittedly it never crossed my mind that OSM could be at fault here. I'll go ahead and make those edits, whoopsies.

[bdon]

Added stricter parsing in 3.2.0 that heights match X.Y but https://wiki.openstreetmap.org/wiki/Key:height suggests we need to take units into account.