All buildings are now rendered as GeoDataBuildings
|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)
|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.
|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
|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.
|68 ↗||(On Diff #16029)|
Can we have a case where no unit is specified, too?
|171 ↗||(On Diff #15654)|
See Dennis' comment below: MarbleGlobal.h also features FT2M to convert feet to meters.
|26 ↗||(On Diff #15654)|
|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.
|25–26 ↗||(On Diff #15654)|