All buildings are now rendered as GeoDataBuildings
Details
Diff Detail
- Repository
- R34 Marble
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Loks good on quick sight, though I would like a unit test for the building height extraction method as discussed.
Regarding the node reduction, how did you test this? I am missing changes in the osm plugin to ensure buildings are written to osm xml and o5m files. I guess that at the moment tools like kml2kml and likewise the vector tile creation tool do not write buildings anymore. Can you look into it?
@rahn Can you review this as well? I'd like to have more eyes on it as it touches some pretty sensitive areas of the vector tiling pipeline.
src/lib/marble/geodata/data/GeoDataBuilding.cpp | ||
---|---|---|
109 | Wouldn't hurt to also have Q_ASSERT(d->m_multiGeometry.size() == 1); | |
149 | I think this violates "The default unit is meters", see https://wiki.openstreetmap.org/wiki/Key:height#Height_of_buildings | |
175 | Please use IN2M (from MarbleGlobal.h) instead of 0.0254 | |
src/lib/marble/geodata/graphicsitem/AbstractGeoPolygonGraphicsItem.cpp | ||
228 | Do we need to care (e.g. Q_ASSERT on) the value of m_polygon, m_building here? Same in setPolygon. | |
tests/TestGeoDataBuilding.cpp | ||
68 | Can we have a case where no unit is specified, too? |
src/lib/marble/geodata/data/GeoDataBuilding.cpp | ||
---|---|---|
171 | See Dennis' comment below: MarbleGlobal.h also features FT2M to convert feet to meters. | |
src/lib/marble/geodata/data/GeoDataBuilding_p.h | ||
26 | not needed | |
src/lib/marble/geodata/graphicsitem/AbstractGeoPolygonGraphicsItem.h | ||
32 | Should these really be marked explicit? Although C++11 allows for implicit conversion for more than a single parameter that case isn't practically relevant I guess. | |
src/lib/marble/geodata/graphicsitem/BuildingGraphicsItem.h | ||
25–26 | see above. |
Looks good to me. Please do another thorough test before pushing as discussed in IRC.