Initial integration of the GeoDataBuilding into the parser and renderer
ClosedPublic

Authored by mnafees on Jun 20 2017, 7:16 PM.

Details

Summary

All buildings are now rendered as GeoDataBuildings

Diff Detail

Repository
R34 Marble
Lint
Lint Skipped
Unit
Unit Tests Skipped
nienhueser added inline comments.Jun 20 2017, 9:10 PM
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

mnafees updated this revision to Diff 15672.Jun 21 2017, 7:37 AM
mnafees updated this revision to Diff 15676.Jun 21 2017, 8:27 AM
mnafees marked 3 inline comments as done.

Make changes to vectorosm-tilecreator to accomodate for GeoDataBuilding parsing

nienhueser edited edge metadata.Jun 22 2017, 4:30 AM

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?

mnafees updated this revision to Diff 16029.Jun 29 2017, 7:23 PM
nienhueser requested changes to this revision.Jul 2 2017, 5:09 PM

@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?

This revision now requires changes to proceed.Jul 2 2017, 5:09 PM
rahn added inline comments.Jul 2 2017, 7:50 PM
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.

mnafees updated this revision to Diff 16118.Jul 3 2017, 9:31 AM
mnafees edited edge metadata.
mnafees marked 9 inline comments as done.
nienhueser accepted this revision.Jul 3 2017, 7:38 PM

Looks good to me. Please do another thorough test before pushing as discussed in IRC.

This revision is now accepted and ready to land.Jul 3 2017, 7:38 PM
This revision was automatically updated to reflect the committed changes.