From 668d9b4e3b31db7faf1d482184425818e0e6a3d3 Mon Sep 17 00:00:00 2001 From: Brandon Witham Date: Thu, 7 Mar 2019 13:23:44 -0500 Subject: [PATCH] fix Attribute Conflation dropped multipoly building relations bug (#3038) Changes were previously made to Attribute Conflation to remove building relations after conflation when they had already had a corresponding unioned multipoly relation created for them after conflation. However, BuildingOutlineUpdateOp was incorrectly removing all multipoly relations, regardless of whether they had been conflated and then unioned with anything. So, multipoly building relations were disappearing from the output. These changes fix that. --- .../main/cpp/hoot/core/cmd/ConflateCmd.cpp | 9 +- .../hoot/core/ops/BuildingOutlineUpdateOp.cpp | 18 +- .../Expected.osm | 771 ++++++++++++++++++ .../Input1.osm | 356 ++++++++ .../Input2.osm | 4 + .../README.txt | 2 + 6 files changed, 1154 insertions(+), 6 deletions(-) create mode 100644 test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/Expected.osm create mode 100644 test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/Input1.osm create mode 100644 test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/Input2.osm create mode 100644 test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/README.txt diff --git a/hoot-core/src/main/cpp/hoot/core/cmd/ConflateCmd.cpp b/hoot-core/src/main/cpp/hoot/core/cmd/ConflateCmd.cpp index 7e02949126..c92685d1a2 100644 --- a/hoot-core/src/main/cpp/hoot/core/cmd/ConflateCmd.cpp +++ b/hoot-core/src/main/cpp/hoot/core/cmd/ConflateCmd.cpp @@ -364,11 +364,13 @@ void ConflateCmd::_updateConfigOptionsForAttributeConflation() // are involved in reviews. QStringList postConflateOps = ConfigOptions().getConflatePostOps(); + LOG_DEBUG("Post conflate ops before Attribute Conflation adjustment: " << postConflateOps); // Currently, all these things will be true if we're running Attribute Conflation, but I'm // specifying them anyway to harden this a bit. if (ConfigOptions().getBuildingOutlineUpdateOpRemoveBuildingRelations() && postConflateOps.contains("hoot::RemoveElementsVisitor") && - ConfigOptions().getRemoveElementsVisitorElementCriterion() == "hoot::ReviewRelationCriterion" && + ConfigOptions().getRemoveElementsVisitorElementCriterion() == + "hoot::ReviewRelationCriterion" && postConflateOps.contains("hoot::BuildingOutlineUpdateOp")) { const int removeElementsVisIndex = postConflateOps.indexOf("hoot::RemoveElementsVisitor"); @@ -388,7 +390,10 @@ void ConflateCmd::_updateConfigOptionsForAttributeConflation() conf().set( ConfigOptions::getRemoveElementsVisitorElementCriterionKey(), "hoot::ReviewScoreCriterion"); } - LOG_VARD(conf().get(ConfigOptions::getRemoveElementsVisitorElementCriterionKey())); + + LOG_DEBUG( + "Post conflate ops after Attribute Conflation adjustment: " << + conf().get("conflate.post.ops").toStringList()); } } diff --git a/hoot-core/src/main/cpp/hoot/core/ops/BuildingOutlineUpdateOp.cpp b/hoot-core/src/main/cpp/hoot/core/ops/BuildingOutlineUpdateOp.cpp index e424726516..862f6b2541 100644 --- a/hoot-core/src/main/cpp/hoot/core/ops/BuildingOutlineUpdateOp.cpp +++ b/hoot-core/src/main/cpp/hoot/core/ops/BuildingOutlineUpdateOp.cpp @@ -148,7 +148,6 @@ void BuildingOutlineUpdateOp::apply(boost::shared_ptr &map) if (BuildingCriterion().isSatisfied(r)) { _createOutline(r); - _buildingRelationIds.insert(r->getElementId()); } } @@ -169,13 +168,14 @@ void BuildingOutlineUpdateOp::_deleteBuildingRelations() void BuildingOutlineUpdateOp::_createOutline(const RelationPtr& building) { - LOG_VART(building->toString()); + LOG_TRACE("Input building: " << building->toString()); boost::shared_ptr outline(GeometryFactory::getDefaultInstance()->createEmptyGeometry()); const vector entries = building->getMembers(); for (size_t i = 0; i < entries.size(); i++) { + LOG_VART(entries[i].role); if (entries[i].role == MetadataTags::RoleOutline()) { LOG_TRACE("Removing outline role from: " << entries[i] << "..."); @@ -228,6 +228,7 @@ void BuildingOutlineUpdateOp::_createOutline(const RelationPtr& building) else if (entries[i].getElementId().getType() == ElementType::Relation) { RelationPtr relation = _map->getRelation(entries[i].getElementId().getId()); + LOG_VART(relation); if (relation->isMultiPolygon()) { LOG_TRACE("Unioning multipoly relation: " << relation << "..."); @@ -296,13 +297,22 @@ void BuildingOutlineUpdateOp::_createOutline(const RelationPtr& building) // We don't need the relation "type" tag. outlineElement->getTags().remove("type"); LOG_VART(outlineElement); - if (!_removeBuildingRelations) + if (_removeBuildingRelations) + { + LOG_TRACE("Marking building: " << building->getElementId() << " for deletion..."); + _buildingRelationIds.insert(building->getElementId()); + } + else { building->addElement(MetadataTags::RoleOutline(), outlineElement); } } + else + { + LOG_TRACE("Building outline is empty. Skipping creation of multipoly relation."); + } - LOG_VART(building); + LOG_DEBUG("Output building: " << building); } void BuildingOutlineUpdateOp::_mergeNodes(const boost::shared_ptr& changed, diff --git a/test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/Expected.osm b/test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/Expected.osm new file mode 100644 index 0000000000..344f17cf2f --- /dev/null +++ b/test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/Expected.osm @@ -0,0 +1,771 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/Input1.osm b/test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/Input1.osm new file mode 100644 index 0000000000..8fe6d28e6e --- /dev/null +++ b/test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/Input1.osm @@ -0,0 +1,356 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/Input2.osm b/test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/Input2.osm new file mode 100644 index 0000000000..81aeb17646 --- /dev/null +++ b/test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/Input2.osm @@ -0,0 +1,4 @@ + + + + diff --git a/test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/README.txt b/test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/README.txt new file mode 100644 index 0000000000..14ae12b280 --- /dev/null +++ b/test-files/cases/attribute/unifying/building-3037-dropped-multipoly-relations-1/README.txt @@ -0,0 +1,2 @@ +This is an Attribute Conflation test to ensure that multipoly building relations in the reference layer do not get dropped in the output. Note +that the secondary layer is empty.