Merge pull request #4245 from NREL/4166_CZ_ModelMerger

Fix #4166 - Merging FloorSpaceJS can delete unique model Objects such as Facility, Building, Site (and children)

src/model/ModelMerger.cpp

@@ -89,9 +89,13 @@ namespace model {
 
   ModelMerger::ModelMerger() {
     // DLM: TODO expose this to user to give more control over merging?
+
+    // Unique Objects
     m_iddObjectTypesToMerge.push_back(IddObjectType::OS_Site);
     m_iddObjectTypesToMerge.push_back(IddObjectType::OS_Facility);
     m_iddObjectTypesToMerge.push_back(IddObjectType::OS_Building);
+
+    // Non unique Objects
     m_iddObjectTypesToMerge.push_back(IddObjectType::OS_Space);
     m_iddObjectTypesToMerge.push_back(IddObjectType::OS_ShadingSurfaceGroup);
     m_iddObjectTypesToMerge.push_back(IddObjectType::OS_ThermalZone);
@@ -1012,12 +1016,18 @@ namespace model {
     for (const auto& iddObjectType : iddObjectTypesToMerge()) {
       for (auto& currenObject : currentModel.getObjectsByType(iddObjectType)) {
         if (m_currentToNewHandleMapping.find(currenObject.handle()) == m_currentToNewHandleMapping.end()) {
-          currenObject.remove();
+          if ((iddObjectType == IddObjectType::OS_Site) || (iddObjectType == IddObjectType::OS_Facility)
+              || (iddObjectType == IddObjectType::OS_Building)) {
+            // These are unique objects, so no need to delete them
+            continue;
+          } else {
+            currenObject.remove();
+          }
         }
       }
     }
 
-    //** Merge objects from new model into curret model **//
+    //** Merge objects from new model into current model **//
     for (const auto& iddObjectType : iddObjectTypesToMerge()) {
       for (auto& newObject : newModel.getObjectsByType(iddObjectType)) {
         getCurrentModelObject(newObject);

src/model/test/ModelMerger_GTest.cpp

@@ -47,6 +47,8 @@
 #include "../SubSurface_Impl.hpp"
 #include "../ThermalZone.hpp"
 #include "../ThermalZone_Impl.hpp"
+#include "../ClimateZones.hpp"
+#include "../ClimateZones_Impl.hpp"
 
 using namespace openstudio;
 using namespace openstudio::model;
@@ -869,3 +871,36 @@ TEST_F(ModelFixture, ModelMerger_SuggestMapping_ExampleModel) {
     EXPECT_EQ(model1.getObject(mapPair.first)->nameString(), model2.getObject(mapPair.second)->nameString());
   }
 }
+
+TEST_F(ModelFixture, ModelMerger_ClimateZones_4166) {
+
+  // Test for #4166 - Merging FloorSpaceJS strips out climate zone assignment.
+
+  Model model1;
+  Model model2;
+
+  // Add a Climate Zone to model1 only
+  ClimateZones czs = model1.getUniqueModelObject<ClimateZones>();
+  ClimateZone cz = czs.setClimateZone(ClimateZones::ashraeInstitutionName(), "4A");
+  EXPECT_EQ(1, czs.numClimateZones());
+  EXPECT_EQ(1, czs.climateZones().size());
+
+  // To reproduce the original issue, we also need a Site object since it's the fact that the Site object is deleted, and along with it its children
+  // and that includes the ClimateZones
+  Site site1 = model1.getUniqueModelObject<Site>();
+  ASSERT_EQ(1, site1.children().size());
+  EXPECT_EQ(czs, site1.children().front());
+
+  EXPECT_TRUE(model1.getOptionalUniqueModelObject<ClimateZones>());
+  EXPECT_FALSE(model2.getOptionalUniqueModelObject<ClimateZones>());
+
+  // do merge
+  ModelMerger mm;
+  std::map<UUID, UUID> handleMapping;
+  mm.mergeModels(model1, model2, handleMapping);
+
+  ASSERT_TRUE(model1.getOptionalUniqueModelObject<ClimateZones>());
+  EXPECT_EQ("4A", model1.getOptionalUniqueModelObject<ClimateZones>()->climateZones()[0].value());
+  EXPECT_EQ(ClimateZones::ashraeInstitutionName(), model1.getOptionalUniqueModelObject<ClimateZones>()->climateZones()[0].institution());
+  EXPECT_FALSE(model2.getOptionalUniqueModelObject<ClimateZones>());
+}

src/model/test/ThreeJSReverseTranslator_GTest.cpp

@@ -57,6 +57,12 @@
 #include "../DefaultConstructionSet_Impl.hpp"
 #include "../RenderingColor.hpp"
 #include "../RenderingColor_Impl.hpp"
+#include "../ClimateZones.hpp"
+#include "../ClimateZones_Impl.hpp"
+#include "../Site.hpp"
+#include "../Site_Impl.hpp"
+#include "../Building.hpp"
+#include "../Building_Impl.hpp"
 
 #include "../../utilities/geometry/ThreeJS.hpp"
 #include "../../utilities/geometry/FloorplanJS.hpp"
@@ -1179,4 +1185,75 @@ TEST_F(ModelFixture, ThreeJSReverseTranslator_FloorplanJS_OpenToBelow) {
   ASSERT_TRUE(infos[3].ceiling);
   EXPECT_FALSE(infos[3].ceiling->isAirWall());
   EXPECT_FALSE(infos[3].ceiling->adjacentSurface());
-}
+}
+
+TEST_F(ModelFixture, ThreeJSReverseTranslator_FloorplanJS_Site_ClimateZones_4166) {
+
+  // Test for #4166 - Merging FloorSpaceJS strips out climate zone assignment.
+
+  // Start with a base model, put some climate zone in there
+  // Add a Climate Zone to model1 only
+  Model model;
+  ClimateZones czs = model.getUniqueModelObject<ClimateZones>();
+  ClimateZone cz = czs.setClimateZone(ClimateZones::ashraeInstitutionName(), "4A");
+  EXPECT_EQ(1, czs.numClimateZones());
+  EXPECT_EQ(1, czs.climateZones().size());
+
+  // To reproduce the original issue, we also need a Site object since it's the fact that the Site object is deleted, and along with it its children
+  // and that includes the ClimateZones
+  Site site = model.getUniqueModelObject<Site>();
+  ASSERT_EQ(1, site.children().size());
+  EXPECT_EQ(czs, site.children().front());
+
+  // I'm going to instantiate the Building object as well, to check if the floorplan.json north_axis (30) is properly written anyways
+  Building building = model.getUniqueModelObject<Building>();
+  EXPECT_TRUE(building.setNominalFloortoFloorHeight(2.5));
+
+  // Now RT (any) floor plan back to a model
+  ThreeJSReverseTranslator rt;
+
+  openstudio::path p = resourcesPath() / toPath("utilities/Geometry/floorplan.json");
+  ASSERT_TRUE(exists(p));
+
+  boost::optional<FloorplanJS> floorPlan = FloorplanJS::load(toString(p));
+  ASSERT_TRUE(floorPlan);
+
+  // not triangulated, for model transport/translation
+  ThreeScene scene = floorPlan->toThreeScene(true);
+  std::string json = scene.toJSON();
+  EXPECT_TRUE(ThreeScene::load(json));
+
+  boost::optional<Model> newModel_ = rt.modelFromThreeJS(scene);
+  ASSERT_TRUE(newModel_);
+
+  EXPECT_TRUE(model.getOptionalUniqueModelObject<ClimateZones>());
+  EXPECT_FALSE(newModel_->getOptionalUniqueModelObject<ClimateZones>());
+
+  EXPECT_EQ(0.0, building.northAxis());
+  EXPECT_EQ(2.5, building.nominalFloortoFloorHeight());
+
+  ASSERT_TRUE(newModel_->getOptionalUniqueModelObject<Building>());
+  EXPECT_EQ(-30.0, newModel_->getOptionalUniqueModelObject<Building>()->northAxis());
+  EXPECT_FALSE(newModel_->getOptionalUniqueModelObject<Building>()->nominalFloortoFloorHeight());
+
+  model::ModelMerger mm;
+  mm.mergeModels(model, newModel_.get(), rt.handleMapping());
+
+  // Expect to still have retained the ClimateZone
+  ASSERT_TRUE(model.getOptionalUniqueModelObject<ClimateZones>());
+  EXPECT_EQ("4A", model.getOptionalUniqueModelObject<ClimateZones>()->climateZones()[0].value());
+  EXPECT_EQ(ClimateZones::ashraeInstitutionName(), model.getOptionalUniqueModelObject<ClimateZones>()->climateZones()[0].institution());
+
+  EXPECT_FALSE(newModel_->getOptionalUniqueModelObject<ClimateZones>());
+
+  // It should have overridden only the things that were actually not defaulted, so building name and north axis
+  ASSERT_TRUE(model.getOptionalUniqueModelObject<Building>());
+  EXPECT_EQ(-30.0, model.getOptionalUniqueModelObject<Building>()->northAxis());
+  ASSERT_TRUE(model.getOptionalUniqueModelObject<Building>()->nominalFloortoFloorHeight());
+  EXPECT_EQ(2.5, model.getOptionalUniqueModelObject<Building>()->nominalFloortoFloorHeight());
+
+  // New Model isn't touched anyways...
+  ASSERT_TRUE(newModel_->getOptionalUniqueModelObject<Building>());
+  EXPECT_EQ(-30.0, newModel_->getOptionalUniqueModelObject<Building>()->northAxis());
+  EXPECT_FALSE(newModel_->getOptionalUniqueModelObject<Building>()->nominalFloortoFloorHeight());
+}