All buildings are now rendered as GeoDataBuildings
Details
Diff Detail
- Repository
- R34 Marble
- Lint
Lint Skipped - Unit
Unit Tests Skipped
src/lib/marble/geodata/data/GeoDataBuilding.h | ||
---|---|---|
18 ↗ | (On Diff #15654) | Should not be in the header. Moving the NamedEntry enum out of the private class should work around it. |
120 ↗ | (On Diff #15654) | No need to return a reference (otherwise should not be const) |
src/plugins/runner/osm/OsmWay.h | ||
15 ↗ | (On Diff #15654) | No private header inclusion here as well |
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 ↗ | (On Diff #15654) | Wouldn't hurt to also have Q_ASSERT(d->m_multiGeometry.size() == 1); |
149 ↗ | (On Diff #15654) | I think this violates "The default unit is meters", see https://wiki.openstreetmap.org/wiki/Key:height#Height_of_buildings |
175 ↗ | (On Diff #15654) | Please use IN2M (from MarbleGlobal.h) instead of 0.0254 |
src/lib/marble/geodata/graphicsitem/AbstractGeoPolygonGraphicsItem.cpp | ||
228 ↗ | (On Diff #15654) | 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 ↗ | (On Diff #16029) | Can we have a case where no unit is specified, too? |
src/lib/marble/geodata/data/GeoDataBuilding.cpp | ||
---|---|---|
171 ↗ | (On Diff #15654) | See Dennis' comment below: MarbleGlobal.h also features FT2M to convert feet to meters. |
src/lib/marble/geodata/data/GeoDataBuilding_p.h | ||
26 ↗ | (On Diff #15654) | not needed |
src/lib/marble/geodata/graphicsitem/AbstractGeoPolygonGraphicsItem.h | ||
33 ↗ | (On Diff #15654) | 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 ↗ | (On Diff #15654) | see above. |
Looks good to me. Please do another thorough test before pushing as discussed in IRC.